[Webkit-unassigned] [Bug 69835] [GStreamer] WebAudio AudioDestination

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 9 23:40:28 PST 2011


https://bugs.webkit.org/show_bug.cgi?id=69835


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114025|review?                     |review-
               Flag|                            |




--- Comment #12 from Martin Robinson <mrobinson at webkit.org>  2011-11-09 23:40:28 PST ---
(From update of attachment 114025)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list