Bug 35333 – [GTK] video playback position query flood when mouse over the video element

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

In RenderThemeGtk::paintMediaPlayButton mediaElement.canPlay() is called. This triggers a position query in the media player. So when the mouse is over the video and the controls are active, the playback position is queried about 30 times/second.

[Philippe Normand]

Created attachment 49382 [details]
proposed patch

[Sebastian Dröge (slomo)]

+        if (g_str_equal(GST_MESSAGE_SRC_NAME(message), "pipeline"))
+            mp->updateStates();

Do a pointer comparison instead

Position/Duration queries can fail in the beginning but could return something useful later. So the approach taken here is wrong IMHO.

Why do you rename the playbin2 to "pipeline"? :)

[Philippe Normand]

(In reply to comment #2)
> +        if (g_str_equal(GST_MESSAGE_SRC_NAME(message), "pipeline"))
> +            mp->updateStates();
> 
> Do a pointer comparison instead
> 

you mean comparing with m_playBin? It can't be accessed from there :/ If found this simpler than passing n_playBin into callback data or implementing an accessor.

> Position/Duration queries can fail in the beginning but could return something
> useful later. So the approach taken here is wrong IMHO.
> 

I don't cache position ;) For duration it is indeed cached but I made sure that if the query fails at the beginning it will be re-attempted later when playback started.

> Why do you rename the playbin2 to "pipeline"? :)

Dunno... suggestions? :)

[Philippe Normand]

Created attachment 49802 [details]
updated patch

Rebased patch against master and implemented platformMedia() method so
the message callback doesn't need to check message object source name anymore.

[Gustavo Noronha (kov)]

Comment on attachment 49802 [details]
updated patch

 1572     float maxTime = maxTimeSeekable();
15721573     // FIXME real ranges support
1573      if (!maxTimeSeekable())
 1574     if (!maxTime)
15741575         return TimeRanges::create();
1575      return TimeRanges::create(minTimeSeekable(), maxTimeSeekable());
 1576     return TimeRanges::create(minTimeSeekable(), maxTime);

Unrelated micro-optimization? Is this really a problem? I'd put this in a separate patch, and get Eric Carlson to look at it.

5864 // backend can live at runtime.
5965 typedef struct PlatformMedia {
6066     QTMovie* qtMovie;
 67     GstElement* gstPipeline;
6168 } PlatformMedia;

This looks weird to me. I don't think our struct should have a QTMovie in it. Maybe we need a #if/#else here, instead? Or event better, just write an implementation for platformMedia that returns the playbin! See:

77	    virtual PlatformMedia platformMedia() const { return NoPlatformMedia; }

676      return paintMediaButton(paintInfo.context, r, mediaElement->canPlay() ? m_playButton.get() : m_pauseButton.get(), m_panelColor, m_mediaIconSize);
 676     return paintMediaButton(paintInfo.context, r, mediaElement->paused() ? m_playButton.get() : m_pauseButton.get(), m_panelColor, m_mediaIconSize);

This seems totally unrelated?

[Philippe Normand]

(In reply to comment #5)
> (From update of attachment 49802 [details])
>  1572     float maxTime = maxTimeSeekable();
> 15721573     // FIXME real ranges support
> 1573      if (!maxTimeSeekable())
>  1574     if (!maxTime)
> 15741575         return TimeRanges::create();
> 1575      return TimeRanges::create(minTimeSeekable(), maxTimeSeekable());
>  1576     return TimeRanges::create(minTimeSeekable(), maxTime);
> 
> Unrelated micro-optimization? Is this really a problem? I'd put this in a
> separate patch, and get Eric Carlson to look at it.
> 

I can remove it, no worries.

> 5864 // backend can live at runtime.
> 5965 typedef struct PlatformMedia {
> 6066     QTMovie* qtMovie;
>  67     GstElement* gstPipeline;
> 6168 } PlatformMedia;
> 
> This looks weird to me. I don't think our struct should have a QTMovie in it.
> Maybe we need a #if/#else here, instead? Or event better, just write an
> implementation for platformMedia that returns the playbin! See:
> 

The comment above the declaration:

// Structure that will hold every native
// types supported by the current media player.
// We have to do that has multiple media players
// backend can live at runtime.

So as I understood it we need to stuff inside this structure. Or should we instead conditionally define it depending on the platform? Now I am confused! :)

> 77        virtual PlatformMedia platformMedia() const { return NoPlatformMedia;
> }
> 
> 676      return paintMediaButton(paintInfo.context, r, mediaElement->canPlay()
> ? m_playButton.get() : m_pauseButton.get(), m_panelColor, m_mediaIconSize);
>  676     return paintMediaButton(paintInfo.context, r, mediaElement->paused() ?
> m_playButton.get() : m_pauseButton.get(), m_panelColor, m_mediaIconSize);
> 
> This seems totally unrelated?

