Bug 91611 – [GStreamer] Stopping playback of html5 media when receiving a higher priority audio event needs implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

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 :)

[Xabier Rodríguez Calvar]

I can give it a try.

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

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?

[Philippe Normand]

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.

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

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.

[Jer Noble]

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.

[Martin Robinson]

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.

[Build Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Xabier Rodríguez Calvar]

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.

[Philippe Normand]

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()?

[Eric Carlson]

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.

[Philippe Normand]

Perhaps the API additions can be guarded with if USE(GSTREAMER). Would that be ok Eric?

[Eric Carlson]

(In reply to comment #19)
> Perhaps the API additions can be guarded with if USE(GSTREAMER). Would that be ok Eric?

That seems reasonable.

[Xabier Rodríguez Calvar]

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.

[Philippe Normand]

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.

[Philippe Normand]

The patch needs to be rebased too.

[Philippe Normand]

Comment on attachment 190026 [details]
Patch

r- for now.

[Philippe Normand]

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 :)

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

Comment on attachment 191531 [details]
Patch

Incorrect freeing method

[Xabier Rodríguez Calvar]

Created attachment 191684 [details]
Patch

Rebased and changed the structure freeing.

[Build Bot]

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

[Philippe Normand]

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?

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

Created attachment 191967 [details]
Patch

Changes suggested by Phil

[Philippe Normand]

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?

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

Created attachment 192906 [details]
Patch

Changed according to Phil's comments.

[Philippe Normand]

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.

[Xabier Rodríguez Calvar]

Created attachment 193111 [details]
Patch

Changed according to Phil's comments.

[WebKit Review Bot]

Comment on attachment 193111 [details]
Patch

Clearing flags on attachment: 193111

Committed r145811: <http://trac.webkit.org/changeset/145811>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Xabier Rodríguez Calvar]

Reopening to attach new patch.

[Xabier Rodríguez Calvar]

Created attachment 193140 [details]
Patch

Adding guards for ENABLE(VIDEO). This should fix qt minimal bot

[Philippe Normand]

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.

[Xabier Rodríguez Calvar]

Build problem is being managed in bug 112358. Closing this.

[Eric Carlson]

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?

[Xabier Rodríguez Calvar]

(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.

[Eric Carlson]

(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.

[Xabier Rodríguez Calvar]

(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?

[Eric Carlson]

(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.