Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
519583003:
Use the solid color detection to create solid layers (Closed)
DescriptionThis is an optimization that uses solid color detection to
prevent PictureLayerImpl from creating tiles if the entire
layer is solid.
If the entire layer is solid, PictureLayerImpl will now draw
solid quads using a method provided in SolidColorLayerImpl.
The is_solid_color_ and solid_color members have been moved
from PicturePile To BasePicturePile so that the properties
are copied between PicturePile and PicturePileImpl.
BUG=396908
Committed: https://crrev.com/c78b11fa0057ae197b03ac9de5a150482efe6faf
Cr-Commit-Position: refs/heads/master@{#295860}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Address review comments #Patch Set 3 : Passed the right color to the quad #
Total comments: 1
Patch Set 4 : move solid color dawing to static function in solidlayerimpl #Patch Set 5 : Added a unit test for solid color on picturelayerimpl #
Total comments: 17
Patch Set 6 : Review comment fixes #Patch Set 7 : Address review comments and add more unit tests #Patch Set 8 : remove unused variable #
Total comments: 26
Patch Set 9 : Another round of addressing review comments #Patch Set 10 : Clear the tilings if we are solid #Patch Set 11 : Fixed my DCHECKS #Patch Set 12 : Added another test to make sure the tilings are cleared correctly #
Total comments: 19
Patch Set 13 : Address more review comments #
Total comments: 2
Patch Set 14 : final merge #
Messages
Total messages: 41 (2 generated)
Hi, here's the solid color layer stuff in action, I haven't written any tests yet, and I need to figure out how to share the code to append the solid tiles with the SolidColorLayerImpl (which will likely mean deriving from it since it uses internal functions and vars) The virtual function to set the solid color was needed to work around tests that were failing due to the change.
hendrikw@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org, vmpstr@chromium.org
vmpstr@chromium.org: Please review changes in danakj@chromium.org: Please review changes in enne@chromium.org: Please review changes in
https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.cc#n... cc/layers/picture_layer.cc:45: SetSolidColorPropertiesTo(layer_impl); Why not just layer_impl->SetSolidColorState(...) directly? https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h File cc/layers/picture_layer.h (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h#ne... cc/layers/picture_layer.h:55: virtual void SetSolidColorPropertiesTo(PictureLayerImpl* layer_impl) const; Why virtual? https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:133: layer_impl->tilings_->SetClient(layer_impl); If you push solid color == true, it shouldn't have tilings. Or do you always create tilings? I think solid color shouldn't have a tiling set at all https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { Can you put this code in a helper and do if (IsSolidColor()) { AppendQuadsForSolidColor(...); return; } https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:162: const int tile_size_ = 256; // this comes from solidcolorlayerimpl trailing _ is for member variables. This should be const int kSolidColorTileSize in the anonymous namespace https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:167: AppendDebugBorderQuad( Should each of the solid color quads also have a debug border quad of some other color? https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:630: if (!pile_->CanRaster(tiling->contents_scale(), content_rect) || Hmm, I was hoping to avoid creating tilings at all for this, since things like tile priorities updates and whatever else have non-zero cost that is unneeded for solid color. Is that possible?
https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.cc#n... cc/layers/picture_layer.cc:45: SetSolidColorPropertiesTo(layer_impl); On 2014/08/29 06:49:14, vmpstr wrote: > Why not just layer_impl->SetSolidColorState(...) directly? see prev comment https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h File cc/layers/picture_layer.h (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h#ne... cc/layers/picture_layer.h:55: virtual void SetSolidColorPropertiesTo(PictureLayerImpl* layer_impl) const; On 2014/08/29 06:49:14, vmpstr wrote: > Why virtual? There are four tests in layer try host unit test which set SetMaxTransferBufferUsageBytes, to some value, then tests against that value. Obviously now, when we play the layer, it gets detected as solid and doesn't draw to any gles textures, so it never gets the chance to test the buffer usage (it returns zero). I'm using this as a way to override the solid color state for the fake picture layer used in the test :/ https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:133: layer_impl->tilings_->SetClient(layer_impl); On 2014/08/29 06:49:15, vmpstr wrote: > If you push solid color == true, it shouldn't have tilings. Or do you always > create tilings? I think solid color shouldn't have a tiling set at all We always create tilings_, but it's empty, so it shouldn't do any work. we're already doing this for the tens of empty layers we for some reason think we need to create. Any attempt I've made to avoid the tilings leaves us in a bad state. If you like, I can continue to try to avoid creating the tilings, but this change will become much more invasive. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { On 2014/08/29 06:49:15, vmpstr wrote: > Can you put this code in a helper and do > > if (IsSolidColor()) { > AppendQuadsForSolidColor(...); > return; > } > As I mentioned in the post above, I will need to derive PictureLayerImpl from SolidColorLayer and share that functionality completely. I didn't have time to complete this, I wanted to get this cl out quickly since I'm technically on vacation now. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:162: const int tile_size_ = 256; // this comes from solidcolorlayerimpl On 2014/08/29 06:49:15, vmpstr wrote: > trailing _ is for member variables. This should be const int kSolidColorTileSize > in the anonymous namespace This will go away when I derive from solid color. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:167: AppendDebugBorderQuad( On 2014/08/29 06:49:15, vmpstr wrote: > Should each of the solid color quads also have a debug border quad of some other > color? This functionality will exist in solid color when I'm done, I don't plan on changing it, in fact, all of this code should be rewritten, we're not batching the quads. I'm not convinced that it makes sense to check occlusions and output rects (on hardware), we probably should just output one rect, but this is out of scope for this change. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:630: if (!pile_->CanRaster(tiling->contents_scale(), content_rect) || On 2014/08/29 06:49:15, vmpstr wrote: > Hmm, I was hoping to avoid creating tilings at all for this, since things like > tile priorities updates and whatever else have non-zero cost that is unneeded > for solid color. Is that possible? If there are no tiles in the tiling, why would updating tile priorities do anything?
https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.cc#n... cc/layers/picture_layer.cc:45: SetSolidColorPropertiesTo(layer_impl); On 2014/08/29 14:03:21, Hendrik wrote: > On 2014/08/29 06:49:14, vmpstr wrote: > > Why not just layer_impl->SetSolidColorState(...) directly? > > see prev comment Acknowledged. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h File cc/layers/picture_layer.h (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h#ne... cc/layers/picture_layer.h:55: virtual void SetSolidColorPropertiesTo(PictureLayerImpl* layer_impl) const; On 2014/08/29 14:03:21, Hendrik wrote: > On 2014/08/29 06:49:14, vmpstr wrote: > > Why virtual? > > There are four tests in layer try host unit test which set > SetMaxTransferBufferUsageBytes, to some value, then tests against that value. > Obviously now, when we play the layer, it gets detected as solid and doesn't > draw to any gles textures, so it never gets the chance to test the buffer usage > (it returns zero). I'm using this as a way to override the solid color state > for the fake picture layer used in the test :/ Ah ok. Can you leave a "Virtual for testing." comment here? https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:133: layer_impl->tilings_->SetClient(layer_impl); On 2014/08/29 14:03:21, Hendrik wrote: > On 2014/08/29 06:49:15, vmpstr wrote: > > If you push solid color == true, it shouldn't have tilings. Or do you always > > create tilings? I think solid color shouldn't have a tiling set at all > > We always create tilings_, but it's empty, so it shouldn't do any work. we're > already doing this for the tens of empty layers we for some reason think we need > to create. > > Any attempt I've made to avoid the tilings leaves us in a bad state. > > If you like, I can continue to try to avoid creating the tilings, but this > change will become much more invasive. No, I guess not having tilings but having a tiling set is fine. I would then ask to put a few DCHECKs here and there along the lines of DCHECK(!IsSolidColor() || !tilings_->num_tilings()); https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { On 2014/08/29 14:03:21, Hendrik wrote: > On 2014/08/29 06:49:15, vmpstr wrote: > > Can you put this code in a helper and do > > > > if (IsSolidColor()) { > > AppendQuadsForSolidColor(...); > > return; > > } > > > > As I mentioned in the post above, I will need to derive PictureLayerImpl from > SolidColorLayer and share that functionality completely. I didn't have time to > complete this, I wanted to get this cl out quickly since I'm technically on > vacation now. > Can you have a SolidColorLayerImpl as a member instead? It feels weird deriving from it. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:162: const int tile_size_ = 256; // this comes from solidcolorlayerimpl On 2014/08/29 14:03:21, Hendrik wrote: > On 2014/08/29 06:49:15, vmpstr wrote: > > trailing _ is for member variables. This should be const int > kSolidColorTileSize > > in the anonymous namespace > > This will go away when I derive from solid color. Acknowledged. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:167: AppendDebugBorderQuad( On 2014/08/29 14:03:21, Hendrik wrote: > On 2014/08/29 06:49:15, vmpstr wrote: > > Should each of the solid color quads also have a debug border quad of some > other > > color? > > This functionality will exist in solid color when I'm done, I don't plan on > changing it, in fact, all of this code should be rewritten, we're not batching > the quads. I'm not convinced that it makes sense to check occlusions and output > rects (on hardware), we probably should just output one rect, but this is out of > scope for this change. Acknowledged. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:630: if (!pile_->CanRaster(tiling->contents_scale(), content_rect) || On 2014/08/29 14:03:21, Hendrik wrote: > On 2014/08/29 06:49:15, vmpstr wrote: > > Hmm, I was hoping to avoid creating tilings at all for this, since things like > > tile priorities updates and whatever else have non-zero cost that is unneeded > > for solid color. Is that possible? > > If there are no tiles in the tiling, why would updating tile priorities do > anything? If we don't create tilings, then this function should never be called. So you should DCHECK(!IsSolidColor()) instead then.
On 2014/08/29 14:20:45, vmpstr wrote: > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.cc > File cc/layers/picture_layer.cc (right): > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.cc#n... > cc/layers/picture_layer.cc:45: SetSolidColorPropertiesTo(layer_impl); > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:14, vmpstr wrote: > > > Why not just layer_impl->SetSolidColorState(...) directly? > > > > see prev comment > > Acknowledged. > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h > File cc/layers/picture_layer.h (right): > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h#ne... > cc/layers/picture_layer.h:55: virtual void > SetSolidColorPropertiesTo(PictureLayerImpl* layer_impl) const; > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:14, vmpstr wrote: > > > Why virtual? > > > > There are four tests in layer try host unit test which set > > SetMaxTransferBufferUsageBytes, to some value, then tests against that value. > > Obviously now, when we play the layer, it gets detected as solid and doesn't > > draw to any gles textures, so it never gets the chance to test the buffer > usage > > (it returns zero). I'm using this as a way to override the solid color state > > for the fake picture layer used in the test :/ > > Ah ok. Can you leave a "Virtual for testing." comment here? > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl.cc > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... > cc/layers/picture_layer_impl.cc:133: > layer_impl->tilings_->SetClient(layer_impl); > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:15, vmpstr wrote: > > > If you push solid color == true, it shouldn't have tilings. Or do you always > > > create tilings? I think solid color shouldn't have a tiling set at all > > > > We always create tilings_, but it's empty, so it shouldn't do any work. we're > > already doing this for the tens of empty layers we for some reason think we > need > > to create. > > > > Any attempt I've made to avoid the tilings leaves us in a bad state. > > > > If you like, I can continue to try to avoid creating the tilings, but this > > change will become much more invasive. > > No, I guess not having tilings but having a tiling set is fine. I would then ask > to put a few DCHECKs here and there along the lines of > > DCHECK(!IsSolidColor() || !tilings_->num_tilings()); > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... > cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:15, vmpstr wrote: > > > Can you put this code in a helper and do > > > > > > if (IsSolidColor()) { > > > AppendQuadsForSolidColor(...); > > > return; > > > } > > > > > > > As I mentioned in the post above, I will need to derive PictureLayerImpl from > > SolidColorLayer and share that functionality completely. I didn't have time > to > > complete this, I wanted to get this cl out quickly since I'm technically on > > vacation now. > > > > Can you have a SolidColorLayerImpl as a member instead? It feels weird deriving > from it. > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... > cc/layers/picture_layer_impl.cc:162: const int tile_size_ = 256; // this comes > from solidcolorlayerimpl > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:15, vmpstr wrote: > > > trailing _ is for member variables. This should be const int > > kSolidColorTileSize > > > in the anonymous namespace > > > > This will go away when I derive from solid color. > > Acknowledged. > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... > cc/layers/picture_layer_impl.cc:167: AppendDebugBorderQuad( > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:15, vmpstr wrote: > > > Should each of the solid color quads also have a debug border quad of some > > other > > > color? > > > > This functionality will exist in solid color when I'm done, I don't plan on > > changing it, in fact, all of this code should be rewritten, we're not batching > > the quads. I'm not convinced that it makes sense to check occlusions and > output > > rects (on hardware), we probably should just output one rect, but this is out > of > > scope for this change. > > Acknowledged. > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... > cc/layers/picture_layer_impl.cc:630: if > (!pile_->CanRaster(tiling->contents_scale(), content_rect) || > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:15, vmpstr wrote: > > > Hmm, I was hoping to avoid creating tilings at all for this, since things > like > > > tile priorities updates and whatever else have non-zero cost that is > unneeded > > > for solid color. Is that possible? > > > > If there are no tiles in the tiling, why would updating tile priorities do > > anything? > > If we don't create tilings, then this function should never be called. So you > should DCHECK(!IsSolidColor()) instead then. I agree with all of your comments, but I'm not sure I like making solid layer member either, I'll see if I can come up with another way, maybe. I'll make the changes when I get to toronto.
https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h File cc/layers/picture_layer.h (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h#ne... cc/layers/picture_layer.h:55: virtual void SetSolidColorPropertiesTo(PictureLayerImpl* layer_impl) const; On 2014/08/29 at 14:20:44, vmpstr wrote: > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:14, vmpstr wrote: > > > Why virtual? > > > > There are four tests in layer try host unit test which set > > SetMaxTransferBufferUsageBytes, to some value, then tests against that value. > > Obviously now, when we play the layer, it gets detected as solid and doesn't > > draw to any gles textures, so it never gets the chance to test the buffer usage > > (it returns zero). I'm using this as a way to override the solid color state > > for the fake picture layer used in the test :/ > > Ah ok. Can you leave a "Virtual for testing." comment here? Virtual for testing. :( Can you make the fake picture layer not detected as solid in that test? https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { On 2014/08/29 at 14:20:45, vmpstr wrote: > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:15, vmpstr wrote: > > > Can you put this code in a helper and do > > > > > > if (IsSolidColor()) { > > > AppendQuadsForSolidColor(...); > > > return; > > > } > > > > > > > As I mentioned in the post above, I will need to derive PictureLayerImpl from > > SolidColorLayer and share that functionality completely. I didn't have time to > > complete this, I wanted to get this cl out quickly since I'm technically on > > vacation now. > > > > Can you have a SolidColorLayerImpl as a member instead? It feels weird deriving from it. Please, no to either deriving from PictureLayerImpl or having a SolidColorLayerImpl as a member. What about making some SolidColorQuadGenerator class that both of them could use? It could even just have no members and only static functions that you have to pass the right things into. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:170: // We create a series of smaller quads instead of just one large one so that This code is not your fault, but we shouldn't do this anymore. Occlusion used to be outside the layer at the quad level and so the solid color layer had to be split up into small enough quads that some of them could be occluded. You can now ask the occlusion tracker for the occlusion rect directly, and so it'd be better to generate the minimal set of rects from that. Each quad is a needless draw call, so it'd be better to not have to do this splitting. If you want to punt to another patch, that's fine, but it'd be a good followup.
I've put in DCHECKS, made sure the tilings_ is empty, found a way to remove the virtual, placed a AppendSolidQuads helper function in the base class. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h File cc/layers/picture_layer.h (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h#ne... cc/layers/picture_layer.h:55: virtual void SetSolidColorPropertiesTo(PictureLayerImpl* layer_impl) const; On 2014/08/29 16:53:43, enne wrote: > On 2014/08/29 at 14:20:44, vmpstr wrote: > > On 2014/08/29 14:03:21, Hendrik wrote: > > > On 2014/08/29 06:49:14, vmpstr wrote: > > > > Why virtual? > > > > > > There are four tests in layer try host unit test which set > > > SetMaxTransferBufferUsageBytes, to some value, then tests against that > value. > > > Obviously now, when we play the layer, it gets detected as solid and doesn't > > > draw to any gles textures, so it never gets the chance to test the buffer > usage > > > (it returns zero). I'm using this as a way to override the solid color > state > > > for the fake picture layer used in the test :/ > > > > Ah ok. Can you leave a "Virtual for testing." comment here? > > Virtual for testing. :( > > Can you make the fake picture layer not detected as solid in that test? I found a way to do the same thing without the virtual https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:133: layer_impl->tilings_->SetClient(layer_impl); On 2014/08/29 14:20:44, vmpstr wrote: > On 2014/08/29 14:03:21, Hendrik wrote: > > On 2014/08/29 06:49:15, vmpstr wrote: > > > If you push solid color == true, it shouldn't have tilings. Or do you always > > > create tilings? I think solid color shouldn't have a tiling set at all > > > > We always create tilings_, but it's empty, so it shouldn't do any work. we're > > already doing this for the tens of empty layers we for some reason think we > need > > to create. > > > > Any attempt I've made to avoid the tilings leaves us in a bad state. > > > > If you like, I can continue to try to avoid creating the tilings, but this > > change will become much more invasive. > > No, I guess not having tilings but having a tiling set is fine. I would then ask > to put a few DCHECKs here and there along the lines of > > DCHECK(!IsSolidColor() || !tilings_->num_tilings()); Acknowledged. https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { On 2014/08/29 16:53:43, enne wrote: > On 2014/08/29 at 14:20:45, vmpstr wrote: > > On 2014/08/29 14:03:21, Hendrik wrote: > > > On 2014/08/29 06:49:15, vmpstr wrote: > > > > Can you put this code in a helper and do > > > > > > > > if (IsSolidColor()) { > > > > AppendQuadsForSolidColor(...); > > > > return; > > > > } > > > > > > > > > > As I mentioned in the post above, I will need to derive PictureLayerImpl > from > > > SolidColorLayer and share that functionality completely. I didn't have time > to > > > complete this, I wanted to get this cl out quickly since I'm technically on > > > vacation now. > > > > > > > Can you have a SolidColorLayerImpl as a member instead? It feels weird > deriving from it. > > Please, no to either deriving from PictureLayerImpl or having a > SolidColorLayerImpl as a member. > > What about making some SolidColorQuadGenerator class that both of them could > use? It could even just have no members and only static functions that you have > to pass the right things into. I already tried that. It calls AppendDebugBorderQuad, which is a protected function of LayerImpl. I can think of several ways around this -- this last one has a helper function in layerimpl https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:170: // We create a series of smaller quads instead of just one large one so that On 2014/08/29 16:53:43, enne wrote: > This code is not your fault, but we shouldn't do this anymore. Occlusion used > to be outside the layer at the quad level and so the solid color layer had to be > split up into small enough quads that some of them could be occluded. > > You can now ask the occlusion tracker for the occlusion rect directly, and so > it'd be better to generate the minimal set of rects from that. Each quad is a > needless draw call, so it'd be better to not have to do this splitting. > > If you want to punt to another patch, that's fine, but it'd be a good followup. Acknowledged.
On Tue, Sep 2, 2014 at 10:19 AM, <hendrikw@chromium.org> wrote: > Reviewers: enne, danakj, vmpstr, > > Message: > I've put in DCHECKS, made sure the tilings_ is empty, found a way to > remove the > virtual, placed a AppendSolidQuads helper function in the base class. > > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer.h > File cc/layers/picture_layer.h (right): > > https://codereview.chromium.org/519583003/diff/1/cc/ > layers/picture_layer.h#newcode55 > cc/layers/picture_layer.h:55: virtual void > SetSolidColorPropertiesTo(PictureLayerImpl* layer_impl) const; > On 2014/08/29 16:53:43, enne wrote: > >> On 2014/08/29 at 14:20:44, vmpstr wrote: >> > On 2014/08/29 14:03:21, Hendrik wrote: >> > > On 2014/08/29 06:49:14, vmpstr wrote: >> > > > Why virtual? >> > > >> > > There are four tests in layer try host unit test which set >> > > SetMaxTransferBufferUsageBytes, to some value, then tests against >> > that > >> value. >> > > Obviously now, when we play the layer, it gets detected as solid >> > and doesn't > >> > > draw to any gles textures, so it never gets the chance to test the >> > buffer > >> usage >> > > (it returns zero). I'm using this as a way to override the solid >> > color > >> state >> > > for the fake picture layer used in the test :/ >> > >> > Ah ok. Can you leave a "Virtual for testing." comment here? >> > > Virtual for testing. :( >> > > Can you make the fake picture layer not detected as solid in that >> > test? > > I found a way to do the same thing without the virtual > > https://codereview.chromium.org/519583003/diff/1/cc/ > layers/picture_layer_impl.cc > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/519583003/diff/1/cc/ > layers/picture_layer_impl.cc#newcode133 > cc/layers/picture_layer_impl.cc:133: > layer_impl->tilings_->SetClient(layer_impl); > On 2014/08/29 14:20:44, vmpstr wrote: > >> On 2014/08/29 14:03:21, Hendrik wrote: >> > On 2014/08/29 06:49:15, vmpstr wrote: >> > > If you push solid color == true, it shouldn't have tilings. Or do >> > you always > >> > > create tilings? I think solid color shouldn't have a tiling set at >> > all > >> > >> > We always create tilings_, but it's empty, so it shouldn't do any >> > work. we're > >> > already doing this for the tens of empty layers we for some reason >> > think we > >> need >> > to create. >> > >> > Any attempt I've made to avoid the tilings leaves us in a bad state. >> > >> > If you like, I can continue to try to avoid creating the tilings, >> > but this > >> > change will become much more invasive. >> > > No, I guess not having tilings but having a tiling set is fine. I >> > would then ask > >> to put a few DCHECKs here and there along the lines of >> > > DCHECK(!IsSolidColor() || !tilings_->num_tilings()); >> > > Acknowledged. > > https://codereview.chromium.org/519583003/diff/1/cc/ > layers/picture_layer_impl.cc#newcode160 > cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { > On 2014/08/29 16:53:43, enne wrote: > >> On 2014/08/29 at 14:20:45, vmpstr wrote: >> > On 2014/08/29 14:03:21, Hendrik wrote: >> > > On 2014/08/29 06:49:15, vmpstr wrote: >> > > > Can you put this code in a helper and do >> > > > >> > > > if (IsSolidColor()) { >> > > > AppendQuadsForSolidColor(...); >> > > > return; >> > > > } >> > > > >> > > >> > > As I mentioned in the post above, I will need to derive >> > PictureLayerImpl > >> from >> > > SolidColorLayer and share that functionality completely. I didn't >> > have time > >> to >> > > complete this, I wanted to get this cl out quickly since I'm >> > technically on > >> > > vacation now. >> > > >> > >> > Can you have a SolidColorLayerImpl as a member instead? It feels >> > weird > >> deriving from it. >> > > Please, no to either deriving from PictureLayerImpl or having a >> SolidColorLayerImpl as a member. >> > > What about making some SolidColorQuadGenerator class that both of them >> > could > >> use? It could even just have no members and only static functions >> > that you have > >> to pass the right things into. >> > > I already tried that. It calls AppendDebugBorderQuad, which is a > protected function of LayerImpl. > If it makes the code cleaner, you could make this public or move it to the same helper class. Right now that method lives on LayerImpl cuz it uses the LayerTreeImpl pointer to figure out if it should append the quad. But this is kind of that crappy pattern where class A (LTHI) calls class B (LayerImpl), and class B calls getters back on class A (LTHI). Although in this case, the 2nd class A is LTI, it's still pulling data that could have been pushed in. What I mean to say is, if we put a "debug borders" bool on the AppendQuadsData so that we push that data down instead of pulling it later, would that let you decouple this stuff from the LayerImpl base class? > I can think of several ways around this -- this last one has a helper > function in layerimpl > > https://codereview.chromium.org/519583003/diff/1/cc/ > layers/picture_layer_impl.cc#newcode170 > cc/layers/picture_layer_impl.cc:170: // We create a series of smaller > quads instead of just one large one so that > On 2014/08/29 16:53:43, enne wrote: > >> This code is not your fault, but we shouldn't do this anymore. >> > Occlusion used > >> to be outside the layer at the quad level and so the solid color layer >> > had to be > >> split up into small enough quads that some of them could be occluded. >> > > You can now ask the occlusion tracker for the occlusion rect directly, >> > and so > >> it'd be better to generate the minimal set of rects from that. Each >> > quad is a > >> needless draw call, so it'd be better to not have to do this >> > splitting. > > If you want to punt to another patch, that's fine, but it'd be a good >> > followup. > > Acknowledged. > > Description: > Use the solid color detection to create solid layers > > BUG=396908 > > Please review this at https://codereview.chromium.org/519583003/ > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+83, -32 lines): > M cc/layers/layer_impl.h > M cc/layers/layer_impl.cc > M cc/layers/picture_layer.h > M cc/layers/picture_layer.cc > M cc/layers/picture_layer_impl.h > M cc/layers/picture_layer_impl.cc > M cc/layers/solid_color_layer_impl.cc > M cc/test/fake_picture_layer.cc > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { On 2014/09/02 14:19:17, Hendrik wrote: > On 2014/08/29 16:53:43, enne wrote: > > On 2014/08/29 at 14:20:45, vmpstr wrote: > > > On 2014/08/29 14:03:21, Hendrik wrote: > > > > On 2014/08/29 06:49:15, vmpstr wrote: > > > > > Can you put this code in a helper and do > > > > > > > > > > if (IsSolidColor()) { > > > > > AppendQuadsForSolidColor(...); > > > > > return; > > > > > } > > > > > > > > > > > > > As I mentioned in the post above, I will need to derive PictureLayerImpl > > from > > > > SolidColorLayer and share that functionality completely. I didn't have > time > > to > > > > complete this, I wanted to get this cl out quickly since I'm technically > on > > > > vacation now. > > > > > > > > > > Can you have a SolidColorLayerImpl as a member instead? It feels weird > > deriving from it. > > > > Please, no to either deriving from PictureLayerImpl or having a > > SolidColorLayerImpl as a member. > > > > What about making some SolidColorQuadGenerator class that both of them could > > use? It could even just have no members and only static functions that you > have > > to pass the right things into. > > I already tried that. It calls AppendDebugBorderQuad, which is a protected > function of LayerImpl. > > I can think of several ways around this -- this last one has a helper function > in layerimpl I feel that LayerImpl should really move more towards being an interface type of thing. In that spirit, I think putting more functionality there that is kind of specific to a couple of implementations is moving in the other direction. Feel free to disagree, but I would vote for trying to make a separate helper class for PictureLayerImpl and SolidColorLayerImpl instead of using LayerImpl for that purpose. https://codereview.chromium.org/519583003/diff/40001/cc/layers/picture_layer.h File cc/layers/picture_layer.h (right): https://codereview.chromium.org/519583003/diff/40001/cc/layers/picture_layer.... cc/layers/picture_layer.h:19: class PictureLayerImpl; nit: is this still needed?
On 2014/09/02 19:02:25, vmpstr wrote: > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl.cc > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/519583003/diff/1/cc/layers/picture_layer_impl... > cc/layers/picture_layer_impl.cc:160: if (IsSolidColor()) { > On 2014/09/02 14:19:17, Hendrik wrote: > > On 2014/08/29 16:53:43, enne wrote: > > > On 2014/08/29 at 14:20:45, vmpstr wrote: > > > > On 2014/08/29 14:03:21, Hendrik wrote: > > > > > On 2014/08/29 06:49:15, vmpstr wrote: > > > > > > Can you put this code in a helper and do > > > > > > > > > > > > if (IsSolidColor()) { > > > > > > AppendQuadsForSolidColor(...); > > > > > > return; > > > > > > } > > > > > > > > > > > > > > > > As I mentioned in the post above, I will need to derive PictureLayerImpl > > > from > > > > > SolidColorLayer and share that functionality completely. I didn't have > > time > > > to > > > > > complete this, I wanted to get this cl out quickly since I'm technically > > on > > > > > vacation now. > > > > > > > > > > > > > Can you have a SolidColorLayerImpl as a member instead? It feels weird > > > deriving from it. > > > > > > Please, no to either deriving from PictureLayerImpl or having a > > > SolidColorLayerImpl as a member. > > > > > > What about making some SolidColorQuadGenerator class that both of them could > > > use? It could even just have no members and only static functions that you > > have > > > to pass the right things into. > > > > I already tried that. It calls AppendDebugBorderQuad, which is a protected > > function of LayerImpl. > > > > I can think of several ways around this -- this last one has a helper function > > in layerimpl > > I feel that LayerImpl should really move more towards being an interface type of > thing. In that spirit, I think putting more functionality there that is kind of > specific to a couple of implementations is moving in the other direction. > > Feel free to disagree, but I would vote for trying to make a separate helper > class for PictureLayerImpl and SolidColorLayerImpl instead of using LayerImpl > for that purpose. > > https://codereview.chromium.org/519583003/diff/40001/cc/layers/picture_layer.h > File cc/layers/picture_layer.h (right): > > https://codereview.chromium.org/519583003/diff/40001/cc/layers/picture_layer.... > cc/layers/picture_layer.h:19: class PictureLayerImpl; > nit: is this still needed? Hi, back from vacation. @vmpstr Making a new class for one function seems to be overkill. And why a class when a function would work? And what would you even call it, SolidAndPictureLayerImplHelper? This why deriving PictureLayerImpl from SolidColorLayerImpl made sense, PictureLI sometimes behaves as a solid color layer, it is a superset of the SolidColorLI's behaviour. As is, LayerImpl can never be an interface, to do so you'd have to create a new empty class LayerImplBase (or whatever), and make LayerImpl derive from that. LayerImpl has WAY too much common functionality to ever move to an interface. What you would gain from this is an extra class, nothing else :/ @danakj ShowDebugBorders() is not the only thing needed, it also has a color and a width, and it comes from a virtual function that different layerimpl derivations can override. Since the layer being drawn knows the color, width and that it should be shown, this information shouldn't be passed in from outside the layer (through AppendQuadsData), because we'd need to ask the layer for the info, only to pass it back to the layer when we AppendQuads. That said, I agree, the code is a bit crappy, a couple of the virtual functions could be removed, and the code could be simplified a bit, and a function gets called when it doesn't need to, but I think you'd agree that's out of scope for this change. If you really like, I could fix this later. @everyone If you want me to make a helper class, or function, or if you agree that PictureLI is a superset of SolidColorLI, please let me know which method will get me the LGTM (and if you want the class, give me a class name), I want to avoid having this review go back and forth several more times if possible :)
hendrikw@chromium.org changed reviewers: - enne@chromium.org
Hi, I've moved the DrawSolidQuads to a static function within SolidColorLayerImpl. The parts that come from LayerImpl are now duplicated in both functions. Hopefully this pleases vmpstr :) @danakj - I've noticed that the code that populates the shared_quad_state is slightly different between SCLI and PLI, in that PLI uses the scale. I've tried using scale and no scale for AppendSolidQuads, both work (at 2x scale). Right now I have PLI using the unscaled version (via PopulateSharedQuadState), since that's what SCLI does, is this ok?
On Tue, Sep 9, 2014 at 4:08 PM, <hendrikw@chromium.org> wrote: > Hi, I've moved the DrawSolidQuads to a static function within > SolidColorLayerImpl. The parts that come from LayerImpl are now > duplicated in > both functions. Hopefully this pleases vmpstr :) > > @danakj - I've noticed that the code that populates the shared_quad_state > is > slightly different between SCLI and PLI, in that PLI uses the scale. I've > tried > using scale and no scale for AppendSolidQuads, both work (at 2x scale). > Right > now I have PLI using the unscaled version (via PopulateSharedQuadState), > since > that's what SCLI does, is this ok? > No, it's not. The quad bounds won't match the SQS bounds, and that'll break edge AA. Try out the compositing/ layout tests with your change. (Maybe we should have some cc pixel tests for AntiAliasing). To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
@danakj - as discussed, since I'm passing the unscaled content_bounds and scale = 1, it looks like the AA is still working correctly when the scale is changed. I've added a unit tests that makes sure the solid state is being passed from the pile->layer->picturelayerimpl correctly.
https://codereview.chromium.org/519583003/diff/80001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/layers/layer_impl.cc#... cc/layers/layer_impl.cc:25: #include "cc/quads/solid_color_draw_quad.h" not needed? https://codereview.chromium.org/519583003/diff/80001/cc/layers/layer_impl.cc#... cc/layers/layer_impl.cc:29: #include "cc/trees/occlusion_tracker.h" ditto https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer.... cc/layers/picture_layer.cc:39: layer_impl->SetSolidColorState(pile_->IsSolidColor(), pile_->GetSolidColor()); can the layerimpl not pull this off its own pile instead of adding these class members which just double the ones on the PLI's pile?
https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:162: render_pass->CreateAndAppendSharedQuadState(); nit: whitespace below this to show it's not only used for that if but also below it? https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:164: PopulateSharedQuadState(shared_quad_state); can you add a PLI unittest that hits this code path? https://codereview.chromium.org/519583003/diff/80001/cc/test/fake_picture_lay... File cc/test/fake_picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/test/fake_picture_lay... cc/test/fake_picture_layer.cc:37: // the tests expect a non-solid layer Comments should be complete sentence including capitals and punctuation. https://codereview.chromium.org/519583003/diff/80001/cc/test/fake_picture_lay... cc/test/fake_picture_layer.cc:38: layer_impl->SetSolidColorState(false, SK_ColorBLACK); This seems awkward. Maybe we should make FakeContentLayerClient not draw a solid color by default and add a bool that lets you override it with a solid color instead if needed?
Overall approach seems fine, mostly concerned with tests and not duplicating the pile state into the layer
Fixes for some review comments, Thanks! And a few questions https://codereview.chromium.org/519583003/diff/80001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/layers/layer_impl.cc#... cc/layers/layer_impl.cc:25: #include "cc/quads/solid_color_draw_quad.h" On 2014/09/10 18:40:50, danakj wrote: > not needed? Acknowledged. https://codereview.chromium.org/519583003/diff/80001/cc/layers/layer_impl.cc#... cc/layers/layer_impl.cc:29: #include "cc/trees/occlusion_tracker.h" On 2014/09/10 18:40:50, danakj wrote: > ditto Acknowledged. https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer.... cc/layers/picture_layer.cc:39: layer_impl->SetSolidColorState(pile_->IsSolidColor(), pile_->GetSolidColor()); On 2014/09/10 18:40:50, danakj wrote: > can the layerimpl not pull this off its own pile instead of adding these class > members which just double the ones on the PLI's pile? PLI doesn't know if it's solid or not (only PL knows this). Besides, I need this function to override the solid state for the test code. https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:162: render_pass->CreateAndAppendSharedQuadState(); On 2014/09/10 18:46:40, danakj wrote: > nit: whitespace below this to show it's not only used for that if but also below > it? Acknowledged. https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:164: PopulateSharedQuadState(shared_quad_state); On 2014/09/10 18:46:40, danakj wrote: > can you add a PLI unittest that hits this code path? There are several tests which hit both code paths, but I could also call AppendQuads in test that I just wrote, would that be sufficient? https://codereview.chromium.org/519583003/diff/80001/cc/test/fake_picture_lay... File cc/test/fake_picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/test/fake_picture_lay... cc/test/fake_picture_layer.cc:37: // the tests expect a non-solid layer On 2014/09/10 18:46:40, danakj wrote: > Comments should be complete sentence including capitals and punctuation. Acknowledged. https://codereview.chromium.org/519583003/diff/80001/cc/test/fake_picture_lay... cc/test/fake_picture_layer.cc:38: layer_impl->SetSolidColorState(false, SK_ColorBLACK); On 2014/09/10 18:46:40, danakj wrote: > This seems awkward. Maybe we should make FakeContentLayerClient not draw a solid > color by default and add a bool that lets you override it with a solid color > instead if needed? Hmm, well, the test does require a solid color to be rendered via the non-solid code path, so the case you are describing never exists :) I agree, awkward is a good word for this. If I remember correctly, the test is setting some max buffer size, and then checks that after we render, we never use more than that buffer size. If the layer is solid, we no longer render to a buffer, so the test no longer works. I suppose I could replace the solid render with something else, but that would probably be even more awkward :(
https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer.... cc/layers/picture_layer.cc:39: layer_impl->SetSolidColorState(pile_->IsSolidColor(), pile_->GetSolidColor()); On 2014/09/11 00:27:40, Hendrik wrote: > On 2014/09/10 18:40:50, danakj wrote: > > can the layerimpl not pull this off its own pile instead of adding these class > > members which just double the ones on the PLI's pile? > > PLI doesn't know if it's solid or not (only PL knows this). Besides, I need > this function to override the solid state for the test code. Can't the test set the bool in the pile? Take a look at the is_mask_ in pile for an example. PLI uses that to change behaviour. And tests set it on the pile to change behaviour.
Sorry missed the other 2 questions. https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:164: PopulateSharedQuadState(shared_quad_state); On 2014/09/11 00:27:40, Hendrik wrote: > On 2014/09/10 18:46:40, danakj wrote: > > can you add a PLI unittest that hits this code path? > > There are several tests which hit both code paths, but I could also call > AppendQuads in test that I just wrote, would that be sufficient? A test designed to explictly hit both would be nice, existing tests may hit it by chance and change what they test over time by seemingly unrelated changes. The test you wrote is a picture layer test, so it does not have an impl layer to call this on. You could add one and push to it, but it'd be more straightforward to add something to picture_layer_impl_unittest.cc i think. https://codereview.chromium.org/519583003/diff/80001/cc/test/fake_picture_lay... File cc/test/fake_picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/80001/cc/test/fake_picture_lay... cc/test/fake_picture_layer.cc:38: layer_impl->SetSolidColorState(false, SK_ColorBLACK); On 2014/09/11 00:27:40, Hendrik wrote: > On 2014/09/10 18:46:40, danakj wrote: > > This seems awkward. Maybe we should make FakeContentLayerClient not draw a > solid > > color by default and add a bool that lets you override it with a solid color > > instead if needed? > > Hmm, well, the test does require a solid color to be rendered via the non-solid > code path, so the case you are describing never exists :) I agree, awkward is a You need a layer with a solid color with a pile that says it's not solid color? I'm confused by this, the test sounds weird? > good word for this. If I remember correctly, the test is setting some max > buffer size, and then checks that after we render, we never use more than that > buffer size. If the layer is solid, we no longer render to a buffer, so the > test no longer works. > > I suppose I could replace the solid render with something else, but that would > probably be even more awkward :( It would be more realistic though, doing like an alternating of 2 colors every other pixel or something. Previously all our tests were built assuming painting => tiles with resources. Making that the default behaviour still by actually providing content that causes that seems nicer than overriding some behaviour on PLI here. What happens when a test wants to make a picture layer with solid color pile and see the result be solid color with no resources on the impl side? This totally blocks that.
Hi, please take a look, I think I've addressed all of the issues.
https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:120: DCHECK(!pile_->is_solid_color() || !tilings_->num_tilings()); I like this dcheck, and I think you should put it in more functions both as a check and as a statement that makes the intentions of what we expect to happen in a function clear. https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:123: layer_impl->tilings_.swap(tilings_); In the case where a non-solid layer becomes solid, I think you have to remove all tilings from the new recycled tree. We have this notion to ensure that the recycle tree does not have any unshared tiles, which is important to maintain. https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:172: draw_properties().target_space_transform, Here we pass a target_space_transform, but below we use scaled_draw_transform. Is this supposed to be consistent? danakj, do you know? :) I'm more curious because I plan to change this to be Occlusion instead of OcclusionTracker. https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:319: std::vector<Tile*> tiles = you should ASSERT_TRUE(active_layer_->tilings()); and ASSERT_GT(active_layer_->tilings()->num_tilings(), 0u); to ensure that if the test is to fail, it won't just crash. https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:111: pile->SetTileGridSize(gfx::Size(1000, 1000)); Is this and the previous line required? I'd prefer not to have them unless the actual code also sets them to these values (just so we're testing the thing that we're actually running). https://codereview.chromium.org/519583003/diff/140001/cc/test/fake_picture_la... File cc/test/fake_picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/test/fake_picture_la... cc/test/fake_picture_layer.cc:36: PictureLayerImpl* layer_impl = static_cast<PictureLayerImpl*>(layer); this is not needed? https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_pixeltest_readback.cc (left): https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_pixeltest_readback.cc:939: background_impl->HighResTiling()->contents_scale()); Why this change?
https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:123: layer_impl->tilings_.swap(tilings_); On 2014/09/15 15:45:07, vmpstr wrote: > In the case where a non-solid layer becomes solid, I think you have to remove > all tilings from the new recycled tree. We have this notion to ensure that the > recycle tree does not have any unshared tiles, which is important to maintain. Please add a test for that also. https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:172: draw_properties().target_space_transform, On 2014/09/15 15:45:07, vmpstr wrote: > Here we pass a target_space_transform, but below we use scaled_draw_transform. > Is this supposed to be consistent? danakj, do you know? :) I'm more curious > because I plan to change this to be Occlusion instead of OcclusionTracker. This is cool. The transform is consistent with the bounds etc that's given to this function here. https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:273: void TestQuadsForSolidColor(bool test_for_solid) { This code is soo far away from the actual tests. Can you just make a PictureLayerImplTest subclass down by the actual tests and move this function there? https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_pixeltest_readback.cc (left): https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_pixeltest_readback.cc:939: background_impl->HighResTiling()->contents_scale()); On 2014/09/15 15:45:08, vmpstr wrote: > Why this change? This test makes solid color quads now so it has no tilings. It's still testing a readback with a DSF so I think that's fine. https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_pixeltest_readback.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_pixeltest_readback.cc:934: LayerImpl* root_impl = host_impl->active_tree()->root_layer(); Just remove all these layers, they're unused.
https://codereview.chromium.org/519583003/diff/140001/cc/resources/picture_pi... File cc/resources/picture_pile_base.h (right): https://codereview.chromium.org/519583003/diff/140001/cc/resources/picture_pi... cc/resources/picture_pile_base.h:53: bool is_solid_color() const { return is_solid_color_; } nit: whitespace above https://codereview.chromium.org/519583003/diff/140001/cc/resources/picture_pi... cc/resources/picture_pile_base.h:54: SkColor get_solid_color() const { return solid_color_; } style nit: accessors of this form should match the variable name (no get_). CamelCase functions could add a Get tho.
Please take another look, thanks! https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:120: DCHECK(!pile_->is_solid_color() || !tilings_->num_tilings()); On 2014/09/15 15:45:07, vmpstr wrote: > I like this dcheck, and I think you should put it in more functions both as a > check and as a statement that makes the intentions of what we expect to happen > in a function clear. This is the only place in the code that the state of solidness can change, There is already a DCHECK in CreateTile, and CanHaveTiling returns on it, these are the most relevant locations. If you want it in other functions, just let me know which ones and I'll add it for you. https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:123: layer_impl->tilings_.swap(tilings_); On 2014/09/15 15:45:07, vmpstr wrote: > In the case where a non-solid layer becomes solid, I think you have to remove > all tilings from the new recycled tree. We have this notion to ensure that the > recycle tree does not have any unshared tiles, which is important to maintain. We had discussed this a couple of weeks ago, you said doing this would be expensive since it would force us to recreate tilings that could be reused in certain situations. Have you changed your mind on this? https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:273: void TestQuadsForSolidColor(bool test_for_solid) { On 2014/09/15 16:26:35, danakj wrote: > This code is soo far away from the actual tests. Can you just make a > PictureLayerImplTest subclass down by the actual tests and move this function > there? Acknowledged. https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:319: std::vector<Tile*> tiles = On 2014/09/15 15:45:07, vmpstr wrote: > you should ASSERT_TRUE(active_layer_->tilings()); and > ASSERT_GT(active_layer_->tilings()->num_tilings(), 0u); to ensure that if the > test is to fail, it won't just crash. Acknowledged. But a failure in a test doesn't prevent the rest of the test from being executed. Would you also like me to add if statements to prevent possible crashes? https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:111: pile->SetTileGridSize(gfx::Size(1000, 1000)); On 2014/09/15 15:45:07, vmpstr wrote: > Is this and the previous line required? I'd prefer not to have them unless the > actual code also sets them to these values (just so we're testing the thing that > we're actually running). Acknowledged. https://codereview.chromium.org/519583003/diff/140001/cc/resources/picture_pi... File cc/resources/picture_pile_base.h (right): https://codereview.chromium.org/519583003/diff/140001/cc/resources/picture_pi... cc/resources/picture_pile_base.h:53: bool is_solid_color() const { return is_solid_color_; } On 2014/09/15 16:32:31, danakj wrote: > nit: whitespace above Acknowledged. https://codereview.chromium.org/519583003/diff/140001/cc/resources/picture_pi... cc/resources/picture_pile_base.h:54: SkColor get_solid_color() const { return solid_color_; } On 2014/09/15 16:32:31, danakj wrote: > style nit: accessors of this form should match the variable name (no get_). > CamelCase functions could add a Get tho. Acknowledged. https://codereview.chromium.org/519583003/diff/140001/cc/test/fake_picture_la... File cc/test/fake_picture_layer.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/test/fake_picture_la... cc/test/fake_picture_layer.cc:36: PictureLayerImpl* layer_impl = static_cast<PictureLayerImpl*>(layer); On 2014/09/15 15:45:07, vmpstr wrote: > this is not needed? Acknowledged. https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_pixeltest_readback.cc (left): https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_pixeltest_readback.cc:939: background_impl->HighResTiling()->contents_scale()); On 2014/09/15 15:45:08, vmpstr wrote: > Why this change? Because we no longer have HiResTiling(), i.e. it is nullptr, the layer is solid. https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_pixeltest_readback.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_pixeltest_readback.cc:934: LayerImpl* root_impl = host_impl->active_tree()->root_layer(); On 2014/09/15 16:26:35, danakj wrote: > Just remove all these layers, they're unused. Acknowledged.
https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:319: std::vector<Tile*> tiles = On 2014/09/15 16:59:40, Hendrik wrote: > On 2014/09/15 15:45:07, vmpstr wrote: > > you should ASSERT_TRUE(active_layer_->tilings()); and > > ASSERT_GT(active_layer_->tilings()->num_tilings(), 0u); to ensure that if the > > test is to fail, it won't just crash. > > Acknowledged. > > But a failure in a test doesn't prevent the rest of the test from being > executed. Would you also like me to add if statements to prevent possible > crashes? An EXPECT_FOO failure continues the test. An ASSERT_FOO failure exits the test.
On 2014/09/15 17:01:23, danakj wrote: > https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl_unittest.cc (right): > > https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... > cc/layers/picture_layer_impl_unittest.cc:319: std::vector<Tile*> tiles = > On 2014/09/15 16:59:40, Hendrik wrote: > > On 2014/09/15 15:45:07, vmpstr wrote: > > > you should ASSERT_TRUE(active_layer_->tilings()); and > > > ASSERT_GT(active_layer_->tilings()->num_tilings(), 0u); to ensure that if > the > > > test is to fail, it won't just crash. > > > > Acknowledged. > > > > But a failure in a test doesn't prevent the rest of the test from being > > executed. Would you also like me to add if statements to prevent possible > > crashes? > > An EXPECT_FOO failure continues the test. An ASSERT_FOO failure exits the test. Ah, I see, cool!
https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:123: layer_impl->tilings_.swap(tilings_); On 2014/09/15 16:59:40, Hendrik wrote: > On 2014/09/15 15:45:07, vmpstr wrote: > > In the case where a non-solid layer becomes solid, I think you have to remove > > all tilings from the new recycled tree. We have this notion to ensure that the > > recycle tree does not have any unshared tiles, which is important to maintain. > > > We had discussed this a couple of weeks ago, you said doing this would be > expensive since it would force us to recreate tilings that could be reused in > certain situations. Have you changed your mind on this? Yeah, in retrospect, this is important to do right away. In the iterators world we rely on the fact that we don't have to evict recycle tree tiles since they can be accessed via some other tree (active). This change without clearing the tilings would violate that.
Cleared the tiles, added DCHECKS, added tilings test PTAL
lgtm % danakj, can you also update the description to put a bit more information there?
On 2014/09/16 16:43:33, vmpstr wrote: > lgtm % danakj, can you also update the description to put a bit more information > there? Description updated, thanks!
https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:126: tilings_->RemoveAllTilings(); can you do this down by RemoveTilesInRegion (ie after SetClient happens)? https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:767: DCHECK(!pile_->is_solid_color() || !tilings_->num_tilings()); this seems like a funny place to dcheck this.. maybe DCHECK(!solid color) after the early out would make mode sense? but we don't dcheck all the things in CanHaveTilings(). so if it was me I'd remove this but I don't feel strongly about it. https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:4269: EXPECT_EQ(0, active_layer_->tilings()->num_tilings()); 0u https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:4346: EXPECT_NE(size_t(0), active_layer_->tilings()->num_tilings()); nit: s/size_t(0)/0u/ https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:4370: EXPECT_EQ(0, active_layer_->tilings()->num_tilings()); 0u https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:70: EXPECT_TRUE(layer_impl->pile()->is_solid_color()); do we care if it's solid color when it's empty? is this something we want to enforce over time? if not, remove https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:104: TEST(PictureLayerTest, SolidColorAnalysisForPictureLayer) { should this test move to picture_pile_unittest.cc? what does this test do that PicturePileTest.SolidRectangleIsSolid does not? can you trim it down to just the difference and name it after that difference? https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:165: EXPECT_FALSE(layer_impl->pile()->is_solid_color()); you could just CreateFrom() another pile and verify that it has the right value. i don't think this test needs any layers anymore. https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:185: // EXPECT_TRUE(layer_impl->pile()->is_solid_color()); leftovers https://codereview.chromium.org/519583003/diff/220001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/519583003/diff/220001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:4205: client_.set_fill_with_nonsolid_color(true); +sohan FYI you might have to do this for some tests after this lands
PTAL https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:126: tilings_->RemoveAllTilings(); On 2014/09/19 20:44:28, danakj wrote: > can you do this down by RemoveTilesInRegion (ie after SetClient happens)? The only reason we're removing the tiles is because we just swapped the tiles, moving it will separate the reason and result. But, I'm moving it anyway. https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:767: DCHECK(!pile_->is_solid_color() || !tilings_->num_tilings()); On 2014/09/19 20:44:28, danakj wrote: > this seems like a funny place to dcheck this.. > > maybe DCHECK(!solid color) after the early out would make mode sense? but we > don't dcheck all the things in CanHaveTilings(). > > so if it was me I'd remove this but I don't feel strongly about it. I added this on request, now I'm removing it on request. Next time, may I suggest thunderdome? https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:4269: EXPECT_EQ(0, active_layer_->tilings()->num_tilings()); On 2014/09/19 20:44:28, danakj wrote: > 0u Acknowledged. https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:4346: EXPECT_NE(size_t(0), active_layer_->tilings()->num_tilings()); On 2014/09/19 20:44:28, danakj wrote: > nit: s/size_t(0)/0u/ :/ Acknowledged. https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:4370: EXPECT_EQ(0, active_layer_->tilings()->num_tilings()); On 2014/09/19 20:44:28, danakj wrote: > 0u Acknowledged. https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:70: EXPECT_TRUE(layer_impl->pile()->is_solid_color()); On 2014/09/19 20:44:28, danakj wrote: > do we care if it's solid color when it's empty? is this something we want to > enforce over time? if not, remove Acknowledged. https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:104: TEST(PictureLayerTest, SolidColorAnalysisForPictureLayer) { On 2014/09/19 20:44:28, danakj wrote: > should this test move to picture_pile_unittest.cc? > > what does this test do that PicturePileTest.SolidRectangleIsSolid does not? can > you trim it down to just the difference and name it after that difference? It was testing that the solid state was being transfered to the layer, but now the the layer doesn't contain the solid stateness, this whole thing can be deleted.
LGTM https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:126: tilings_->RemoveAllTilings(); On 2014/09/19 22:27:13, Hendrik wrote: > On 2014/09/19 20:44:28, danakj wrote: > > can you do this down by RemoveTilesInRegion (ie after SetClient happens)? > > The only reason we're removing the tiles is because we just swapped the tiles, > moving it will separate the reason and result. But, I'm moving it anyway. Ya, it's the same with RemoveTilesInRegion, but the Remove code may want to use the client (this was a bug with RemoveTilesInRegion before) https://codereview.chromium.org/519583003/diff/220001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:767: DCHECK(!pile_->is_solid_color() || !tilings_->num_tilings()); On 2014/09/19 22:27:13, Hendrik wrote: > On 2014/09/19 20:44:28, danakj wrote: > > this seems like a funny place to dcheck this.. > > > > maybe DCHECK(!solid color) after the early out would make mode sense? but we > > don't dcheck all the things in CanHaveTilings(). > > > > so if it was me I'd remove this but I don't feel strongly about it. > > I added this on request, now I'm removing it on request. Next time, may I > suggest thunderdome? Darn I tried to find a review request to add it so I wouldn't cause that situation, and missed it :( https://codereview.chromium.org/519583003/diff/240001/cc/layers/picture_layer... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/519583003/diff/240001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:10: #include "cc/test/fake_content_layer_client.h" no longer used
merged. https://codereview.chromium.org/519583003/diff/240001/cc/layers/picture_layer... File cc/layers/picture_layer_unittest.cc (right): https://codereview.chromium.org/519583003/diff/240001/cc/layers/picture_layer... cc/layers/picture_layer_unittest.cc:10: #include "cc/test/fake_content_layer_client.h" On 2014/09/19 22:46:38, danakj wrote: > no longer used Acknowledged.
The CQ bit was checked by hendrikw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/519583003/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as cd508cf463b2851177d1f2a8f654f6e45fd5fea6
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c78b11fa0057ae197b03ac9de5a150482efe6faf Cr-Commit-Position: refs/heads/master@{#295860} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