It is not! :) canPlay() triggers a call to endedPlayback() which queries the duration. This query is useless, just checking paused() is enough in this context and this what MediaControlPlayButtonElement::updateDisplayType does, FWIW.

[Gustavo Noronha (kov)]

(In reply to comment #6)
> // Structure that will hold every native
> // types supported by the current media player.
> // We have to do that has multiple media players
> // backend can live at runtime.
> 
> So as I understood it we need to stuff inside this structure. Or should we
> instead conditionally define it depending on the platform? Now I am confused!
> :)

Made sense to me after reading a bit more of the code. No, I think your original patch is right. This is so that, say, if a port is able to use both the Mac Media Player, and the GStreamer one, you'll have both available, and choose which to use in run time.

> > This seems totally unrelated?
> 
> It is not! :) canPlay() triggers a call to endedPlayback() which queries the
> duration. This query is useless, just checking paused() is enough in this
> context and this what MediaControlPlayButtonElement::updateDisplayType does,
> FWIW.

Hah. OK, then. I wonder if it makes sense as a separate change, though?

[Eric Carlson]

(In reply to comment #7)
> (In reply to comment #6)
> > // Structure that will hold every native
> > // types supported by the current media player.
> > // We have to do that has multiple media players
> > // backend can live at runtime.
> > 
> > So as I understood it we need to stuff inside this structure. Or should we
> > instead conditionally define it depending on the platform? Now I am confused!
> > :)
> 
> Made sense to me after reading a bit more of the code. No, I think your
> original patch is right. This is so that, say, if a port is able to use both
> the Mac Media Player, and the GStreamer one, you'll have both available, and
> choose which to use in run time.
> 
Actually I don't agree. The current member name was poorly chosen and should change. The value is really media engine specific, and I think it would make more sense to have a generic type that is cast to the specific type when it is used.

If a port is able to use both a QuickTime and GStreamer backend I would expect it to have a different media engine for each, and obviously only one will be used for a single movie.

[Philippe Normand]

(In reply to comment #7)

> 
> > > This seems totally unrelated?
> > 
> > It is not! :) canPlay() triggers a call to endedPlayback() which queries the
> > duration. This query is useless, just checking paused() is enough in this
> > context and this what MediaControlPlayButtonElement::updateDisplayType does,
> > FWIW.
> 
> Hah. OK, then. I wonder if it makes sense as a separate change, though?

Please re-read the bug description :) This change is the main reason for this bug.

[Gustavo Noronha (kov)]

