Summary: | [Qt][Gtk] Wrong state when pausing a video in the "playing" event handler | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | eric.carlson, menard, pnormand, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Yael
2011-09-21 17:26:45 PDT
Created attachment 108262 [details]
Patch.
That test passes fine here. Just to be clear, is the issue happening when you explicitely play the video? 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? (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. Created attachment 108319 [details]
log
The change looks good but it be best to have a real Layout test for it. Created attachment 108344 [details]
Patch.
Added a layout test.
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. Created attachment 108360 [details] Patch. Address comment #8 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 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? Created attachment 108465 [details]
Patch for landing
Removed redundant line in the new test and rebaselined the results.
(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 on attachment 108465 [details] Patch for landing Clearing flags on attachment: 108465 Committed r95799: <http://trac.webkit.org/changeset/95799> All reviewed patches have been landed. Closing bug. |