[webkit-reviews] review denied: [Bug 69835] [GStreamer] WebAudio AudioDestination : [Attachment 114025] WebAudio GStreamer implementation: playback support

Wed Nov 9 23:40:27 PST 2011

Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 69835: [GStreamer] WebAudio AudioDestination
https://bugs.webkit.org/show_bug.cgi?id=69835

Attachment 114025: WebAudio GStreamer implementation: playback support
https://bugs.webkit.org/attachment.cgi?id=114025&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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
<pnormand at igalia.com>");
> +

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.