Run LayerTreeTests with synchronous scheduling. |
||
Issue descriptionI recently had a regression with impl-side invalidations on WebView because the synchronous scheduling code path was not tested. I suppose this should have been tested within the Scheduler, since that was the component where the core logic it affected was. But I kind of assumed that a LayerTreeTest integration test would capture this. I think we should either have a framework for SchedulerTests that would serve as the primary thing you use when making significant changes in the stack to make sure all settings which are not enabled by default are tested, similar to threaded vs single-threaded LayerTreeTests. Or may be we should extend the LayerTreeTests to also run with synchronous scheduling mode. This ensures that the complete interaction between LayerTreeFrameSink <-> Scheduler is tested. I already made a change to add this for the image animation code that use impl-side invalidation, so it should be possible to easily extend it to other tests. But, does it sound like something like LayerTreeTests should run for different scheduling knobs?
,
Nov 21 2017
I'm not sure what it means to run in synchronous mode and interact with the Scheduler. I thought it only exists in non-synchronous mode. In synchronous mode scheduling is entirely done by the embedder, so each embedder would need its own unit tests for its behaviour.
,
Nov 21 2017
Its about testing the different code paths with a setup that runs with the behaviour expected from the embedder. In the default mode, you get a BeginFrame, draw at the internally decided deadline and wait for the FrameSink to ack it. In sync code, you call Invalidate on the LayerTreeFrameSink if there is new content and wait for the FrameSink to call onDraw to pull a new frame. And these calls to the FrameSink have to happen at a particular impl frame phase. For instance, if you want to defer something until the deadline, default scheduling has an internally managed deadline so no extra consideration is required. But for sync mode, you have to remember to invalidate during the impl frame to make sure there is a deadline. Since we enter the deadline phase only in OnDraw which won't happen unless you invalidate. An example of this for PrepareTiles is here: https://cs.chromium.org/chromium/src/cc/scheduler/scheduler_state_machine.cc?l=563. And the same thing happened in the case of crbug.com/780240 . I suggested LayerTreeTests because then you get the complete interaction between FrameSink in LTHI and the Scheduler code. Does that sound like overkill for this?
,
Nov 21 2017
Oh I think I'm just stumbling over terminology. You're saying sync mode as in WebView synchronous compositing, where we draw synchronously inside a BeginFrame call. I thought you meant synchronous compositing as in LayerTreeSettings using_synchronous_renderer_compositor wherein there is no Scheduler at all and the LayerTreeHost embedder controls when to draw and does so from the main thread. Running all LayerTreeTests in webview mode is somewhat appealing but I wonder if it's a little overkill, how much surface area of the compositor changes under webview? I can buy that we have some test coverage holes to fill - we always do and always find it and add tests as we do - but would this be more work to maintain and make tests harder to write that don't interact with webview differences anyway? It seems like it would be a small % of tests that do.. mostly tests wrt scheduling.. is that wrong? Maybe all the scheduler(and state machine) tests should be tested against a webview config?
,
Nov 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1 commit ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1 Author: Khushal <khushalsagar@chromium.org> Date: Wed Nov 22 02:49:53 2017 cc: Run threaded LayerTreeTests with sync scheduling. Add an additional mode that runs multi-threaded LayerTreeTests with sync/webview scheduling. R=danakj@chromium.org Bug: 787257 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel Change-Id: Ie2c12b772de6e992d1caa238159ea9bc2592ee2e Reviewed-on: https://chromium-review.googlesource.com/783637 Commit-Queue: Khushal <khushalsagar@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: enne <enne@chromium.org> Cr-Commit-Position: refs/heads/master@{#518510} [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/test/layer_tree_pixel_test.cc [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/test/layer_tree_test.cc [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/test/layer_tree_test.h [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/trees/layer_tree_host_common_perftest.cc [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/trees/layer_tree_host_perftest.cc [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/trees/layer_tree_host_unittest.cc [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/trees/layer_tree_host_unittest_context.cc [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/trees/layer_tree_host_unittest_copyrequest.cc [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/cc/trees/layer_tree_host_unittest_scroll.cc [modify] https://crrev.com/ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1/components/viz/service/display/bsp_tree_perftest.cc
,
Nov 22 2017
,
Nov 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8994685157b2d07b4cd27be836077480d2df753 commit e8994685157b2d07b4cd27be836077480d2df753 Author: Olga Sharonova <olka@chromium.org> Date: Wed Nov 22 12:05:22 2017 Revert "cc: Run threaded LayerTreeTests with sync scheduling." This reverts commit ee41ffee3151b23dfe5a9e25b2ffe0d8a255b6a1. Reason for revert: Bug: 787793 Original change's description: > cc: Run threaded LayerTreeTests with sync scheduling. > > Add an additional mode that runs multi-threaded LayerTreeTests with > sync/webview scheduling. > > R=danakj@chromium.org > > Bug: 787257 > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel > Change-Id: Ie2c12b772de6e992d1caa238159ea9bc2592ee2e > Reviewed-on: https://chromium-review.googlesource.com/783637 > Commit-Queue: Khushal <khushalsagar@chromium.org> > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: enne <enne@chromium.org> > Cr-Commit-Position: refs/heads/master@{#518510} TBR=danakj@chromium.org,enne@chromium.org,khushalsagar@chromium.org Change-Id: Ibb83ff13c50a9c131ec9a8aefd8aa9bcd2df670e No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 787257 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/785390 Reviewed-by: Olga Sharonova <olka@chromium.org> Commit-Queue: Olga Sharonova <olka@chromium.org> Cr-Commit-Position: refs/heads/master@{#518612} [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/test/layer_tree_pixel_test.cc [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/test/layer_tree_test.cc [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/test/layer_tree_test.h [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/trees/layer_tree_host_common_perftest.cc [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/trees/layer_tree_host_perftest.cc [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/trees/layer_tree_host_unittest.cc [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/trees/layer_tree_host_unittest_context.cc [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/trees/layer_tree_host_unittest_copyrequest.cc [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/cc/trees/layer_tree_host_unittest_scroll.cc [modify] https://crrev.com/e8994685157b2d07b4cd27be836077480d2df753/components/viz/service/display/bsp_tree_perftest.cc |
||
►
Sign in to add a comment |
||
Comment 1 by khushals...@chromium.org
, Nov 21 2017