Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
475633008:
cc: Use impl-side painting in LTH context lost tests. (Closed)
Descriptioncc: Use impl-side painting in LTH context lost tests.
BUG=401492
Committed: https://crrev.com/fa27ef28c604c3bd343f7d8298ac3f328a9a0037
Cr-Commit-Position: refs/heads/master@{#295241}
Patch Set 1 #Patch Set 2 : update tests to use PictureLayers along with Content. #
Total comments: 35
Patch Set 3 : review comments updated. #Patch Set 4 : initialize locals #Patch Set 5 : draw non-solid color. #
Total comments: 4
Patch Set 6 : updated non solid color painting #Patch Set 7 : set high res to draw when new output surface is used. #
Total comments: 1
Patch Set 8 : rebase to ToT #Patch Set 9 : check isReadyToDraw for evict texture test. #
Total comments: 20
Patch Set 10 : review comments addressed. #
Total comments: 3
Patch Set 11 : review comments addressed. #Patch Set 12 : remove extra add_draw_rect #
Messages
Total messages: 37 (4 generated)
sohan.jyoti@samsung.com changed reviewers: + danakj@chromium.org, enne@chromium.org
Can you please take a look? Thanks. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (left): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:473: EXPECT_TRUE(layer_->HaveBackingAt(0, 0)); if we use impl-side painting, this test cant be used, is it critical here ? https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:654: EXPECT_EQ(0u, grandchild->lost_output_surface_count()); if we use impl-side, these test cant be used, are these critical here ? https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:662: EXPECT_GE(1u, grandchild->lost_output_surface_count()); if we use impl-side, these test cant be used, are these critical here ? https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:1422: EXPECT_EQ(1u, layer_->output_surface_created_count()); this seems to be the only thing this test checks, after removing them is this test-suite meaningful anymore? https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); hope this is the closest to having resource for tile in impl side world?
https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (left): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:654: EXPECT_EQ(0u, grandchild->lost_output_surface_count()); On 2014/08/26 09:36:47, sohanjg wrote: > if we use impl-side, these test cant be used, are these critical here ? can we check something like LayerPropertyChanged ? and have 1 common test. or more specifically have 2 tests, FakeContentLayer will check lost_output_surface_count like before, and have 1 new test to make FakePictureLayer test some push_properties_count ? https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:1422: EXPECT_EQ(1u, layer_->output_surface_created_count()); On 2014/08/26 09:36:47, sohanjg wrote: > this seems to be the only thing this test checks, after removing them is this > test-suite meaningful anymore? like LayerTreeHostContextTestLayersNotified test, can we do similar here ?
https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (left): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:473: EXPECT_TRUE(layer_->HaveBackingAt(0, 0)); On 2014/08/26 09:36:47, sohanjg wrote: > if we use impl-side painting, this test cant be used, is it critical here ? we can't check this here on the main thread with impl painting. we should continue to check it with ContentLayers https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:654: EXPECT_EQ(0u, grandchild->lost_output_surface_count()); On 2014/08/26 09:36:47, sohanjg wrote: > if we use impl-side, these test cant be used, are these critical here ? add this to FakePictureLayerImpl. https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:1422: EXPECT_EQ(1u, layer_->output_surface_created_count()); On 2014/08/26 09:36:47, sohanjg wrote: > this seems to be the only thing this test checks, after removing them is this > test-suite meaningful anymore? Add this to FakePictureLayer. https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:319: class LayerTreeHostContextTestLostContextSucceedsWithContentAndImpl leave the test name https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:328: layer_ = PictureLayer::Create(&client_); FakePictureLayer https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:330: layer_ = ContentLayer::Create(&client_); You mean FakeContentLayer https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); On 2014/08/26 09:36:47, sohanjg wrote: > hope this is the closest to having resource for tile in impl side world? picture_impl->HighResTiling()->TileAt(0, 0)->HasResources() ? https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:457: layer_ = PictureLayer::Create(&client_); FakePictureLayer https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:492: virtual void CommitCompleteOnThread(LayerTreeHostImpl* impl) OVERRIDE { Change this to DrawLayersOnThread https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:496: ++num_commits_; when impl painting, check the picture layer has a resource here. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:648: root_ = PictureLayer::Create(&client_); FakePictureLayers, or you can't cast the impl to a FakePictureLayerImpl.
please take a look, thanks. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (left): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:473: EXPECT_TRUE(layer_->HaveBackingAt(0, 0)); On 2014/08/26 18:54:40, danakj wrote: > On 2014/08/26 09:36:47, sohanjg wrote: > > if we use impl-side painting, this test cant be used, is it critical here ? > > we can't check this here on the main thread with impl painting. we should > continue to check it with ContentLayers Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:654: EXPECT_EQ(0u, grandchild->lost_output_surface_count()); On 2014/08/26 18:54:40, danakj wrote: > On 2014/08/26 09:36:47, sohanjg wrote: > > if we use impl-side, these test cant be used, are these critical here ? > > add this to FakePictureLayerImpl. > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:1422: EXPECT_EQ(1u, layer_->output_surface_created_count()); On 2014/08/26 18:54:40, danakj wrote: > On 2014/08/26 09:36:47, sohanjg wrote: > > this seems to be the only thing this test checks, after removing them is this > > test-suite meaningful anymore? > > Add this to FakePictureLayer. > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:319: class LayerTreeHostContextTestLostContextSucceedsWithContentAndImpl On 2014/08/26 18:54:41, danakj wrote: > leave the test name Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:328: layer_ = PictureLayer::Create(&client_); On 2014/08/26 18:54:41, danakj wrote: > FakePictureLayer Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:330: layer_ = ContentLayer::Create(&client_); On 2014/08/26 18:54:41, danakj wrote: > You mean FakeContentLayer Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); On 2014/08/26 18:54:40, danakj wrote: > On 2014/08/26 09:36:47, sohanjg wrote: > > hope this is the closest to having resource for tile in impl side world? > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources() ? yea! but i think for impl-side paint, can we be sure that the layer will have resource? tests fails here, so have removed them. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:457: layer_ = PictureLayer::Create(&client_); On 2014/08/26 18:54:40, danakj wrote: > FakePictureLayer Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:492: virtual void CommitCompleteOnThread(LayerTreeHostImpl* impl) OVERRIDE { On 2014/08/26 18:54:40, danakj wrote: > Change this to DrawLayersOnThread Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:496: ++num_commits_; On 2014/08/26 18:54:40, danakj wrote: > when impl painting, check the picture layer has a resource here. Done. https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:648: root_ = PictureLayer::Create(&client_); On 2014/08/26 18:54:41, danakj wrote: > FakePictureLayers, or you can't cast the impl to a FakePictureLayerImpl. Done.
https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); On 2014/08/27 06:59:50, sohanjg wrote: > On 2014/08/26 18:54:40, danakj wrote: > > On 2014/08/26 09:36:47, sohanjg wrote: > > > hope this is the closest to having resource for tile in impl side world? > > > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources() ? > > yea! but i think for impl-side paint, can we be sure that the layer will have > resource? tests fails here, so have removed them. It should not activate and draw without a resource. What is failing?
https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); On 2014/08/27 12:59:26, danakj wrote: > On 2014/08/27 06:59:50, sohanjg wrote: > > On 2014/08/26 18:54:40, danakj wrote: > > > On 2014/08/26 09:36:47, sohanjg wrote: > > > > hope this is the closest to having resource for tile in impl side world? > > > > > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources() ? > > > > yea! but i think for impl-side paint, can we be sure that the layer will have > > resource? tests fails here, so have removed them. > > It should not activate and draw without a resource. What is failing? Hmm. picture_impl->HighResTiling()->TileAt(0, 0)->HasResources(), returns FALSE for multi thread direct and delegating renderer impl test
https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); On 2014/08/27 13:33:01, sohanjg wrote: > On 2014/08/27 12:59:26, danakj wrote: > > On 2014/08/27 06:59:50, sohanjg wrote: > > > On 2014/08/26 18:54:40, danakj wrote: > > > > On 2014/08/26 09:36:47, sohanjg wrote: > > > > > hope this is the closest to having resource for tile in impl side world? > > > > > > > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources() ? > > > > > > yea! but i think for impl-side paint, can we be sure that the layer will > have > > > resource? tests fails here, so have removed them. > > > > It should not activate and draw without a resource. What is failing? > > Hmm. > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources(), returns FALSE for > multi thread direct and delegating renderer impl test That sounds wrong, does it do this every time it draws or just after lost context or what? Can you figure out why it's doing that? Seems like a bug.
https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); On 2014/08/27 13:49:32, danakj wrote: > On 2014/08/27 13:33:01, sohanjg wrote: > > On 2014/08/27 12:59:26, danakj wrote: > > > On 2014/08/27 06:59:50, sohanjg wrote: > > > > On 2014/08/26 18:54:40, danakj wrote: > > > > > On 2014/08/26 09:36:47, sohanjg wrote: > > > > > > hope this is the closest to having resource for tile in impl side > world? > > > > > > > > > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources() ? > > > > > > > > yea! but i think for impl-side paint, can we be sure that the layer will > > have > > > > resource? tests fails here, so have removed them. > > > > > > It should not activate and draw without a resource. What is failing? > > > > Hmm. > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources(), returns FALSE for > > multi thread direct and delegating renderer impl test > > That sounds wrong, does it do this every time it draws or just after lost > context or what? Can you figure out why it's doing that? Seems like a bug. DrawLayersOnThread is always having no resource, irrespective of lost context, i will check it more.
https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); On 2014/08/27 14:13:34, sohanjg wrote: > On 2014/08/27 13:49:32, danakj wrote: > > On 2014/08/27 13:33:01, sohanjg wrote: > > > On 2014/08/27 12:59:26, danakj wrote: > > > > On 2014/08/27 06:59:50, sohanjg wrote: > > > > > On 2014/08/26 18:54:40, danakj wrote: > > > > > > On 2014/08/26 09:36:47, sohanjg wrote: > > > > > > > hope this is the closest to having resource for tile in impl side > > world? > > > > > > > > > > > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources() ? > > > > > > > > > > yea! but i think for impl-side paint, can we be sure that the layer will > > > have > > > > > resource? tests fails here, so have removed them. > > > > > > > > It should not activate and draw without a resource. What is failing? > > > > > > Hmm. > > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources(), returns FALSE > for > > > multi thread direct and delegating renderer impl test > > > > That sounds wrong, does it do this every time it draws or just after lost > > context or what? Can you figure out why it's doing that? Seems like a bug. > > DrawLayersOnThread is always having no resource, irrespective of lost context, i > will check it more. Sorry for the late response. For tests we use solid color mode resources which are released when we complete raster tasks, https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/tile_... hence we get tile w/o resource in DrawLayersOnThread here with impl-side paint. Should we skip the resource check, and add a solid color mode check ?
https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/20001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:354: EXPECT_TRUE(picture_impl->HasValidTilePriorities()); On 2014/09/01 10:51:28, sohanjg wrote: > On 2014/08/27 14:13:34, sohanjg wrote: > > On 2014/08/27 13:49:32, danakj wrote: > > > On 2014/08/27 13:33:01, sohanjg wrote: > > > > On 2014/08/27 12:59:26, danakj wrote: > > > > > On 2014/08/27 06:59:50, sohanjg wrote: > > > > > > On 2014/08/26 18:54:40, danakj wrote: > > > > > > > On 2014/08/26 09:36:47, sohanjg wrote: > > > > > > > > hope this is the closest to having resource for tile in impl side > > > world? > > > > > > > > > > > > > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources() ? > > > > > > > > > > > > yea! but i think for impl-side paint, can we be sure that the layer > will > > > > have > > > > > > resource? tests fails here, so have removed them. > > > > > > > > > > It should not activate and draw without a resource. What is failing? > > > > > > > > Hmm. > > > > picture_impl->HighResTiling()->TileAt(0, 0)->HasResources(), returns FALSE > > for > > > > multi thread direct and delegating renderer impl test > > > > > > That sounds wrong, does it do this every time it draws or just after lost > > > context or what? Can you figure out why it's doing that? Seems like a bug. > > > > DrawLayersOnThread is always having no resource, irrespective of lost context, > i > > will check it more. > > Sorry for the late response. > For tests we use solid color mode resources which are released when we complete > raster tasks, > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/tile_... > > hence we get tile w/o resource in DrawLayersOnThread here with impl-side paint. > Should we skip the resource check, and add a solid color mode check ? Oh, maybe we need to make the client_ draw something other than a solid color then, to make us have an actual resource we can look for. But alternatively, instead of checking TileAt(0, 0)->HasResource() could you check TileAt(0, 0)->IsReadyToDraw()?
On 2014/09/03 at 19:51:40, danakj wrote: > > Sorry for the late response. > > For tests we use solid color mode resources which are released when we complete > > raster tasks, > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/tile_... > > > > hence we get tile w/o resource in DrawLayersOnThread here with impl-side paint. > > Should we skip the resource check, and add a solid color mode check ? > > Oh, maybe we need to make the client_ draw something other than a solid color then, to make us have an actual resource we can look for. > > But alternatively, instead of checking TileAt(0, 0)->HasResource() could you check TileAt(0, 0)->IsReadyToDraw()? If you're going to check ready to draw, I think it has to be something other than a solid color for this test to be real.
On Wed, Sep 3, 2014 at 3:59 PM, <enne@chromium.org> wrote: > On 2014/09/03 at 19:51:40, danakj wrote: > > > Sorry for the late response. >> > For tests we use solid color mode resources which are released when we >> > complete > >> > raster tasks, >> > >> > >> > https://code.google.com/p/chromium/codesearch#chromium/ > src/cc/resources/tile_manager.cc&sq=package:chromium&type= > cs&l=1108&rcl=1409414148 > >> > >> > hence we get tile w/o resource in DrawLayersOnThread here with impl-side >> > paint. > >> > Should we skip the resource check, and add a solid color mode check ? >> > > Oh, maybe we need to make the client_ draw something other than a solid >> color >> > then, to make us have an actual resource we can look for. > > But alternatively, instead of checking TileAt(0, 0)->HasResource() could >> you >> > check TileAt(0, 0)->IsReadyToDraw()? > > If you're going to check ready to draw, I think it has to be something > other > than a solid color for this test to be real. > Ah.. yes thank you for mentioning that. I agree. So we should change client_ to draw something other than a solid color for this test. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/03 20:00:40, danakj wrote: > On Wed, Sep 3, 2014 at 3:59 PM, <mailto:enne@chromium.org> wrote: > > > On 2014/09/03 at 19:51:40, danakj wrote: > > > > > Sorry for the late response. > >> > For tests we use solid color mode resources which are released when we > >> > > complete > > > >> > raster tasks, > >> > > >> > > >> > > https://code.google.com/p/chromium/codesearch#chromium/ > > src/cc/resources/tile_manager.cc&sq=package:chromium&type= > > cs&l=1108&rcl=1409414148 > > > >> > > >> > hence we get tile w/o resource in DrawLayersOnThread here with impl-side > >> > > paint. > > > >> > Should we skip the resource check, and add a solid color mode check ? > >> > > > > Oh, maybe we need to make the client_ draw something other than a solid > >> color > >> > > then, to make us have an actual resource we can look for. > > > > But alternatively, instead of checking TileAt(0, 0)->HasResource() could > >> you > >> > > check TileAt(0, 0)->IsReadyToDraw()? > > > > If you're going to check ready to draw, I think it has to be something > > other > > than a solid color for this test to be real. > > > > Ah.. yes thank you for mentioning that. I agree. So we should change > client_ to draw something other than a solid color for this test. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. OK. So we should try and draw in RESOURCE mode here ? PICTURE_PILE mode would require to setup the tree with picture piles, which we arent doing for layer tree tests now.
On 2014/09/04 01:37:07, sohanjg wrote: > On 2014/09/03 20:00:40, danakj wrote: > > On Wed, Sep 3, 2014 at 3:59 PM, <mailto:enne@chromium.org> wrote: > > > > > On 2014/09/03 at 19:51:40, danakj wrote: > > > > > > > Sorry for the late response. > > >> > For tests we use solid color mode resources which are released when we > > >> > > > complete > > > > > >> > raster tasks, > > >> > > > >> > > > >> > > > https://code.google.com/p/chromium/codesearch#chromium/ > > > src/cc/resources/tile_manager.cc&sq=package:chromium&type= > > > cs&l=1108&rcl=1409414148 > > > > > >> > > > >> > hence we get tile w/o resource in DrawLayersOnThread here with impl-side > > >> > > > paint. > > > > > >> > Should we skip the resource check, and add a solid color mode check ? > > >> > > > > > > Oh, maybe we need to make the client_ draw something other than a solid > > >> color > > >> > > > then, to make us have an actual resource we can look for. > > > > > > But alternatively, instead of checking TileAt(0, 0)->HasResource() could > > >> you > > >> > > > check TileAt(0, 0)->IsReadyToDraw()? > > > > > > If you're going to check ready to draw, I think it has to be something > > > other > > > than a solid color for this test to be real. > > > > > > > Ah.. yes thank you for mentioning that. I agree. So we should change > > client_ to draw something other than a solid color for this test. > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > OK. > So we should try and draw in RESOURCE mode here ? PICTURE_PILE mode would > require to setup the tree with picture piles, which we arent doing for layer > tree tests now. Yes. We use picture piles with picture layer already. You should just need to make client_ paint something not solid cokor
I am missing something in client_ non solid color paint, can you please give your comment. thanks. https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:340: This draws a non solid color, when i verify here with Analysis canvas GetColorIfSolid() check. But, when Analysis task runs (AnalyzeInRect), it still gets a solid color. Can you please point out, what maybe wrong ? https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:367: } else { this check is not doing any good still, as AnalyzeInRect gets a solid color :/
https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:336: client_.PaintContents(canvas, You need to change what PaintContents does, not call it yourself here. The PictureLayer will call it. https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... This method draws nothing unless you setup some draw_rects_. https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... The add_draw_rect sets up what SkPaint to use in what area. So if you add a whole lot of paints of different colors in small areas, you won't get a solid color result.
https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest_context.cc:336: client_.PaintContents(canvas, On 2014/09/04 14:19:35, danakj wrote: > You need to change what PaintContents does, not call it yourself here. The > PictureLayer will call it. > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > This method draws nothing unless you setup some draw_rects_. > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > The add_draw_rect sets up what SkPaint to use in what area. So if you add a > whole lot of paints of different colors in small areas, you won't get a solid > color result. > As discussed, even after drawing a non slid color, for a few case DrawLayersOnThread, still doesnt have resource for Tile at 0,0. Looks like it happens for tiles which do not have a twin tile for it, as a result in MarkVisibleTilesAsRequired, we skip setting MarkRequiredForActivation for that tile, and avoid raster and assigning resource to tile state. But, strangely there are other tiles without twin tiling, which we are not activating, but they complete raster tasks and we assign resources to them, and they are ready to draw. Not sure what the bug is here :/
On 2014/09/05 15:15:46, sohanjg wrote: > https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... > File cc/trees/layer_tree_host_unittest_context.cc (right): > > https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... > cc/trees/layer_tree_host_unittest_context.cc:336: client_.PaintContents(canvas, > On 2014/09/04 14:19:35, danakj wrote: > > You need to change what PaintContents does, not call it yourself here. The > > PictureLayer will call it. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > This method draws nothing unless you setup some draw_rects_. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > The add_draw_rect sets up what SkPaint to use in what area. So if you add a > > whole lot of paints of different colors in small areas, you won't get a solid > > color result. > > > > As discussed, even after drawing a non slid color, for a few case > DrawLayersOnThread, still doesnt have resource for Tile at 0,0. > > Looks like it happens for tiles which do not have a twin tile for it, as a > result in MarkVisibleTilesAsRequired, we skip setting MarkRequiredForActivation > for that tile, and avoid raster and assigning resource to tile state. > > But, strangely there are other tiles without twin tiling, which we are not > activating, but they complete raster tasks and we assign resources to them, and > they are ready to draw. > > Not sure what the bug is here :/ It sounds like a race between activation+drawing and rastering these tiles, which is a bug. We should block either activation or drawing on these tiles being complete. Probably drawing, which is what SetRequiresHighResForDrawing() is meant to do.
On 2014/09/05 15:19:35, danakj wrote: > On 2014/09/05 15:15:46, sohanjg wrote: > > > https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... > > File cc/trees/layer_tree_host_unittest_context.cc (right): > > > > > https://codereview.chromium.org/475633008/diff/80001/cc/trees/layer_tree_host... > > cc/trees/layer_tree_host_unittest_context.cc:336: > client_.PaintContents(canvas, > > On 2014/09/04 14:19:35, danakj wrote: > > > You need to change what PaintContents does, not call it yourself here. The > > > PictureLayer will call it. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > > > This method draws nothing unless you setup some draw_rects_. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_conte... > > > > > > The add_draw_rect sets up what SkPaint to use in what area. So if you add a > > > whole lot of paints of different colors in small areas, you won't get a > solid > > > color result. > > > > > > > As discussed, even after drawing a non slid color, for a few case > > DrawLayersOnThread, still doesnt have resource for Tile at 0,0. > > > > Looks like it happens for tiles which do not have a twin tile for it, as a > > result in MarkVisibleTilesAsRequired, we skip setting > MarkRequiredForActivation > > for that tile, and avoid raster and assigning resource to tile state. > > > > But, strangely there are other tiles without twin tiling, which we are not > > activating, but they complete raster tasks and we assign resources to them, > and > > they are ready to draw. > > > > Not sure what the bug is here :/ > > It sounds like a race between activation+drawing and rastering these tiles, > which is a bug. We should block either activation or drawing on these tiles > being complete. Probably drawing, which is what SetRequiresHighResForDrawing() > is meant to do. Moved the draw blocking code to a separate CL for better tracking, https://codereview.chromium.org/548223003/. we will land this CL once, https://codereview.chromium.org/548223003/ is committed. hope its ok?
Please ignore the previous comment on branching to new CL, its not meaningful. Please take a look. thanks.
please take a look, thanks ! https://codereview.chromium.org/475633008/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/475633008/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1613: active_tree()->ResetRequiresHighResToDraw(); we reset this on swap buffers too, but tests don't invoke it, so added it here. Or we should remove the reset from swapbuffers ?
This looks great, a few comments https://codereview.chromium.org/475633008/diff/160001/cc/test/fake_picture_la... File cc/test/fake_picture_layer_impl.h (right): https://codereview.chromium.org/475633008/diff/160001/cc/test/fake_picture_la... cc/test/fake_picture_layer_impl.h:115: size_t lost_output_surface_count() const { nit: release_resources_count? https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1625: active_tree()->ResetRequiresHighResToDraw(); Can you just make the tests call Swap if they need to? They're skipping behaviour we'd expect to see in reality. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2090: active_tree_->SetRequiresHighResToDraw(); can you move this down below us creating all the things? Maybe at the end of this method is fine. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:2034: bool tile_missing = false; can you add a pre-draw to these other tests too, so they're not testing the first frame with the OS. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:2057: bool tile_missing = false; and here, etc https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:5912: host_impl_->ActivateSyncTree(); can you change this to just host_impl_->active_tree()->ResetRequiresHighRes..() https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:5921: host_impl_->CreatePendingTree(); can you change this to just host_impl_->active_tree()->ResetRequiresHighRes..() https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:5936: host_impl_->ActivateSyncTree(); can you change this to just host_impl_->active_tree()->ResetRequiresHighRes..() https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:5945: host_impl_->CreatePendingTree(); can you change this to just host_impl_->active_tree()->ResetRequiresHighRes..() https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:492: impl_host_->EvictTexturesForTesting(); can you after this, do: EXPECT_FALSE(picture_impl->HighResTiling()->TileAt(0, 0)->IsReadyToDraw()); ?
please take a look, thanks. https://codereview.chromium.org/475633008/diff/160001/cc/test/fake_picture_la... File cc/test/fake_picture_layer_impl.h (right): https://codereview.chromium.org/475633008/diff/160001/cc/test/fake_picture_la... cc/test/fake_picture_layer_impl.h:115: size_t lost_output_surface_count() const { On 2014/09/10 16:03:31, danakj wrote: > nit: release_resources_count? Done. should we change in fakecontentlayerimpl too ? https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1625: active_tree()->ResetRequiresHighResToDraw(); On 2014/09/10 16:03:31, danakj wrote: > Can you just make the tests call Swap if they need to? They're skipping > behaviour we'd expect to see in reality. Done. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2090: active_tree_->SetRequiresHighResToDraw(); On 2014/09/10 16:03:31, danakj wrote: > can you move this down below us creating all the things? Maybe at the end of > this method is fine. Done. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:2034: bool tile_missing = false; On 2014/09/10 16:03:32, danakj wrote: > can you add a pre-draw to these other tests too, so they're not testing the > first frame with the OS. Done. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:2057: bool tile_missing = false; On 2014/09/10 16:03:32, danakj wrote: > and here, etc Done. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:5912: host_impl_->ActivateSyncTree(); On 2014/09/10 16:03:32, danakj wrote: > can you change this to just host_impl_->active_tree()->ResetRequiresHighRes..() Done. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:5921: host_impl_->CreatePendingTree(); On 2014/09/10 16:03:32, danakj wrote: > can you change this to just host_impl_->active_tree()->ResetRequiresHighRes..() Done. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:5936: host_impl_->ActivateSyncTree(); On 2014/09/10 16:03:32, danakj wrote: > can you change this to just host_impl_->active_tree()->ResetRequiresHighRes..() Done. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:5945: host_impl_->CreatePendingTree(); On 2014/09/10 16:03:32, danakj wrote: > can you change this to just host_impl_->active_tree()->ResetRequiresHighRes..() Done. https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/160001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:492: impl_host_->EvictTexturesForTesting(); On 2014/09/10 16:03:32, danakj wrote: > can you after this, do: > > EXPECT_FALSE(picture_impl->HighResTiling()->TileAt(0, 0)->IsReadyToDraw()); > > ? Done.
LGTM w/ 1 thing https://codereview.chromium.org/475633008/diff/180001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:332: client_.add_draw_rect(gfx::Rect(10, 10, 10, 10), paint); the layer is only 10x10, it seems like the last three draw_rects are pointless?
https://codereview.chromium.org/475633008/diff/180001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:332: client_.add_draw_rect(gfx::Rect(10, 10, 10, 10), paint); On 2014/09/13 13:13:47, danakj wrote: > the layer is only 10x10, it seems like the last three draw_rects are pointless? Done. increased bound to 100X100.
https://codereview.chromium.org/475633008/diff/180001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/475633008/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_context.cc:332: client_.add_draw_rect(gfx::Rect(10, 10, 10, 10), paint); On 2014/09/15 05:14:31, sohanjg wrote: > On 2014/09/13 13:13:47, danakj wrote: > > the layer is only 10x10, it seems like the last three draw_rects are > pointless? > > Done. > increased bound to 100X100. Or, how about just remove the three extra draw_rects, since the 1 is enough, and leave the bounds alone?
The CQ bit was checked by sohanjg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/475633008/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/475633008/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as 3dcfd5c81406e261de687b7f5ef05eadaad623d4
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/fa27ef28c604c3bd343f7d8298ac3f328a9a0037 Cr-Commit-Position: refs/heads/master@{#295241}
Message was sent while issue was closed.
Hi, This patch caused LayerTreeHostContextTestLostContextAndEvictTextures.LoseBeforeEvict_MultiThread_DelegatingRenderer_ImplSidePaint to crash flakily, so I've reverted it in https://codereview.chromium.org/581643005/ . sample crash: https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests%20(1)/buil... Sorry!
Message was sent while issue was closed.
Description was changed from ========== cc: Use impl-side painting in LTH context lost tests. BUG=401492 Committed: https://crrev.com/fa27ef28c604c3bd343f7d8298ac3f328a9a0037 Cr-Commit-Position: refs/heads/master@{#295241} ========== to ========== cc: Use impl-side painting in LTH context lost tests. BUG=401492 Committed: https://crrev.com/fa27ef28c604c3bd343f7d8298ac3f328a9a0037 Cr-Commit-Position: refs/heads/master@{#295241} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
