[webkit-reviews] review denied: [Bug 69835] [GStreamer] WebAudio AudioDestination : [Attachment 115118] 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 115118: WebAudio GStreamer implementation: playback support
https://bugs.webkit.org/attachment.cgi?id=115118&action=review

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