Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1266113003:
Rebaseline yuv-video-on-accelerated-canvas.html (Closed)
|
Created:
5 years, 4 months ago by Daniele Castagna Modified:
5 years, 4 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRebaseline yuv-video-on-accelerated-canvas.html
crrev.com/1144323003 refactors SkCanvasVideoRenderer and produces a slightly
different YUV conversion (differences between hw/sw) in certain cases.
This CL removes yuv-video-on-accelerated-canvas-expected.html and change
yuv-video-on-accelerated-canvas.html into a pixel test that will be
re-baselined once the refactor lands.
More info about this change in crrev.com/1144323003 (msg69).
BUG=449197
Patch Set 1 #Patch Set 2 : NeedsManualRebaseline and comment. #Patch Set 3 : Rebase on master. #
Total comments: 2
Messages
Total messages: 25 (7 generated)
dcastagna@chromium.org changed reviewers: + dongseong.hwang@intel.com
dcastagna@chromium.org changed reviewers: + junov@chromium.org
I am not a big fan of removing test coverage. Instead of removing this test, could we just change it to make int invariant to whether bilinear interpolation happens in YUV or RGB space? I see two ways of achieving this: a) use a video of a solid color b) change this test so that it does not scale the video
On 2015/08/10 at 19:09:55, junov wrote: > I am not a big fan of removing test coverage. > > Instead of removing this test, could we just change it to make int invariant to whether bilinear interpolation happens in YUV or RGB space? I see two ways of achieving this: > a) use a video of a solid color > b) change this test so that it does not scale the video Hi junov! Thank you for taking a look at this. Quick thoughts: a) The final color might still be slightly different. Floating point operations on CPU and GPU are not guaranteed to produce the same result. b) Not scaling the video would still produce different results. The UV planes are subsampled and the sampling for a non-scaled video doesn't happen at the center of the texel, causing the bilinear filter to produce different result than in the software conversion. The original goal for yuv-video-on-accelerated-canvas.html was to check that the software/hardware cache output would stay consistent. Now we're relaxing that constraint, accepting different results and getting some potential gain in performance. Given that, it seems reasonable to me to get rid of yuv-video-on-accelerated-canvas-expected.html. If you prefer not to remove yuv-video-on-accelerated-canvas-expected.html, I'd probably try to go with a) and find a video that does match the out color in both the conversion paths. dongseong.hwang, what do you think?
This test verifies that video->gpu-canvas works at all. The fact that it uses a SW canvas as a reference is just a design choice. A choice that I'd be glad to re-visit. There are plenty of ways of re-writing the test. Here is a suggestion: you could re-write this as a test with text results. The test would work by peeking the values of specific pixels of the canvas and checking them with some tolerance. In LayoutTests/resources/js-test.js there is a convenient helper function called 'shouldBeCloseTo'. You can use that on pixel values acquired by calling getImageData. There are plenty of canvas tests that are written this way. Most use 'shouldBe', but the ones that need some tolerance use 'shouldBeCloseTo'. A good example is: fast/canvas/canvas-blending-shadow.html
Ok I misread this CL. It removes the "Expected.html", not the actual test. Effectively transforming the test from a ref test into a pixel test. This is OK. As discussed, let's use NeedsManualRebaseline while we wait for the chromium-side CL to land
Also, should put a comment in TestExpectation that refers to the the Chromium code review, stating that we are waiting for that to land before re-baselining.
Summary/description is not clear. What matters about this CL is: a) Changing yuv-video-on-accelerated-canvas.hml into a pixel test; b) waiting for a chromium CL to land before rebaselining.
Thank you junov for all the suggestions. PTAL.
On 2015/08/10 20:46:55, Daniele Castagna wrote: > Thank you junov for all the suggestions. > PTAL. lgtm
The CQ bit was checked by dcastagna@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266113003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266113003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65679)
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/1266113003/#ps40001 (title: "Rebase on master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266113003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266113003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65681)
lgtm https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:773: crbug.com/477668 crbug.com/450699 crbug.com/449197 virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html [ NeedsManualRebaseline ] Following two tests need to be marked, also fast/canvas/yuv-video-on-accelerated-canvas.html virtual/display_list_2d_canvas/fast/canvas/yuv-video-on-accelerated-canvas.html
https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:773: crbug.com/477668 crbug.com/450699 crbug.com/449197 virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html [ NeedsManualRebaseline ] On 2015/08/11 at 06:34:43, dshwang wrote: > Following two tests need to be marked, also > fast/canvas/yuv-video-on-accelerated-canvas.html > virtual/display_list_2d_canvas/fast/canvas/yuv-video-on-accelerated-canvas.html I guess we should also remove the corresponding -expected.html files then, right?
On 2015/08/11 17:06:07, Daniele Castagna wrote: > https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... > File LayoutTests/TestExpectations (right): > > https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... > LayoutTests/TestExpectations:773: crbug.com/477668 crbug.com/450699 > crbug.com/449197 virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html [ > NeedsManualRebaseline ] > On 2015/08/11 at 06:34:43, dshwang wrote: > > Following two tests need to be marked, also > > fast/canvas/yuv-video-on-accelerated-canvas.html > > > virtual/display_list_2d_canvas/fast/canvas/yuv-video-on-accelerated-canvas.html > > I guess we should also remove the corresponding -expected.html files then, > right? correct
On 2015/08/11 18:11:03, dshwang wrote: > On 2015/08/11 17:06:07, Daniele Castagna wrote: > > > https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... > > File LayoutTests/TestExpectations (right): > > > > > https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... > > LayoutTests/TestExpectations:773: crbug.com/477668 crbug.com/450699 > > crbug.com/449197 virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > [ > > NeedsManualRebaseline ] > > On 2015/08/11 at 06:34:43, dshwang wrote: > > > Following two tests need to be marked, also > > > fast/canvas/yuv-video-on-accelerated-canvas.html > > > > > > virtual/display_list_2d_canvas/fast/canvas/yuv-video-on-accelerated-canvas.html > > > > I guess we should also remove the corresponding -expected.html files then, > > right? > > correct Eh, what corresponding -expected.html files? The virtual test suites all use the same reference.
On 2015/08/11 at 18:24:23, junov wrote: > On 2015/08/11 18:11:03, dshwang wrote: > > On 2015/08/11 17:06:07, Daniele Castagna wrote: > > > > > https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... > > > File LayoutTests/TestExpectations (right): > > > > > > > > https://codereview.chromium.org/1266113003/diff/40001/LayoutTests/TestExpecta... > > > LayoutTests/TestExpectations:773: crbug.com/477668 crbug.com/450699 > > > crbug.com/449197 virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html > > [ > > > NeedsManualRebaseline ] > > > On 2015/08/11 at 06:34:43, dshwang wrote: > > > > Following two tests need to be marked, also > > > > fast/canvas/yuv-video-on-accelerated-canvas.html > > > > > > > > > virtual/display_list_2d_canvas/fast/canvas/yuv-video-on-accelerated-canvas.html > > > > > > I guess we should also remove the corresponding -expected.html files then, > > > right? > > > > correct > > Eh, what corresponding -expected.html files? The virtual test suites all use the same reference. Got it. Since I found other two yuv-to-canvas tests that should be re-baselined I'm going to close this CL and create crrev.com/1273163005. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
