[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 #115118|review?                     |review-
               Flag|                            |




--- Comment #23 from Martin Robinson <mrobinson at webkit.org>  2011-11-19 13:19:38 PST ---
(From update of attachment 115118)
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.

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