Bug 78883 - [GStreamer] AudioSourceProvider support in the MediaPlayer
Summary: [GStreamer] AudioSourceProvider support in the MediaPlayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 105293
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-17 01:07 PST by sangho park
Modified: 2014-12-10 09:19 PST (History)
21 users (show)

See Also:


Attachments
Test Case: Processing audio from Media Element Source (47 bytes, text/plain)
2012-12-26 06:30 PST, James Cready
no flags Details
AudioSourceProvider (29.86 KB, patch)
2012-12-27 08:11 PST, Philippe Normand
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
AudioSourceProvider (29.93 KB, patch)
2012-12-27 08:22 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
AudioSourceProvider (30.46 KB, patch)
2012-12-29 05:34 PST, Philippe Normand
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
AudioSourceProvider (30.78 KB, patch)
2012-12-30 09:05 PST, Philippe Normand
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
AudioSourceProvider (30.79 KB, patch)
2013-01-04 09:59 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
AudioSourceProvider (31.99 KB, patch)
2013-01-16 13:11 PST, Philippe Normand
buildbot: commit-queue-
Details | Formatted Diff | Diff
AudioSourceProvider (33.75 KB, patch)
2013-01-19 02:05 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
AudioSourceProvider (33.57 KB, patch)
2013-01-20 06:04 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Audio pipeline dump (672.56 KB, image/png)
2013-01-26 05:50 PST, Philippe Normand
no flags Details
AudioSourceProvider (36.57 KB, patch)
2013-01-26 06:12 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
AudioSourceProvider (36.90 KB, patch)
2013-02-01 02:44 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
rebased patch, not for review yet (31.47 KB, patch)
2013-12-06 02:36 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
rebased patch that works (32.15 KB, patch)
2013-12-06 03:39 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (31.64 KB, patch)
2014-12-08 08:59 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (31.59 KB, patch)
2014-12-08 09:08 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (30.42 KB, patch)
2014-12-09 06:21 PST, Philippe Normand
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sangho park 2012-02-17 01:07:41 PST
Implement an interface that can be used by 'WebAudio API'.
the interface can use the audio data that is being played on a MediaElement .
Comment 1 Philippe Normand 2012-11-05 00:49:48 PST
I started working on this... not working yet though. Will try to post a patch soon.
Comment 2 James Cready 2012-12-26 06:30:02 PST
Created attachment 180746 [details]
Test Case: Processing audio from Media Element Source
Comment 3 Philippe Normand 2012-12-26 08:53:35 PST
I'll upload a first version of the patch in the following days.
Comment 4 Philippe Normand 2012-12-26 12:21:54 PST
(In reply to comment #2)
> Created an attachment (id=180746) [details]
> Test Case: Processing audio from Media Element Source

James, please note this bug concerns ports using GStreamer only. So for the mac port you might want to open another bug.
Comment 5 Philippe Normand 2012-12-26 12:23:16 PST
OTOH your test page works with WebKitGTK and my patch applied :P
Comment 6 Philippe Normand 2012-12-27 08:11:44 PST
Created attachment 180804 [details]
AudioSourceProvider
Comment 7 kov's GTK+ EWS bot 2012-12-27 08:17:22 PST
Comment on attachment 180804 [details]
AudioSourceProvider

Attachment 180804 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15546531
Comment 8 Philippe Normand 2012-12-27 08:22:41 PST
Created attachment 180805 [details]
AudioSourceProvider
Comment 9 Martin Robinson 2012-12-27 09:58:51 PST
Comment on attachment 180805 [details]
AudioSourceProvider

View in context: https://bugs.webkit.org/attachment.cgi?id=180805&action=review

Okay. There's a great deal of this that I don't understand yet, so I can't really do a full review. Here are some questions and suggestions though.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:23
> +#include "AudioSourceProviderGStreamer.h"

AudioSourceProviderGStreamer.h also has the ENABLE(WEB_AUDIO) guard so maybe it can go above the guard here.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:35
> +using namespace std;

This is actually a violation of style rule 3 in the "using" Statement section:

In C++ implementation files, do not use "using" declarations of any kind to import names in the standard template library. Directly qualify the names at the point they're used instead.

You basically need to use "std::function" everywhere.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:46
> +    return gst_caps_new_simple("audio/x-raw", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, channels, "format", G_TYPE_STRING, gst_audio_format_to_string(GST_AUDIO_FORMAT_F32), "layout", G_TYPE_STRING, "interleaved", NULL);
> +#else
> +    return gst_caps_new_simple("audio/x-raw-float", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, channels, "endianness", G_TYPE_INT, G_BYTE_ORDER, "width", G_TYPE_INT, 32, NULL);
> +#endif
> +}

This is a situation where I think having a single line for each pair makes this more readable.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:58
> +static void copyGstreamerBuffersToAudioChannel(GstBufferList* buffers, AudioBus* bus , int channelNumber, size_t framesToProcess)

Looks like you have an extra space after "bus". Is framesToProcess totally unused here?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:71
> +    memcpy(bus->channel(channelNumber)->mutableData(), reinterpret_cast<float*>(info.data), info.size);
> +    gst_buffer_unmap(buffer, &info);
> +    gst_buffer_list_remove(buffers, 0, 1);

Hrm. Is there a way to process the data directly into the AudioBus or is provideInput called after handleAudioBuffer?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:131
> +    GRefPtr<GstElement> deinterleave = adoptGRef(gst_bin_get_by_name(GST_BIN(m_audioSinkBin), "deinterleave"));

