Bug 68589 - [Qt][Gtk] Wrong state when pausing a video in the "playing" event handler
Summary: [Qt][Gtk] Wrong state when pausing a video in the "playing" event handler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-21 17:26 PDT by Yael
Modified: 2011-09-23 07:03 PDT (History)
4 users (show)

See Also:


Attachments
Patch. (1.63 KB, patch)
2011-09-21 17:56 PDT, Yael
no flags Details | Formatted Diff | Diff
log (1.34 MB, text/plain)
2011-09-22 06:00 PDT, Yael
no flags Details
Patch. (27.65 KB, patch)
2011-09-22 09:17 PDT, Yael
pnormand: review-
Details | Formatted Diff | Diff
Patch. (36.15 KB, patch)
2011-09-22 10:54 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (27.33 KB, patch)
2011-09-22 14:22 PDT, Yael
pnormand: review+
Details | Formatted Diff | Diff
Patch for landing (27.25 KB, patch)
2011-09-23 05:44 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2011-09-21 17:26:45 PDT
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.
Comment 1 Yael 2011-09-21 17:56:56 PDT
Created attachment 108262 [details]
Patch.
Comment 2 Philippe Normand 2011-09-22 01:17:11 PDT
That test passes fine here. Just to be clear, is the issue happening when you explicitely play the video?
Comment 3 Philippe Normand 2011-09-22 05:25:31 PDT
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?
Comment 4 Alexis Menard (darktears) 2011-09-22 05:39:22 PDT
(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.
Comment 5 Yael 2011-09-22 06:00:28 PDT
Created attachment 108319 [details]
log
Comment 6 Philippe Normand 2011-09-22 07:16:28 PDT
The change looks good but it be best to have a real Layout test for it.
Comment 7 Yael 2011-09-22 09:17:13 PDT
Created attachment 108344 [details]
Patch.

Added a layout test.
Comment 8 Philippe Normand 2011-09-22 09:39:39 PDT
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.
Comment 9 Yael 2011-09-22 10:54:39 PDT
Created attachment 108360 [details]
Patch.

Address comment #8
Comment 10 Yael 2011-09-22 14:22:43 PDT
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.
Comment 11 Philippe Normand 2011-09-23 01:48:28 PDT
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?
Comment 12 Yael 2011-09-23 05:44:15 PDT
Created attachment 108465 [details]
Patch for landing

Removed redundant line in the new test and rebaselined the results.
Comment 13 Yael 2011-09-23 05:44:34 PDT
(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 :)
Comment 14 WebKit Review Bot 2011-09-23 07:02:55 PDT
Comment on attachment 108465 [details]
Patch for landing

Clearing flags on attachment: 108465

Committed r95799: <http://trac.webkit.org/changeset/95799>
Comment 15 WebKit Review Bot 2011-09-23 07:03:01 PDT
All reviewed patches have been landed.  Closing bug.