Bug 78883

I started working on this... not working yet though. Will try to post a patch soon.

Created attachment 180746 [details] Test Case: Processing audio from Media Element Source

I'll upload a first version of the patch in the following days.

(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.

OTOH your test page works with WebKitGTK and my patch applied :P

Created attachment 180804 [details] AudioSourceProvider

Comment on attachment 180804 [details] AudioSourceProvider Attachment 180804 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15546531

Created attachment 180805 [details] AudioSourceProvider

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?

(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 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.

(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.

(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 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.

Created attachment 180935 [details] AudioSourceProvider

Comment on attachment 180935 [details] AudioSourceProvider Attachment 180935 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15594080

Created attachment 180956 [details] AudioSourceProvider

Comment on attachment 180956 [details] AudioSourceProvider Attachment 180956 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15599404

Created attachment 181320 [details] AudioSourceProvider

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

(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 :)

Created attachment 183025 [details] AudioSourceProvider

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 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.

(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.

(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

Created attachment 183612 [details] AudioSourceProvider

(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 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.

Created attachment 183670 [details] AudioSourceProvider

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.

(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.

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 :)

Created attachment 184861 [details] AudioSourceProvider

Created attachment 185994 [details] AudioSourceProvider Rebased against ToT. Removed an audioconvert ! audioresample before tee and added some comments about the ones needed downstream tee.

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.

(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?

(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 on attachment 185994 [details] AudioSourceProvider I haven't had time to rework this patch yet. Pulling out of review queue.

Created attachment 218582 [details] rebased patch, not for review yet

Created attachment 218586 [details] rebased patch that works

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/

(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 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

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 :)

Created attachment 242817 [details] patch

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.

Created attachment 242818 [details] patch

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

(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.

Created attachment 242916 [details] patch

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 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 :)

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.

Committed r177058: <http://trac.webkit.org/changeset/177058>