Bug 54910

Created attachment 83217 [details] Patch

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.

Created attachment 83236 [details] Patch

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 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 :).

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 on attachment 83236 [details] Patch Clearing flags on attachment: 83236 Committed r79401: <http://trac.webkit.org/changeset/79401>

All reviewed patches have been landed. Closing bug.

Created attachment 83583 [details] Patch

adding new baselines to match the new tests.

Comment on attachment 83583 [details] Patch OK.

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 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.

Thanks Mihai :-D. I don't think those build errors have anything to do with this patch.

Comment on attachment 83583 [details] Patch Clearing flags on attachment: 83583 Committed r79635: <http://trac.webkit.org/changeset/79635>

All reviewed patches have been landed. Closing bug.

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

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.

Created attachment 83929 [details] Patch

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

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 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?

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?

(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.

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

Created attachment 84132 [details] Patch

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.

Created attachment 84297 [details] Patch

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.

(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.

Created attachment 84318 [details] Patch

Created attachment 84319 [details] Patch

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 on attachment 84319 [details] Patch Clearing flags on attachment: 84319 Committed r80098: <http://trac.webkit.org/changeset/80098>

All reviewed patches have been landed. Closing bug.