Bug 78883 - [GStreamer] AudioSourceProvider support in the MediaPlayer
: [GStreamer] AudioSourceProvider support in the MediaPlayer
Status: UNCONFIRMED
: WebKit
Platform
: 528+ (Nightly build)
: Other Linux
: P2 Normal
Assigned To:
:
:
: 105293
:
  Show dependency treegraph
 
Reported: 2012-02-17 01:07 PST by
Modified: 2013-12-09 02:50 PST (History)


Attachments
Test Case: Processing audio from Media Element Source (47 bytes, url)
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-
Review Patch | Details | Formatted Diff | Diff
AudioSourceProvider (29.93 KB, patch)
2012-12-27 08:22 PST, Philippe Normand
no flags Review Patch | Details | Formatted Diff | Diff
AudioSourceProvider (30.46 KB, patch)
2012-12-29 05:34 PST, Philippe Normand
eflews.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
AudioSourceProvider (30.78 KB, patch)
2012-12-30 09:05 PST, Philippe Normand
eflews.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
AudioSourceProvider (30.79 KB, patch)
2013-01-04 09:59 PST, Philippe Normand
no flags Review Patch | Details | Formatted Diff | Diff
AudioSourceProvider (31.99 KB, patch)
2013-01-16 13:11 PST, Philippe Normand
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
AudioSourceProvider (33.75 KB, patch)
2013-01-19 02:05 PST, Philippe Normand
no flags Review Patch | Details | Formatted Diff | Diff
AudioSourceProvider (33.57 KB, patch)
2013-01-20 06:04 PST, Philippe Normand
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
AudioSourceProvider (36.90 KB, patch)
2013-02-01 02:44 PST, Philippe Normand
no flags Review Patch | Details | Formatted Diff | Diff
rebased patch, not for review yet (31.47 KB, patch)
2013-12-06 02:36 PST, Philippe Normand
no flags Review Patch | Details | Formatted Diff | Diff
rebased patch that works (32.15 KB, patch)
2013-12-06 03:39 PST, Philippe Normand
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 2012-12-26 06:30:02 PST -------
Created an attachment (id=180746) [details]
Test Case: Processing audio from Media Element Source
------- Comment #3 From 2012-12-26 08:53:35 PST -------
I'll upload a first version of the patch in the following days.
------- Comment #4 From 2012-12-26 12:21:54 PST -------
(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.
------- Comment #5 From 2012-12-26 12:23:16 PST -------
OTOH your test page works with WebKitGTK and my patch applied :P
------- Comment #6 From 2012-12-27 08:11:44 PST -------
Created an attachment (id=180804) [details]
AudioSourceProvider
------- Comment #7 From 2012-12-27 08:17:22 PST -------
(From update of attachment 180804 [details])
Attachment 180804 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15546531
------- Comment #8 From 2012-12-27 08:22:41 PST -------
Created an attachment (id=180805) [details]
AudioSourceProvider
------- Comment #9 From 2012-12-27 09:58:51 PST -------
(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.

> 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 From 2012-12-27 10:34:25 PST -------
(In reply to comment #9)
> (From update of attachment 180805 [details] [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 From 2012-12-27 10:42:15 PST -------
(From update of attachment 180805 [details])
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 From 2012-12-27 17:05:55 PST -------
(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.
------- Comment #13 From 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] [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 From 2012-12-28 06:15:43 PST -------
(From update of attachment 180805 [details])
I'm adding proper multi-channel support to the provider, reworking the patch and hopefully avoiding the change to AudioContext.
------- Comment #15 From 2012-12-29 05:34:04 PST -------
Created an attachment (id=180935) [details]
AudioSourceProvider
------- Comment #16 From 2012-12-29 05:37:34 PST -------
(From update of attachment 180935 [details])
Attachment 180935 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15594080
------- Comment #17 From 2012-12-30 09:05:00 PST -------
Created an attachment (id=180956) [details]
AudioSourceProvider
------- Comment #18 From 2012-12-30 09:11:52 PST -------
(From update of attachment 180956 [details])
Attachment 180956 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15599404
------- Comment #19 From 2013-01-04 09:59:23 PST -------
Created an attachment (id=181320) [details]
AudioSourceProvider
------- Comment #20 From 2013-01-11 06:27:12 PST -------
(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

> 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 From 2013-01-14 12:14:50 PST -------
(In reply to comment #20)
> (From update of attachment 181320 [details] [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 From 2013-01-16 13:11:01 PST -------
Created an attachment (id=183025) [details]
AudioSourceProvider
------- Comment #23 From 2013-01-16 13:37:32 PST -------
(From update of attachment 183025 [details])
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 From 2013-01-18 09:36:28 PST -------
(From update of attachment 183025 [details])
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 From 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 From 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 From 2013-01-19 02:05:05 PST -------
Created an attachment (id=183612) [details]
AudioSourceProvider
------- Comment #28 From 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 From 2013-01-19 07:40:14 PST -------
(From update of attachment 183612 [details])
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 From 2013-01-20 06:04:30 PST -------
Created an attachment (id=183670) [details]
AudioSourceProvider
------- Comment #31 From 2013-01-24 18:37:15 PST -------
(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?

> 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 From 2013-01-26 04:26:44 PST -------
(In reply to comment #31)
> (From update of attachment 183670 [details] [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 From 2013-01-26 05:50:52 PST -------
Created an attachment (id=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 From 2013-01-26 06:12:42 PST -------
Created an attachment (id=184861) [details]
AudioSourceProvider
------- Comment #35 From 2013-02-01 02:44:50 PST -------
Created an attachment (id=185994) [details]
AudioSourceProvider

Rebased against ToT. Removed an audioconvert ! audioresample before
tee and added some comments about the ones needed downstream tee.
------- Comment #36 From 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 From 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 From 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 From 2013-02-28 00:15:27 PST -------
(From update of attachment 185994 [details])
I haven't had time to rework this patch yet. Pulling out of review queue.
------- Comment #40 From 2013-12-06 02:36:30 PST -------
Created an attachment (id=218582) [details]
rebased patch, not for review yet
------- Comment #41 From 2013-12-06 03:39:03 PST -------
Created an attachment (id=218586) [details]
rebased patch that works
------- Comment #42 From 2013-12-07 02:21:06 PST -------
(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?

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

s/TODO/FIXME/
------- Comment #43 From 2013-12-09 02:50:57 PST -------
(In reply to comment #42)
> (From update of attachment 218586 [details] [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!