Bug 54910 – media/video-replaces-poster.html fails on all platforms

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Anna Cavender]

This test fails because it assumes the results of a seek will be immediately available, when in fact the seek is asynchronous.  See also, http://code.google.com/p/chromium/issues/detail?id=60244.

Flakiness dashboard: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=video-replaces&showExpectations=true

[Anna Cavender]

Created attachment 83217 [details]
Patch

[Darin Adler]

Comment on attachment 83217 [details]
Patch

The change looks fine.

But the patch has tabs in it, so it will fail to apply. You need to make a new version without tabs.

[Anna Cavender]

Created attachment 83236 [details]
Patch

[Eric Carlson]

Comment on attachment 83236 [details]
Patch

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

This is a separate issue, but won't other tests that use video-paint-test.js have this same problem?

Marking r+ but I would prefer to see the ChangeLog comment broken into multiple lines so it is easier to read in an editor that doesn't wrap.

> LayoutTests/ChangeLog:5
> +        Fix for media/video-replaces-poster.html.  The test was failing because it assumes the results of a seek will be immediately available, when in fact the seek is asynchronous.  I did not remove the line in test-expectations because rebaselines for many (all?) platforms will likely be needed after this patch lands.  See also, http://code.google.com/p/chromium/issues/detail?id=60244

That is one very long line for a ChangeLog ;-)

[Anna Cavender]

Comment on attachment 83236 [details]
Patch

In this case, relying on "playing" event led to inconsistent results in pixel diffs.  Other tests that use video-paint-test.js seem less affected by this timing issue (so far), but I will look into those as well.

And I promise to make changelog comments smaller in the future :).

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 83236 [details]:

animations/suspend-resume-animation.html bug 48161 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 83236 [details]
Patch

Clearing flags on attachment: 83236

Committed r79401: <http://trac.webkit.org/changeset/79401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Anna Cavender]

Created attachment 83583 [details]
Patch

[Anna Cavender]

adding new baselines to match the new tests.

[Eric Seidel (no email)]

Comment on attachment 83583 [details]
Patch

OK.

[WebKit Commit Bot]

Comment on attachment 83583 [details]
Patch

Rejecting attachment 83583 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'bu..." exit_code: 2

Last 500 characters of output:
l x86_64 objective-c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/PageCache.o /Projects/CommitQueue/Source/WebCore/history/PageCache.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/PageGroup.o /Projects/CommitQueue/Source/WebCore/page/PageGroup.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(4 failures)


Full output: http://queues.webkit.org/results/7984543

[WebKit Review Bot]

Comment on attachment 83583 [details]
Patch

Rejecting attachment 83583 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Anna Cavender]

Thanks Mihai :-D.

I don't think those build errors have anything to do with this patch.

[WebKit Commit Bot]

Comment on attachment 83583 [details]
Patch

Clearing flags on attachment: 83583

Committed r79635: <http://trac.webkit.org/changeset/79635>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

FYI this test is still really flaky on all chromium platforms (it often fails in pixel mode).  I'm unsure if it is flaky on other platforms as well, since it's a pixel failure only the other bots will not pick it up:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=media%2Fvideo-replaces-poster.html&group=%40ToT%20-%20chromium.org

[Anna Cavender]

Shoot.  My original intent was to remove any ambiguity about what frame the video lands on, but forgot about the autoplay flag.

Let's try that again, this time with no autoplay and make sure the video lands on 0.  Patch should be up in a sec.

[Anna Cavender]

Created attachment 83929 [details]
Patch

[Anna Cavender]

I want to make sure all platforms report the 0th frame before rebaselining and removing from test_expectations.txt

[Anna Cavender]

My original intent was to remove any ambiguity about what frame the video lands on, but I forgot about the autoplay flag.  This patch removes autoplay and makes sure the video lands on 0.

[Eric Carlson]

Comment on attachment 83929 [details]
Patch

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

