Bug 68589 – [Qt][Gtk] Wrong state when pausing a video in the "playing" event handler

• Component: Media
• Status: RESOLVED FIXED

[Yael]

When running the test from http://dvcs.w3.org/hg/html/file/233364893066/tests/approved/video/event_playing.html , the video is playing, but the controls are paused.

The test has an event listener on the "playing" event, and the event listener pauses the video.
The call to pause arrives before gstreamer switches state to GST_STATE_PLAY, so we do not propagate the pause to gstreamer.
I see at least 2 ways to fix this:
1. Always pause gstreamer, if m_pausedInternal is true.
2. In MediaPLayerPrivateGstreamer::paused() always check gstreamer's state instead of returning the cached state.
I will submit a patch soon, and would appreciate feedback on it.

[Yael]

Created attachment 108262 [details]
Patch.

[Philippe Normand]

That test passes fine here. Just to be clear, is the issue happening when you explicitely play the video?

[Philippe Normand]

It seems to work fine in webkitgtk. I hear the first "tick" of the video and the controls show a paused video (in the gtk controls we show the "play" icon in that case).

Could it be an issue in the Qt RenderTheme code?

[Alexis Menard (darktears)]

(In reply to comment #2)
> That test passes fine here. Just to be clear, is the issue happening when you explicitely play the video?

Setting the src will play the video, then it will start the playback in MediaPlayerGstreamer which will not set its internal state to play right away (i.e. m_paused to false) but HMTMLMediaElement will trigger the play event which is catch in the test by the handler and it will then call pause on the HTMLMediaElement, which will try to pause the media player but one of its code path check if the player is not already paused by calling paused() on it, and MediaPlayerGStreamer will return m_paused (which is still true, the default) as we haven't enter *yet* to busMessage to process the state change.

Chrome and Safari behaves correctly but Firefox doesn't for example.

On a side the QuickTime backend has also an internal m_startedPlaying to manage the state but the paused() function always query QuickTime rather than using it.

I hope I was clear enough with my explanation.

(In reply to comment #3)
> It seems to work fine in webkitgtk. I hear the first "tick" of the video and the controls show a paused video (in the gtk controls we show the "play" icon in that case).
> 
> Could it be an issue in the Qt RenderTheme code?

Actually I don't think so. With Qt our controls behave perfectly, like they show a play button, don't update the time and so on. What is wrong is that the video playback continues.

[Yael]

Created attachment 108319 [details]
log

[Philippe Normand]

The change looks good but it be best to have a real Layout test for it.

[Yael]

Created attachment 108344 [details]
Patch.

Added a layout test.

[Philippe Normand]

Comment on attachment 108344 [details]
Patch.

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

Thanks for the test, just some nits to fix.

> LayoutTests/media/video-playing-and-pause.html:7
> +            var timeAfterPlay;

This variable is not used, not needed :)

> LayoutTests/media/video-playing-and-pause.html:13
> +                v.src="content/counting.ogv";

findMediaElement();
video.src = findMediaFile("video", "content/test");

doesn't work?

> LayoutTests/media/video-playing-and-pause.html:26
> +                var v = document.getElementById("v");

This is not needed. findMediaElement(), if called before, will set a global video variable.

> LayoutTests/media/video-playing-and-pause.html:27
> +                v.pause();

video.pause() should work

> LayoutTests/media/video-playing-and-pause.html:35
> +                    document.getElementById("parentDiv").innerHTML = "FAIL";

You need endTest() after this statement, I think. Otherwise the test might just time out.

> LayoutTests/media/video-playing-and-pause.html:47
> +        <p>Test that pausing the media element in "playing" event handler pauses the media immediately.</p>

Maybe worth addind a comment about the result showing the first frame, I leave that up to you.

> LayoutTests/media/video-playing-and-pause.html:48
> +        <video id="v" autoplay controls></video>

No need for an id.

[Yael]

Created attachment 108360 [details]
Patch.

Address comment #8

[Yael]

Created attachment 108401 [details]
Patch.

Update the test to use video-paint-test.js instead of video-test.js.
This test is very similar to controls-strict.html, but it does not perform a "seek" after pausing.

[Philippe Normand]

Comment on attachment 108401 [details]
Patch.

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

Looks good, just a nit to fix before landing. Thanks!

> LayoutTests/media/video-paint-test.js:46
> +            document.body.offsetLeft;

I don't think this is needed. I have no idea what the purpose of this is in the other init function either ;) Can you remove it from initAndPause() please?

[Yael]

Created attachment 108465 [details]
Patch for landing

Removed redundant line in the new test and rebaselined the results.

[Yael]

(In reply to comment #11)
> (From update of attachment 108401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108401&action=review
> 
> Looks good, just a nit to fix before landing. Thanks!
> 
> > LayoutTests/media/video-paint-test.js:46
> > +            document.body.offsetLeft;
> 
> I don't think this is needed. I have no idea what the purpose of this is in the other init function either ;) Can you remove it from initAndPause() please?

Thanks for reviewing :)

[WebKit Review Bot]

Comment on attachment 108465 [details]
Patch for landing

Clearing flags on attachment: 108465

Committed r95799: <http://trac.webkit.org/changeset/95799>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.