[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