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

[bugzilla-daemon at webkit.org]

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 114487: WebAudio GStreamer implementation: playback support
https://bugs.webkit.org/attachment.cgi?id=114487&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.