The deinterleave element isn't unreffed automatically by the destuction of m_audioSinkBin? By adopting here and putting it in a GRefPtr you are doing an implicit unref.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:139
> +    if (m_audioSinkBin) {
> +        gst_object_unref(m_audioSinkBin);
> +        m_audioSinkBin = 0;
> +    }

There are a few things that I don't understand here:
1. m_audioSinkBin seems like it could be a GRefPtr, but it isn't.
2. You only unref m_audioSinkBin for older versions of GStreamer.
3. You don't actually need to zero out members in the destructor, unless you are about to read it somewhere. The object is about to disappear.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:158
> +#ifdef GST_API_VERSION_1

Okay. This is a bit of a nit, but you use  #ifdef GST_API_VERSION_1 some places and #ifndef GST_API_VERSION_1 other places. It's a little clearer to use the same everywhere.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:229
> +    GstElement* audioQueue2 = gst_element_factory_make("queue", 0);

There's no audioQueue in this context, so I guess you can just call this audioQueue?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:236
> +    int size = 256 * sizeof(float);

This should probably be static const and maybe named something like chopperDataSize.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:242
> +    GstCaps* caps = getGStreamerAudioCaps(2, 44100);

Maybe some global constants like gNumberOfChannels and gBitRate?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:306
> +void AudioSourceProviderGStreamer::cleanElementsAfterDeinterleaveSourcePad(GstPad* pad)

English nit: clean -> cleanUp

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:317
> +    gst_bin_remove_many(GST_BIN(m_audioSinkBin), queue.get(), sink.get(), NULL);

Will removing the element decrease their reference count? If so, I guess you don't need to also place them in GRefPtrs?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.h:26
> +#include <gst/gst.h>

Can you avoid using the gstreamer header here?
Comment 10 Philippe Normand 2012-12-27 10:34:25 PST
(In reply to comment #9)
> (From update of attachment 180805 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180805&action=review
> 
> Okay. There's a great deal of this that I don't understand yet, so I can't really do a full review. Here are some questions and suggestions though.
> 

Thanks :)

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:23
> > +#include "AudioSourceProviderGStreamer.h"
> 
> AudioSourceProviderGStreamer.h also has the ENABLE(WEB_AUDIO) guard so maybe it can go above the guard here.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:35
> > +using namespace std;
> 
> This is actually a violation of style rule 3 in the "using" Statement section:
> 
> In C++ implementation files, do not use "using" declarations of any kind to import names in the standard template library. Directly qualify the names at the point they're used instead.
> 
> You basically need to use "std::function" everywhere.
> 

Hum I didn't know that rule, I suspect other code I wrote breaks the law :)
Anyway, not sure I actually use any std:: function, I'll check.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:46
> > +    return gst_caps_new_simple("audio/x-raw", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, channels, "format", G_TYPE_STRING, gst_audio_format_to_string(GST_AUDIO_FORMAT_F32), "layout", G_TYPE_STRING, "interleaved", NULL);
> > +#else
> > +    return gst_caps_new_simple("audio/x-raw-float", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, channels, "endianness", G_TYPE_INT, G_BYTE_ORDER, "width", G_TYPE_INT, 32, NULL);
> > +#endif
> > +}
> 
> This is a situation where I think having a single line for each pair makes this more readable.
> 

Yes but the style bot will complain. I'd like to move this function to GStreamerVersioning or Utilities btw as it's a copy/paste of the one in AudioFileReader.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:58
> > +static void copyGstreamerBuffersToAudioChannel(GstBufferList* buffers, AudioBus* bus , int channelNumber, size_t framesToProcess)
> 
> Looks like you have an extra space after "bus". Is framesToProcess totally unused here?
> 

Yes unused but we should ASSERT its value I think, because the constants used in this file depend on it.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:71
> > +    memcpy(bus->channel(channelNumber)->mutableData(), reinterpret_cast<float*>(info.data), info.size);
> > +    gst_buffer_unmap(buffer, &info);
> > +    gst_buffer_list_remove(buffers, 0, 1);
> 
> Hrm. Is there a way to process the data directly into the AudioBus or is provideInput called after handleAudioBuffer?
> 

provideInput is called periodically, I'm afraid there's no way to avoid this memcpy(), but I'll think again about it.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:131
> > +    GRefPtr<GstElement> deinterleave = adoptGRef(gst_bin_get_by_name(GST_BIN(m_audioSinkBin), "deinterleave"));
> 
> The deinterleave element isn't unreffed automatically by the destuction of m_audioSinkBin? By adopting here and putting it in a GRefPtr you are doing an implicit unref.
> 

Yes but gst_bin_get_by_name refs the element.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:139
> > +    if (m_audioSinkBin) {
> > +        gst_object_unref(m_audioSinkBin);
> > +        m_audioSinkBin = 0;
> > +    }
> 
> There are a few things that I don't understand here:
> 1. m_audioSinkBin seems like it could be a GRefPtr, but it isn't.
> 2. You only unref m_audioSinkBin for older versions of GStreamer.
> 3. You don't actually need to zero out members in the destructor, unless you are about to read it somewhere. The object is about to disappear.
> 

