Bug 69835 – [GStreamer] WebAudio AudioDestination

• Component: Web Audio
• Status: RESOLVED FIXED

[Philippe Normand]

This is for playback! We need to get floating point data from WebCore and feed it to a pipeline like this:

webkitwebaudiosrc ! wavparse ! audioconvert ! autoaudiosink

webkitwebaudiosrc is a bin containing:

queue ! capsfilter ! audioconvert -\
...                                  interleave -- wavenc -- srcpad

queue ! capsfilter ! audioconvert -/

[Philippe Normand]

Created attachment 112685 [details]
WebAudio GStreamer implementation: playback support

[WebKit Review Bot]

Attachment 112685 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:114:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:156:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:47:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Hi Sebastian, can you please have a look at this patch?

[Philippe Normand]

(In reply to comment #3)
> Hi Sebastian, can you please have a look at this patch?

From IRC:

<slomo_> philn-tp: you should check if wavparse and the other elements are available (gst_element_factory_create() returns non-NULL)

15:39:02 <slomo_> philn-tp: you should check if wavparse and the other elements are available (gst_element_factory_create() returns non-NULL)
15:40:14 <philn-tp> ah, right, the ones not in -core and -base at least
15:40:48 <slomo_> philn-tp: center and mono are not the same
15:41:42 <philn-tp> yeah, i know, go tell that to Chris :)
15:42:45 <philn-tp> slomo_: it's in AudioBus.h, line 48
15:43:54 <philn-tp> slomo_: i'll file a bug about that
15:44:03 <slomo_> philn-tp: why is this using a separate task for the srcpad?
15:45:20 <slomo_> philn-tp: other than that this looks good
15:45:37 <philn-tp> slomo_: hum, not sure to understand your question ;)
15:47:19 <slomo_> philn-tp: i don't understand why there is a GstTask :)
15:49:03 <philn-tp> slomo_: well I need to pull the data from AudioBus when needed, would you suggest some other way than a task?
15:50:14 <slomo_> philn-tp: oh i see... no, that's fine :)

[Philippe Normand]

Created attachment 113874 [details]
WebAudio GStreamer implementation: playback support

Added in this version, checks for missing autoaudiosink, wavenc, wavparse and
interleave elements, as suggested by Sebastian.

[WebKit Review Bot]

Attachment 113874 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:92:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:117:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:159:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:47:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

(In reply to comment #5)
> Created an attachment (id=113874) [details]
> WebAudio GStreamer implementation: playback support
> 
> Added in this version, checks for missing autoaudiosink, wavenc, wavparse and
> interleave elements, as suggested by Sebastian.

There's still an issue with audio latency, I'm looking at it now, it's quite noticeable especially in that demo: http://www.iquilezles.org/apps/soundtoy/index.html

[Philippe Normand]

Created attachment 114022 [details]
WebAudio GStreamer implementation: playback support

Turns out the default queue max-buffer-size property set to 200
buffers was triggering this latency issue. I think it's safe to set it
to... 2 for instance. I get much better results here.

[WebKit Review Bot]

Attachment 114022 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:92:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:117:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:159:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:47:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 114025 [details]
WebAudio GStreamer implementation: playback support

One buffer limit is also working, even better!

[WebKit Review Bot]

Attachment 114025 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:92:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:117:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:159:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:47:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 114025 [details]
WebAudio GStreamer implementation: playback support

View in context: https://bugs.webkit.org/attachment.cgi?id=114025&action=review

Just a lot of nitpicky stuff below. I'll try to get a moment later to get a deeper understanding of this patch. Since GStreamer experts have already looked at it though, I'm not too worried.

> Source/WebCore/ChangeLog:31
> +        * GNUmakefile.list.am:
> +        * platform/audio/gstreamer/AudioDestinationGStreamer.cpp:
> +        (WebCore::onGStreamerWavparsePadAddedCallback):
> +        (WebCore::AudioDestinationGStreamer::AudioDestinationGStreamer):
> +        (WebCore::AudioDestinationGStreamer::~AudioDestinationGStreamer):
> +        (WebCore::AudioDestinationGStreamer::plugSink):
> +        (WebCore::AudioDestinationGStreamer::start):
> +        (WebCore::AudioDestinationGStreamer::stop):
> +        * platform/audio/gstreamer/AudioDestinationGStreamer.h:
> +        * platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: Added.
> +        (getGStreamerMonoAudioCaps):
> +        (webKitWebAudioGStreamerChannelPosition):
> +        (webkit_web_audio_src_class_init):
> +        (webkit_web_audio_src_init):
> +        (webKitWebAudioSrcConstructed):
> +        (webKitWebAudioSrcFinalize):
> +        (webKitWebAudioSrcSetProperty):
> +        (webKitWebAudioSrcGetProperty):
> +        (webKitWebAudioSrcLoop):
> +        (webKitWebAudioSrcChangeState):
> +        * platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h: Added.

Please fill in this information.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:31
> +#include "WebKitWebAudioSourceGStreamer.h"
> +
> +#include <gst/gst.h>
> +#include <gst/pbutils/pbutils.h>

No newline here.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:58
> +    static bool gstInitialized = false;
> +    if (!gstInitialized)
> +        gstInitialized = gst_init_check(0, 0, 0);

This code seems to be the same as the block in AudioFileReader::createBus. Is there any way to abstract it to a shared function somewhere?

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:63
> +    GstElement* webkitAudioSrc = reinterpret_cast<GstElement*>(g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC, "rate", static_cast<gfloat>(sampleRate), "bus", static_cast<gpointer>(&m_renderBus), "provider", &m_provider, NULL));

