[webkit-reviews] review denied: [Bug 91611] [GStreamer] Stopping playback of html5 media when receiving a higher priority audio event needs implementation : [Attachment 191684] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 6 08:53:36 PST 2013


Philippe Normand <pnormand at igalia.com> has denied Xabier Rodríguez Calvar
<calvaris at igalia.com>'s request for review:
Bug 91611: [GStreamer] Stopping playback of html5 media when receiving a higher
priority audio event needs implementation
https://bugs.webkit.org/show_bug.cgi?id=91611

Attachment 191684: Patch
https://bugs.webkit.org/attachment.cgi?id=191684&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=191684&action=review


It looks good but there are various small issues to fix-up

> Source/WebCore/ChangeLog:8
> +	   updated accodingly.

typo: accordingly

> Source/WebCore/ChangeLog:10
> +	   Added a method to be able to inject the message in the pipeline

What about, "A method was added in the MediaPlayer class to allow  the	the
test runner to simulate an audio interruption" ?

> Source/WebCore/ChangeLog:23
> +	   delegate on the private to stop because of a stream with bigger

What about: New method delegating an audio interruption to the private backend
to simulate the use-case where an external application needs exclusive access
to the audio device.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:126
> +    if (g_strcmp0(G_OBJECT_TYPE_NAME(object), "GstPulseSink"))

Hum that doesn't handle the case where g_strcmp0 returns -1.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1539

> +    g_signal_connect(sink, "child-added",
G_CALLBACK(setAudioStreamProperties), this);

It'd be nice to disconnect from this signal once the properties were set and in
the destructor, just in case :)

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1590

> +    GstElement* playBin = m_playBin.get();
> +    GstMessage* message = gst_message_new_request_state(GST_OBJECT(playBin),
GST_STATE_PAUSED);
> +    gst_element_post_message(playBin, message);

Nit: no need for a playBin variable.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:124
> +    static void setAudioStreamProperties(GstChildProxy*, GObject*, gchar*
name,
> +	   MediaPlayerPrivateGStreamer* self);

Can this be one line?


More information about the webkit-reviews mailing list