Bug 91611 - [GStreamer] Stopping playback of html5 media when receiving a higher priority audio event needs implementation
Summary: [GStreamer] Stopping playback of html5 media when receiving a higher priority...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-18 03:46 PDT by Philippe Normand
Modified: 2013-04-01 07:26 PDT (History)
16 users (show)

See Also:


Attachments
First approach (15.33 KB, patch)
2013-02-21 00:46 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (16.46 KB, patch)
2013-02-21 10:22 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (17.41 KB, patch)
2013-02-22 07:14 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (18.55 KB, patch)
2013-02-25 03:55 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (19.86 KB, patch)
2013-03-05 12:06 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (19.84 KB, patch)
2013-03-06 01:30 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (20.45 KB, patch)
2013-03-07 04:27 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (21.36 KB, patch)
2013-03-13 05:46 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (21.05 KB, patch)
2013-03-14 05:53 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2013-03-14 09:46 PDT, Xabier Rodríguez Calvar
pnormand: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-07-18 03:46:08 PDT
We need to support the request-state GstMessage in the MediaPlayerPrivateGStreamer backend so as to play/pause our pipeline upon downstream request.

http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstMessage.html#gst-message-new-request-state

Any takers? Please raise your hand :)
Comment 1 Xabier Rodríguez Calvar 2012-07-19 08:12:30 PDT
I can give it a try.
Comment 2 Xabier Rodríguez Calvar 2012-08-09 09:03:27 PDT
Interesting finding. I added the hook to the message and we were not receiving it so I had a look Rhythmbox. There is no trace of that message in their code and the developers told me they do not manage it. I compiled it and hooked to gst_element_set_state when getting a call in rhythmbox and this is what I saw:


(gdb) bt
#0  gst_element_set_state (element=0x1826710, state=GST_STATE_PAUSED) at gstelement.c:2614
#1  0x00007ffff7f7badc in start_state_change (mp=0x43da60, state=GST_STATE_PAUSED, action=STOP_TICK_TIMER) at rb-player-gst.c:383
#2  0x00007ffff7f7d394 in impl_pause (player=0x43da60) at rb-player-gst.c:874
#3  0x00007ffff7f77e3c in rb_player_pause (player=0x43da60) at rb-player.c:450
#4  0x00007ffff7ee943a in rb_shell_player_playpause (player=0x738160, unused=0, error=0x0) at rb-shell-player.c:2381
#5  0x00007ffff7eeb2eb in rb_shell_player_pause (player=0x738160, error=0x0) at rb-shell-player.c:3240
#6  0x00007fffd09bd47c in media_player_key_pressed (proxy=0xd78100, sender=0x14840f0 ":1.5", signal=0x1435220 "MediaPlayerKeyPressed", parameters=0x19f0230, plugin=0xd7fb70) at rb-mmkeys-plugin.c:120
#7  0x00007fffed7637bc in ffi_call_unix64 () from /usr/lib/x86_64-linux-gnu/libffi.so.5
#8  0x00007fffed763237 in ffi_call () from /usr/lib/x86_64-linux-gnu/libffi.so.5
#9  0x00007fffedf8709b in g_cclosure_marshal_generic () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#10 0x00007fffedf86724 in g_closure_invoke () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#11 0x00007fffedf977b0 in ?? () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#12 0x00007fffedf9f72c in g_signal_emit_valist () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#13 0x00007fffedf9f8c2 in g_signal_emit () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#14 0x00007fffee9dff14 in ?? () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#15 0x00007fffee9cfbe5 in ?? () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#16 0x00007fffed4ab205 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#17 0x00007fffed4ab538 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#18 0x00007fffed4ab5f4 in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#19 0x00007fffee9b238c in g_application_run () from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#20 0x000000000040151f in main (argc=1, argv=0x7fffffffd848) at main.c:98