(In reply to comment #8)
> Actually I don't agree. The current member name was poorly chosen and should
> change. The value is really media engine specific, and I think it would make
> more sense to have a generic type that is cast to the specific type when it is
> used.
>
> If a port is able to use both a QuickTime and GStreamer backend I would expect
> it to have a different media engine for each, and obviously only one will be
> used for a single movie.

Right, but how will it decide which one to use in runtime, then?

[Gustavo Noronha (kov)]

(In reply to comment #9)
> (In reply to comment #7)
> 
> > 
> > > > This seems totally unrelated?
> > > 
> > > It is not! :) canPlay() triggers a call to endedPlayback() which queries the
> > > duration. This query is useless, just checking paused() is enough in this
> > > context and this what MediaControlPlayButtonElement::updateDisplayType does,
> > > FWIW.
> > 
> > Hah. OK, then. I wonder if it makes sense as a separate change, though?
> 
> Please re-read the bug description :) This change is the main reason for this
> bug.

I know, but AFAIK we can commit this change first, along with the media duration changes, and then on a second commit add the check to only handle messages coming from playbin, right?

[Eric Carlson]

(In reply to comment #10)
> (In reply to comment #8)
> > Actually I don't agree. The current member name was poorly chosen and should
> > change. The value is really media engine specific, and I think it would make
> > more sense to have a generic type that is cast to the specific type when it is
> > used.
> >
> > If a port is able to use both a QuickTime and GStreamer backend I would expect
> > it to have a different media engine for each, and obviously only one will be
> > used for a single movie.
> 
> Right, but how will it decide which one to use in runtime, then?

Register a media engine for both backends and *you* will get to decide based on the content type. chooseBestEngineForTypeAndCodecs() in MediaPlayer.cpp always iterates through every registered media engine when it is asked to open a movie, asking each one if it supports the movie's (extended) MIME type, and uses the one that claims to have the best support.

[Philippe Normand]

Created attachment 50698 [details]
updated patch

[Philippe Normand]

(In reply to comment #13)
> Created an attachment (id=50698) [details]
> updated patch

This patch doesn't contain the platformMedia stuff, it will be added in a patch for Bug 36112. So hopefully we can now get this reduced patch in? :)

[Gustavo Noronha (kov)]

Comment on attachment 50698 [details]
updated patch

You're still adding too much to one single change. You're changing three separate things that are easily separatable: the decision on how to paint the button, the changes to media duration query, and the filtering of the messages. I understand they are all part of fixing this bug, but this does not mean they should go in at the same time. Having all of them in at the same time will make this commit semi-unuseful for bisecting. What caused a problem with the play/pause behavior changing, if it does? If we find this commit to be the problem, we'll have to manually check which of the three changes is responsible. So, I'm OK with all three changes, but let's split them, and then we'll get them in, yeah =).

[Philippe Normand]

Created attachment 50703 [details]
micro patched :)

[Gustavo Noronha (kov)]

Comment on attachment 50703 [details]
micro patched :)

Thanks!

[Philippe Normand]

Landed in r56006

[Gustavo Noronha (kov)]

This makes videos not go out of 'playing' when they end, UI-wise. I assume the real problem is we are not marking the video 'paused' attribute to true correctly, but before we fix this, using that attribute to make UI decisions sounds unwise. Let's make this testable, fix the root cause, and then we can reland the optimization.

[Gustavo Noronha (kov)]

Created attachment 50789 [details]
a possible layout test for the root cause

The spec is not very specific on whether video.paused should be true when the video ends, but I think it should be if we're not looping, from my reading, since I consider that to be a case of 'paused for user interaction' - the user needs to take action to get the video playing again, or to seek it. If it is not, then we need to improve this optimization to also take the ended attribute into consideration. I made this test based on media/video-loop.html, and we currently fail it. Eric, can you check if Mac passes this test? Your help would be appreciated in figuring this behaviour out =).

[Philippe Normand]

The spec also mentions:

It is possible for a media element to have both ended playback and paused for user interaction at the same time.

Our player pauses itself at the end of playback but the media element doesn't know about it because its pause() method doesn't query the player.

So I think we could have it. It would require the HTMLMediaElement::paused() to proxy to MediaPlayer::paused(), when a player is available. If not the m_paused attribute could be used as fallback. What do you think?

[Eric Carlson]

(In reply to comment #21)
> The spec also mentions:
> 
> It is possible for a media element to have both ended playback and paused for user interaction at the 
> same time.
> 
> Our player pauses itself at the end of playback but the media element doesn't know about it because its 
> pause() method doesn't query the player.
> 
> So I think we could have it. It would require the HTMLMediaElement::paused() to proxy to 
> MediaPlayer::paused(), when a player is available. If not the m_paused attribute could be used as 
> fallback. What do you think?

I think it is definitely worth a try. Hopefully it won't require significant changes to layout tests, if it does I fear that it will break existing content, but lets see.

[Philippe Normand]

(In reply to comment #22)
> (In reply to comment #21)

> > So I think we could have it. It would require the HTMLMediaElement::paused() to proxy to 
> > MediaPlayer::paused(), when a player is available. If not the m_paused attribute could be used as 
> > fallback. What do you think?
> 
> I think it is definitely worth a try. Hopefully it won't require significant changes to layout tests, if it does I fear that it will break existing content, but lets see.

Tried this, the patch breaks 7 media tests in the GTK+ port. So I think we should leave the ::paused() method as it is now.

Eric, what do you think about Gustavo's proposed test in comment 20 ?

[Philippe Normand]

(In reply to comment #20)
> Created an attachment (id=50789) [details]
> a possible layout test for the root cause
> 
> The spec is not very specific on whether video.paused should be true when the video ends, but I think it should be if we're not looping, from my reading, since I consider that to be a case of 'paused for user interaction' - the user needs to take action to get the video playing again, or to seek it. If it is not, then we need to improve this optimization to also take the ended attribute into consideration. I made this test based on media/video-loop.html, and we currently fail it. Eric, can you check if Mac passes this test? Your help would be appreciated in figuring this behaviour out =).


The test fails on Mac too. At the end of playback ended is true and paused is false.

[Philippe Normand]

Created attachment 61367 [details]
updated patch

[Eric Carlson]

Comment on attachment 61367 [details]
updated patch

> +    Node* node = o->node();
> +    if (!node)
> +        return false;
> +
> +    MediaControlPlayButtonElement* btn = static_cast<MediaControlPlayButtonElement*>(node);
> +    if (!btn)
>          return false;
>  
Is it possible for node to be non-NULL and the static cast to return NULL? IOW, is the second test necessary?

r=me

[Philippe Normand]

(In reply to comment #26)
> (From update of attachment 61367 [details])
> > +    Node* node = o->node();
> > +    if (!node)
> > +        return false;
> > +
> > +    MediaControlPlayButtonElement* btn = static_cast<MediaControlPlayButtonElement*>(node);
> > +    if (!btn)
> >          return false;
> >  
> Is it possible for node to be non-NULL and the static cast to return NULL? IOW, is the second test necessary?
> 
> r=me

Right the second test is overkill , sorry ;)

[Philippe Normand]

Created attachment 61386 [details]
updated patch

[Philippe Normand]

Landed in http://trac.webkit.org/changeset/63214
Thanks for the review!