Bug 54910

Summary: media/video-replaces-poster.html fails on all platforms
Product: WebKit Reporter: Anna Cavender <annacc>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, jamesr, mihaip
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Anna Cavender 2011-02-21 14:47:54 PST
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
Comment 1 Anna Cavender 2011-02-21 14:50:51 PST
Created attachment 83217 [details]
Patch
Comment 2 Darin Adler 2011-02-21 15:01:10 PST
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.
Comment 3 Anna Cavender 2011-02-21 16:11:38 PST
Created attachment 83236 [details]
Patch
Comment 4 Eric Carlson 2011-02-22 08:54:42 PST
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 ;-)
Comment 5 Anna Cavender 2011-02-22 11:01:19 PST
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 :).
Comment 6 WebKit Commit Bot 2011-02-22 21:49:49 PST
The commit-queue encountered the following flaky tests while processing attachment 83236 [details]:

animations/suspend-resume-animation.html bug 48161 (author: cmarrin@apple.com)
The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2011-02-22 21:51:28 PST
Comment on attachment 83236 [details]
Patch

Clearing flags on attachment: 83236

Committed r79401: <http://trac.webkit.org/changeset/79401>
Comment 8 WebKit Commit Bot 2011-02-22 21:51:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Anna Cavender 2011-02-23 17:01:23 PST
Created attachment 83583 [details]
Patch
Comment 10 Anna Cavender 2011-02-23 17:03:56 PST
adding new baselines to match the new tests.
Comment 11 Eric Seidel (no email) 2011-02-24 02:36:42 PST
Comment on attachment 83583 [details]
Patch

OK.
Comment 12 WebKit Commit Bot 2011-02-24 12:26:47 PST
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
Comment 13 WebKit Review Bot 2011-02-24 13:05:21 PST
Comment on attachment 83583 [details]
Patch

Rejecting attachment 83583 [details] from commit-queue.

annacc@chromium.org 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.
Comment 14 Anna Cavender 2011-02-24 13:30:48 PST
Thanks Mihai :-D.

I don't think those build errors have anything to do with this patch.
Comment 15 WebKit Commit Bot 2011-02-24 15:41:42 PST
Comment on attachment 83583 [details]
Patch

Clearing flags on attachment: 83583

Committed r79635: <http://trac.webkit.org/changeset/79635>
Comment 16 WebKit Commit Bot 2011-02-24 15:41:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 James Robinson 2011-02-25 12:00:45 PST
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
Comment 18 Anna Cavender 2011-02-25 22:18:03 PST
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.
Comment 19 Anna Cavender 2011-02-25 22:22:12 PST
Created attachment 83929 [details]
Patch
Comment 20 Anna Cavender 2011-02-25 22:23:33 PST
I want to make sure all platforms report the 0th frame before rebaselining and removing from test_expectations.txt
Comment 21 Anna Cavender 2011-02-28 08:42:22 PST
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.
Comment 22 Eric Carlson 2011-02-28 09:17:10 PST
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?
Comment 23 Anna Cavender 2011-02-28 09:28:31 PST
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?
Comment 24 Eric Carlson 2011-02-28 09:38:33 PST
(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.
Comment 25 Anna Cavender 2011-02-28 09:41:43 PST
Sounds like a good plan.  I'll put up a patch later this afternoon.
Comment 26 Anna Cavender 2011-02-28 15:17:56 PST
Created attachment 84132 [details]
Patch
Comment 27 Eric Carlson 2011-03-01 09:51:23 PST
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.
Comment 28 Anna Cavender 2011-03-01 14:39:51 PST
Created attachment 84297 [details]
Patch
Comment 29 Anna Cavender 2011-03-01 14:44:10 PST
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.
Comment 30 Eric Carlson 2011-03-01 14:57:40 PST
(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.
Comment 31 Anna Cavender 2011-03-01 16:01:14 PST
Created attachment 84318 [details]
Patch
Comment 32 Anna Cavender 2011-03-01 16:06:02 PST
Created attachment 84319 [details]
Patch
Comment 33 WebKit Commit Bot 2011-03-01 23:32:29 PST
The commit-queue encountered the following flaky tests while processing attachment 84319 [details]:

media/controls-without-preload.html bug 55555 (authors: jamesr@chromium.org and pnormand@igalia.com)
The commit-queue is continuing to process your patch.
Comment 34 WebKit Commit Bot 2011-03-01 23:34:37 PST
Comment on attachment 84319 [details]
Patch

Clearing flags on attachment: 84319

Committed r80098: <http://trac.webkit.org/changeset/80098>
Comment 35 WebKit Commit Bot 2011-03-01 23:34:44 PST
All reviewed patches have been landed.  Closing bug.