Bug 33671 – media/video-loop.html fails intermittently on GTK and Chromium bots

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Philippe Normand]

That test does a seek() and fires up a timer. In our player the pipeline can take time to complete the seek and in some cases the timer will be fired too early. That test should be rewritten to rely on timeupdate events instead of a timer, in my opinion.

[Philippe Normand]

Created attachment 46571 [details]
refactored test

[Philippe Normand]

(In reply to comment #1)
> Created an attachment (id=46571) [details]
> refactored test

This is a proposed solution, can it be tested on mac as well? I had to change a line of the expected result and I am not totally sure it is correct.

[Eric Seidel (no email)]

*** Bug 33497 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

It seems that either r53277 or 53281 caused this to start failing consistently.  It's been failing for the last 8 builds on the Gtk builder.

[Eric Seidel (no email)]

Never mind, it failed 7 out of 8 builds.  It did not fail in http://build.webkit.org/results/GTK%20Linux%20Release/r53283%20(7663)/results.html

[Gustavo Noronha (kov)]

I preffer to have Eric Carlson or someone else with more knowledge than me reviewing this patch. I'll skip the test while you get this one reviewed, please unskip when committing! =)

[Eric Carlson]

Comment on attachment 46571 [details]
refactored test

This patch has several problems: 

> else if (timeupdateEventCount >= 10)
>
This magic number seems fragile, if a media engine doesn't fire 10 'timeupdate' events (because of processor load or whatever) it will fail.

> testExpected("mediaElement.currentTime", (video.duration - 0.4).toFixed(2), '<');
>
This will log the movie's duration, which is not exactly the on every platform because of the different movie files used.

> if (timeupdateEventCount == 2) {
>    // make sure we are playing, seek to near the end so
>
Three space tabs?

> function ended() {
>
Brace should be on the next line.

In general, replacing a timer with a 'timeupdate' event handler seems wrong, as that event is extremely platform and machine dependent. I would rather see this test rewritten in a way that removes as much indeterminism as possible. 

I would also like to see revised tests cleaned up so they have reasonable looking markup. You didn't make this any worse, but the lack of a body, the stray "</head>", etc should be cleaned up as long as the file is being changed.

marking r-

[Eric Carlson]

Why don't I revise this test and reattach so you can test on a GTK machine to make sure my changes fix the intermittent failures.

[Philippe Normand]

(In reply to comment #8)
> Why don't I revise this test and reattach so you can test on a GTK machine to
> make sure my changes fix the intermittent failures.

Ok, you will be faster than me at reworking this test :) Thanks for the review anyway, I will be happy to test here the reworked test if needed.

[Eric Carlson]

Created attachment 47354 [details]
Revised test

Can you see if is more consistent?

[Philippe Normand]

(In reply to comment #10)
> Created an attachment (id=47354) [details]
> Revised test
> 
> Can you see if is more consistent?

I was unable to make it fail :)

[Eric Carlson]

Created attachment 47777 [details]
Proposed patch

[Eric Seidel (no email)]

Attachment 47777 [details] was posted by a committer and has review+, assigning to Eric Carlson for commit.

[Eric Carlson]

Comment on attachment 47777 [details]
Proposed patch

Clearing the r+ flag for now because I need to generate new results but won't be able to get to it for a bit.

[Eric Carlson]

Created attachment 64860 [details]
patch

Revised slightly to pause before seeking to try to avoid looping before the 'seeked' event fires on really slow machines.

[Gustavo Noronha (kov)]

Could you please take the opportunity to unskip the test, too? =)

[Eric Carlson]

trac.webkit.org/changeset/65684

[Eric Carlson]

(In reply to comment #16)
> Could you please take the opportunity to unskip the test, too? =)

Darn, missed your comment.

[Philippe Normand]

(In reply to comment #18)
> (In reply to comment #16)
> > Could you please take the opportunity to unskip the test, too? =)
> 
> Darn, missed your comment.

Unskipped in http://trac.webkit.org/changeset/66487