I think it would be good to break up this line in the way we often break up g_object_new and g_object_set calls. You don't need to cast float to gfloat nor do I think you need to cast &m_renderBus to gpointer. gfloat is just a thin veil over float and gpointer is just a thin veil over void*.  In general, pointers to data types never need to be cast to void *, unless it's a const pointer, in which case you should use const_cast.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:65
> +    GstElement* wavparse = gst_element_factory_make("wavparse", 0);

wavParser?

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:114
> +    GstPad* sinkPad = gst_element_get_static_pad(audioconvert, "sink");
> +    gst_pad_link(pad, sinkPad);
> +    gst_object_unref(GST_OBJECT(sinkPad));

RefPtr here?

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:126
> +    ASSERT(m_wavParserAvailable);
> +    if (!m_wavParserAvailable)
> +        return;

Could the wav parser not be avilable for a valid reason (missing codec,  for instance)? If that's the case I don't think an assert is the right thing here. That will cause a debug build to crash immediately, of course. That shouldn't happen in a valid error condition.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:30
> +#include "GOwnPtr.h"
> +
> +#include <gst/audio/multichannel.h>
> +#include <gst/pbutils/pbutils.h>

No blank line here.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:34
> +#define WEBKIT_WEB_AUDIO_SRC_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrcPrivate))

Ideally the private data would be allocated via the WTF allocator and not via the GLib allocator.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:42
> +struct _WebKitWebAudioSrcPrivate {

_WebKitWebAudioSrcPrivate -> _WebKitWebAudioSourcePrivate

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:47
> +    guint64 offset; // current buffer offset.

Instead of making a comment why not just call the member current_buffer_offset?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:50
> +    GstElement* wavenc;

wavEncoder?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:55
> +    GSList* pads; // list of queue src pads. One queue for each planar audio channel.

'list' should be capitalized.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:56
> +    GstPad* srcpad; // src pad of the element, interleaved wav data is pushed to it.

sourcePad?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:71
> +static void webKitWebAudioSrcSetProperty(GObject*, guint, const GValue*, GParamSpec*);
> +static void webKitWebAudioSrcGetProperty(GObject*, guint, GValue*, GParamSpec*);

The guint parameters should have names.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:77
> +    return gst_caps_new_simple("audio/x-raw-float", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, 1, "endianness", G_TYPE_INT, G_BYTE_ORDER, "width", G_TYPE_INT, 32, NULL);

I think it will make this easier to read if you break it up.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:123
> +    gst_element_class_add_pad_template(eklass,
> +                                       gst_static_pad_template_get(&srcTemplate));

This can be one line.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:129
> +                                         (gchar*) "WebKit WebAudio source element",
> +                                         (gchar*) "Source",
> +                                         (gchar*) "Handles WebAudio data from WebCore",
> +                                         (gchar*) "Philippe Normand <[e-mail]>");
> +

You don't need to cast const char* to char*.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:142
> +                                                       (GParamFlags) (G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE)));

Please use static_cast here. In the API layer we typically save this to a temporary to avoid all the chatter.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:175
> +    priv->task = gst_task_create((GstTaskFunction) webKitWebAudioSrcLoop, src);

Please use a C++ style cast here.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:179
> +static void webKitWebAudioSrcConstructed(GObject *object)

The asterisk is in the wrong place here.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:217
> +        GstCaps* monoCaps = getGStreamerMonoAudioCaps(priv->sampleRate);
> +        g_object_set(capsfilter, "caps", monoCaps, NULL);
> +        gst_caps_unref(monoCaps);
> +

monoCaps should be a smart pointer.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:219
> +        g_object_set(queue, "max-size-buffers", static_cast<guint>(1), NULL);

You should not need to cast 1 to a uint.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:251
> +    if (priv->interleave)
> +        gst_object_unref(GST_OBJECT(priv->interleave));
> +    if (priv->wavenc)
> +        gst_object_unref(GST_OBJECT(priv->wavenc));
> +

