Bug 69834 – [GStreamer] WebAudio AudioFileReader implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 110510 [details]
WebAudio GStreamer implementation: step 1

[Martin Robinson]

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.

[Philippe Normand]

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

[Philippe Normand]

Created attachment 110827 [details]
WebAudio GStreamer implementation: step 1

[WebKit Review Bot]

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.

[Philippe Normand]

Created attachment 110835 [details]
WebAudio GStreamer implementation: step 1

[Philippe Normand]

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

[Martin Robinson]

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

[Martin Robinson]

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

[Philippe Normand]

Created attachment 110867 [details]
WebAudio GStreamer implementation: step 1

[Philippe Normand]

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

[WebKit Review Bot]

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.

[Martin Robinson]

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?

[Philippe Normand]

Created attachment 110974 [details]
WebAudio GStreamer implementation: step 1

[WebKit Review Bot]

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.

[Philippe Normand]

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

[Philippe Normand]

Created attachment 110979 [details]
WebAudio GStreamer implementation: step 1

[Philippe Normand]

(In reply to comment #17)
> Created an attachment (id=110979) [details]
> WebAudio GStreamer implementation: step 1

This one fixes the style-bot complains, hopefully!

[Philippe Normand]

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

[Sebastian Dröge (slomo)]

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.

[Zan Dobersek]

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

[Philippe Normand]

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

[Philippe Normand]

Created attachment 111762 [details]
WebAudio GStreamer implementation: step 1

[Philippe Normand]

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

[WebKit Review Bot]

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.

[Philippe Normand]

(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

[Sebastian Dröge (slomo)]

Looks good IMHO

[Philippe Normand]

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!

[Philippe Normand]

Created attachment 111782 [details]
WebAudio GStreamer implementation: step 1

[Philippe Normand]

Ping Martin? :)

[Martin Robinson]

Comment on attachment 111782 [details]
WebAudio GStreamer implementation: step 1

Okay. This looks sane to me and the other reviews seem positive.

[Philippe Normand]

Comment on attachment 111782 [details]
WebAudio GStreamer implementation: step 1

Clearing flags on attachment: 111782

Committed r98554: <http://trac.webkit.org/changeset/98554>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.

[Dongwoo Joshua Im (dwim)]

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

[Philippe Normand]

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