Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
656683007:
Add a layout test for potential bug on drawing video on SkCanvas (Closed)
DescriptionAdd a layout test for potential bug on drawing video on SkCanvas
When drawing video frame on SkCanvas, chromium reuse the same cached SkBitmap
on both software SkCanvas and accelerated SkCanvas. After SkBitmap is used on
both SkCanvas, SkBitmap includes both GrTexture and system memory buffer. It's
weird and looks error-prone. Fortunately, it works now.
This CL adds new layout test to check if it will be broken. This potential bug is
fixed in https://codereview.chromium.org/662033002/
TEST=fast/canvas/yuv-video-on-accelerated-canvas.html
BUG=424591
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184073
Patch Set 1 #
Total comments: 2
Patch Set 2 : use window.internals.settings.setMinimumAccelerated2dCanvasSize #
Total comments: 7
Patch Set 3 : improve variable names and indentation #Patch Set 4 : Add more detailed comments including why two videos are needed. #
Messages
Total messages: 22 (2 generated)
dongseong.hwang@intel.com changed reviewers: + junov@chromium.org, rileya@chromium.org, scherkus@chromium.org
junov@, could you review? Let me explain more. yuv-video-on-accelerated-canvas.html test is meaningful only on virtual/gpu/ test. yuv-video-on-accelerated-canvas.html draws the same video interleaving on software canvas and accelerated canvas many time. However yuv-video-on-accelerated-canvas-expected.html draws different video on software canvas and accelerated canvas separately. If yuv-video-on-accelerated-canvas.html causes crash or draw different result, test fails.
https://codereview.chromium.org/656683007/diff/1/LayoutTests/fast/canvas/yuv-... File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html (right): https://codereview.chromium.org/656683007/diff/1/LayoutTests/fast/canvas/yuv-... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html:11: <canvas id="hw-canvas" width=300 height=300></canvas> For layouts test it does not work like this for controlling gpu-acceleration. You only need one canvas (of any size), and the test harness will take care of running the test with and without gpu acceleration. To run the test manually in both modes you use these commands: To build the harness: ninja -C out/Release blink_tests To run unaccelerated: blink/tools/run_layout_tests.sh fast/canvas/yuv-video-on-accelerated-canvas.html To run with acceleration: blink/tools/run_layout_tests.sh virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html https://codereview.chromium.org/656683007/diff/1/LayoutTests/fast/canvas/yuv-... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html:27: // less than 257*256 is not accelerated. Not true in layout tests
On 2014/10/17 16:23:47, junov wrote: > https://codereview.chromium.org/656683007/diff/1/LayoutTests/fast/canvas/yuv-... > File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html (right): > > https://codereview.chromium.org/656683007/diff/1/LayoutTests/fast/canvas/yuv-... > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html:11: <canvas > id="hw-canvas" width=300 height=300></canvas> > For layouts test it does not work like this for controlling gpu-acceleration. > You only need one canvas (of any size), and the test harness will take care of > running the test with and without gpu acceleration. > To run the test manually in both modes you use these commands: > To build the harness: ninja -C out/Release blink_tests > To run unaccelerated: blink/tools/run_layout_tests.sh > fast/canvas/yuv-video-on-accelerated-canvas.html > To run with acceleration: blink/tools/run_layout_tests.sh > virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html I need two canvases because both software canvas and accelerated canvas should be tested in the same pages. This CL wants to check the bug caused by interaction between software canvas and accelerated canvas. So only virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html is meaningful. > https://codereview.chromium.org/656683007/diff/1/LayoutTests/fast/canvas/yuv-... > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html:27: // less than > 257*256 is not accelerated. > Not true in layout tests Ah, thanks for pointing it. how about adding window.internals.settings.setMinimumAccelerated2dCanvasSize(int) and use it in this test?
> > > > https://codereview.chromium.org/656683007/diff/1/LayoutTests/fast/canvas/yuv-... > > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html:27: // less than > > 257*256 is not accelerated. > > Not true in layout tests > > Ah, thanks for pointing it. how about adding > window.internals.settings.setMinimumAccelerated2dCanvasSize(int) and use it in > this test? Good idea, but depending on size is a bit yucky as far as test design is concerned. I think something more direct would be nice: window.testRunner.disableAcceleration(canvasElement) HTMLCanvasElement already has a m_accelerationDisabled member (or some other similar name) all you have to do is set it to true.
On 2014/10/17 21:42:53, junov wrote: > > > > > > > > https://codereview.chromium.org/656683007/diff/1/LayoutTests/fast/canvas/yuv-... > > > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html:27: // less > than > > > 257*256 is not accelerated. > > > Not true in layout tests > > > > Ah, thanks for pointing it. how about adding > > window.internals.settings.setMinimumAccelerated2dCanvasSize(int) and use it in > > this test? > > Good idea, but depending on size is a bit yucky as far as test design is > concerned. > I think something more direct would be nice: > window.testRunner.disableAcceleration(canvasElement) > HTMLCanvasElement already has a m_accelerationDisabled member (or some other > similar name) all you have to do is set it to true. That's good idea, but I've concern about implementation side. To implement it, content/shell/renderer/test_runner/test_runner.cc, content/shell/renderer/layout_test/webkit_test_runner.cc and public/web/WebView need to know HTMLCanvasElement. Is it acceptable?
On 2014/10/20 12:19:11, dshwang wrote: > That's good idea, but I've concern about implementation side. To implement it, > content/shell/renderer/test_runner/test_runner.cc, > content/shell/renderer/layout_test/webkit_test_runner.cc and public/web/WebView > need to know HTMLCanvasElement. Is it acceptable? After rethinking, it's not acceptiable because HTMLCanvasElement is not exposed in public/web Exposing HTMLCanvasElement feels overkill. I think window.internals.settings.setMinimumAccelerated2dCanvasSize(int) is only practical solution although I agree on you "more direct would be nice"
On 2014/10/20 13:44:59, dshwang wrote: > On 2014/10/20 12:19:11, dshwang wrote: > > That's good idea, but I've concern about implementation side. To implement it, > > content/shell/renderer/test_runner/test_runner.cc, > > content/shell/renderer/layout_test/webkit_test_runner.cc and > public/web/WebView > > need to know HTMLCanvasElement. Is it acceptable? > > After rethinking, it's not acceptiable because HTMLCanvasElement is not exposed > in public/web > Exposing HTMLCanvasElement feels overkill. > I think window.internals.settings.setMinimumAccelerated2dCanvasSize(int) is only > practical solution although I agree on you "more direct would be nice" I agree that HTMLCanvasElement should not be exposed outside of Blink just for testing. I was assuming that there would be an easy way to delegate the call to a helper class in blink, and do all the work from there. Also I would rather we not create APIs that modify global settings because this is not good for the stability of the test harness. Let me explain: for performance reasons, a batch of layout tests will be run in the same render process without restarting the process, so if a test changes a global state and does not put it back to its original value (perhaps because the test failed and normal JS flow was interrupted), it can affect other tests that will run after it in the same process. This is super evil because it can cause tests to fail only if run in the same process as another test, which is super hard to track down and debug.
On 2014/10/20 14:47:25, junov wrote: > On 2014/10/20 13:44:59, dshwang wrote: > > On 2014/10/20 12:19:11, dshwang wrote: > > > That's good idea, but I've concern about implementation side. To implement > it, > > > content/shell/renderer/test_runner/test_runner.cc, > > > content/shell/renderer/layout_test/webkit_test_runner.cc and > > public/web/WebView > > > need to know HTMLCanvasElement. Is it acceptable? > > > > After rethinking, it's not acceptiable because HTMLCanvasElement is not > exposed > > in public/web > > Exposing HTMLCanvasElement feels overkill. > > I think window.internals.settings.setMinimumAccelerated2dCanvasSize(int) is > only > > practical solution although I agree on you "more direct would be nice" > > I agree that HTMLCanvasElement should not be exposed outside of Blink just for > testing. I was assuming that there would be an easy way to delegate the call to > a helper class in blink, and do all the work from there. Also I would rather we > not create APIs that modify global settings because this is not good for the > stability of the test harness. Let me explain: for performance reasons, a batch > of layout tests will be run in the same render process without restarting the > process, so if a test changes a global state and does not put it back to its > original value (perhaps because the test failed and normal JS flow was > interrupted), it can affect other tests that will run after it in the same > process. This is super evil because it can cause tests to fail only if run in > the same process as another test, which is super hard to track down and debug. Thank you for deep explanation. I have a question. each layout test creates new Page instance while it reuse the render process, right?
On 2014/10/20 17:09:39, dshwang wrote: > Thank you for deep explanation. I have a question. > each layout test creates new Page instance while it reuse the render process, > right? I asked because window.internals.settings.setMinimumAccelerated2dCanvasSize(int) changes Page::m_settings, not a global variable.
On 2014/10/20 17:10:57, dshwang wrote: > On 2014/10/20 17:09:39, dshwang wrote: > > Thank you for deep explanation. I have a question. > > each layout test creates new Page instance while it reuse the render process, > > right? > > I asked because window.internals.settings.setMinimumAccelerated2dCanvasSize(int) > changes Page::m_settings, not a global variable. Correct, but it is created from global settings.
On 2014/10/20 17:13:24, junov wrote: > On 2014/10/20 17:10:57, dshwang wrote: > > On 2014/10/20 17:09:39, dshwang wrote: > > > Thank you for deep explanation. I have a question. > > > each layout test creates new Page instance while it reuse the render > process, > > > right? > > > > I asked because > window.internals.settings.setMinimumAccelerated2dCanvasSize(int) > > changes Page::m_settings, not a global variable. > > Correct, but it is created from global settings. window.internals.settings.setMinimumAccelerated2dCanvasSize() already exists and work very well. I checked if window.internals.settings.setMinimumAccelerated2dCanvasSize() affects following tests after yuv-video-on-accelerated-canvas.html in conclusion, window.internals.settings.setMinimumAccelerated2dCanvasSize() only affects yuv-video-on-accelerated-canvas.html The code (InternalSettings.idl/cpp) also just updates Page::m_settings. IMO. there is not any global variables which you concern about. https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html (right): https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas.html:24: window.internals.settings.setMinimumAccelerated2dCanvasSize(257*256); this line is added.
https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html (right): https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:18: <video id="sw-video" loop> What exactly makes this video sw and the other one hw? https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:22: </video> Inconsistent indentation
https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html (right): https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:18: <video id="sw-video" loop> On 2014/10/20 20:39:02, junov wrote: > What exactly makes this video sw and the other one hw? sw-canvas uses sw-video while hw-canvas uses hw-video. both sw-video and hw-video is accelerated or not as depending on platform. https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:22: </video> On 2014/10/20 20:39:02, junov wrote: > Inconsistent indentation do you mean 4-space indentation?
On 2014/10/20 20:46:48, dshwang wrote: > https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... > File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html > (right): > > https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:18: <video > id="sw-video" loop> > On 2014/10/20 20:39:02, junov wrote: > > What exactly makes this video sw and the other one hw? > > sw-canvas uses sw-video while hw-canvas uses hw-video. > both sw-video and hw-video is accelerated or not as depending on platform. in linux, both sw-canvas uses sw-video are not accelerated. in Cros, both sw-canvas uses sw-video are accelerated. This test doesn't need to enable accelerated video decoding.
https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html (right): https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:18: <video id="sw-video" loop> On 2014/10/20 20:46:47, dshwang wrote: > On 2014/10/20 20:39:02, junov wrote: > > What exactly makes this video sw and the other one hw? > > sw-canvas uses sw-video while hw-canvas uses hw-video. > both sw-video and hw-video is accelerated or not as depending on platform. I don't understand what is the difference between the two video elements they look identical to me. Am I missing something? Why do you need the two? https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:22: </video> On 2014/10/20 20:46:47, dshwang wrote: > On 2014/10/20 20:39:02, junov wrote: > > Inconsistent indentation > > do you mean 4-space indentation? No. I mean <video> and the matching </video> are not indented the same.
On 2014/10/20 20:51:42, junov wrote: > https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... > File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html > (right): > > https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:18: <video > id="sw-video" loop> > On 2014/10/20 20:46:47, dshwang wrote: > > On 2014/10/20 20:39:02, junov wrote: > > > What exactly makes this video sw and the other one hw? > > > > sw-canvas uses sw-video while hw-canvas uses hw-video. > > both sw-video and hw-video is accelerated or not as depending on platform. > > I don't understand what is the difference between the two video elements they > look identical to me. Am I missing something? Why do you need the two? Sorry for incomplete explanation. This test is made to check video bug rather than canvas bug. When we use both sw canvas and hw canvas, the video bug appears. The video impl has its own cache mechanism to cache SkBitmap after converting video frame to SkBitmap. See media/filters/skcanvas_video_renderer.cc When hw canvas draws video, video caches SkBitmap backed on GrTexture. When sw canvas draws video, video caches SkBitmap backed on system memory. The problem appears when we draws the video on both hw canvas and sw canvas. When hw canvas draws, the video caches GrTexture based SkBitmap. and then when sw canvas draws, the video draws GrTexture based SkBitmap on sw canvas. Otherwise, if sw canvas draws prior, the video draws system memory SkBitmap on hw canvas. If we creates two video elements for each canvas, each video caches sw bitmap or hw bitmap respectively. So the -expected.html creates two videos. For more impl detail, plz refer to https://codereview.chromium.org/662033002/ > > https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:22: > </video> > On 2014/10/20 20:46:47, dshwang wrote: > > On 2014/10/20 20:39:02, junov wrote: > > > Inconsistent indentation > > > > do you mean 4-space indentation? > No. I mean <video> and the matching </video> are not indented the same. Done. thx
On 2014/10/20 21:51:46, dshwang wrote: > On 2014/10/20 20:51:42, junov wrote: > > > https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... > > File LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html > > (right): > > > > > https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... > > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:18: > <video > > id="sw-video" loop> > > On 2014/10/20 20:46:47, dshwang wrote: > > > On 2014/10/20 20:39:02, junov wrote: > > > > What exactly makes this video sw and the other one hw? > > > > > > sw-canvas uses sw-video while hw-canvas uses hw-video. > > > both sw-video and hw-video is accelerated or not as depending on platform. > > > > I don't understand what is the difference between the two video elements they > > look identical to me. Am I missing something? Why do you need the two? > > Sorry for incomplete explanation. > This test is made to check video bug rather than canvas bug. When we use both sw > canvas and hw canvas, the video bug appears. > The video impl has its own cache mechanism to cache SkBitmap after converting > video frame to SkBitmap. See media/filters/skcanvas_video_renderer.cc > When hw canvas draws video, video caches SkBitmap backed on GrTexture. When sw > canvas draws video, video caches SkBitmap backed on system memory. > The problem appears when we draws the video on both hw canvas and sw canvas. > When hw canvas draws, the video caches GrTexture based SkBitmap. and then when > sw canvas draws, the video draws GrTexture based SkBitmap on sw canvas. > Otherwise, if sw canvas draws prior, the video draws system memory SkBitmap on > hw canvas. > > If we creates two video elements for each canvas, each video caches sw bitmap or > hw bitmap respectively. So the -expected.html creates two videos. > > For more impl detail, plz refer to https://codereview.chromium.org/662033002/ > > > > > > https://codereview.chromium.org/656683007/diff/20001/LayoutTests/fast/canvas/... > > LayoutTests/fast/canvas/yuv-video-on-accelerated-canvas-expected.html:22: > > </video> > > On 2014/10/20 20:46:47, dshwang wrote: > > > On 2014/10/20 20:39:02, junov wrote: > > > > Inconsistent indentation > > > > > > do you mean 4-space indentation? > > No. I mean <video> and the matching </video> are not indented the same. > > Done. thx I see please add a short explanation in comments in the test code. Lgtm
On 2014/10/20 22:54:13, junov wrote: > I see please add a short explanation in comments in the test code. Lgtm Thank you. yes, done.
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/656683007/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 184073 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