If your private data structure was a C++ object you could use a the destructor and smart pointers could take care of most of this cleanup. We use this pattern quite a bit in the API layer.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:257
> +    GST_CALL_PARENT(G_OBJECT_CLASS, finalize, ((GObject* )(src)));

G_OBJECT(src) here?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:265
> +    switch (propID) {

Nit: propID = propId

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:270
> +        priv->bus = reinterpret_cast<AudioBus*>(g_value_get_pointer(value));

You only need a staic cast here.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:273
> +        priv->provider = reinterpret_cast<AudioSourceProvider*>(g_value_get_pointer(value));

Ditto.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:281
> +static void webKitWebAudioSrcGetProperty(GObject* object, guint propID, GValue* value, GParamSpec* pspec)

propID -> propId.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:311
> +    size_t requestedFrames = 128;

This should probably be static with a comment explaining why you chose 128 here.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:312
> +    guint bufferSize = requestedFrames * sizeof(float);

Please use unsigned instead of guint.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:315
> +    for (guint index = 0; index < g_slist_length(priv->pads); index++) {

Ditto.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:324
> +        GstCaps* monoCaps = getGStreamerMonoAudioCaps(priv->sampleRate);

This should be  asmart pointer.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:340
> +    GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;

ret -> returnValue

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:347
> +            gst_element_post_message(element,
> +                                     gst_missing_element_message_new(element, "interleave"));

This can be one line

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:348
> +            GST_ELEMENT_ERROR(src, CORE, MISSING_PLUGIN, (0), ("no interleave"));

Why the parenthesis around 0?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:354
> +            gst_element_post_message(element,
> +                                     gst_missing_element_message_new(element, "wavenc"));
> +            GST_ELEMENT_ERROR(src, CORE, MISSING_PLUGIN, (0), ("no wavenc"));

Dittos.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:363
> +    if (G_UNLIKELY(ret == GST_STATE_CHANGE_FAILURE)) {

There's a WTF version of LIKELY and UNLIKELY. I think we should use those in WebCore code.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:25
> +G_BEGIN_DECLS

Is this public API? G_BEGIN_DECLS ensures that these symbols are unmangled so that they can be called from a foriegn C compilation unit. Is that necessary? A lot of this header looks like stuff you can omit actually. A lot of it can probably be private to the implementation file and the dead code (uncalled) should probably be removed. I think we should make this change for all private GObjects. Basically just trim it down to what you need in some other file.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:40
> +    WebKitWebAudioSrcPrivate *priv;

The asterisk is in the wrong place here.

[Philippe Normand]

(In reply to comment #12)
> (From update of attachment 114025 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114025&action=review
> 
> Just a lot of nitpicky stuff below. I'll try to get a moment later to get a deeper understanding of this patch. Since GStreamer experts have already looked at it though, I'm not too worried.
> 
> > Source/WebCore/ChangeLog:31
> > +        * GNUmakefile.list.am:
> > +        * platform/audio/gstreamer/AudioDestinationGStreamer.cpp:
> > +        (WebCore::onGStreamerWavparsePadAddedCallback):
> > +        (WebCore::AudioDestinationGStreamer::AudioDestinationGStreamer):
> > +        (WebCore::AudioDestinationGStreamer::~AudioDestinationGStreamer):
> > +        (WebCore::AudioDestinationGStreamer::plugSink):
> > +        (WebCore::AudioDestinationGStreamer::start):
> > +        (WebCore::AudioDestinationGStreamer::stop):
> > +        * platform/audio/gstreamer/AudioDestinationGStreamer.h:
> > +        * platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp: Added.
> > +        (getGStreamerMonoAudioCaps):
> > +        (webKitWebAudioGStreamerChannelPosition):
> > +        (webkit_web_audio_src_class_init):
> > +        (webkit_web_audio_src_init):
> > +        (webKitWebAudioSrcConstructed):
> > +        (webKitWebAudioSrcFinalize):
> > +        (webKitWebAudioSrcSetProperty):
> > +        (webKitWebAudioSrcGetProperty):
> > +        (webKitWebAudioSrcLoop):
> > +        (webKitWebAudioSrcChangeState):
> > +        * platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h: Added.
> 
> Please fill in this information.
> 

Done

> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:31
> > +#include "WebKitWebAudioSourceGStreamer.h"
> > +
> > +#include <gst/gst.h>
> > +#include <gst/pbutils/pbutils.h>
> 
> No newline here.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:58
> > +    static bool gstInitialized = false;
> > +    if (!gstInitialized)
> > +        gstInitialized = gst_init_check(0, 0, 0);
> 
> This code seems to be the same as the block in AudioFileReader::createBus. Is there any way to abstract it to a shared function somewhere?
> 

Hum. The issue I see with that is that AudioDestination and the FileReader can run in different threads, we really need at least two calls of gst_init_check on each side and avoid additional ones, not sure how to implement that in a separate function.

> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:63
> > +    GstElement* webkitAudioSrc = reinterpret_cast<GstElement*>(g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC, "rate", static_cast<gfloat>(sampleRate), "bus", static_cast<gpointer>(&m_renderBus), "provider", &m_provider, NULL));
> 
> I think it would be good to break up this line in the way we often break up g_object_new and g_object_set calls. You don't need to cast float to gfloat nor do I think you need to cast &m_renderBus to gpointer. gfloat is just a thin veil over float and gpointer is just a thin veil over void*.  In general, pointers to data types never need to be cast to void *, unless it's a const pointer, in which case you should use const_cast.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:65
> > +    GstElement* wavparse = gst_element_factory_make("wavparse", 0);
> 
> wavParser?
> 

Sure!

> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:114
> > +    GstPad* sinkPad = gst_element_get_static_pad(audioconvert, "sink");
> > +    gst_pad_link(pad, sinkPad);
> > +    gst_object_unref(GST_OBJECT(sinkPad));
> 
> RefPtr here?
> 

Sure, I'll send a separate patch for GstPad support in GRefPtrGStreamer though.

> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:126
> > +    ASSERT(m_wavParserAvailable);
> > +    if (!m_wavParserAvailable)
> > +        return;
> 
> Could the wav parser not be avilable for a valid reason (missing codec,  for instance)? If that's the case I don't think an assert is the right thing here. That will cause a debug build to crash immediately, of course. That shouldn't happen in a valid error condition.
> 

wavparse is in gst-plugins-good which is a runtime dependency. Not having -good will make this useless anyway. I thought asserting was the way to go in this case but I'm ok with removing it :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:30
> > +#include "GOwnPtr.h"
> > +
> > +#include <gst/audio/multichannel.h>
> > +#include <gst/pbutils/pbutils.h>
> 
> No blank line here.
> 

Sure!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:34
> > +#define WEBKIT_WEB_AUDIO_SRC_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrcPrivate))
> 
> Ideally the private data would be allocated via the WTF allocator and not via the GLib allocator.

Hum, ok. Trying this with help from Carlos.

> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:42
> > +struct _WebKitWebAudioSrcPrivate {
> 
> _WebKitWebAudioSrcPrivate -> _WebKitWebAudioSourcePrivate
> 

OK.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:47
> > +    guint64 offset; // current buffer offset.
> 
> Instead of making a comment why not just call the member current_buffer_offset?
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:50
> > +    GstElement* wavenc;
> 
> wavEncoder?
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:55
> > +    GSList* pads; // list of queue src pads. One queue for each planar audio channel.
> 
> 'list' should be capitalized.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:56
> > +    GstPad* srcpad; // src pad of the element, interleaved wav data is pushed to it.
> 
> sourcePad?
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:71
> > +static void webKitWebAudioSrcSetProperty(GObject*, guint, const GValue*, GParamSpec*);
> > +static void webKitWebAudioSrcGetProperty(GObject*, guint, GValue*, GParamSpec*);
> 
> The guint parameters should have names.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:77
> > +    return gst_caps_new_simple("audio/x-raw-float", "rate", G_TYPE_INT, static_cast<int>(sampleRate), "channels", G_TYPE_INT, 1, "endianness", G_TYPE_INT, G_BYTE_ORDER, "width", G_TYPE_INT, 32, NULL);
> 
> I think it will make this easier to read if you break it up.
> 

Ok, but style bot won't like it! :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:123
> > +    gst_element_class_add_pad_template(eklass,
> > +                                       gst_static_pad_template_get(&srcTemplate));
> 
> This can be one line.
> 

Sure.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:129
> > +                                         (gchar*) "WebKit WebAudio source element",
> > +                                         (gchar*) "Source",
> > +                                         (gchar*) "Handles WebAudio data from WebCore",
> > +                                         (gchar*) "Philippe Normand <[e-mail]>");
> > +
> 
> You don't need to cast const char* to char*.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:142
> > +                                                       (GParamFlags) (G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE)));
> 
> Please use static_cast here. In the API layer we typically save this to a temporary to avoid all the chatter.
> 

Done!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:175
> > +    priv->task = gst_task_create((GstTaskFunction) webKitWebAudioSrcLoop, src);
> 
> Please use a C++ style cast here.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:179
> > +static void webKitWebAudioSrcConstructed(GObject *object)
> 
> The asterisk is in the wrong place here.
> 

Oops.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:217
> > +        GstCaps* monoCaps = getGStreamerMonoAudioCaps(priv->sampleRate);
> > +        g_object_set(capsfilter, "caps", monoCaps, NULL);
> > +        gst_caps_unref(monoCaps);
> > +
> 
> monoCaps should be a smart pointer.
> 

Hum, this will need support in GRefPtrGStreamer as well :( Ok... :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:219
> > +        g_object_set(queue, "max-size-buffers", static_cast<guint>(1), NULL);
> 
> You should not need to cast 1 to a uint.
> 

Well it turns out I do :(
Spent an hour figuring it out when I added this.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:251
> > +    if (priv->interleave)
> > +        gst_object_unref(GST_OBJECT(priv->interleave));
> > +    if (priv->wavenc)
> > +        gst_object_unref(GST_OBJECT(priv->wavenc));
> > +
> 
> If your private data structure was a C++ object you could use a the destructor and smart pointers could take care of most of this cleanup. We use this pattern quite a bit in the API layer.
> 

Alright.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:257
> > +    GST_CALL_PARENT(G_OBJECT_CLASS, finalize, ((GObject* )(src)));
> 
> G_OBJECT(src) here?
> 

Sure.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:265
> > +    switch (propID) {
> 
> Nit: propID = propId
> 

I made it propertyId.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:270
> > +        priv->bus = reinterpret_cast<AudioBus*>(g_value_get_pointer(value));
> 
> You only need a staic cast here.
> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:273
> > +        priv->provider = reinterpret_cast<AudioSourceProvider*>(g_value_get_pointer(value));
> 
> Ditto.
> 

Ok.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:281
> > +static void webKitWebAudioSrcGetProperty(GObject* object, guint propID, GValue* value, GParamSpec* pspec)
> 
> propID -> propId.
> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:311
> > +    size_t requestedFrames = 128;
> 
> This should probably be static with a comment explaining why you chose 128 here.
> 

Alright I'll make it a constant in the AudioDestination implementation and pass it as new property to the src element, if that's OK with you.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:312
> > +    guint bufferSize = requestedFrames * sizeof(float);
> 
> Please use unsigned instead of guint.
> 

Ok.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:315
> > +    for (guint index = 0; index < g_slist_length(priv->pads); index++) {
> 
> Ditto.
> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:324
> > +        GstCaps* monoCaps = getGStreamerMonoAudioCaps(priv->sampleRate);
> 
> This should be  asmart pointer.
> 

I tried a GRefPtr here but it doesn't work when I need to update the GstStructure just below.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:340
> > +    GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;
> 
> ret -> returnValue
> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:347
> > +            gst_element_post_message(element,
> > +                                     gst_missing_element_message_new(element, "interleave"));
> 
> This can be one line
> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:348
> > +            GST_ELEMENT_ERROR(src, CORE, MISSING_PLUGIN, (0), ("no interleave"));
> 
> Why the parenthesis around 0?
> 

In the macro the (0) text argument is used like this: gchar *__txt = _gst_element_error_printf text;
So here it translates to gchar *__txt = _gst_element_error_printf(0);

I can add a more relevant value if necessary :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:354
> > +            gst_element_post_message(element,
> > +                                     gst_missing_element_message_new(element, "wavenc"));
> > +            GST_ELEMENT_ERROR(src, CORE, MISSING_PLUGIN, (0), ("no wavenc"));
> 
> Dittos.
> 

Okeys ^ :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:363
> > +    if (G_UNLIKELY(ret == GST_STATE_CHANGE_FAILURE)) {
> 
> There's a WTF version of LIKELY and UNLIKELY. I think we should use those in WebCore code.
> 

Sure.

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:25
> > +G_BEGIN_DECLS
> 
> Is this public API? G_BEGIN_DECLS ensures that these symbols are unmangled so that they can be called from a foriegn C compilation unit. Is that necessary? A lot of this header looks like stuff you can omit actually. A lot of it can probably be private to the implementation file and the dead code (uncalled) should probably be removed. I think we should make this change for all private GObjects. Basically just trim it down to what you need in some other file.
> 

Hum. OK!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:40
> > +    WebKitWebAudioSrcPrivate *priv;
> 
> The asterisk is in the wrong place here.

Oh, my! :)
Thanks for the review Martin!

[Philippe Normand]

Created attachment 114487 [details]
WebAudio GStreamer implementation: playback support

[WebKit Review Bot]

Attachment 114487 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:66:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:91:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:131:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:176:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 114487 [details]
WebAudio GStreamer implementation: playback support

View in context: https://bugs.webkit.org/attachment.cgi?id=114487&action=review

Just a couple more nitpicky things. I'll try to do a real review soon!

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:35
> +const unsigned framesToPull = 128;

This definitely goes beyond what I suggested, but I think it's an interesting approach. I was just interested in having a small comment explaining why you pull 128 frames at a time.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:65
> +    GstElement* webkitAudioSrc = reinterpret_cast<GstElement*>(g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC,

Sorry maybe just format like this:

...g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC,
                "rate", sampleRate,
                "bus",  &m_renderBus,

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:69
> +enum {
> +    PROP_RATE = 1,
> +    PROP_BUS = 2,
> +    PROP_PROVIDER = 3,
> +    PROP_FRAMES = 4
> +};

I think you can omit the values here...but I'm not certain.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:134
> +    GObjectClass* oklass = G_OBJECT_CLASS(klass);
> +    GstElementClass* eklass = GST_ELEMENT_CLASS(klass);

I think I'd prefer objectClass and elementClass here.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:220
> +    // gst_object_ref_sink(GST_OBJECT(priv->interleave));
> +    // gst_object_ref_sink(GST_OBJECT(priv->wavEncoder));

Was this left in by mistake?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:26
> +#define WEBKIT_TYPE_WEB_AUDIO_SRC (webkit_web_audio_src_get_type())
> +#define WEBKIT_WEB_AUDIO_SRC(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrc))

Nit. You you should remove the extra space before the parenthesis.

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30
>> +GType webkit_web_audio_src_get_type(void);
> 
> webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

You can omit the "void" from the argument list.

[Philippe Normand]

(In reply to comment #16)
> (From update of attachment 114487 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114487&action=review
> 
> Just a couple more nitpicky things. I'll try to do a real review soon!
> 
> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:35
> > +const unsigned framesToPull = 128;
> 
> This definitely goes beyond what I suggested, but I think it's an interesting approach. I was just interested in having a small comment explaining why you pull 128 frames at a time.
> 

When you create an AudioBus with a specific size you can only pull that number of frames at a time, from my understanding. That's why I made it constant in AudioDestination.

> > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:65
> > +    GstElement* webkitAudioSrc = reinterpret_cast<GstElement*>(g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC,
> 
> Sorry maybe just format like this:
> 
> ...g_object_new(WEBKIT_TYPE_WEB_AUDIO_SRC,
>                 "rate", sampleRate,
>                 "bus",  &m_renderBus,
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:69
> > +enum {
> > +    PROP_RATE = 1,
> > +    PROP_BUS = 2,
> > +    PROP_PROVIDER = 3,
> > +    PROP_FRAMES = 4
> > +};
> 
> I think you can omit the values here...but I'm not certain.
> 

The first one no but the others yes indeed :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:134
> > +    GObjectClass* oklass = G_OBJECT_CLASS(klass);
> > +    GstElementClass* eklass = GST_ELEMENT_CLASS(klass);
> 
> I think I'd prefer objectClass and elementClass here.
> 

Ok!

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:220
> > +    // gst_object_ref_sink(GST_OBJECT(priv->interleave));
> > +    // gst_object_ref_sink(GST_OBJECT(priv->wavEncoder));
> 
> Was this left in by mistake?
> 

Heh. Yes :)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:26
> > +#define WEBKIT_TYPE_WEB_AUDIO_SRC (webkit_web_audio_src_get_type())
> > +#define WEBKIT_WEB_AUDIO_SRC(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrc))
> 
> Nit. You you should remove the extra space before the parenthesis.
> 

What extra space?

> >> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30
> >> +GType webkit_web_audio_src_get_type(void);
> > 
> > webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> You can omit the "void" from the argument list.

Sure!

[Philippe Normand]

Created attachment 114658 [details]
WebAudio GStreamer implementation: playback support

[WebKit Review Bot]

Attachment 114658 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:91:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:131:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:176:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 115118 [details]
WebAudio GStreamer implementation: playback support

[WebKit Review Bot]

Attachment 115118 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:91:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:131:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:176:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Martin, any chance we can get this one in soon or should we just wait the hackfest?

[Martin Robinson]

Comment on attachment 115118 [details]
WebAudio GStreamer implementation: playback support

View in context: https://bugs.webkit.org/attachment.cgi?id=115118&action=review

Looks good! I have a few comments. I feel confident enough to r+ this after fixes now.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:91
> +void AudioDestinationGStreamer::plugSink(GstPad* pad)

I think this method deserves a more interesting name. plugSink is pretty generic. How about finishBuildingPipelineAfterWavParserPadReady or something similar?

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:95
> +    GstElement* audiosink = gst_element_factory_make("autoaudiosink", 0);

RefPtr here?

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:98
> +    if (audiosink) {

Just move the early return below before here.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:114
> +    ASSERT_WITH_MESSAGE(m_audioSinkAvailable, "Failed to create GStreamer autoaudiosink element");
> +    if (!m_audioSinkAvailable)
> +        return;

(this one)

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:121
> +    GRefPtr<GstPad> sinkPad(gst_element_get_static_pad(audioconvert, "sink"));
> +    gst_pad_link(pad, sinkPad.get());

You are leaking sinkPad here because you didn't call adoptGRef.

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:139
>  void AudioDestinationGStreamer::stop()

Comment unrelated to your patch: "stop" seems confusing, because in many audio contexts it means "pause and then reset the location to 0."

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:57
> +    GstTask* task;

GRefPtr here?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:207
> +    priv->interleave = gst_element_factory_make("interleave", 0);
> +    priv->wavEncoder = gst_element_factory_make("wavenc", 0);

Do these have floating references or full references? If full references you need to call adoptGRef here.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:243
> +        GRefPtr<GstPad> srcPad(gst_element_get_static_pad(audioconvert, "src"));
> +        GRefPtr<GstPad> sinkPad(gst_element_get_request_pad(priv->interleave.get(), "sink%d"));

You are leadking the pads here because of missing adoptGRef.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:335
> +        memcpy(GST_BUFFER_DATA(buffer), priv->bus->channel(index)->data(), bufferSize);
> +        GST_BUFFER_OFFSET(buffer) = priv->currentBufferOffset;
> +        GST_BUFFER_OFFSET_END(buffer) = priv->currentBufferOffset + priv->framesToPull;
> +

Hrm. Isn't there any way to have a GstBuffer point to some static memory that it won't free when it finishes? It seems this is possible if you don't use mallocdata. If so, you can avoid a memcpy here. Maybe it even makes sense to have one large buffer and create sub GstBuffers here. That would avoid fragmentation, at least.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:336
> +        GstCaps* monoCaps = getGStreamerMonoAudioCaps(priv->sampleRate);

This should be GRefPtr<GstCaps> monoCaps = adoptGRef(getGStreamerMonoAudioCaps(priv->samleRate));

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:26
> +#define WEBKIT_TYPE_WEB_AUDIO_SRC (webkit_web_audio_src_get_type())
> +#define WEBKIT_WEB_AUDIO_SRC(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrc))

Nit: Extra spaces  after macro names.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:28
> +typedef struct _WebKitWebAudioSrc        WebKitWebAudioSrc;

no need to align this.

[Philippe Normand]

Comment on attachment 115118 [details]
WebAudio GStreamer implementation: playback support

View in context: https://bugs.webkit.org/attachment.cgi?id=115118&action=review

>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:98
>> +    if (audiosink) {
> 
> Just move the early return below before here.

Hum I can't because m_audioSinkAvailable might change just below, line 106.

[Philippe Normand]

Comment on attachment 115118 [details]
WebAudio GStreamer implementation: playback support

View in context: https://bugs.webkit.org/attachment.cgi?id=115118&action=review

>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:91
>> +void AudioDestinationGStreamer::plugSink(GstPad* pad)
> 
> I think this method deserves a more interesting name. plugSink is pretty generic. How about finishBuildingPipelineAfterWavParserPadReady or something similar?

Sure!

>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:95
>> +    GstElement* audiosink = gst_element_factory_make("autoaudiosink", 0);
> 
> RefPtr here?

Doesn't work :( I need to unref only if the element is not added to the bin.

>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:121

> 
> You are leaking sinkPad here because you didn't call adoptGRef.

:( Fixed!

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:57
>> +    GstTask* task;
> 
> GRefPtr here?

Ok

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:207
>> +    priv->wavEncoder = gst_element_factory_make("wavenc", 0);
> 
> Do these have floating references or full references? If full references you need to call adoptGRef here.

Well, full references afaik, so trying to use adoptGRef here, the destructor is called immediately :(

#0  WTF::derefGPtr<_GstElement> (ptr=0x8077260) at ../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:42
#1  0xb6af957d in WTF::GRefPtr<_GstElement>::~GRefPtr (this=0xbfffd75c, __in_chrg=<optimized out>)
    at ../../Source/JavaScriptCore/wtf/gobject/GRefPtr.h:72
#2  0xb6af851d in webKitWebAudioSrcConstructed (object=0x84c7150)
    at ../../Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:206

Not sure what's going on here :( So the GstElement is unreffed and destructed even before we get a chance to add it to the bin...

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:243
>> +        GRefPtr<GstPad> sinkPad(gst_element_get_request_pad(priv->interleave.get(), "sink%d"));
> 
> You are leadking the pads here because of missing adoptGRef.

Right :/ Fixed

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:335

> 
> Hrm. Isn't there any way to have a GstBuffer point to some static memory that it won't free when it finishes? It seems this is possible if you don't use mallocdata. If so, you can avoid a memcpy here. Maybe it even makes sense to have one large buffer and create sub GstBuffers here. That would avoid fragmentation, at least.

A "large buffer"? How large would it be given that it's possible for the playback pipeline to run as long as your browser keeps a WebAudio-enabled page open?

I'm not sure it's possible to have a GstBuffer point to static memory. Another thing that's not possible here (afaik) is using gst_pad_alloc which requires srcpads while we need to chain to queue's sink pad. :/

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:336
>> +        GstCaps* monoCaps = getGStreamerMonoAudioCaps(priv->sampleRate);
> 
> This should be GRefPtr<GstCaps> monoCaps = adoptGRef(getGStreamerMonoAudioCaps(priv->samleRate));

I tried that already and it didn't work out, the gst_structure_set below failing.

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:26
>> +#define WEBKIT_WEB_AUDIO_SRC(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrc))
> 
> Nit: Extra spaces  after macro names.

I'm sorry but I see nothing wrong here! :/ There's only one space between the macro name and its definition.

#define WEBKIT_WEB_AUDIO_SRC(obj)(G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEB_AUDIO_SRC, WebKitWebAudioSrc))

obviously won't work.

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:28
>> +typedef struct _WebKitWebAudioSrc        WebKitWebAudioSrc;
> 
> no need to align this.

Right.

[Philippe Normand]

Created attachment 116095 [details]
WebAudio GStreamer implementation: playback support

[WebKit Review Bot]

Attachment 116095 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:91:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:131:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:176:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 115118 [details]
WebAudio GStreamer implementation: playback support

View in context: https://bugs.webkit.org/attachment.cgi?id=115118&action=review

>>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:95
>>> +    GstElement* audiosink = gst_element_factory_make("autoaudiosink", 0);
>> 
>> RefPtr here?
> 
> Doesn't work :( I need to unref only if the element is not added to the bin.

You can use GRefPtr::leakRef in that case with a note that gst_add_many is transfer full.

>>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:98
>>> +    if (audiosink) {
>> 
>> Just move the early return below before here.
> 
> Hum I can't because m_audioSinkAvailable might change just below, line 106.

If m_audioSinkAvailable changes you return early right away, so it shouldn't matter. What I'm suggesting is:

ASSERT_WITH_MESSAGE(audioSink, "Failed to create GStreamer autoaudiosink element");
if (!audioSink)
    return;

GstStateChangeReturn stateChangeReturn = gst_element_set_state(audiosink, GST_STATE_READY);
ASSERT_WITH_MESSAGE(m_audioSinkAvailable, "Failed to change autoaudiosink element state");
if (stateChangeReturn == GST_STATE_CHANGE_FAILURE) {
    gst_element_set_state(audiosink, GST_STATE_NULL);
    m_audioSinkAvailable = false;
    gst_object_unref(audiosink);
            return;

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:112
> +    ASSERT_WITH_MESSAGE(m_audioSinkAvailable, "Failed to create GStreamer autoaudiosink element");

ASSERT actually crashes the application in debug mode. Can happen as the result of a missing installed plugin or some other non-fatal gstreamer error? If that's the case, I think you sould just print a message and not assert. The main signal that this shouldn't be an assert is that you are handling the failure case really thoroughly.

>>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:207
>>> +    priv->wavEncoder = gst_element_factory_make("wavenc", 0);
>> 
>> Do these have floating references or full references? If full references you need to call adoptGRef here.
> 
> Well, full references afaik, so trying to use adoptGRef here, the destructor is called immediately :(
> 
> #0  WTF::derefGPtr<_GstElement> (ptr=0x8077260) at ../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:42
> #1  0xb6af957d in WTF::GRefPtr<_GstElement>::~GRefPtr (this=0xbfffd75c, __in_chrg=<optimized out>)
>     at ../../Source/JavaScriptCore/wtf/gobject/GRefPtr.h:72
> #2  0xb6af851d in webKitWebAudioSrcConstructed (object=0x84c7150)
>     at ../../Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:206
> 
> Not sure what's going on here :( So the GstElement is unreffed and destructed even before we get a chance to add it to the bin...

That would suggest they are floating references. The documentation seems to confirm that: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstObject.html

I think that this means the GRefPtr needs to do:
gst_object_ref(GST_OBJECT(m_ptr));
gst_object_sink(GST_OBJECT(m_ptr));

This is because the gst_object_sink function works differently than the g_object_ref_sink function. gst_object_sink does nothing if the GstObject does not have a floating reference, while in this case g_object_ref_sink increases the reference count by one. I think that should fix the crash.

[Philippe Normand]

Created attachment 116339 [details]
WebAudio GStreamer implementation: playback support

[WebKit Review Bot]

Attachment 116339 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:92:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:132:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:177:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 116355 [details]
WebAudio GStreamer implementation: playback support

[WebKit Review Bot]

Attachment 116355 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:91:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:131:  webkit_web_audio_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:176:  webkit_web_audio_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.h:30:  webkit_web_audio_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 116355 [details]
WebAudio GStreamer implementation: playback support

View in context: https://bugs.webkit.org/attachment.cgi?id=116355&action=review

> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:106
> +    // Autoaudiosink does the real sink detection in the
> +    // GST_STATE_NULL->READY transition so it's best to roll it to READY as
> +    // soon as possible to ensure the underlying platform
> +    // audiosink was loaded correctly.

Super nit: Do you mind evening out these lines?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:324
> +    if (!priv->provider || !priv->bus)
> +        return;

You can probably remove this since you assert above.

[Philippe Normand]

Committed r101138: <http://trac.webkit.org/changeset/101138>