Bug 33015 - [GTK] Failing media/video-seek-past-end-playing.html
: [GTK] Failing media/video-seek-past-end-playing.html
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
: Gtk
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-29 05:03 PST by Gustavo Noronha (kov)
Modified: 2010-01-14 04:12 PST (History)
1 user (show)

See Also:


Attachments
listen on timeupdate event instead of waiting a fixed amount of time, which is prone to test flackyness (4.92 KB, patch)
2010-01-12 05:32 PST, Philippe Normand
eric.carlson: review+
Details | Formatted Diff | Diff
Removed the last timer of this test (2.51 KB, patch)
2010-01-13 00:15 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Removed the last timer of this test (3.01 KB, patch)
2010-01-14 03:56 PST, Philippe Normand
xan.lopez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2009-12-29 05:03:44 PST
This test runs on and off locally, but seems to always fail in the release bot.
Comment 1 Gustavo Noronha (kov) 2009-12-29 05:05:51 PST
Skipped in r52616.
Comment 2 Philippe Normand 2010-01-11 03:28:39 PST
The seek back to beginning is done and the pipeline is in PLAYING but haven't had time to play any data :/
Comment 3 Philippe Normand 2010-01-12 03:19:41 PST
That test is really flacky in our case because it expects that currentTime has been updated 200ms after play() was called. But the pipeline can take more than 200ms to preroll and start playing for real...

So I suggest to remove that 200ms timeout from the test and listen on timeupdate instead. The second time it is fired we are sure that playback really started and that currentTime is > 0. What do you think?

I can see the exact same issue in media/audio-mpeg-supported.html.

Moreover I found out that our player performs un-needed seeks:

- seek(0): un-needed when currentTime is 0 already
- setRate(1): un-needed at beginning of playback as it is already the default in playbin.
Comment 4 Gustavo Noronha (kov) 2010-01-12 03:59:40 PST
(In reply to comment #3)
> That test is really flacky in our case because it expects that currentTime has
> been updated 200ms after play() was called. But the pipeline can take more than
> 200ms to preroll and start playing for real...
> 
> So I suggest to remove that 200ms timeout from the test and listen on
> timeupdate instead. The second time it is fired we are sure that playback
> really started and that currentTime is > 0. What do you think?

Sounds like a plan to me. =)

> I can see the exact same issue in media/audio-mpeg-supported.html.

Aha, well, I'm all for removing timeouts, and making the tests state-based.

> Moreover I found out that our player performs un-needed seeks:
> 
> - seek(0): un-needed when currentTime is 0 already
> - setRate(1): un-needed at beginning of playback as it is already the default
> in playbin.

Make patches =D.
Comment 5 Philippe Normand 2010-01-12 05:32:39 PST
Created attachment 46364 [details]
listen on timeupdate event instead of waiting a fixed amount of time, which is prone to test flackyness
Comment 6 Eric Carlson 2010-01-12 08:45:20 PST
Comment on attachment 46364 [details]
listen on timeupdate event instead of waiting a fixed amount of time, which is prone to test flackyness

In both tests set timeupdateEventCount to 0 before ending the test. This isn't necessary and should be removed. A comment about why the test ends after two timpupdate events would be nice.

r=me with these minor changes.

Extra points if you cleanup video-seek-past-end-playing.html to make it a reasonable html file instead of the ugly fragment it is now :-)
Comment 7 Philippe Normand 2010-01-12 10:34:52 PST
Added the comments, made the test an html document and landed as r53147. Thanks!
Comment 8 Philippe Normand 2010-01-12 11:35:30 PST
The last timer of the test needs to be killed, the test still fails on the release bot.
Comment 9 Philippe Normand 2010-01-13 00:15:04 PST
Created attachment 46430 [details]
Removed the last timer of this test
Comment 10 Philippe Normand 2010-01-14 03:51:52 PST
Comment on attachment 46364 [details]
listen on timeupdate event instead of waiting a fixed amount of time, which is prone to test flackyness

This one was landed.
Comment 11 Philippe Normand 2010-01-14 03:56:49 PST
Created attachment 46554 [details]
Removed the last timer of this test
Comment 12 Xan Lopez 2010-01-14 03:58:56 PST
Comment on attachment 46554 [details]
Removed the last timer of this test

LGTM.
Comment 13 Philippe Normand 2010-01-14 04:12:22 PST
Landed as r53258, Thanks!