Hum I used the same approach as in the MediaPlayerPrivate. I'll revise this part with a fresh mind :)

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:158
> > +#ifdef GST_API_VERSION_1
> 
> Okay. This is a bit of a nit, but you use  #ifdef GST_API_VERSION_1 some places and #ifndef GST_API_VERSION_1 other places. It's a little clearer to use the same everywhere.
> 

Yes, good point again.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:229
> > +    GstElement* audioQueue2 = gst_element_factory_make("queue", 0);
> 
> There's no audioQueue in this context, so I guess you can just call this audioQueue?
> 

Ah yes forgot to rename it.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:236
> > +    int size = 256 * sizeof(float);
> 
> This should probably be static const and maybe named something like chopperDataSize.
> 

Yes I forgot to document this value as well.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:242
> > +    GstCaps* caps = getGStreamerAudioCaps(2, 44100);
> 
> Maybe some global constants like gNumberOfChannels and gBitRate?
> 

Yes!

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:306
> > +void AudioSourceProviderGStreamer::cleanElementsAfterDeinterleaveSourcePad(GstPad* pad)
> 
> English nit: clean -> cleanUp
> 

Oh, ok!

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:317
> > +    gst_bin_remove_many(GST_BIN(m_audioSinkBin), queue.get(), sink.get(), NULL);
> 
> Will removing the element decrease their reference count? If so, I guess you don't need to also place them in GRefPtrs?
> 
> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.h:26
> > +#include <gst/gst.h>
> 
> Can you avoid using the gstreamer header here?

Hum it can't because of GstFlowReturn which is an enum which can't be forward-declared.

Thanks I'll upload a new patch tomorrow hopefully :)
Comment 11 Martin Robinson 2012-12-27 10:42:15 PST
Comment on attachment 180805 [details]
AudioSourceProvider

View in context: https://bugs.webkit.org/attachment.cgi?id=180805&action=review

>>> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:46
>>> +}
>> 
>> This is a situation where I think having a single line for each pair makes this more readable.
> 
> Yes but the style bot will complain. I'd like to move this function to GStreamerVersioning or Utilities btw as it's a copy/paste of the one in AudioFileReader.

Yeah, the new style bot rule is pretty annoying. You can follow the style rule by simply using a 4 space indent instead of aligning things. For instance:

return gst_caps_new_simple("audio/x-raw", 
    "rate", G_TYPE_INT, static_cast<int>(sampleRate), 
    "channels", G_TYPE_INT, channels,
    "format", G_TYPE_STRING, gst_audio_format_to_string(GST_AUDIO_FORMAT_F32),
    "layout", G_TYPE_STRING, "interleaved",
    NULL);