What rhythmbox seems to receive is a press of the multimedia key and not a request for a state change.
Comment 3 Xabier Rodríguez Calvar 2012-08-09 09:06:17 PDT
(In reply to comment #2)
> What rhythmbox seems to receive is a press of the multimedia key and not a request for a state change.

This makes me think if we might be taking the wrong approach here. We might want to hook to this message and pause anyway, but if we want to really stop in gnome 3, we'd need to hook to that key and pause, or at least reinject that message to the pipeline.

Comments are welcome.
Comment 4 Xabier Rodríguez Calvar 2012-08-10 09:18:48 PDT
After running rhythmbox and totem at the same time and receiving a call and activating debug in pulseaudio I saw that the messages were being sent.

Aug 10 17:36:45 localhost pulseaudio[31056]: [pulseaudio] module-role-cork.c: Found a 'phone' stream that will trigger the auto-cork.
Aug 10 17:36:45 localhost pulseaudio[31056]: [pulseaudio] module-role-cork.c: Found a 'music' stream that should be corked/muted.
Aug 10 17:36:45 localhost pulseaudio[31056]: [pulseaudio] module-role-cork.c: Found a 'video' stream that should be corked/muted.

I also saw there were not being received at the other side. I thought that the problem was setting the role [1] and so it seems because after setting that, gst-launch stopped. We need to set the role when playing. There is API for that in GStreamer so it will be next step.

[1] http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/Clients/ApplicationProperties
Comment 5 Xabier Rodríguez Calvar 2012-08-14 10:32:04 PDT
The problem was assigning the role. Now we receive the message and we can stop the pipeline. I need to ensure that the patch does not break any test, though.
Comment 6 Xabier Rodríguez Calvar 2012-08-17 02:45:49 PDT
I have the patch and no new tests are broken. Now I am writing the test.

Eric, Philippe suggested me to CC you about the way to implement the test. As we need a way to inject the event, he thought that the best could be adding a new vmethod to MediaPlayerClient and implement it in HTMLMediaElement for this and do stuff in the internals to be able to call it from the javascript test code. What do you think?
Comment 7 Philippe Normand 2012-08-17 03:22:02 PDT
Just to add some more context, the use-case is that a multimedia  application requiring exclusive use of the audio device can request other running applications to temporarily suspend their media playback.

In GNOME for instance the video chat application typically requests such use and asks other applications, via PulseAudio to pause their playback.

Maybe this would make sense on Mac OSX as well, from a user-perspective? I have no idea how this would be technically possible though.

So we're asking if it would be possible to slightly extend the MediaPlayerClient interface in order to write a test for this feature we'd like to support in the GStreamer MediaPlayerPrivate backend.
Comment 8 Xabier Rodríguez Calvar 2012-08-17 03:58:36 PDT
(In reply to comment #7)
> So we're asking if it would be possible to slightly extend the MediaPlayerClient interface in order to write a test for this feature we'd like to support in the GStreamer MediaPlayerPrivate backend.

We could name it like requestStopByHigherPrioStream . In the case of our port this only makes sense for the testing but in any other ports it could make sense for functionality too.
Comment 9 Xabier Rodríguez Calvar 2013-02-21 00:46:51 PST
Created attachment 189467 [details]
First approach

Eric, I have a first version of the patch for you to have a look at the the changes I propose to carry on with the task. It basically adds methods at the media element and the cascade to the port private parts to inject the stop event and it's added to the Internals to call it from javascript.
Comment 10 Xabier Rodríguez Calvar 2013-02-21 10:22:46 PST
Created attachment 189551 [details]
Patch

Now the test is working and the expectation is created too. I still need to add expectations for the other ports.
Comment 11 Jer Noble 2013-02-21 10:39:50 PST
Comment on attachment 189551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189551&action=review

> Source/WebCore/testing/Internals.idl:253
> +    void stopElementByHigherPrioAudioStream(in Node node);

This seems a poor name for this method (and throughout the call chain down to MediaPlayerPrivate).   How about "simulateAudioInterruption()"?

> Source/WebCore/testing/Internals.cpp:2006
> +void Internals::stopElementByHigherPrioAudioStream(Node *node)
> +{
> +    HTMLMediaElement* element = toMediaElement(node);
> +    element->stopByHigherPrioAudioStream();
> +}

Rather than exposing this method on HMTLMediaElement, this could be:

HTMLMediaElement* element = toMediaElement(node);
element->player()->simulateAudioInterruption();

> Source/WebCore/html/HTMLMediaElement.h:348
> +    virtual void stopByHigherPrioAudioStream();
> +

Then this would be unnecessary.

> Source/WebCore/html/HTMLMediaElement.cpp:4871
> +void HTMLMediaElement::stopByHigherPrioAudioStream()
> +{
> +    if (!m_player)
> +        return;
> +    m_player->stopByHigherPrioAudioStream();
> +}
> +

Ditto.
Comment 12 Martin Robinson 2013-02-21 10:44:07 PST
Comment on attachment 189551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189551&action=review

Since this touches platform-independent code, I'll remove the [GStreamer] tag.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1079
> +        // We consider a state change when going to GST_STATE_PLAYING
> +        // or to GST_STATE_PAUSED from GST_STATE_PLAYING. We also
> +        // consider GST_STATE_NULL because playbin2 can avoid
> +        // reporting state changes when transitioning to
> +        // GST_STATE_NULL but we do not consider GST_STATE_READY
> +        // because if it is now GST_STATE_READY it means that it has
> +        // already transitioned through GST_STATE_PAUSED and that's
> +        // already contemplated.
> +        if ((m_previousState == GST_STATE_PAUSED
> +            && state == GST_STATE_PLAYING)
> +            || ((state == GST_STATE_PAUSED || state == GST_STATE_NULL)
> +                && m_previousState == GST_STATE_PLAYING)) {
> +            shouldUpdatePlaybackState = true;
> +            m_previousState = state;
> +            LOG_MEDIA_MESSAGE("State change to %s completed",
> +                gst_element_state_get_name(m_previousState));
> +        }

It's okay for lines to be longer than 80 characters. In WebKit a good target is 120 characters.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1579
> +    GstElement * playBin = m_playBin.get();

Watch the style.
Comment 13 Build Bot 2013-02-21 11:07:16 PST
Comment on attachment 189551 [details]
Patch

Attachment 189551 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16695201

New failing tests:
media/media-higher-prio-audio-stream.html
Comment 14 WebKit Review Bot 2013-02-21 11:17:24 PST
Comment on attachment 189551 [details]
Patch

Attachment 189551 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16698175

New failing tests:
media/media-higher-prio-audio-stream.html
Comment 15 Build Bot 2013-02-22 05:54:47 PST
Comment on attachment 189551 [details]
Patch

Attachment 189551 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16692856

New failing tests:
media/media-higher-prio-audio-stream.html
Comment 16 Xabier Rodríguez Calvar 2013-02-22 07:14:08 PST
Created attachment 189770 [details]
Patch

Changed according to the comments and added expectations for Mac, Chromium and Qt. I would need some help to flag the tests in Qt because I don't know the qt subports that are not running GStreamer.
Comment 17 Philippe Normand 2013-02-22 07:31:12 PST
Comment on attachment 189770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189770&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:145
> +    g_setenv("PULSE_PROP_media.role", "video", TRUE);

I'm not sure, we, as a library for applications, should set an environment variable. IMHO it should be done on application side and from the test program.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:723
> +            LOG_MEDIA_MESSAGE("%s Requested state change to %s", gst_element_get_name(GST_MESSAGE_SRC(message)),

Perhaps rephrase a bit the beginning. "Element %s requested ..."

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1073
> +            LOG_MEDIA_MESSAGE("State change to %s completed",
> +                gst_element_state_get_name(m_previousState));

Make this one line please.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1150
> +        LOG_MEDIA_MESSAGE("State changed to %s",
> +            gst_element_state_get_name(state));

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1575
> +    GstBus* bus = gst_pipeline_get_bus(GST_PIPELINE(playBin));

Please use webkitGstPipelineGetBus().

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1577
> +    GstMessage* message = gst_message_new_request_state(GST_OBJECT(playBin),
> +        GST_STATE_PAUSED);

Please make this one line.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578
> +    gst_bus_post(bus, message);

Why not use gst_element_post_message()?
Comment 18 Eric Carlson 2013-02-22 08:29:51 PST
Comment on attachment 189770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189770&action=review

> Source/WebCore/ChangeLog:5
> +        Downstream state change request support for the GStreamer backend
> +        https://bugs.webkit.org/show_bug.cgi?id=91611
> +

This title tells me nothing about what the change does. Please add a sentence or two describing what you are changing and why.

> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:192
> +    virtual void simulateAudioInterruption() { }

I don't like that this adds a method to every HTMLMediaElement, every MediaPlayer, and every MediaPlayerPrivate for this platform specific test. Is there any other way to test this feature? If not, I am not sure the test is worth the ancillary cost.
Comment 19 Philippe Normand 2013-02-22 09:37:08 PST
Perhaps the API additions can be guarded with if USE(GSTREAMER). Would that be ok Eric?
Comment 20 Eric Carlson 2013-02-22 09:57:31 PST
(In reply to comment #19)
> Perhaps the API additions can be guarded with if USE(GSTREAMER). Would that be ok Eric?

That seems reasonable.
Comment 21 Xabier Rodríguez Calvar 2013-02-25 03:55:07 PST
Created attachment 190026 [details]
Patch

Updated Qt expectations, made suggested style changes, added #if guards to avoid the MediaPlayer being added to non GStreamer ports, changed to use gst_element_post_message and not setting the media role as environment variable, but as media stream properties on pulsesink.
Comment 22 Philippe Normand 2013-02-25 04:07:36 PST
Comment on attachment 190026 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190026&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1521
> -    GstElement* sink = gst_element_factory_make("autoaudiosink", 0);
> +    GstElement* sink = gst_element_factory_make("pulsesink", 0);

I think autoaudiosink does more than checking it can create pulsesink. We should rely on autoaudiosink as much as possible.
Comment 23 Philippe Normand 2013-02-25 04:08:22 PST
The patch needs to be rebased too.
Comment 24 Philippe Normand 2013-02-28 00:14:32 PST
Comment on attachment 190026 [details]
Patch

r- for now.
Comment 25 Philippe Normand 2013-02-28 05:50:32 PST
Comment on attachment 190026 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190026&action=review

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1521
>> +    GstElement* sink = gst_element_factory_make("pulsesink", 0);
> 
> I think autoaudiosink does more than checking it can create pulsesink. We should rely on autoaudiosink as much as possible.

Well, on second thought, perhaps it's better to go this way.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1526
> +        GstStructure* structure = gst_structure_new("stream-properties", "media.role", G_TYPE_STRING, "video", NULL);

This should be set only if the client ::mediaPlayerIsVideo() returns true (eg, we deal with a <video> element). Otherwise we could set the role to "music", would that work?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1527
> +        g_object_set(sink, "stream-properties", structure, NULL);

Is the structure copied? If so it needs to be freed. Can you double-check this please?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1580
> +    GstMessage* message = gst_message_new_request_state(GST_OBJECT(playBin),
> +        GST_STATE_PAUSED);

Can this be a single line please? :)

> Source/WebCore/testing/Internals.cpp:2016
> +void Internals::simulateAudioInterruption(Node *node)
> +{
> +#if USE(GSTREAMER)

Perhaps the #if should wrap the whole method.

> Source/WebCore/testing/Internals.h:293
> +    void simulateAudioInterruption(Node*);

And have this protected by an ifdef.

> Source/WebCore/testing/Internals.idl:254
> +    void simulateAudioInterruption(in Node node);

[Conditional=USE_GSTREAMER] ? Needs to be double-checked :)
Comment 26 Xabier Rodríguez Calvar 2013-03-05 12:06:23 PST
Created attachment 191531 [details]
Patch

Changed style suggestions, not using pulsesink by default but hooking to child-added signal in autoaudiosink to set the properties, and properly unrefing the properties after setting them.
Comment 27 Xabier Rodríguez Calvar 2013-03-05 12:19:02 PST
Comment on attachment 191531 [details]
Patch

Incorrect freeing method
Comment 28 Xabier Rodríguez Calvar 2013-03-06 01:30:04 PST
Created attachment 191684 [details]
Patch

Rebased and changed the structure freeing.
Comment 29 Build Bot 2013-03-06 02:05:00 PST
Comment on attachment 191684 [details]
Patch

Attachment 191684 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17031090
Comment 30 Philippe Normand 2013-03-06 08:56:01 PST
Comment on attachment 191684 [details]
Patch

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?
Comment 31 Xabier Rodríguez Calvar 2013-03-07 04:18:54 PST
(In reply to comment #30)
> > 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 :)

I cannot disconnect the signal when receiving it because then, if the pipeline is restarted, we loose those props.
Comment 32 Xabier Rodríguez Calvar 2013-03-07 04:27:45 PST
Created attachment 191967 [details]
Patch

Changes suggested by Phil
Comment 33 Philippe Normand 2013-03-07 04:33:02 PST
Comment on attachment 191967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191967&action=review

> Source/WebCore/ChangeLog:23
> +        (WebCore::MediaPlayer::simulateAudioInterruption): Added to
> +        delegate on the private to stop because of a stream with bigger
> +        priority.

Can this be reworded a bit like suggested in the previous review?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1545
> +    g_signal_connect(sink, "child-added", G_CALLBACK(setAudioStreamProperties), this);

No need to explicitly disconnect from this signal at least in the dtor?
Comment 34 Xabier Rodríguez Calvar 2013-03-07 15:16:32 PST
(In reply to comment #33)
> (From update of attachment 191967 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191967&action=review
> 
> > Source/WebCore/ChangeLog:23
> > +        (WebCore::MediaPlayer::simulateAudioInterruption): Added to
> > +        delegate on the private to stop because of a stream with bigger
> > +        priority.
> 
> Can this be reworded a bit like suggested in the previous review?

Yeah, I had missed that.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1545
> > +    g_signal_connect(sink, "child-added", G_CALLBACK(setAudioStreamProperties), this);
> 
> No need to explicitly disconnect from this signal at least in the dtor?

It's already done.

I also forgot to test the code in GStreamer 0.10.
Comment 35 Xabier Rodríguez Calvar 2013-03-13 05:46:19 PDT
Created attachment 192906 [details]
Patch

Changed according to Phil's comments.
Comment 36 Philippe Normand 2013-03-13 06:58:01 PDT
Comment on attachment 192906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192906&action=review

One last iteration is needed I think ;)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:117
> +static void setAudioStreamPropertiesCallback(GstChildProxy* proxy, GObject* object, gchar* name,

proxy is unused, so no need to give it a name. Same for the "name" variable.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1548
> +    if (m_autoAudioSink)

Can this really happen?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1558
> +    g_signal_connect(sink, "child-added", reinterpret_cast<GCallback>(setAudioStreamPropertiesCallback), this);

Nit: we use the G_CALLBACK in other places I think.
Comment 37 Xabier Rodríguez Calvar 2013-03-14 05:53:00 PDT
Created attachment 193111 [details]
Patch

Changed according to Phil's comments.
Comment 38 WebKit Review Bot 2013-03-14 07:41:33 PDT
Comment on attachment 193111 [details]
Patch

Clearing flags on attachment: 193111

Committed r145811: <http://trac.webkit.org/changeset/145811>
Comment 39 WebKit Review Bot 2013-03-14 07:41:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Xabier Rodríguez Calvar 2013-03-14 09:46:13 PDT
Reopening to attach new patch.
Comment 41 Xabier Rodríguez Calvar 2013-03-14 09:46:18 PDT
Created attachment 193140 [details]
Patch

Adding guards for ENABLE(VIDEO). This should fix qt minimal bot
Comment 42 Philippe Normand 2013-03-14 09:50:21 PDT
Comment on attachment 193140 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193140&action=review

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1147
> +#if ENABLE(VIDEO) && USE(GSTREAMER)

Not needed.

> Source/WebCore/platform/graphics/MediaPlayer.h:465
> +#if ENABLE(VIDEO) && USE(GSTREAMER)

Ditto

> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:191
> +#if ENABLE(VIDEO) && USE(GSTREAMER)

Ditto

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1601
> +#if ENABLE(VIDEO)

This is not needed.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:93
> +#if ENABLE(VIDEO)

Ditto

> Source/WebCore/testing/Internals.cpp:54
> +#if ENABLE(VIDEO) && USE(GSTREAMER)

only ENABLE(VIDEO) is needed.
Comment 43 Xabier Rodríguez Calvar 2013-03-14 10:15:47 PDT
Build problem is being managed in bug 112358. Closing this.
Comment 44 Eric Carlson 2013-03-28 18:59:21 PDT
Comment on attachment 192906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192906&action=review

> Source/WebCore/platform/graphics/MediaPlayer.h:467
> +#if USE(GSTREAMER)
> +    virtual void simulateAudioInterruption();
> +#endif

Why is this virtual?
Comment 45 Xabier Rodríguez Calvar 2013-03-29 05:19:04 PDT
(In reply to comment #44)
> (From update of attachment 192906 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192906&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:467
> > +#if USE(GSTREAMER)
> > +    virtual void simulateAudioInterruption();
> > +#endif
> 
> Why is this virtual?

I just assumed somebody might want to redefine it.
Comment 46 Eric Carlson 2013-03-29 08:51:47 PDT
(In reply to comment #45)
> (In reply to comment #44)
> > (From update of attachment 192906 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=192906&action=review
> > 
> > > Source/WebCore/platform/graphics/MediaPlayer.h:467
> > > +#if USE(GSTREAMER)
> > > +    virtual void simulateAudioInterruption();
> > > +#endif
> > 
> > Why is this virtual?
> 
> I just assumed somebody might want to redefine it.

There are no classes that subclass MediaPlayer, so the only other virtual method in the class is the destructor.
Comment 47 Xabier Rodríguez Calvar 2013-04-01 03:38:16 PDT
(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #44)
> > > (From update of attachment 192906 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=192906&action=review
> > > 
> > > > Source/WebCore/platform/graphics/MediaPlayer.h:467
> > > > +#if USE(GSTREAMER)
> > > > +    virtual void simulateAudioInterruption();
> > > > +#endif
> > > 
> > > Why is this virtual?
> > 
> > I just assumed somebody might want to redefine it.
> 
> There are no classes that subclass MediaPlayer, so the only other virtual method in the class is the destructor.

Ok, I can submit a patch for this, but I think this is not urgent. I guess I can do it after bug 112548, right?
Comment 48 Eric Carlson 2013-04-01 07:26:29 PDT
(In reply to comment #47)
> (In reply to comment #46)
> > (In reply to comment #45)
> > > (In reply to comment #44)
> > > > (From update of attachment 192906 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=192906&action=review
> > > > 
> > > > > Source/WebCore/platform/graphics/MediaPlayer.h:467
> > > > > +#if USE(GSTREAMER)
> > > > > +    virtual void simulateAudioInterruption();
> > > > > +#endif
> > > > 
> > > > Why is this virtual?
> > > 
> > > I just assumed somebody might want to redefine it.
> > 
> > There are no classes that subclass MediaPlayer, so the only other virtual method in the class is the destructor.
> 
> Ok, I can submit a patch for this, but I think this is not urgent. I guess I can do it after bug 112548, right?

That sounds fine.