A pipeline needs to be setup to decode memory chunks (using giostreamsrc) and files using filesrc. filesrc ! decodebin2 ! deinterleave ! queue ! appsink ! queue ! appsink Individual channel data has to be stored in float arrays and an AudioBus is created from this. For now we should support only one or 2 channels, like the Chromium implementation.
Created attachment 110510 [details] WebAudio GStreamer implementation: step 1
Comment on attachment 110510 [details] WebAudio GStreamer implementation: step 1 View in context: https://bugs.webkit.org/attachment.cgi?id=110510&action=review Nice. Below are some initial comments. I have not yet been able to look at the way you are setting up your pipeline in detail, but perhaps sometime later you can walk me through it. I have tried not to Martinize too intensely! > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:27 > +#include "AudioDestinationGStreamer.h" > + > +#include "AudioChannel.h" > +#include "AudioSourceProvider.h" > +#include "GOwnPtr.h" This should be one block of headers. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:38 > + return 44100.0; Omit the final .0 here. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:35 > +#include "AudioBus.h" > +#include "AudioFileReader.h" > +#include "GOwnPtr.h" > +#include "GRefPtr.h" > + > +#include <gio/gio.h> > +#include <gst/app/gstappsink.h> > +#include <gst/audio/multichannel.h> > +#include <gst/base/gstbytereader.h> > +#include <gst/pbutils/pbutils.h> Clump these together. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:37 > +#define WEBAUDIO_GST_CAPS_TEMPLATE "audio/x-raw-float, rate=(int)%d, channels=(int)%d, endianness=(int)%d, width=(int)32" String constants like this are typically defined as static const char*. Since it's only used in getGStreamerAudioCaps it would be better to have it in the body of that method too. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:43 > +#if G_BYTE_ORDER == G_LITTLE_ENDIAN > +#define _webaudio_gst_byte_reader_get_float gst_byte_reader_get_float32_le > +#else > +#define _webaudio_gst_byte_reader_get_float gst_byte_reader_get_float32_be > +#endif Instead of doing it this way, I prefer something like: static inline bool getFloatFromByteReader(GstByteReader *reader, float *val) { #if G_BYTE_ORDER == G_LITTLE_ENDIAN #else #endif } > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:56 > + gpointer buf = g_malloc0(size); > + float* current = (float*) buf; I think you can avoid this allocation (see below). If not, please use the WTF allocator here instead of the GLib one and a C++ cast. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:62 > + while (_webaudio_gst_byte_reader_get_float(byteReader, &value)) > + *current++ = (float) value; > + gst_byte_reader_free(byteReader); You shouldn't need to cast between float and gfloat. In fact, it might be better to use float here to begin with. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:69 > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > + reader->handleEndOfStream(); Might as well make this just one line. Alternatively, just accept an AudioFileReader as an argument. I'm almost certain you only need a static_cast here. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:75 > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > + return reader->handleBuffer(sink); Ditto. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:81 > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > + return reader->handleMessage(message); Ditto. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:88 > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > + reader->handleNewDeinterleavePad(pad); > +} Ditto. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:94 > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > + reader->deinterleavePadsConfigured(); > +} Ditto. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:99 > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > + reader->plugDeinterleave(pad); Ditto. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:105 > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > + reader->start(); Ditto. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:169 > + const GValue* value = gst_structure_get_value(structure, "channel-positions"); Might want to give this a more descrptive name like channelPositionsValue. I had to look back to figure out what it was. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:173 > + if (channels && value && gst_value_is_fixed(value)) { Is this an error situation? You don't return GST_FLOW_ERROR in the else case. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:180 > + mergedBuffer = gst_buffer_join(m_bufferFrontLeft, buffer); > + m_bufferFrontLeft = gst_buffer_ref(mergedBuffer); Why not just do m_bufferFrontLeft = gst_buffer_ref(gst_buffer_join(m_bufferFrontLeft, buffer)); and dispsense with mergedBuffer altogether? > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:215 > + gchar* padName = gst_pad_get_name(pad); I recommend a GOwnPtr here. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:233 > + gst_object_unref(GST_OBJECT(sinkPad)); Please use GRefPtr in these cases. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:252 > + GstElement* audioconvert = gst_element_factory_make("audioconvert", 0); > + GstElement* audioresample = gst_element_factory_make("audioresample", 0); > + GstElement* capsfilter = gst_element_factory_make("capsfilter", 0); > + GstElement* deinterleave = gst_element_factory_make("deinterleave", "deinterleave"); Please use camelCase for these names. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:258 > + g_object_set(capsfilter, "caps", getGStreamerAudioCaps(2, m_sampleRate), NULL); Does gst_caps_from_string return a newly-allocated GstCaps? If so, isn't this a memory leak? > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:313 > + GOwnPtr<GError> error; > + gstInitialized = gst_init_check(0, 0, &error.outPtr()); > + if (!gstInitialized) > + return nullptr; Will this interact nicely with the version of this in MediaPlayerPrivateGStreamer.cpp? Nice use of nullptr! > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:316 > + // properly dispatch its messages to our end. Nit: dispatches > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:319 > + m_context = g_main_context_new(); > + g_main_context_push_thread_default(m_context); > + m_loop = g_main_loop_new(m_context, FALSE); I guess this blocks the main loop. :( > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:327 > + g_source_set_callback(timeoutSource, (GSourceFunc) enteredMainLoopCallback, this, 0); Please use reinterpret_cast instead of a C style cast. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:333 > + guint size = GST_BUFFER_SIZE(m_bufferFrontLeft); We try not to use the simple Glib wrappers for C types that are local. Thus this can be unsigned. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:344 > + float* bufferLeft = getGStreamerAudioData(m_bufferFrontLeft, size); > + float* bufferRight = getGStreamerAudioData(m_bufferFrontRight, size); > + > + memcpy(audioBus->channel(0)->data(), bufferLeft, size); > + if (!mixToMono) > + memcpy(audioBus->channel(1)->data(), bufferRight, size); > + I think here you should pass (audioBus->channel(0)->data() directly to getGStreamerAudioData and avoid a memory copy and duplicate allocation entirely. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:360 > + AudioFileReader* reader = new AudioFileReader(filePath); > + return reader->createBus(sampleRate, mixToMono); Aren't you leaking the reader here? Why not just stack allocate it? > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:366 > + AudioFileReader* reader = new AudioFileReader(data, dataSize); > + return reader->createBus(sampleRate, mixToMono); Ditto. This factory seems to be unused currently... > Source/WebCore/platform/audio/gtk/AudioBusGtk.cpp:37 > + return createBusFromAudioFile(absoluteFilename.get(), false, sampleRate); You only use the createBusFromAudioFile in one place and it's just two lines. I think it's clearer to just put the code here, unless you plan to use it a lot in followup patches.
(In reply to comment #2) > (From update of attachment 110510 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110510&action=review > > Nice. Below are some initial comments. I have not yet been able to look at the way you are setting up your pipeline in detail, but perhaps sometime later you can walk me through it. I have tried not to Martinize too intensely! > > > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:27 > > +#include "AudioDestinationGStreamer.h" > > + > > +#include "AudioChannel.h" > > +#include "AudioSourceProvider.h" > > +#include "GOwnPtr.h" > > This should be one block of headers. > Hum. nop! If I do that the style-bot complains: Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:23: You should add a blank line after implementation file's own header. [build/include_order] [4] > > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:38 > > + return 44100.0; > > Omit the final .0 here. > Ok > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:35 > > +#include "AudioBus.h" > > +#include "AudioFileReader.h" > > +#include "GOwnPtr.h" > > +#include "GRefPtr.h" > > + > > +#include <gio/gio.h> > > +#include <gst/app/gstappsink.h> > > +#include <gst/audio/multichannel.h> > > +#include <gst/base/gstbytereader.h> > > +#include <gst/pbutils/pbutils.h> > > Clump these together. > Ok > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:37 > > +#define WEBAUDIO_GST_CAPS_TEMPLATE "audio/x-raw-float, rate=(int)%d, channels=(int)%d, endianness=(int)%d, width=(int)32" > > String constants like this are typically defined as static const char*. Since it's only used in getGStreamerAudioCaps it would be better to have it in the body of that method too. > Ok! > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:43 > > +#if G_BYTE_ORDER == G_LITTLE_ENDIAN > > +#define _webaudio_gst_byte_reader_get_float gst_byte_reader_get_float32_le > > +#else > > +#define _webaudio_gst_byte_reader_get_float gst_byte_reader_get_float32_be > > +#endif > > Instead of doing it this way, I prefer something like: > > static inline bool getFloatFromByteReader(GstByteReader *reader, float *val) > { > #if G_BYTE_ORDER == G_LITTLE_ENDIAN > > #else > > #endif > } > Looks cleaner indeed :) > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:56 > > + gpointer buf = g_malloc0(size); > > + float* current = (float*) buf; > > I think you can avoid this allocation (see below). If not, please use the WTF allocator here instead of the GLib one and a C++ cast. > Oh yes! Seems indeed the allocation can be avoided. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:62 > > + while (_webaudio_gst_byte_reader_get_float(byteReader, &value)) > > + *current++ = (float) value; > > + gst_byte_reader_free(byteReader); > > You shouldn't need to cast between float and gfloat. In fact, it might be better to use float here to begin with. > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:69 > > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > > + reader->handleEndOfStream(); > > Might as well make this just one line. Alternatively, just accept an AudioFileReader as an argument. I'm almost certain you only need a static_cast here. > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:75 > > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > > + return reader->handleBuffer(sink); > > Ditto. > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:81 > > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > > + return reader->handleMessage(message); > > Ditto. > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:88 > > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > > + reader->handleNewDeinterleavePad(pad); > > +} > > Ditto. > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:94 > > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > > + reader->deinterleavePadsConfigured(); > > +} > > Ditto. > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:99 > > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > > + reader->plugDeinterleave(pad); > > Ditto. > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:105 > > + AudioFileReader* reader = reinterpret_cast<AudioFileReader*>(userData); > > + reader->start(); > > Ditto. > Ok > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:169 > > + const GValue* value = gst_structure_get_value(structure, "channel-positions"); > > Might want to give this a more descrptive name like channelPositionsValue. I had to look back to figure out what it was. > Yep :) > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:173 > > + if (channels && value && gst_value_is_fixed(value)) { > > Is this an error situation? You don't return GST_FLOW_ERROR in the else case. > Hum, you're right indeed, the else case should be there. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:180 > > + mergedBuffer = gst_buffer_join(m_bufferFrontLeft, buffer); > > + m_bufferFrontLeft = gst_buffer_ref(mergedBuffer); > > Why not just do m_bufferFrontLeft = gst_buffer_ref(gst_buffer_join(m_bufferFrontLeft, buffer)); and dispsense with mergedBuffer altogether? > Yeah, why not ;) > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:215 > > + gchar* padName = gst_pad_get_name(pad); > > I recommend a GOwnPtr here. > Ok > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:233 > > + gst_object_unref(GST_OBJECT(sinkPad)); > > Please use GRefPtr in these cases. > Will do. I actually have a patch somewhere for GRefPtrGStreamer about this, I'll send a separate patch to uniformize the GstPad usage in the graphics/gstreamer code. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:252 > > + GstElement* audioconvert = gst_element_factory_make("audioconvert", 0); > > + GstElement* audioresample = gst_element_factory_make("audioresample", 0); > > + GstElement* capsfilter = gst_element_factory_make("capsfilter", 0); > > + GstElement* deinterleave = gst_element_factory_make("deinterleave", "deinterleave"); > > Please use camelCase for these names. > Hum ok, this is not what I use to do in other GStreamer code though. Usually the convention is that the variable name matches the element name when possible. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:258 > > + g_object_set(capsfilter, "caps", getGStreamerAudioCaps(2, m_sampleRate), NULL); > > Does gst_caps_from_string return a newly-allocated GstCaps? If so, isn't this a memory leak? > Heh. Right, the caps needs to be unref'd. Same in handleNewDeinterleavePad(). > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:313 > > + GOwnPtr<GError> error; > > + gstInitialized = gst_init_check(0, 0, &error.outPtr()); > > + if (!gstInitialized) > > + return nullptr; > > Will this interact nicely with the version of this in MediaPlayerPrivateGStreamer.cpp? Nice use of nullptr! > I believe so! I'll do some tests about this in a page containing media elements and WebAudio code. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:316 > > + // properly dispatch its messages to our end. > > Nit: dispatches > Ok > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:319 > > + m_context = g_main_context_new(); > > + g_main_context_push_thread_default(m_context); > > + m_loop = g_main_loop_new(m_context, FALSE); > > I guess this blocks the main loop. :( > Well yeah indeed, the createBus needs to block, not much I can do about this I'm afraid. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:327 > > + g_source_set_callback(timeoutSource, (GSourceFunc) enteredMainLoopCallback, this, 0); > > Please use reinterpret_cast instead of a C style cast. > Oh, right. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:333 > > + guint size = GST_BUFFER_SIZE(m_bufferFrontLeft); > > We try not to use the simple Glib wrappers for C types that are local. Thus this can be unsigned. > Ok. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:344 > > + float* bufferLeft = getGStreamerAudioData(m_bufferFrontLeft, size); > > + float* bufferRight = getGStreamerAudioData(m_bufferFrontRight, size); > > + > > + memcpy(audioBus->channel(0)->data(), bufferLeft, size); > > + if (!mixToMono) > > + memcpy(audioBus->channel(1)->data(), bufferRight, size); > > + > > I think here you should pass (audioBus->channel(0)->data() directly to getGStreamerAudioData and avoid a memory copy and duplicate allocation entirely. > Very good point indeed! This should boost the performance a bit :) > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:360 > > + AudioFileReader* reader = new AudioFileReader(filePath); > > + return reader->createBus(sampleRate, mixToMono); > > Aren't you leaking the reader here? Why not just stack allocate it? > Yeah, good point. > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:366 > > + AudioFileReader* reader = new AudioFileReader(data, dataSize); > > + return reader->createBus(sampleRate, mixToMono); > > Ditto. This factory seems to be unused currently... > Well createBusFromAudioFile is used in HRTFElevation at least. createBusFromInMemoryAudioFile is called in the AudioBuffer code. > > Source/WebCore/platform/audio/gtk/AudioBusGtk.cpp:37 > > + return createBusFromAudioFile(absoluteFilename.get(), false, sampleRate); > > You only use the createBusFromAudioFile in one place and it's just two lines. I think it's clearer to just put the code here, unless you plan to use it a lot in followup patches. I have a followup patch about this yeah. Anyway createBusFromAudioFile is defined in AudioFileReader.h so we need to implement it. Moreover I wanted to split the "Gtk" part from the GStreamer code. Later on other ports using this WebAudio implementation will most likely only need a port specific AudioBus implementation, apart from the websettings and DRT stuff.
Created attachment 110827 [details] WebAudio GStreamer implementation: step 1
Attachment 110827 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1 Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:23: You should add a blank line after implementation file's own header. [build/include_order] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 110835 [details] WebAudio GStreamer implementation: step 1
(In reply to comment #2) > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:319 > > + m_context = g_main_context_new(); > > + g_main_context_push_thread_default(m_context); > > + m_loop = g_main_loop_new(m_context, FALSE); > > I guess this blocks the main loop. :( > This issue is quite annoying, I'll give it some more thinking.
(In reply to comment #7) > (In reply to comment #2) > > > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:319 > > > + m_context = g_main_context_new(); > > > + g_main_context_push_thread_default(m_context); > > > + m_loop = g_main_loop_new(m_context, FALSE); > > > > I guess this blocks the main loop. :( > > > > This issue is quite annoying, I'll give it some more thinking. I'm surprised the platform-independent code isn't loading this on another thread and making sounds only after it's loaded.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 110510 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=110510&action=review > > > > Nice. Below are some initial comments. I have not yet been able to look at the way you are setting up your pipeline in detail, but perhaps sometime later you can walk me through it. I have tried not to Martinize too intensely! > > > > > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:27 > > > +#include "AudioDestinationGStreamer.h" > > > + > > > +#include "AudioChannel.h" > > > +#include "AudioSourceProvider.h" > > > +#include "GOwnPtr.h" > > > > This should be one block of headers. Apologies. I misread that section.
Created attachment 110867 [details] WebAudio GStreamer implementation: step 1
Changes: - killed some leaks found by Martin and Zan - don't block the main loop anymore - added comments about how the pipeline is built, hopefully it's now easier to understand. I can now run the drum-machine demo :)
Attachment 110867 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1 Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:302: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 110867 [details] WebAudio GStreamer implementation: step 1 View in context: https://bugs.webkit.org/attachment.cgi?id=110867&action=review I'm not sure I understand why you export the AudioFileReader class in AudioFileReaderGStreamer.h. Will you be using from another class in a later patch? if not, might be better to simply stick the class definition in AudioFileReaderGstreamer.cpp and delete AudioFileReaderGStreamer.h. Looking good in general. I'm tempted to r+ this, but I'll let you respond to my latest comments. Perhaps someone with more GStreamer experience can take a peak too. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:138 > + if (channels && channelPositionsValue && gst_value_is_fixed(channelPositionsValue)) { > + GstAudioChannelPosition* positions = gst_audio_get_channel_positions(structure); It'd be better to ues an early return here for the error case. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:164 > + GOwnPtr<GError> err; err should be called "error" > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:165 > + GOwnPtr<gchar> debug; debug is unused, so you can just pass 0 (null) to gst_message_parse_warning as the last argument. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:259 > +static bool gstInitialized = false; This variable should be declared locally to method it's used in. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:268 > + gstInitialized = gst_init_check(0, 0, &error.outPtr()); > + if (!gstInitialized) > + return nullptr; You never check the value of gstInitialized before running gst_init_check. What's the point of making it static? >> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:302 > > One line control clauses should not use braces. [whitespace/braces] [4] Please fix. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.h:34 > +PassOwnPtr<AudioBus> createBusFromAudioFile(const char* filePath, bool mixToMono, float sampleRate); > +PassOwnPtr<AudioBus> createBusFromInMemoryAudioFile(const void* data, size_t dataSize, bool mixToMono, float sampleRate); If you don't delete this header, I think it would be better to simply include AudioFileReader.h, instead of redclaring these factories. It's weird to have the declarations in two places. > Source/WebCore/platform/audio/gtk/AudioBusGtk.cpp:29 > +#include "AudioFileReaderGStreamer.h" > +#include "GOwnPtr.h" > + > +#include <gio/gio.h> > +#include <glib.h> These can be one clump. Wouldn't it make more sense to just include AudioFileReader.h?
Created attachment 110974 [details] WebAudio GStreamer implementation: step 1
Attachment 110974 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1 Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:25: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:30: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Hi Sebastian, Would you mind having a look at this patch? It's the first of a series dedicated to a GStreamer WebAudio implementation! See also bug 61355. Martin and Zan already helped quite a lot on the iterations of this patch, it's almost ready to land and it'd be great to have your opinion as well :)
Created attachment 110979 [details] WebAudio GStreamer implementation: step 1
(In reply to comment #17) > Created an attachment (id=110979) [details] > WebAudio GStreamer implementation: step 1 This one fixes the style-bot complains, hopefully!
(In reply to comment #13) > (From update of attachment 110867 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110867&action=review > > I'm not sure I understand why you export the AudioFileReader class in AudioFileReaderGStreamer.h. Will you be using from another class in a later patch? if not, might be better to simply stick the class definition in AudioFileReaderGstreamer.cpp and delete AudioFileReaderGStreamer.h. > Indeed no need to export that class! Thanks for the reviews Martin, I uploaded a new patch taking in account your latest feedback :)
Comment on attachment 110979 [details] WebAudio GStreamer implementation: step 1 View in context: https://bugs.webkit.org/attachment.cgi?id=110979&action=review > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:73 > + GOwnPtr<gchar> capsDescription(g_strdup_printf(capsTemplate, static_cast<int>(sampleRate), channels, G_BYTE_ORDER)); You can use BYTE_ORDER directly in the caps template string or even easier: return gst_caps_new_simple ("audio/x-raw-float", "rate", G_TYPE_INT, (int) sampleRate, "channels", G_TYPE_INT, channels, "endianness", G_TYPE_INT, G_BYTE_ORDER, "width", G_TYPE_INT, 32, NULL) > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:88 > + GstByteReader* byteReader = gst_byte_reader_new_from_buffer(buffer); GstByteReader byteReader = GST_BYTE_READER_INIT_FROM_BUFFER(buffer) No need to free it later then, keeps memory fragmentation smaller, etc. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:92 > + *audioChannel++ = value; You can simple memcpy() here. Just cast GST_BUFFER_DATA() to a float*. It already has correct alignment and endianness and everything. Also, how do you know that audioChannel is large enough for all samples inside the buffer? > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:162 > + if (!channels || !channelPositionsValue || !gst_value_is_fixed(channelPositionsValue)) { No need to check for !fixed here. That can never ever happen > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:174 > + m_bufferFrontRight = gst_buffer_join(m_bufferFrontRight, buffer); Instead of joining the buffers here you might want to keep a list of buffers and copy from them in copyGstreamerBufferToAudioChannel(). That keeps the copying, reallocations, etc lower. Just make sure to get an additional ref of the buffer. > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:216 > + GstElement* sink = gst_element_factory_make("appsink", 0); You probably want to set "sync" to FALSE on appsink. Otherwise it will only give you buffers in real time, not as fast as it can.
> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:201 > + ASSERT_WITH_MESSAGE("Fatal error: %d, %s", error->code, error->message); This assert is incorrectly utilized, causing build failures in debug build. You probably had a zero as the first argument in mind.
(In reply to comment #21) > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:201 > > + ASSERT_WITH_MESSAGE("Fatal error: %d, %s", error->code, error->message); > > This assert is incorrectly utilized, causing build failures in debug build. You probably had a zero as the first argument in mind. Right, I fixed it locally.
Created attachment 111762 [details] WebAudio GStreamer implementation: step 1
(In reply to comment #23) > Created an attachment (id=111762) [details] > WebAudio GStreamer implementation: step 1 Sebastian, do you mind having another look? I think I managed to apply all the things you asked :) The style-bot will complain about NULL and gst_caps_new_simple... will file a bug for this.
Attachment 111762 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1 Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:74: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #25) > Attachment 111762 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1 > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:74: Use 0 instead of NULL. [readability/null] [5] > Total errors found: 1 in 13 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. See bug 70498
Looks good IMHO
Comment on attachment 111762 [details] WebAudio GStreamer implementation: step 1 View in context: https://bugs.webkit.org/attachment.cgi?id=111762&action=review > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:156 > + gst_buffer_list_iterator_add(m_frontLeftBuffersIterator, gst_buffer_ref(buffer)); oops this gst_buffer_ref call should not be!
Created attachment 111782 [details] WebAudio GStreamer implementation: step 1
Ping Martin? :)
Comment on attachment 111782 [details] WebAudio GStreamer implementation: step 1 Okay. This looks sane to me and the other reviews seem positive.
Comment on attachment 111782 [details] WebAudio GStreamer implementation: step 1 Clearing flags on attachment: 111782 Committed r98554: <http://trac.webkit.org/changeset/98554>
All reviewed patches have been landed. Closing bug.
View in context: https://bugs.webkit.org/attachment.cgi?id=111782&action=review > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:319 > + g_main_context_iteration(0, FALSE); While I'm reading your patches (and conversation history with mrobinson), it came to me a question why you have not give a separate/dedicated context to "g_main_context_iteration". AFAIK, this will pop out a task from the work queue of the main thread if you give '0' to this API. Wouldn't it be clearer to create a new context (using g_main_context_new()) and pass the newly created context to the first argument of g_main_context_iteration()...?
(In reply to comment #34) > View in context: https://bugs.webkit.org/attachment.cgi?id=111782&action=review > > > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:319 > > + g_main_context_iteration(0, FALSE); > > While I'm reading your patches (and conversation history with mrobinson), it came to me a question why you have not give a separate/dedicated context to "g_main_context_iteration". AFAIK, this will pop out a task from the work queue of the main thread if you give '0' to this API. > > Wouldn't it be clearer to create a new context (using g_main_context_new()) and pass the newly created context to the first argument of g_main_context_iteration()...? I tried that at some point but it wasn't working well, I don't remember exactly what were the issues but if you can come up with a patch (in a new bugzilla entry) that uses a different context and works well, we can consider it :)