This change defeats the purpose of the test, to ensure that the poster image is replaced by a frame of video, because the poster is displayed until playback begins or a seek occurs (see https://bugs.webkit.org/show_bug.cgi?id=37591).

Seeking to some time and seeking back to time 0 may work, though I don't see why that would be different from seeking to 1 as the current test does.

> LayoutTests/media/video-replaces-poster.html:13
> -                video.pause();
> -                video.currentTime = 1;  // so the snapshot always has the same frame.
> +                video.currentTime = 0;  // so the snapshot always has the same frame.

Do you have a theory about why a snapshot at time 0 works better than a snapshot at 1 second?

[Anna Cavender]

Seeking to time 0 will replace the poster with the first frame of the video, so this should still test the right thing.

But, I think you are right, seeking to time 1 should have the same effect as seeking to time 0.  I had just changed that because I was trying to figure out why some tests showed 00:00.24 in the image and some showed 00:02.03.  Pretty sure that is due to the pause() after 'autoplay' landing on different times for different platforms combined with the asynchronous response to "seeked" (or any other event) resulting in unpredictable pixel repainting.

Do we really have to play() or 'autoplay' the video for this test?

[Eric Carlson]

(In reply to comment #23)
> Seeking to time 0 will replace the poster with the first frame of the video, so this should still test the right thing.
> 
  Oops, you are correct!


> But, I think you are right, seeking to time 1 should have the same effect as seeking to time 0.  I had just changed that because I was trying to figure out why some tests showed 00:00.24 in the image and some showed 00:02.03.  Pretty sure that is due to the pause() after 'autoplay' landing on different times for different platforms combined with the asynchronous response to "seeked" (or any other event) resulting in unpredictable pixel repainting.
> 
  I wonder if the problem is that the video pipeline hasn't necessarily pushed out the new frame of video yet when the 'seeked' event fires?

> Do we really have to play() or 'autoplay' the video for this test?
>
  No, seeking like you have done should also replace the poster. 

  I am afraid that the only way to check one method or another is to land a change because the results are inconsistent. 

  Maybe it is worth seeking to time 1 and adding "checkExpected(video.currentTime, 1)" in the 'seeked' function so we will at least have more information if the test continues to fail.

[Anna Cavender]

Sounds like a good plan.  I'll put up a patch later this afternoon.

[Anna Cavender]

Created attachment 84132 [details]
Patch

[Eric Carlson]

Comment on attachment 84132 [details]
Patch

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

r+ but I would prefer to have at least the comment updated before landing.

> LayoutTests/media/video-replaces-poster.html:25
> +        function checkExpected(actual, expected)
> +        {
> +            if (actual != expected)
> +                document.writeln("<p>FAILURE: expected " + expected + ", actual " + actual + "</p>");
> +        }

As long as this test needs to be updated, can you include video-test.js and use one of its functions?

> LayoutTests/media/video-replaces-poster.html:31
>      You should see the video play below.</p>

This comment is no longer correct.

[Anna Cavender]

Created attachment 84297 [details]
Patch

[Anna Cavender]

Please note, including video-test.js (in order to use testExpected()) reduces the output for the text part of the results.  This is because it prefers layoutTestController.dumpAsText().  I think this should be fine (rebaselines will be needed anywase), but just wanted to let reviewers know.

[Eric Carlson]

(In reply to comment #29)
> Please note, including video-test.js (in order to use testExpected()) reduces the output for the text part of the results.  This is because it prefers layoutTestController.dumpAsText().  I think this should be fine (rebaselines will be needed anywase), but just wanted to let reviewers know.

This is a test of the video output so I agree that is should be OK. A side benefit is that you can get rid of all of the platform specific video-replaces-poster-expected.txt.

[Anna Cavender]

Created attachment 84318 [details]
Patch

[Anna Cavender]

Created attachment 84319 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 84319 [details]:

media/controls-without-preload.html bug 55555 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 84319 [details]
Patch

Clearing flags on attachment: 84319

Committed r80098: <http://trac.webkit.org/changeset/80098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.