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

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

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




--- Comment #16 from Martin Robinson <mrobinson at webkit.org>  2011-11-10 09:32:42 PST ---
(From update of attachment 114487)
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.

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