Bug 69834 - [GStreamer] WebAudio AudioFileReader implementation
Summary: [GStreamer] WebAudio AudioFileReader implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61355 69835
  Show dependency treegraph
 
Reported: 2011-10-11 06:32 PDT by Philippe Normand
Modified: 2012-01-17 01:04 PST (History)
9 users (show)

See Also:


Attachments
WebAudio GStreamer implementation: step 1 (41.62 KB, patch)
2011-10-11 06:52 PDT, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
WebAudio GStreamer implementation: step 1 (40.77 KB, patch)
2011-10-13 04:45 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
WebAudio GStreamer implementation: step 1 (40.73 KB, patch)
2011-10-13 06:03 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
WebAudio GStreamer implementation: step 1 (40.40 KB, patch)
2011-10-13 10:16 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
WebAudio GStreamer implementation: step 1 (38.27 KB, patch)
2011-10-14 01:23 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
WebAudio GStreamer implementation: step 1 (38.27 KB, patch)
2011-10-14 01:38 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
WebAudio GStreamer implementation: step 1 (38.52 KB, patch)
2011-10-20 06:32 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
WebAudio GStreamer implementation: step 1 (38.49 KB, patch)
2011-10-20 08:53 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2011-10-11 06:32:09 PDT
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.
Comment 1 Philippe Normand 2011-10-11 06:52:16 PDT
Created attachment 110510 [details]
WebAudio GStreamer implementation: step 1
Comment 2 Martin Robinson 2011-10-11 08:50:45 PDT
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.
Comment 3 Philippe Normand 2011-10-13 01:03:16 PDT
(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.
Comment 4 Philippe Normand 2011-10-13 04:45:49 PDT
Created attachment 110827 [details]
WebAudio GStreamer implementation: step 1
Comment 5 WebKit Review Bot 2011-10-13 04:48:48 PDT
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.
Comment 6 Philippe Normand 2011-10-13 06:03:04 PDT
Created attachment 110835 [details]
WebAudio GStreamer implementation: step 1
Comment 7 Philippe Normand 2011-10-13 06:35:24 PDT
(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.
Comment 8 Martin Robinson 2011-10-13 08:20:13 PDT
(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.
Comment 9 Martin Robinson 2011-10-13 08:21:26 PDT
(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.
Comment 10 Philippe Normand 2011-10-13 10:16:44 PDT
Created attachment 110867 [details]
WebAudio GStreamer implementation: step 1
Comment 11 Philippe Normand 2011-10-13 10:19:12 PDT
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 :)
Comment 12 WebKit Review Bot 2011-10-13 10:20:58 PDT
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 13 Martin Robinson 2011-10-13 14:34:19 PDT
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?
Comment 14 Philippe Normand 2011-10-14 01:23:53 PDT
Created attachment 110974 [details]
WebAudio GStreamer implementation: step 1
Comment 15 WebKit Review Bot 2011-10-14 01:27:12 PDT
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.
Comment 16 Philippe Normand 2011-10-14 01:30:18 PDT
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 :)
Comment 17 Philippe Normand 2011-10-14 01:38:42 PDT
Created attachment 110979 [details]
WebAudio GStreamer implementation: step 1
Comment 18 Philippe Normand 2011-10-14 01:39:17 PDT
(In reply to comment #17)
> Created an attachment (id=110979) [details]
> WebAudio GStreamer implementation: step 1

This one fixes the style-bot complains, hopefully!
Comment 19 Philippe Normand 2011-10-14 01:41:51 PDT
(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 20 Sebastian Dröge (slomo) 2011-10-14 02:09:22 PDT
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.
Comment 21 Zan Dobersek 2011-10-17 07:24:19 PDT
> 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.
Comment 22 Philippe Normand 2011-10-17 08:05:12 PDT
(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.
Comment 23 Philippe Normand 2011-10-20 06:32:52 PDT
Created attachment 111762 [details]
WebAudio GStreamer implementation: step 1
Comment 24 Philippe Normand 2011-10-20 06:34:26 PDT
(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.
Comment 25 WebKit Review Bot 2011-10-20 06:34:44 PDT
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.
Comment 26 Philippe Normand 2011-10-20 06:45:10 PDT
(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
Comment 27 Sebastian Dröge (slomo) 2011-10-20 07:11:25 PDT
Looks good IMHO
Comment 28 Philippe Normand 2011-10-20 07:13:31 PDT
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!
Comment 29 Philippe Normand 2011-10-20 08:53:39 PDT
Created attachment 111782 [details]
WebAudio GStreamer implementation: step 1
Comment 30 Philippe Normand 2011-10-26 10:42:40 PDT
Ping Martin? :)
Comment 31 Martin Robinson 2011-10-26 21:33:47 PDT
Comment on attachment 111782 [details]
WebAudio GStreamer implementation: step 1

Okay. This looks sane to me and the other reviews seem positive.
Comment 32 Philippe Normand 2011-10-27 05:31:57 PDT
Comment on attachment 111782 [details]
WebAudio GStreamer implementation: step 1

Clearing flags on attachment: 111782

Committed r98554: <http://trac.webkit.org/changeset/98554>
Comment 33 Philippe Normand 2011-10-27 05:32:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Dongwoo Joshua Im (dwim) 2012-01-16 22:16:06 PST
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()...?
Comment 35 Philippe Normand 2012-01-17 01:04:50 PST
(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 :)