I admit it isn't the prettiest thing in the world, but it's easier to see what's going on, I think.
Comment 12 James Cready 2012-12-27 17:05:55 PST
(In reply to comment #4)
> (In reply to comment #2)
> > Created an attachment (id=180746) [details] [details]
> > Test Case: Processing audio from Media Element Source
> 
> James, please note this bug concerns ports using GStreamer only. So for the mac port you might want to open another bug.

I'm not particularly knowledgable about the inner workings of WebKit (or the OS specific APIs it uses). Would you be kind enough to provide me a suitable title for such a bug? Thank you.
Comment 13 Philippe Normand 2012-12-28 01:37:26 PST
(In reply to comment #12)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > Created an attachment (id=180746) [details] [details] [details]
> > > Test Case: Processing audio from Media Element Source
> > 
> > James, please note this bug concerns ports using GStreamer only. So for the mac port you might want to open another bug.
> 
> I'm not particularly knowledgable about the inner workings of WebKit (or the OS specific APIs it uses). Would you be kind enough to provide me a suitable title for such a bug? Thank you.

You can file a new bug for the Mac port, something like "[Mac] WebAudioSourceProvider integration in the MediaPlayer"
Comment 14 Philippe Normand 2012-12-28 06:15:43 PST
Comment on attachment 180805 [details]
AudioSourceProvider

I'm adding proper multi-channel support to the provider, reworking the patch and hopefully avoiding the change to AudioContext.
Comment 15 Philippe Normand 2012-12-29 05:34:04 PST
Created attachment 180935 [details]
AudioSourceProvider
Comment 16 EFL EWS Bot 2012-12-29 05:37:34 PST
Comment on attachment 180935 [details]
AudioSourceProvider

Attachment 180935 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15594080
Comment 17 Philippe Normand 2012-12-30 09:05:00 PST
Created attachment 180956 [details]
AudioSourceProvider
Comment 18 EFL EWS Bot 2012-12-30 09:11:52 PST
Comment on attachment 180956 [details]
AudioSourceProvider

Attachment 180956 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15599404
Comment 19 Philippe Normand 2013-01-04 09:59:23 PST
Created attachment 181320 [details]
AudioSourceProvider
Comment 20 Sebastian Dröge (slomo) 2013-01-11 06:27:12 PST
Comment on attachment 181320 [details]
AudioSourceProvider

View in context: https://bugs.webkit.org/attachment.cgi?id=181320&action=review

Looks good in general

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:75
> +    memcpy(bus->channel(channelNumber)->mutableData(), reinterpret_cast<float*>(info.data), info.size);

The reinterpret_cast here shouldn't be necessary

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:106
> +    GstElement* audioSink = gst_element_factory_make("autoaudiosink", 0);

You might need a audioconvert and audioresample before/after the volume element

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:141
> +        g_signal_handlers_disconnect_by_func(deinterleave.get(), reinterpret_cast<gpointer>(onGStreamerDeinterleaveReadyCallback), this);

It's far more efficient to store the signal handler IDs from g_signal_connect() and disconnect via them here
Comment 21 Philippe Normand 2013-01-14 12:14:50 PST
(In reply to comment #20)
> (From update of attachment 181320 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181320&action=review
> 
> Looks good in general
> 
> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:75
> > +    memcpy(bus->channel(channelNumber)->mutableData(), reinterpret_cast<float*>(info.data), info.size);
> 
> The reinterpret_cast here shouldn't be necessary
> 

Ok

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:106
> > +    GstElement* audioSink = gst_element_factory_make("autoaudiosink", 0);
> 
> You might need a audioconvert and audioresample before/after the volume element
> 

Not sure to understand, both elements before and after volume?

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:141
> > +        g_signal_handlers_disconnect_by_func(deinterleave.get(), reinterpret_cast<gpointer>(onGStreamerDeinterleaveReadyCallback), this);
> 
> It's far more efficient to store the signal handler IDs from g_signal_connect() and disconnect via them here

Ok I didn't know that :)
Comment 22 Philippe Normand 2013-01-16 13:11:01 PST
Created attachment 183025 [details]
AudioSourceProvider
Comment 23 Build Bot 2013-01-16 13:37:32 PST
Comment on attachment 183025 [details]
AudioSourceProvider

Attachment 183025 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15904627

New failing tests:
svg/as-image/img-relative-height.html
Comment 24 Martin Robinson 2013-01-18 09:36:28 PST
Comment on attachment 183025 [details]
AudioSourceProvider

View in context: https://bugs.webkit.org/attachment.cgi?id=183025&action=review

Looking a bit better. I still have some questions though...

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:21
> +#include "config.h"
> +
> +#include "AudioSourceProviderGStreamer.h"

Extra newline here.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:37
> +#include <wtf/UnusedParam.h>

This should go in the first block of includes.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:91
> +#ifdef GST_API_VERSION_1
> +    if (!gst_buffer_list_length(buffers)) {
> +        bus->zero();
> +        return;
> +    }
> +
> +    GstBuffer* buffer = gst_buffer_list_get(buffers, 0);
> +    GstMapInfo info;
> +    gst_buffer_map(buffer, &info, GST_MAP_READ);
> +    memcpy(bus->channel(channelNumber)->mutableData(), info.data, info.size);
> +    gst_buffer_unmap(buffer, &info);
> +    gst_buffer_list_remove(buffers, 0, 1);
> +#else
> +    GstBufferListIterator* iter = gst_buffer_list_iterate(buffers);
> +    gst_buffer_list_iterator_next_group(iter);
> +    GstBuffer* buffer = gst_buffer_list_iterator_next(iter);
> +    if (!buffer) {
> +        bus->zero();
> +        gst_buffer_list_iterator_free(iter);
> +        return;
> +    }
> +
> +    memcpy(bus->channel(channelNumber)->mutableData(), reinterpret_cast<float*>(GST_BUFFER_DATA(buffer)), GST_BUFFER_SIZE(buffer));
> +    gst_buffer_list_iterator_remove(iter);
> +    gst_buffer_list_iterator_free(iter);
> +#endif

Here's on block where the #ifs are still opposite of the rest of the file.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:167
> +#ifndef GST_API_VERSION_1
> +    if (m_audioSinkBin) {
> +        gst_object_unref(m_audioSinkBin);
> +        m_audioSinkBin = 0;
> +    }
> +    gst_buffer_list_iterator_free(m_frontLeftBuffersIterator);
> +    gst_buffer_list_iterator_free(m_frontRightBuffersIterator);
> +#endif
> +    gst_buffer_list_unref(m_frontLeftBuffers);
> +    gst_buffer_list_unref(m_frontRightBuffers);

I still don't really understand why m_audioSinkBin isn't a RefPtr or unreffed for Gstreamer 0.1. I think that the not-unreffing thing deserves at least a comment explaining why.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:188
> +    GstCaps* caps = gst_buffer_get_caps(buffer);

You can use a GRefPtr here, right?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:193
> +    GstAudioChannelPosition* positions = gst_audio_get_channel_positions(structure);

You could use a GOwnPtr here.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:209
> +    GstSample* sample = gst_app_sink_pull_sample(sink);

Why not add GstSample support to GRefPtrGStreamer?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:409
> +    bool done = false;
> +    while (!done) {
> +#ifndef GST_API_VERSION_1
> +        gpointer data;
> +
> +        switch (gst_iterator_next(iter, &data)) {
> +        case GST_ITERATOR_OK: {
> +            GRefPtr<GstPad> pad = adoptGRef(GST_PAD_CAST(data));
> +            cleanUpElementsAfterDeinterleaveSourcePad(pad.get());
> +            break;
> +        }
> +#else
> +        GValue item = G_VALUE_INIT;
> +        switch (gst_iterator_next(iter, &item)) {
> +        case GST_ITERATOR_OK: {
> +            GstPad* pad = static_cast<GstPad*>(g_value_get_object(&item));
> +            cleanUpElementsAfterDeinterleaveSourcePad(pad);
> +            break;
> +        }
> +#endif
> +        case GST_ITERATOR_RESYNC:
> +            gst_iterator_resync(iter);
> +            break;
> +        case GST_ITERATOR_ERROR:
> +            done = true;
> +            break;
> +        case GST_ITERATOR_DONE:
> +            done = true;
> +            break;
> +        }
> +#ifdef GST_API_VERSION_1

I think maybe you can simplify this a bit:

#ifndef GST_API_VERSION_1
gpointer item;
#else
GValue item = G_VALUE_INIT;
#endif


GstIteratorResult result = gst_iterator_next(iter, &item)
while (result != GST_ITERATOR_ERROR && result != GST_ITERATOR_DONE) {
    if (result == GST_ITERATOR_OK) {
#ifndef GST_API_VERSION_1
    GRefPtr<GstPad> pad = adoptGRef(GST_PAD_CAST(data));
     cleanUpElementsAfterDeinterleaveSourcePad(pad.get());
#else
    GstPad* pad = static_cast<GstPad*>(g_value_get_object(&item));
    cleanUpElementsAfterDeinterleaveSourcePad(pad);
#endif
    } elseif (result == GST_ITERATOR_RESYNC
        gst_iterator_resync(iter);
   else
       ASSERT(result == GST_ITERATOR_DONE || result == GST_ITERATOR_ERROR);
}

It seems like you are also missing some error handling here?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:410
> +        g_value_unset(&item);

Do you only need to unset the value at the end, only when result == GST_ITERATOR_OK, or every iteration?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.h:43
> +        static PassOwnPtr<AudioSourceProviderGStreamer> create() { return adoptPtr(new AudioSourceProviderGStreamer()); }
> +        AudioSourceProviderGStreamer();
> +        ~AudioSourceProviderGStreamer();
> +
> +        void provideInput(AudioBus*, size_t framesToProcess);
> +        void setClient(AudioSourceProviderClient*);

"public" and "private" should be at the first level of indentation.
Comment 25 Philippe Normand 2013-01-19 01:30:16 PST
(In reply to comment #24)
> 
> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:167
> > +#ifndef GST_API_VERSION_1
> > +    if (m_audioSinkBin) {
> > +        gst_object_unref(m_audioSinkBin);
> > +        m_audioSinkBin = 0;
> > +    }
> > +    gst_buffer_list_iterator_free(m_frontLeftBuffersIterator);
> > +    gst_buffer_list_iterator_free(m_frontRightBuffersIterator);
> > +#endif
> > +    gst_buffer_list_unref(m_frontLeftBuffers);
> > +    gst_buffer_list_unref(m_frontRightBuffers);
> 
> I still don't really understand why m_audioSinkBin isn't a RefPtr or unreffed for Gstreamer 0.1. I think that the not-unreffing thing deserves at least a comment explaining why.
> 

I wonder now why this is in the 0.10 code path indeed.
We can't use a GRefPtr for the GstBin because gst_bin_new returns a floating reference :/ So a Debug build would ASSERT in the adoptGRef call.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:193
> > +    GstAudioChannelPosition* positions = gst_audio_get_channel_positions(structure);
> 
> You could use a GOwnPtr here.
> 

Well that would mean adding a GOwnPtrGStreamer module again likely only for this gst 0.10 API. I'm not sure it's worth it.

Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:409
> > +    bool done = false;
> > +    while (!done) {
> > +#ifndef GST_API_VERSION_1
> > +        gpointer data;
> > +
> > +        switch (gst_iterator_next(iter, &data)) {
> > +        case GST_ITERATOR_OK: {
> > +            GRefPtr<GstPad> pad = adoptGRef(GST_PAD_CAST(data));
> > +            cleanUpElementsAfterDeinterleaveSourcePad(pad.get());
> > +            break;
> > +        }
> > +#else
> > +        GValue item = G_VALUE_INIT;
> > +        switch (gst_iterator_next(iter, &item)) {
> > +        case GST_ITERATOR_OK: {
> > +            GstPad* pad = static_cast<GstPad*>(g_value_get_object(&item));
> > +            cleanUpElementsAfterDeinterleaveSourcePad(pad);
> > +            break;
> > +        }
> > +#endif
> > +        case GST_ITERATOR_RESYNC:
> > +            gst_iterator_resync(iter);
> > +            break;
> > +        case GST_ITERATOR_ERROR:
> > +            done = true;
> > +            break;
> > +        case GST_ITERATOR_DONE:
> > +            done = true;
> > +            break;
> > +        }
> > +#ifdef GST_API_VERSION_1
> 
> I think maybe you can simplify this a bit:
> 
> #ifndef GST_API_VERSION_1
> gpointer item;
> #else
> GValue item = G_VALUE_INIT;
> #endif
> 
> 
> GstIteratorResult result = gst_iterator_next(iter, &item)
> while (result != GST_ITERATOR_ERROR && result != GST_ITERATOR_DONE) {
>     if (result == GST_ITERATOR_OK) {
> #ifndef GST_API_VERSION_1
>     GRefPtr<GstPad> pad = adoptGRef(GST_PAD_CAST(data));
>      cleanUpElementsAfterDeinterleaveSourcePad(pad.get());
> #else
>     GstPad* pad = static_cast<GstPad*>(g_value_get_object(&item));
>     cleanUpElementsAfterDeinterleaveSourcePad(pad);
> #endif
>     } elseif (result == GST_ITERATOR_RESYNC
>         gst_iterator_resync(iter);
>    else
>        ASSERT(result == GST_ITERATOR_DONE || result == GST_ITERATOR_ERROR);
> }
> 
> It seems like you are also missing some error handling here?
> 

Actually I'm not sure what to do in case of GST_ITERATOR_ERROR in this context...

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:410
> > +        g_value_unset(&item);
> 
> Do you only need to unset the value at the end, only when result == GST_ITERATOR_OK, or every iteration?
> 

It should be needed only if result = GST_ITERATOR_OK but I guess it's acceptable to do it at every iteration.
Comment 26 Philippe Normand 2013-01-19 01:31:32 PST
(In reply to comment #24)
> 
> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:37
> > +#include <wtf/UnusedParam.h>
> 
> This should go in the first block of includes.
> 

The style checker doesn't like that and spits an alphabetical order error because of the gst* includes below the UnusedParam.h include
Comment 27 Philippe Normand 2013-01-19 02:05:05 PST
Created attachment 183612 [details]
AudioSourceProvider
Comment 28 Martin Robinson 2013-01-19 07:38:51 PST
(In reply to comment #25)

> I wonder now why this is in the 0.10 code path indeed.
> We can't use a GRefPtr for the GstBin because gst_bin_new returns a floating reference :/ So a Debug build would ASSERT in the adoptGRef call.

This is the implementation of refGPtr for GstElement:

template <> GstElement* refGPtr<GstElement>(GstElement* ptr)
{
    if (ptr)
        webkitGstObjectRefSink(GST_OBJECT(ptr));

    return ptr;
}

So all you need do to assign it to a GRefPtr is to not call adoptGRef because it returns a floating reference.
> 
> > > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:193
> > > +    GstAudioChannelPosition* positions = gst_audio_get_channel_positions(structure);
> > 
> > You could use a GOwnPtr here.
> > 
> 
> Well that would mean adding a GOwnPtrGStreamer module again likely only for this gst 0.10 API. I'm not sure it's worth it.

Actually no, since positions is freed with g_free you can use the normal GOwnPtr.

> It should be needed only if result = GST_ITERATOR_OK but I guess it's acceptable to do it at every iteration.

In my code above it was a bit simpler/clearer and more effecient just under the block for GST_ITERATOR_OK. I think there's a risk that someone in the future who is refactoring this code could have the wrong idea about when the call is needed.
Comment 29 Martin Robinson 2013-01-19 07:40:14 PST
Comment on attachment 183612 [details]
AudioSourceProvider

View in context: https://bugs.webkit.org/attachment.cgi?id=183612&action=review

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:373
> +    bool done = false;
> +    while (!done) {

I think maybe without removing the outer loop here this becomes an infinite loop.
Comment 30 Philippe Normand 2013-01-20 06:04:30 PST
Created attachment 183670 [details]
AudioSourceProvider
Comment 31 Martin Robinson 2013-01-24 18:37:15 PST
Comment on attachment 183670 [details]
AudioSourceProvider

View in context: https://bugs.webkit.org/attachment.cgi?id=183670&action=review

Seems sane to me, though to be honest, I don't understand this code very well. Does this allow us to unskip any tests?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:35
> +#include <wtf/UnusedParam.h>

This should go with the first block of includes.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:179
> +    GRefPtr<GstCaps> caps = gst_buffer_get_caps(buffer);

Does gst_buffer_get_caps return a floating reference? If not, I think this might be a memory leak unless you use adoptGPtr here.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:194
> +    case GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT:
> +        gst_buffer_list_iterator_add(m_frontLeftBuffersIterator, buffer);
> +        break;
> +    case GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT:
> +        gst_buffer_list_iterator_add(m_frontRightBuffersIterator, buffer);
> +        break;
> +    default:
> +        gst_buffer_unref(buffer);
> +        break;

Is it correct that you only unref the buffer if you get a channel that isn't left/right, or is this supposed to be a fallthrough? If you are always supposed to unref it, perhaps this should just be a GRefPtr. :)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:309
> +    GstCaps* caps = getGstAudioCaps(1, gSampleBitRate);
> +    gst_app_sink_set_caps(GST_APP_SINK(sink), caps);
> +    gst_caps_unref(caps);

Looks like this could be:

GRefPtr<GstCaps> caps = adoptGRef(getGstAudioCaps(1, gSampleBitRate));
gst_app_sink_set_caps(GST_APP_SINK(sink), caps.get());

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.h:26
> +#include "AudioSourceProvider.h"
> +
> +#include "GRefPtrGStreamer.h"

Extra newline here.
Comment 32 Philippe Normand 2013-01-26 04:26:44 PST
(In reply to comment #31)
> (From update of attachment 183670 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183670&action=review
> 
> Seems sane to me, though to be honest, I don't understand this code very well. Does this allow us to unskip any tests?
> 

There is no WAV test for this feature yet, only a test that checks a mediaelementsourcenode can be created.
I'll attach a png dump of the pipeline and comment the code a bit more.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:35
> > +#include <wtf/UnusedParam.h>
> 
> This should go with the first block of includes.
> 

Like I said in comment 26, the style checker says no :)

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:179
> > +    GRefPtr<GstCaps> caps = gst_buffer_get_caps(buffer);
> 
> Does gst_buffer_get_caps return a floating reference? If not, I think this might be a memory leak unless you use adoptGPtr here.
> 

It doesn't return a floating ref, I'll add adopt!

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:194
> > +    case GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT:
> > +        gst_buffer_list_iterator_add(m_frontLeftBuffersIterator, buffer);
> > +        break;
> > +    case GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT:
> > +        gst_buffer_list_iterator_add(m_frontRightBuffersIterator, buffer);
> > +        break;
> > +    default:
> > +        gst_buffer_unref(buffer);
> > +        break;
> 
> Is it correct that you only unref the buffer if you get a channel that isn't left/right, or is this supposed to be a fallthrough? If you are always supposed to unref it, perhaps this should just be a GRefPtr. :)
> 

It's a fallthrough, gst_buffer_list_iterator_add takes ownership of the buffer, so no need to unref it in those cases.
Comment 33 Philippe Normand 2013-01-26 05:50:52 PST
Created attachment 184860 [details]
Audio pipeline dump

Graphical representation of the pipeline playing an audio file with a MediaElementSourceNode, checkout the audio-sink on the right.
The 2 appsinks gather raw audio data and feed it to an AudioBus that is playing to an AudioDestinationNode and thus a separate pipeline not represented here :)
Comment 34 Philippe Normand 2013-01-26 06:12:42 PST
Created attachment 184861 [details]
AudioSourceProvider
Comment 35 Philippe Normand 2013-02-01 02:44:50 PST
Created attachment 185994 [details]
AudioSourceProvider

Rebased against ToT. Removed an audioconvert ! audioresample before
tee and added some comments about the ones needed downstream tee.
Comment 36 Philippe Normand 2013-02-01 07:32:14 PST
I'm afraid we can't avoid the memcpy calls because the AudioBus is not known early enough by the AudioSourceProvider. Moreover ::provideInput is called multiple times, I just don't see how to have our buffer lists configured to point to to pre-allocated AudioChannels in a sane way.
Comment 37 Martin Robinson 2013-02-01 14:03:43 PST
(In reply to comment #36)
> I'm afraid we can't avoid the memcpy calls because the AudioBus is not known early enough by the AudioSourceProvider. Moreover ::provideInput is called multiple times, I just don't see how to have our buffer lists configured to point to to pre-allocated AudioChannels in a sane way.

Hrm. What about AudioBus::setChannelMemory?
Comment 38 Martin Robinson 2013-02-01 14:04:58 PST
(In reply to comment #37)
> (In reply to comment #36)
> > I'm afraid we can't avoid the memcpy calls because the AudioBus is not known early enough by the AudioSourceProvider. Moreover ::provideInput is called multiple times, I just don't see how to have our buffer lists configured to point to to pre-allocated AudioChannels in a sane way.
> 
> Hrm. What about AudioBus::setChannelMemory?

Ah, it looks like that's used on the other side? It seems that what we need is a way to set a GstBuffer on the AudioBus and pass it through.
Comment 39 Philippe Normand 2013-02-28 00:15:27 PST
Comment on attachment 185994 [details]
AudioSourceProvider

I haven't had time to rework this patch yet. Pulling out of review queue.
Comment 40 Philippe Normand 2013-12-06 02:36:30 PST
Created attachment 218582 [details]
rebased patch, not for review yet
Comment 41 Philippe Normand 2013-12-06 03:39:03 PST
Created attachment 218586 [details]
rebased patch that works
Comment 42 Gustavo Noronha (kov) 2013-12-07 02:21:06 PST
Comment on attachment 218586 [details]
rebased patch that works

View in context: https://bugs.webkit.org/attachment.cgi?id=218586&action=review

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:163
> +    // Check the first audio channel. The buffer is supposed to store
> +    // data of a single channel anyway.

I got confused a bit with this comment, are you saying that the buffer will always have only 1 channel represented, so we just need to check the first?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:287
> +    // TODO: Don't use a capsfilter in front of deinterleave,

s/TODO/FIXME/
Comment 43 Philippe Normand 2013-12-09 02:50:57 PST
(In reply to comment #42)
> (From update of attachment 218586 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218586&action=review
> 
> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:163
> > +    // Check the first audio channel. The buffer is supposed to store
> > +    // data of a single channel anyway.
> 
> I got confused a bit with this comment, are you saying that the buffer will always have only 1 channel represented, so we just need to check the first?
> 

Yes it's a mono layout indeed.

> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:287
> > +    // TODO: Don't use a capsfilter in front of deinterleave,
> 
> s/TODO/FIXME/

I was planning to fix that in a future iteration, thanks for the review!
Comment 44 Sebastian Dröge (slomo) 2014-12-07 11:11:03 PST
Comment on attachment 218586 [details]
rebased patch that works

View in context: https://bugs.webkit.org/attachment.cgi?id=218586&action=review

What's missing to land this? I have some comments here, but there's probably more missing? Is channels!=2 support mandatory? I can probably work on this a bit

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:62
> +    GstBuffer* buffer = gst_buffer_list_get(buffers, 0);

Better iterate over all buffers here like in AudioFileReaderGStreamer. gst_buffer_list_get() will merge all buffers first, so you copy twice here.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:67
> +    gst_buffer_list_remove(buffers, 0, 1);

Maybe just unref the buffer list here and create a new one instead

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:181
> +    GstElement* dataChopper = gst_element_factory_make("chopmydata", "dataChopper");

Instead of such dubious elements I would recommend to just use a GstAdapter or similar (in the appsink callback) to chop the data yourself

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:214
> +        "format", G_TYPE_STRING, gst_audio_format_to_string(GST_AUDIO_FORMAT_F32),

GST_AUDIO_NE(F32)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:228
> +    gst_element_link_pads_full(audioTee.get(), 0, audioQueue, "sink", GST_PAD_LINK_CHECK_NOTHING);

Provide the pad name here: "src_%u"

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:320
> +    GstIteratorResult result = gst_iterator_next(iter, &item);

Instead of all this you can simply use gst_iterator_foreach() (make sure to check the return value for resync!) here

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1832
> +    if (m_preservesPitch) {

Maybe instead of all this we can just depend on GStreamer >= 1.4.2 and use the audio-filter property for scaletempo? That would considerably clean up this code
Comment 45 Philippe Normand 2014-12-08 01:02:33 PST
I found a newer version of this patch in a local branch, I'll rebase it according to your comments and give it a try, after all those months :)
Comment 46 Philippe Normand 2014-12-08 08:59:06 PST
Created attachment 242817 [details]
patch
Comment 47 WebKit Commit Bot 2014-12-08 09:01:11 PST
Attachment 242817 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1896:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 48 Philippe Normand 2014-12-08 09:08:33 PST
Created attachment 242818 [details]
patch
Comment 49 Sebastian Dröge (slomo) 2014-12-09 01:24:53 PST
Comment on attachment 242818 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242818&action=review

Looks good, just some comments

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:54
> +static void copyGstreamerBuffersToAudioChannel(GstAdapter* adapter, AudioBus* bus , int channelNumber, size_t framesToProcess)

GStreamer with a capital S :)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:63
> +    if (gst_adapter_available(adapter) >= bytes) {

What if there are >= bytes available after you took bytes out of the adapter? Should there be a "while (avail >= bytes)" here?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:66
> +            memcpy(destination, data, bytes);

gst_adapter_copy() would be more efficient in theory and does not require mapping first (which is why it could be more efficient)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:69
> +            gst_adapter_flush(adapter, bytes);

I think you should gst_adapter_clear() when receiving a FLUSH_STOP event on the appsink (needs a pad probe, appsink does not tell you unfortunately)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:179
> +    case GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT:

You would get POSITION_MONO here when having mono input

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:249
> +

Maybe check if you get more than 2 pads here, just in case

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:315
> +    GRefPtr<GstElement> deinterleave = adoptGRef(gst_bin_get_by_name(GST_BIN(m_audioSinkBin.get()), "deinterleave"));

And unmute the volume element maybe?

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:322
> +    gst_iterator_foreach(iter, cleanUpElementsAfterDeinterleaveSourcePadCallback, this);

You could cause not-linked errors if the pipeline is still running after all deinterleave srcpads are unlinked. I guess in practice not because of the tee before this, but it's not clear to me to which state reset() should reset.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:323
> +    // GValue item = G_VALUE_INIT;

Remove the commented out code :)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1891
>          gst_object_unref(audioSinkBin);

Maybe create the bin only when needed
Comment 50 Philippe Normand 2014-12-09 03:39:13 PST
(In reply to comment #49)
> Comment on attachment 242818 [details]
> patch
> 
> 
> > Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:63
> > +    if (gst_adapter_available(adapter) >= bytes) {
> 
> What if there are >= bytes available after you took bytes out of the
> adapter? Should there be a "while (avail >= bytes)" here?
> 

Well I need only a specific amount of audio frames. So processing all the available data from the adapter would be undesired. In the end we use the adapter as a queue, pulling a specific amount of data on-demand.

Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:249
> > +
> 
> Maybe check if you get more than 2 pads here, just in case
> 

Right, for now I'll emit a warning and plug a fakesink to keep the pipeline rolling.

Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:322
> > +    gst_iterator_foreach(iter, cleanUpElementsAfterDeinterleaveSourcePadCallback, this);
> 
> You could cause not-linked errors if the pipeline is still running after all
> deinterleave srcpads are unlinked. I guess in practice not because of the
> tee before this, but it's not clear to me to which state reset() should
> reset.
> 

::reset() is called by the player before setting the pipeline to READY. So hum perhaps I should make sure errors are not emitted.
Comment 51 Philippe Normand 2014-12-09 06:21:51 PST
Created attachment 242916 [details]
patch
Comment 52 Gustavo Noronha (kov) 2014-12-09 10:38:38 PST
Comment on attachment 242916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242916&action=review

A few nits in addition to the ones we discussed in person =)

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:35
> +// For now the provider supports only stereo files at a fixed sample
> +// bitrate.

Nit: no need to split this line.

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:283
> +    callbacks.eos = 0;
> +    callbacks.new_preroll = 0;

nullptr

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:285
> +    gst_app_sink_set_callbacks(GST_APP_SINK(sink), &callbacks, this, 0);

nullptr
Comment 53 Sebastian Dröge (slomo) 2014-12-10 00:52:01 PST
Comment on attachment 242916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242916&action=review

> Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:79
> +    if (gst_adapter_available(adapter) >= bytes) {

This could lead to accumulating more and more data until OOM if data is pulled slower from here than GStreamer produces. Maybe should just call gst_app_sink_pull_sample() from here to have appsink block in that case? That could still cause problems if appsink produces more than $bytes large buffers, so in the case when there is still data in the adapter you wouldn't pull.

Don't forget to set max-buffers on appsink to something then :)
Comment 54 Philippe Normand 2014-12-10 08:01:41 PST
I'm landing this first working version and plan to improve it following Sebastian's advice soon. I have a WiP version already but there's an issue with the audio sink.
Comment 55 Philippe Normand 2014-12-10 08:02:06 PST
Committed r177058: <http://trac.webkit.org/changeset/177058>