Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12642010:
Implement on demand quad rasterization for PicturePiles. (Closed)
|
Created:
7 years, 9 months ago by Leandro Graciá Gil Modified:
7 years, 9 months ago Reviewers:
aelias_OOO_until_Jul13, nduca, epenner, vmpstr, enne (OOO), danakj, palmer, reveman, piman CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, jam, jdduke (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement on demand quad rasterization for PicturePiles.
BUG=173011
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190969
Patch Set 1 #
Total comments: 24
Patch Set 2 : review fixes, adding unit tests. #
Total comments: 1
Patch Set 3 : build fix. #
Total comments: 67
Patch Set 4 : review fixes. #Patch Set 5 : address vmpstr's comments. #Patch Set 6 : rebase & introduce ResizeResource. #
Total comments: 29
Patch Set 7 : nit fixes & adding ResizeResource unit test. #
Total comments: 11
Patch Set 8 : rebase, PictureDrawQuad unit test and avoid ResizeResource. #
Total comments: 1
Patch Set 9 : adding DrawQuadIteratorTest for PictureDrawQuads. #Patch Set 10 : rebase #
Total comments: 8
Patch Set 11 : nit fixes. #
Total comments: 2
Patch Set 12 : using SetPixels. #Patch Set 13 : nit fix. #
Total comments: 2
Patch Set 14 : move had_incomplete_tile out of the picture pile mode. #Patch Set 15 : rebase. #
Messages
Total messages: 78 (0 generated)
This is a first draft of the feature. vmpstr, nduca: could you please take a high level look and check if this approach goes in the right direction? Any suggestions on how this could be tested are most welcome. palmer: minor changes in the content/common/cc_messages.cc Thanks! https://codereview.chromium.org/12642010/diff/1/content/common/cc_messages.cc File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/1/content/common/cc_messages.cc... content/common/cc_messages.cc:340: NOTREACHED(); Cannot write or read PictureDrawQuads because they hold a reference-counted PicturePileImpl object.
I think is is the right way to do it (at least that's what I would try and do :) ) https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc#newcode1937 cc/gl_renderer.cc:1937: on_demand_tile_raster_texture_ = Context()->createTexture(); Should this be cleaned up in CleanupSharedObjects? https://codereview.chromium.org/12642010/diff/1/cc/picture_layer_impl.cc File cc/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/picture_layer_impl.cc#newc... cc/picture_layer_impl.cc:188: case ManagedTileState::DrawingInfo::PICTURE_PILE_MODE: { I not sure if it's worth combining the two cases, since you only save a few lines, and then have to do another if/else on this variable again. https://codereview.chromium.org/12642010/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/tile_manager.cc#newcode718 cc/tile_manager.cc:718: else if (!managed_tile_state.can_use_gpu_memory) I think the only way to get to this code path is if can_use_gpu_memory is true (hence this else if will never be true). I think the more appropriate spot for this check would be in AssignGpuMemory. One of the conditions there says something along the lines of "if we want to raster, but we're out of memory"
lgtm https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.cc File cc/picture_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.cc#newco... cc/picture_draw_quad.cc:68: } // namespacec cc Typo :)
lets get aelias or danakj to look at this.
Any suggestions about how this could be tested?
On 2013/03/14 02:09:01, Leandro Graciá Gil wrote: > Any suggestions about how this could be tested? (1) Add a pixel test with these quad types to verify that are rastered correctly. (2) Wait until danakj's https://codereview.chromium.org/12518026/ lands and also add a layer-based test to make sure that the layer generates these quads appropriately.
This looks pretty nice to me. Aside from vmpstr's comments, I think we just need a test and it should be good to go.
https://codereview.chromium.org/12642010/diff/1/cc/draw_quad.h File cc/draw_quad.h (right): https://codereview.chromium.org/12642010/diff/1/cc/draw_quad.h#newcode32 cc/draw_quad.h:32: PICTURE_CONTENT, alphabetical please https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc#newcode325 cc/gl_renderer.cc:325: case DrawQuad::PICTURE_CONTENT: alphabetical please https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc#newcode994 cc/gl_renderer.cc:994: ResourceProvider::ResourceId resource_id = quad->IsPictureQuad() ? How would you ever get here with IsPictureQuad() true? If it's true, the material is PICTURE_CONTENT, and you'd be in DrawPictureQuad(). https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.h File cc/picture_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.h#newcode20 cc/picture_draw_quad.h:20: class CC_EXPORT PictureDrawQuad : public TileDrawQuad { These are meant to be simple struct-like things for purpose of IPC, and multiple levels of polymorphism makes that more difficult. For instance now PictureDrawQuad has a resource_id that is not used. I think I'd like this more if PictureDrawQuad didn't need to inherit from TileDrawQuad. I know you use DrawTileQuad() in GLRenderer, but you could solve that by making a templated method in GLRenderer that both DrawTileQuad and DrawPictureQuad called, and it would use the various identical fields off them, and take as input parameters anything that differed between the two. What would you think of that approach? https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.h#newcode32 cc/picture_draw_quad.h:32: scoped_refptr<PicturePileImpl> picture_pile); What is the story for serializing these to the browser compositor for ubercomp? Are we just going to stop using this in M28? Are we going to make PicturePileImpl serializable securely? What's the plan going forward? https://codereview.chromium.org/12642010/diff/1/cc/tile_draw_quad.cc File cc/tile_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/tile_draw_quad.cc#newcode55 cc/tile_draw_quad.cc:55: bool TileDrawQuad::IsPictureQuad() const { this is redundant with the quad's |material| field.
https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc#newcode994 cc/gl_renderer.cc:994: ResourceProvider::ResourceId resource_id = quad->IsPictureQuad() ? On 2013/03/14 18:00:56, danakj wrote: > How would you ever get here with IsPictureQuad() true? If it's true, the > material is PICTURE_CONTENT, and you'd be in DrawPictureQuad(). Sorry, ignore this, I answered this myself later.
On 2013/03/14 18:00:56, danakj wrote: > What is the story for serializing these to the browser compositor for ubercomp? > > Are we just going to stop using this in M28? Are we going to make > PicturePileImpl serializable securely? What's the plan going forward? I'm afraid I don't know the details. Any suggestions/plans from the other reviewers?
On 2013/03/14 18:10:52, Leandro Graciá Gil wrote: > On 2013/03/14 18:00:56, danakj wrote: > > What is the story for serializing these to the browser compositor for > ubercomp? > > > > Are we just going to stop using this in M28? Are we going to make > > PicturePileImpl serializable securely? What's the plan going forward? > > I'm afraid I don't know the details. Any suggestions/plans from the other > reviewers? These on-demand draw quads are trying to meet this sets of requirements: (1) Meet the needs of the Android WebView capture picture API, by creating SkPicture quads and using the existing software renderer to do all the transform and clipping heavy lifting. (2) When we OOM and want to put more content on the screen (correctness at the cost of performance) by uploading immediately before rendering. (3) This is a minor consideration, but this could be the way that we handle drawing with ganesh directly to the backbuffer. We could alternately consider drawing into intermediate textures instead which may be the only reasonable way to handle this with ubercomp. #1 is not a concern for ubercomp. Unfortunately, given the constraints of needing that API, I feel like this is the best way forward to address that need. Everyone is grumpy about Android WebView requirements. Join the club; your membership details are in the mail. #2 and #3 as Dana rightly points out are very rightly a problem for ubercomp since I suspect it's undesirable to serialize an SkPicture across processes. In the short term, maybe the PictureDrawQuad could be inherently unserializable and you just get a missing or checkerboarded layer if you attempt to serialize it. In the longer term, I'm less sure what we can do, but maybe other folks have better brilliant ideas. :)
https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc#newcode994 cc/gl_renderer.cc:994: ResourceProvider::ResourceId resource_id = quad->IsPictureQuad() ? I'd prefer if you just passed drawTileQuad the resource you wanted to draw with rather than interrogating the quad. I agree with Dana that this design of inheriting from TileDrawQuad is a little bit fishy. I don't know if it'd be better to either have both TileDrawQuad and PictureDrawQuad inherit from a base class that has everything on it but the resource id, or if it'd be better to have TileDrawQuad and PictureDrawQuad not related at all and just have a DrawTileQuad() helper function that took a dozen parameters so you didn't have to make the types related just to share code. In any case, I think the current approach has some design smell to it.
On 2013/03/15 03:29:21, enne wrote: > (2) When we OOM and want to put more content on the screen (correctness at the > cost of performance) by uploading immediately before rendering. Fundamentally this one-tile-only OOM approach is incompatible with ubercomp since ubercomp gets a bundle of draw quads and tiles. To support last-minute synchronous raster in ubercomp world, we would need to take a step back and synchronously raster into a bunch of resources, evicting out-of-viewport resources to clear up memory (if tiles in the current viewport fill up our memory limit, we can't do anything in ubercomp world). But instead of building such a system, I think we should focus on avoiding OOM situations in the first place.
https://codereview.chromium.org/12642010/diff/1/content/common/cc_messages.cc File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/1/content/common/cc_messages.cc... content/common/cc_messages.cc:457: NOTREACHED(); drive-by: Not NOTREACHED since a compromized renderer could still send that, and possibly try to use it for a privilege escalation. Return false here instead to refuse the message (rather than continue on inconsistent data).
https://codereview.chromium.org/12642010/diff/1/cc/draw_quad.h File cc/draw_quad.h (right): https://codereview.chromium.org/12642010/diff/1/cc/draw_quad.h#newcode32 cc/draw_quad.h:32: PICTURE_CONTENT, On 2013/03/14 18:00:56, danakj wrote: > alphabetical please Done. https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc#newcode325 cc/gl_renderer.cc:325: case DrawQuad::PICTURE_CONTENT: On 2013/03/14 18:00:56, danakj wrote: > alphabetical please Done. https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc#newcode994 cc/gl_renderer.cc:994: ResourceProvider::ResourceId resource_id = quad->IsPictureQuad() ? On 2013/03/15 03:29:28, enne wrote: > I'd prefer if you just passed drawTileQuad the resource you wanted to draw with > rather than interrogating the quad. > > I agree with Dana that this design of inheriting from TileDrawQuad is a little > bit fishy. I don't know if it'd be better to either have both TileDrawQuad and > PictureDrawQuad inherit from a base class that has everything on it but the > resource id, or if it'd be better to have TileDrawQuad and PictureDrawQuad not > related at all and just have a DrawTileQuad() helper function that took a dozen > parameters so you didn't have to make the types related just to share code. In > any case, I think the current approach has some design smell to it. I agree. I'm going with the base class. Seems a cleaner solution than adding a dozen of params or using templates. https://codereview.chromium.org/12642010/diff/1/cc/gl_renderer.cc#newcode1937 cc/gl_renderer.cc:1937: on_demand_tile_raster_texture_ = Context()->createTexture(); On 2013/03/13 21:03:08, vmpstr wrote: > Should this be cleaned up in CleanupSharedObjects? Yes. This was accidentally missed while rebasing over https://codereview.chromium.org/12353003/. Fixed. https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.cc File cc/picture_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.cc#newco... cc/picture_draw_quad.cc:68: } // namespacec cc On 2013/03/14 00:09:21, Chris P. wrote: > Typo :) Done. https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.h File cc/picture_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/1/cc/picture_draw_quad.h#newcode20 cc/picture_draw_quad.h:20: class CC_EXPORT PictureDrawQuad : public TileDrawQuad { On 2013/03/14 18:00:56, danakj wrote: > These are meant to be simple struct-like things for purpose of IPC, and multiple > levels of polymorphism makes that more difficult. For instance now > PictureDrawQuad has a resource_id that is not used. > > I think I'd like this more if PictureDrawQuad didn't need to inherit from > TileDrawQuad. I know you use DrawTileQuad() in GLRenderer, but you could solve > that by making a templated method in GLRenderer that both DrawTileQuad and > DrawPictureQuad called, and it would use the various identical fields off them, > and take as input parameters anything that differed between the two. What would > you think of that approach? I'm trying enne's suggestion of a base class while keeping the DrawQuads as struct-like as possible. If you still think this is not good enough I can try with your template suggestion. https://codereview.chromium.org/12642010/diff/1/cc/picture_layer_impl.cc File cc/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/picture_layer_impl.cc#newc... cc/picture_layer_impl.cc:188: case ManagedTileState::DrawingInfo::PICTURE_PILE_MODE: { On 2013/03/13 21:03:08, vmpstr wrote: > I not sure if it's worth combining the two cases, since you only save a few > lines, and then have to do another if/else on this variable again. The point is that if we add more code here for TEXTURE_MODE it won't diverge from the code for PICTURE_PILE_MODE. But I have no strong opinion on this. Let me know what you prefer and I'll make any required updates. https://codereview.chromium.org/12642010/diff/1/cc/tile_draw_quad.cc File cc/tile_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/tile_draw_quad.cc#newcode55 cc/tile_draw_quad.cc:55: bool TileDrawQuad::IsPictureQuad() const { On 2013/03/14 18:00:56, danakj wrote: > this is redundant with the quad's |material| field. Removed. https://codereview.chromium.org/12642010/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/1/cc/tile_manager.cc#newcode718 cc/tile_manager.cc:718: else if (!managed_tile_state.can_use_gpu_memory) On 2013/03/13 21:03:08, vmpstr wrote: > I think the only way to get to this code path is if can_use_gpu_memory is true > (hence this else if will never be true). I think the more appropriate spot for > this check would be in AssignGpuMemory. One of the conditions there says > something along the lines of "if we want to raster, but we're out of memory" Done. https://codereview.chromium.org/12642010/diff/1/content/common/cc_messages.cc File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/1/content/common/cc_messages.cc... content/common/cc_messages.cc:457: NOTREACHED(); On 2013/03/15 03:51:57, piman wrote: > drive-by: Not NOTREACHED since a compromized renderer could still send that, and > possibly try to use it for a privilege escalation. > Return false here instead to refuse the message (rather than continue on > inconsistent data). Done.
https://codereview.chromium.org/12642010/diff/32001/cc/test/layer_tree_test_c... File cc/test/layer_tree_test_common.cc (right): https://codereview.chromium.org/12642010/diff/32001/cc/test/layer_tree_test_c... cc/test/layer_tree_test_common.cc:323: if (proxy()) Found flakiness here when creating the new layer-based pixel test. This should fix it.
Ping.
Could somebody take a look a for a moment today so I can make any required changes tomorrow in the GMT timezone? Thanks.
this looks mostly okay. I would like to review again, but also please get vmpstr to review, as well as danakj. You can ping them via email or chat since they haven't yet responded. vmpstr, there are questions for you i would like answered. https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:198: scoped_ptr<TileDrawQuad> quad = TileDrawQuad::Create(); seems like the constrcturo and append call can be outside this branch, no? https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager... cc/resources/tile_manager.cc:599: if (!tile->drawing_info().requires_resource()) @vmpstr: why do we sometimes say tile->drawing info and sometimes mts->drawing info? makes no sense to my addled brain https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager... cc/resources/tile_manager.cc:616: FreeResourcesForTile(tile); so um, where is the part of this loop that says "please use texgture mode?" I dont see it... what happens if a tile gets rasterize on demand one fram, but gets to line 620 on the next frame? https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... File cc/test/layer_tree_test_common.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... cc/test/layer_tree_test_common.cc:322: // Racy timeouts and explicit endTest calls might have cleaned up the tree host. Erm,.... this is a separate patch. You should put that up separately. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:2644: // An auxiliary texture is created for on-demand rasterization. This smells. Can we create the texture only when it is first needed? Or do we think we should create it up-front?
This looks much better to me. Enne should take a look at it for impl-painting-ness stuff. You'll need to rebase which will affect your tests mostly as the base class for LayerTreePixelTest has been chromified. https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1044: void GLRenderer::DrawTileQuad(const DrawingFrame& frame, nit: Can you call this function DrawTileOrPictureQuad(), and add a new DrawTileQuad() that calls it, instead of adding logic to the DrawQuad() switch? https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2010: on_demand_tile_raster_texture_ = Context()->createTexture(); Can you do this in DrawPictureQuad() the first time it is called instead? https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2278: GLC(context_, context_->deleteTexture(on_demand_tile_raster_texture_)); You need to clean up with the ResourceProvider too https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.h File cc/output/gl_renderer.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.h#n... cc/output/gl_renderer.h:143: unsigned resource_id); ResourceProvider::ResourceId is the type https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... File cc/quads/picture_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.cc:20: const gfx::Rect& rect, pass gfx::Rect and gfx::Size by value like the other quads. https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.cc:57: } // TODO(danakj): Convert to TextureDrawQuad? NOTIMPLEMENTED(); https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad.h File cc/quads/tile_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad.h... cc/quads/tile_draw_quad.h:17: const gfx::Rect& rect, please dont change these https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... File cc/quads/tile_draw_quad_base.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... cc/quads/tile_draw_quad_base.h:17: class CC_EXPORT TileDrawQuadBase : public DrawQuad { Since this is the base for more than the TileDrawQuad, I feel like a better name could be had, but I'm not sure what "class" Tile and Picture are a part of, so the best I could come up with was TileOrPictureDrawQuadBase. https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... cc/quads/tile_draw_quad_base.h:21: const gfx::Rect& rect, dont change these types from TileDrawQuad https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... File cc/test/layer_tree_test_common.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... cc/test/layer_tree_test_common.cc:321: } else { else if (proxy()) https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:26: m_settings.implSidePainting = true; This will have to go in an override of InitializeSettings() after rebase https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:61: BlueYellowLayerClient(const gfx::Rect& rect) : rect_(rect) { } by value https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:96: gfx::Rect rect(200, 200); can you give this a better name than "rect"? https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:1050: // Number of textures should be one for each layer plus an auxiliary What if you only create the on-demand texture the first time you need it? Then tests that don't use it don't need to know about it. https://codereview.chromium.org/12642010/diff/35003/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/35003/content/common/cc_message... content/common/cc_messages.cc:324: NOTREACHED(); Instead of assert can you just not send the quad? Maybe NOTIMPLEMENTED()?
On Wed, Mar 20, 2013 at 10:34 PM, <danakj@chromium.org> wrote: > This looks much better to me. Enne should take a look at it for > impl-painting-ness stuff. > oh sorry, Nat I mean! And he already started.
A few replies to the comments. I'll address the rest of them and update the code tomorrow. Thanks for taking a look! https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:198: scoped_ptr<TileDrawQuad> quad = TileDrawQuad::Create(); On 2013/03/21 02:22:48, nduca wrote: > seems like the constrcturo and append call can be outside this branch, no? The type of the scoped_ptr changes and therefore quad->SetNew would refer to different methods. I would move the Append call outside, but that would require moving quad outside too and we'd have the type problem again. The alternative would be to define quad as scoped_ptr<TileDrawQuadBase> and add ugly static_casts on quad when calling SetNew inside the if block. That way both Create and Append would be outside the block. If that sounds acceptable let me know and I'll do it. https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2278: GLC(context_, context_->deleteTexture(on_demand_tile_raster_texture_)); On 2013/03/21 02:34:42, danakj wrote: > You need to clean up with the ResourceProvider too Can I use ResourceProvider to clean external textures? Should this be an external texture? https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... File cc/quads/picture_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.cc:20: const gfx::Rect& rect, On 2013/03/21 02:34:42, danakj wrote: > pass gfx::Rect and gfx::Size by value like the other quads. These are ultimately assigned to gfx::Rect and gfx::Size values in DrawQuad. I might be wrong, but it looks to me like passing them by value has no other effect than wasting stack on calls. The value is going to be copied anyway, but only once instead of once per call. https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.cc:57: } On 2013/03/21 02:34:42, danakj wrote: > // TODO(danakj): Convert to TextureDrawQuad? > NOTIMPLEMENTED(); I'll assume you want me to add that to IterateResources in the next iteration. Correct me if I'm wrong. https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad.h File cc/quads/tile_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad.h... cc/quads/tile_draw_quad.h:17: const gfx::Rect& rect, On 2013/03/21 02:34:42, danakj wrote: > please dont change these Ok, but are you sure about this? I don't think removing the const refs in this case does anything else than additional unrequired copies during calls. (See my comment in picture_draw_quad.cc) https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... File cc/quads/tile_draw_quad_base.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... cc/quads/tile_draw_quad_base.h:17: class CC_EXPORT TileDrawQuadBase : public DrawQuad { On 2013/03/21 02:34:42, danakj wrote: > Since this is the base for more than the TileDrawQuad, I feel like a better name > could be had, but I'm not sure what "class" Tile and Picture are a part of, so > the best I could come up with was TileOrPictureDrawQuadBase. Well, this is actually a base class for tile draw quads. The difference later is that one subclass is backed by a resource id and the other by a PicturePileImpl and the information required to rasterize it. https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager... cc/resources/tile_manager.cc:616: FreeResourcesForTile(tile); On 2013/03/21 02:22:48, nduca wrote: > so um, where is the part of this loop that says "please use texgture mode?" I > dont see it... what happens if a tile gets rasterize on demand one fram, but > gets to line 620 on the next frame? I don't see any specific consequences of that. If it gets to line 620 it will work as it currently does. What do you exactly mean? https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... File cc/test/layer_tree_test_common.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... cc/test/layer_tree_test_common.cc:322: // Racy timeouts and explicit endTest calls might have cleaned up the tree host. On 2013/03/21 02:22:48, nduca wrote: > Erm,.... this is a separate patch. You should put that up separately. Ok, but the problem was exposed by the newly introduced pixel test. If you're fine with uploading the fix alone I can land it in a separate patch. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:2644: // An auxiliary texture is created for on-demand rasterization. On 2013/03/21 02:22:48, nduca wrote: > This smells. Can we create the texture only when it is first needed? Or do we > think we should create it up-front? I agree. I think we should be able to create it when first needed, and destroy it if initialized (texture id != 0). https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:61: BlueYellowLayerClient(const gfx::Rect& rect) : rect_(rect) { } On 2013/03/21 02:34:42, danakj wrote: > by value Why? As we assign the const ref argument to a gfx::Rect object and not to a const gfx::Rect& reference we are still copying the value. If we remove the const ref from the argument we will unnecessarily create a temporary object and trigger the copy constructor and destructor of gfx::Rect in the process. Plus, it will take more stack space to perform the call (the full rect object vs internally a pointer). Please correct me if I'm wrong. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:96: gfx::Rect rect(200, 200); On 2013/03/21 02:34:42, danakj wrote: > can you give this a better name than "rect"? Perhaps bounds? https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:1050: // Number of textures should be one for each layer plus an auxiliary On 2013/03/21 02:34:42, danakj wrote: > What if you only create the on-demand texture the first time you need it? Then > tests that don't use it don't need to know about it. I think I'll do that. Sounds much simpler.
https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2278: GLC(context_, context_->deleteTexture(on_demand_tile_raster_texture_)); On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:34:42, danakj wrote: > > You need to clean up with the ResourceProvider too > > Can I use ResourceProvider to clean external textures? Should this be an > external texture? ya, there's actually no need for this to be an external texture. You could just use CreateGLTexture() and then DeleteResource() here. And you'll need to lock the resource appropriately to get at the texture id when you want to use it. See other uses of ResourceProvider::ScopedWriteLockGL in this file. https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... File cc/quads/picture_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.cc:20: const gfx::Rect& rect, On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:34:42, danakj wrote: > > pass gfx::Rect and gfx::Size by value like the other quads. > > These are ultimately assigned to gfx::Rect and gfx::Size values in DrawQuad. I > might be wrong, but it looks to me like passing them by value has no other > effect than wasting stack on calls. The value is going to be copied anyway, but > only once instead of once per call. We currently follow a style throught cc/ to pass gfx:: types of <= 4 ints or <= 2 floats by value, as it generates less code. The compiler can take care of the temporary values that are just passed through. https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.cc:57: } On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:34:42, danakj wrote: > > // TODO(danakj): Convert to TextureDrawQuad? > > NOTIMPLEMENTED(); > > I'll assume you want me to add that to IterateResources in the next iteration. > Correct me if I'm wrong. Yes please. https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad.h File cc/quads/tile_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad.h... cc/quads/tile_draw_quad.h:17: const gfx::Rect& rect, On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:34:42, danakj wrote: > > please dont change these > > Ok, but are you sure about this? I don't think removing the const refs in this > case does anything else than additional unrequired copies during calls. (See my > comment in picture_draw_quad.cc) Yes, see other comment. https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... File cc/quads/tile_draw_quad_base.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... cc/quads/tile_draw_quad_base.h:17: class CC_EXPORT TileDrawQuadBase : public DrawQuad { On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:34:42, danakj wrote: > > Since this is the base for more than the TileDrawQuad, I feel like a better > name > > could be had, but I'm not sure what "class" Tile and Picture are a part of, so > > the best I could come up with was TileOrPictureDrawQuadBase. > > Well, this is actually a base class for tile draw quads. The difference later is > that one subclass is backed by a resource id and the other by a PicturePileImpl > and the information required to rasterize it. Right, but the latter isn't called a TileDrawQuad or anything like it, it's called a PictureDrawQuad. I'd like the name of this class to somehow reflect that it's more than a base class for the "TileDrawQuad" class. Maybe the other quad should be TileOnDemandDrawQuad or something, that would be more clear also.. but I don't mind PictureDrawQuad at all for it. https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... File cc/test/layer_tree_test_common.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... cc/test/layer_tree_test_common.cc:322: // Racy timeouts and explicit endTest calls might have cleaned up the tree host. On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:22:48, nduca wrote: > > Erm,.... this is a separate patch. You should put that up separately. > > Ok, but the problem was exposed by the newly introduced pixel test. If you're > fine with uploading the fix alone I can land it in a separate patch. I was wondering about this.. does this mean the pixel test is finishing at the time limit of the timeout? Seems like its going to time out on the bots a lot if that's the case. Also seems incredibly slow.. I agree it could be a separate CL (and if that's true, then maybe we need a longer time out on the pixel tests :( ?) https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:61: BlueYellowLayerClient(const gfx::Rect& rect) : rect_(rect) { } On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:34:42, danakj wrote: > > by value > > Why? As we assign the const ref argument to a gfx::Rect object and not to a > const gfx::Rect& reference we are still copying the value. If we remove the > const ref from the argument we will unnecessarily create a temporary object and > trigger the copy constructor and destructor of gfx::Rect in the process. Plus, > it will take more stack space to perform the call (the full rect object vs > internally a pointer). Please correct me if I'm wrong. Same comment here re gfx:: types. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:96: gfx::Rect rect(200, 200); On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:34:42, danakj wrote: > > can you give this a better name than "rect"? > > Perhaps bounds? I don't think that does more than describe the storage type either. How about layer_rect?
https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... File cc/quads/picture_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.cc:20: const gfx::Rect& rect, On 2013/03/21 04:33:39, danakj wrote: > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > On 2013/03/21 02:34:42, danakj wrote: > > > pass gfx::Rect and gfx::Size by value like the other quads. > > > > These are ultimately assigned to gfx::Rect and gfx::Size values in DrawQuad. I > > might be wrong, but it looks to me like passing them by value has no other > > effect than wasting stack on calls. The value is going to be copied anyway, > but > > only once instead of once per call. > > We currently follow a style throught cc/ to pass gfx:: types of <= 4 ints or <= > 2 floats by value, as it generates less code. The compiler can take care of the > temporary values that are just passed through. That's useful under the assumption of working in 64-bit architectures where a reference might be larger than the value itself. However, this is not true for some Chrome platforms like Android (32-bit). What you really want instead is this: template <bool Condition, typename A, typename B> struct TypeIf; template <typename A, typename B> struct TypeIf<true, A, B> { typedef A Type; }; template <typename A, typename B> struct TypeIf<false, A, B> { typedef B Type; }; template <typename T> struct ByValue { typedef typename TypeIf<sizeof(T) <= sizeof(const T&), T, const T&>::Type Type; }; typedef typename ByValue<gfx::Rect>::Type RectByValue; void foo(RectByValue rect) { ... } This might look very weird, but what is doing is to automatically copy the value or pass a const reference depending on what is actually smaller on build time. This can be used to define a simple type (RectByValue) that will do what you want correctly for all architectures. While I think this might be worth considering it's obviously out of the scope of this patch. If you think we should try this let me know and I'll prepare a separate CL for this. For this patch I'll simply use rects by value for now. Additionally you might want to add other param traits checks to the ByValue template like verifying the type is POD, it has trivial default and copy constructors, a trivial destructor, etc. Both C++ TR1 and C++1x provide this information, although we'd need to check all platforms have it (for example, Android and its STL port).
https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:197: ManagedTileState::DrawingInfo::TEXTURE_MODE) { This still looks weird to me switch (foo) { case A: case B: ... if (foo == A) { ... } else { // foo == B ... } break; } I would vote for having separate case A and case B, even if it results in some code duplication. @nduca @danakj, wdyt? https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager... cc/resources/tile_manager.cc:599: if (!tile->drawing_info().requires_resource()) On 2013/03/21 02:22:48, nduca wrote: > @vmpstr: why do we sometimes say tile->drawing info and sometimes mts->drawing > info? makes no sense to my addled brain Yeah that should be fixed. I prefer using tile->drawing_info(), but sometimes using mts is just a lot less typing... I opened crbug.com/222761 to fix this https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager... cc/resources/tile_manager.cc:616: FreeResourcesForTile(tile); > I don't see any specific consequences of that. If it gets to line 620 it will > work as it currently does. What do you exactly mean? If tile->drawing_info().mode() is set to PICTURE_PILE_MODE, then it would not be unset anywhere: this means that we will always rasterize on demand. I think after this if-block we want to set the mode to TEXTURE_MODE
https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:197: ManagedTileState::DrawingInfo::TEXTURE_MODE) { On 2013/03/21 16:34:11, vmpstr wrote: > This still looks weird to me > > switch (foo) { > case A: > case B: > ... > if (foo == A) { > ... > } else { // foo == B > ... > } > break; > } > > I would vote for having separate case A and case B, even if it results in some > code duplication. @nduca @danakj, wdyt? More than code duplication I'd be concerned about diverging copies. Any changes in one code should be always copied to the other or we might eventually have subtle bugs. If maintaining this duplication is something we find acceptable then I'll split this. https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1044: void GLRenderer::DrawTileQuad(const DrawingFrame& frame, On 2013/03/21 02:34:42, danakj wrote: > nit: Can you call this function DrawTileOrPictureQuad(), and add a new > DrawTileQuad() that calls it, instead of adding logic to the DrawQuad() switch? Done. https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2010: on_demand_tile_raster_texture_ = Context()->createTexture(); On 2013/03/21 02:34:42, danakj wrote: > Can you do this in DrawPictureQuad() the first time it is called instead? Done. https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2278: GLC(context_, context_->deleteTexture(on_demand_tile_raster_texture_)); On 2013/03/21 04:33:39, danakj wrote: > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > On 2013/03/21 02:34:42, danakj wrote: > > > You need to clean up with the ResourceProvider too > > > > Can I use ResourceProvider to clean external textures? Should this be an > > external texture? > > ya, there's actually no need for this to be an external texture. You could just > use CreateGLTexture() and then DeleteResource() here. And you'll need to lock > the resource appropriately to get at the texture id when you want to use it. See > other uses of ResourceProvider::ScopedWriteLockGL in this file. Looking at ResourceProvider it seems that doing it that way associates a size to the texture. We would need to destroy the resource and create a new one if the size changes, and apparently the current texture size isn't exposed either. Also, I don't think for this case we want the resource provider to manage allocation or to put it in a texture pool. Perhaps an external texture is more convenient after all. I'm adding the code to delete the resource in the provider. https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.h File cc/output/gl_renderer.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.h#n... cc/output/gl_renderer.h:143: unsigned resource_id); On 2013/03/21 02:34:42, danakj wrote: > ResourceProvider::ResourceId is the type Done. https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... File cc/quads/picture_draw_quad.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.cc:57: } On 2013/03/21 04:33:39, danakj wrote: > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > On 2013/03/21 02:34:42, danakj wrote: > > > // TODO(danakj): Convert to TextureDrawQuad? > > > NOTIMPLEMENTED(); > > > > I'll assume you want me to add that to IterateResources in the next iteration. > > Correct me if I'm wrong. > > Yes please. Done. https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad.h File cc/quads/tile_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad.h... cc/quads/tile_draw_quad.h:17: const gfx::Rect& rect, On 2013/03/21 04:33:39, danakj wrote: > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > On 2013/03/21 02:34:42, danakj wrote: > > > please dont change these > > > > Ok, but are you sure about this? I don't think removing the const refs in this > > case does anything else than additional unrequired copies during calls. (See > my > > comment in picture_draw_quad.cc) > > Yes, see other comment. Done. https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... File cc/quads/tile_draw_quad_base.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... cc/quads/tile_draw_quad_base.h:17: class CC_EXPORT TileDrawQuadBase : public DrawQuad { On 2013/03/21 04:33:39, danakj wrote: > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > On 2013/03/21 02:34:42, danakj wrote: > > > Since this is the base for more than the TileDrawQuad, I feel like a better > > name > > > could be had, but I'm not sure what "class" Tile and Picture are a part of, > so > > > the best I could come up with was TileOrPictureDrawQuadBase. > > > > Well, this is actually a base class for tile draw quads. The difference later > is > > that one subclass is backed by a resource id and the other by a > PicturePileImpl > > and the information required to rasterize it. > > Right, but the latter isn't called a TileDrawQuad or anything like it, it's > called a PictureDrawQuad. I'd like the name of this class to somehow reflect > that it's more than a base class for the "TileDrawQuad" class. > > Maybe the other quad should be TileOnDemandDrawQuad or something, that would be > more clear also.. but I don't mind PictureDrawQuad at all for it. I've considered renaming all references to picture to something like "on-demand", but that ends hiding that a PicturePileImpl is being rasterized. I suggest we could name that TilePictureDrawQuad to make things clearer. The base class could then become something like TiledContentDrawQuad, and then, PICTURE_CONTENT should probably be named TILED_PICTURE_CONTENT or something similar. Let me know what you think. https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... cc/quads/tile_draw_quad_base.h:21: const gfx::Rect& rect, On 2013/03/21 02:34:42, danakj wrote: > dont change these types from TileDrawQuad Done. https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/resources/tile_manager... cc/resources/tile_manager.cc:616: FreeResourcesForTile(tile); On 2013/03/21 16:34:11, vmpstr wrote: > > I don't see any specific consequences of that. If it gets to line 620 it will > > work as it currently does. What do you exactly mean? > > If tile->drawing_info().mode() is set to PICTURE_PILE_MODE, then it would not be > unset anywhere: this means that we will always rasterize on demand. I think > after this if-block we want to set the mode to TEXTURE_MODE I see your point. Fixed. https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... File cc/test/layer_tree_test_common.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... cc/test/layer_tree_test_common.cc:321: } else { On 2013/03/21 02:34:42, danakj wrote: > else if (proxy()) Done in a separate CL. https://codereview.chromium.org/12642010/diff/35003/cc/test/layer_tree_test_c... cc/test/layer_tree_test_common.cc:322: // Racy timeouts and explicit endTest calls might have cleaned up the tree host. On 2013/03/21 04:33:39, danakj wrote: > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > On 2013/03/21 02:22:48, nduca wrote: > > > Erm,.... this is a separate patch. You should put that up separately. > > > > Ok, but the problem was exposed by the newly introduced pixel test. If you're > > fine with uploading the fix alone I can land it in a separate patch. > > I was wondering about this.. does this mean the pixel test is finishing at the > time limit of the timeout? Seems like its going to time out on the bots a lot if > that's the case. Also seems incredibly slow.. I agree it could be a separate CL > (and if that's true, then maybe we need a longer time out on the pixel tests :( > ?) The new pixel test had flaky crashes, apparently because of a race condition between timeout calling endTest and the test finishing and calling this same method. It was easy to reproduce by running the test about 10 times. However, in case it helps, it was very hard to reproduce when attached to gdb. I had to set it to repeat 10000 times and go for lunch in order to get a flake in that case. I'll be moving this into a separate CL as suggested. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl_unittest.cc:2644: // An auxiliary texture is created for on-demand rasterization. On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:22:48, nduca wrote: > > This smells. Can we create the texture only when it is first needed? Or do we > > think we should create it up-front? > > I agree. I think we should be able to create it when first needed, and destroy > it if initialized (texture id != 0). Done. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:26: m_settings.implSidePainting = true; On 2013/03/21 02:34:42, danakj wrote: > This will have to go in an override of InitializeSettings() after rebase Done. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:61: BlueYellowLayerClient(const gfx::Rect& rect) : rect_(rect) { } On 2013/03/21 04:33:39, danakj wrote: > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > On 2013/03/21 02:34:42, danakj wrote: > > > by value > > > > Why? As we assign the const ref argument to a gfx::Rect object and not to a > > const gfx::Rect& reference we are still copying the value. If we remove the > > const ref from the argument we will unnecessarily create a temporary object > and > > trigger the copy constructor and destructor of gfx::Rect in the process. Plus, > > it will take more stack space to perform the call (the full rect object vs > > internally a pointer). Please correct me if I'm wrong. > > Same comment here re gfx:: types. Done. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:96: gfx::Rect rect(200, 200); On 2013/03/21 04:33:39, danakj wrote: > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > On 2013/03/21 02:34:42, danakj wrote: > > > can you give this a better name than "rect"? > > > > Perhaps bounds? > > I don't think that does more than describe the storage type either. How about > layer_rect? Done. https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:1050: // Number of textures should be one for each layer plus an auxiliary On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > On 2013/03/21 02:34:42, danakj wrote: > > What if you only create the on-demand texture the first time you need it? Then > > tests that don't use it don't need to know about it. > > I think I'll do that. Sounds much simpler. Done. https://codereview.chromium.org/12642010/diff/35003/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/35003/content/common/cc_message... content/common/cc_messages.cc:324: NOTREACHED(); On 2013/03/21 02:34:42, danakj wrote: > Instead of assert can you just not send the quad? > > Maybe NOTIMPLEMENTED()? Done.
On 2013/03/21 12:27:25, Leandro Graciá Gil wrote: > https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... > File cc/quads/picture_draw_quad.cc (right): > > https://codereview.chromium.org/12642010/diff/35003/cc/quads/picture_draw_qua... > cc/quads/picture_draw_quad.cc:20: const gfx::Rect& rect, > On 2013/03/21 04:33:39, danakj wrote: > > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > > On 2013/03/21 02:34:42, danakj wrote: > > > > pass gfx::Rect and gfx::Size by value like the other quads. > > > > > > These are ultimately assigned to gfx::Rect and gfx::Size values in DrawQuad. > I > > > might be wrong, but it looks to me like passing them by value has no other > > > effect than wasting stack on calls. The value is going to be copied anyway, > > but > > > only once instead of once per call. > > > > We currently follow a style throught cc/ to pass gfx:: types of <= 4 ints or > <= > > 2 floats by value, as it generates less code. The compiler can take care of > the > > temporary values that are just passed through. > > That's useful under the assumption of working in 64-bit architectures where a > reference might be larger than the value itself. However, this is not true for > some Chrome platforms like Android (32-bit). What you really want instead is > this: There's some conversation going on about this in a google doc, which i've asked to be shared to you. My understanding is that it's not the amount of stack that's smaller, so comparing to a pointer size is not what I meant. Rather, the amount of instructions required to pass and use a pointer instead of the values which may already be on the stack in the case of passing them along. Anyhow, it's the style rule we're following until we decide otherwise. It'd be great if you want to be part of the conversation about it, but let's take it off this CL as it's not really on topic.
https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:197: ManagedTileState::DrawingInfo::TEXTURE_MODE) { On 2013/03/21 16:42:48, Leandro Graciá Gil wrote: > On 2013/03/21 16:34:11, vmpstr wrote: > > This still looks weird to me > > > > switch (foo) { > > case A: > > case B: > > ... > > if (foo == A) { > > ... > > } else { // foo == B > > ... > > } > > break; > > } > > > > I would vote for having separate case A and case B, even if it results in some > > code duplication. @nduca @danakj, wdyt? > > More than code duplication I'd be concerned about diverging copies. Any changes > in one code should be always copied to the other or we might eventually have > subtle bugs. If maintaining this duplication is something we find acceptable > then I'll split this. What about a block above that is "if TEXTURE MODE or PICTURE_PILE_MODE" where we can stick common code - which would be just the hadIncompleteTile piece I think? Then a separate case for each. The texture_rect/opaque_rect stuff is pretty obvious/small and I have more confidence we can maintain that well. But things like setting flags I'd worry more about, hence the above thought. I also am ok with this how it is. But maybe a "mode" temp var would be helpful for readability. The switch and if could both use the var, and avoid line wrappings as well. https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2278: GLC(context_, context_->deleteTexture(on_demand_tile_raster_texture_)); On 2013/03/21 16:42:48, Leandro Graciá Gil wrote: > On 2013/03/21 04:33:39, danakj wrote: > > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > > On 2013/03/21 02:34:42, danakj wrote: > > > > You need to clean up with the ResourceProvider too > > > > > > Can I use ResourceProvider to clean external textures? Should this be an > > > external texture? > > > > ya, there's actually no need for this to be an external texture. You could > just > > use CreateGLTexture() and then DeleteResource() here. And you'll need to lock > > the resource appropriately to get at the texture id when you want to use it. > See > > other uses of ResourceProvider::ScopedWriteLockGL in this file. > > Looking at ResourceProvider it seems that doing it that way associates a size to > the texture. We would need to destroy the resource and create a new one if the > size changes, and apparently the current texture size isn't exposed either. > > Also, I don't think for this case we want the resource provider to manage > allocation or to put it in a texture pool. Perhaps an external texture is more > convenient after all. I'm adding the code to delete the resource in the > provider. I talked to piman@ and he suggested to just not register with the resource provider at all. So what you've got minus registering it as an external texture. Basically resource provider is for managing resources that can be sent between compositors, but this one being internal to the GLRenderer does not need to worry about that. https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... File cc/quads/tile_draw_quad_base.h (right): https://codereview.chromium.org/12642010/diff/35003/cc/quads/tile_draw_quad_b... cc/quads/tile_draw_quad_base.h:17: class CC_EXPORT TileDrawQuadBase : public DrawQuad { On 2013/03/21 16:42:48, Leandro Graciá Gil wrote: > On 2013/03/21 04:33:39, danakj wrote: > > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > > On 2013/03/21 02:34:42, danakj wrote: > > > > Since this is the base for more than the TileDrawQuad, I feel like a > better > > > name > > > > could be had, but I'm not sure what "class" Tile and Picture are a part > of, > > so > > > > the best I could come up with was TileOrPictureDrawQuadBase. > > > > > > Well, this is actually a base class for tile draw quads. The difference > later > > is > > > that one subclass is backed by a resource id and the other by a > > PicturePileImpl > > > and the information required to rasterize it. > > > > Right, but the latter isn't called a TileDrawQuad or anything like it, it's > > called a PictureDrawQuad. I'd like the name of this class to somehow reflect > > that it's more than a base class for the "TileDrawQuad" class. > > > > Maybe the other quad should be TileOnDemandDrawQuad or something, that would > be > > more clear also.. but I don't mind PictureDrawQuad at all for it. > > I've considered renaming all references to picture to something like > "on-demand", but that ends hiding that a PicturePileImpl is being rasterized. > > I suggest we could name that TilePictureDrawQuad to make things clearer. The > base class could then become something like TiledContentDrawQuad, and then, > PICTURE_CONTENT should probably be named TILED_PICTURE_CONTENT or something > similar. Let me know what you think. I had a chat with enne@ about this and we're thinking ContentDrawQuadBase for this class. Keep the subclasses as is - TileDrawQuad/PictureDrawQuad. (enne@ pointed out that picture quads will often be the whole layer, not tiles, as TILED_PICTURE_CONTENT would seem to hint at, so keeping it as PictureDrawQuad seems nicer.)
https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2278: GLC(context_, context_->deleteTexture(on_demand_tile_raster_texture_)); On 2013/03/21 17:20:22, danakj wrote: > On 2013/03/21 16:42:48, Leandro Graciá Gil wrote: > > On 2013/03/21 04:33:39, danakj wrote: > > > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > > > On 2013/03/21 02:34:42, danakj wrote: > > > > > You need to clean up with the ResourceProvider too > > > > > > > > Can I use ResourceProvider to clean external textures? Should this be an > > > > external texture? > > > > > > ya, there's actually no need for this to be an external texture. You could > > just > > > use CreateGLTexture() and then DeleteResource() here. And you'll need to > lock > > > the resource appropriately to get at the texture id when you want to use it. > > See > > > other uses of ResourceProvider::ScopedWriteLockGL in this file. > > > > Looking at ResourceProvider it seems that doing it that way associates a size > to > > the texture. We would need to destroy the resource and create a new one if the > > size changes, and apparently the current texture size isn't exposed either. > > > > Also, I don't think for this case we want the resource provider to manage > > allocation or to put it in a texture pool. Perhaps an external texture is more > > convenient after all. I'm adding the code to delete the resource in the > > provider. > > I talked to piman@ and he suggested to just not register with the resource > provider at all. So what you've got minus registering it as an external texture. > > Basically resource provider is for managing resources that can be sent between > compositors, but this one being internal to the GLRenderer does not need to > worry about that. Registering as an external texture makes things simpler when using it in the common rendering method for normal and on-demand tiles. We could manually bind the texture, but the method also sets texture filtering dynamically. Normally the provider ensures filtering is only changed if different. However, if we use glBindTexture directly rather than the ResourceProvider::ScopedSamplerGL resource lock we'll be setting the filters on every frame even if not really required. That, or we save them separately and start duplicating provider functionalities and making the code more different for each case. I think that's worth registering the external texture. Let me know if you think otherwise.
https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2278: GLC(context_, context_->deleteTexture(on_demand_tile_raster_texture_)); On 2013/03/21 18:14:16, Leandro Graciá Gil wrote: > On 2013/03/21 17:20:22, danakj wrote: > > On 2013/03/21 16:42:48, Leandro Graciá Gil wrote: > > > On 2013/03/21 04:33:39, danakj wrote: > > > > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > > > > On 2013/03/21 02:34:42, danakj wrote: > > > > > > You need to clean up with the ResourceProvider too > > > > > > > > > > Can I use ResourceProvider to clean external textures? Should this be an > > > > > external texture? > > > > > > > > ya, there's actually no need for this to be an external texture. You could > > > just > > > > use CreateGLTexture() and then DeleteResource() here. And you'll need to > > lock > > > > the resource appropriately to get at the texture id when you want to use > it. > > > See > > > > other uses of ResourceProvider::ScopedWriteLockGL in this file. > > > > > > Looking at ResourceProvider it seems that doing it that way associates a > size > > to > > > the texture. We would need to destroy the resource and create a new one if > the > > > size changes, and apparently the current texture size isn't exposed either. > > > > > > Also, I don't think for this case we want the resource provider to manage > > > allocation or to put it in a texture pool. Perhaps an external texture is > more > > > convenient after all. I'm adding the code to delete the resource in the > > > provider. > > > > I talked to piman@ and he suggested to just not register with the resource > > provider at all. So what you've got minus registering it as an external > texture. > > > > Basically resource provider is for managing resources that can be sent between > > compositors, but this one being internal to the GLRenderer does not need to > > worry about that. > > Registering as an external texture makes things simpler when using it in the > common rendering method for normal and on-demand tiles. We could manually bind > the texture, but the method also sets texture filtering dynamically. > > Normally the provider ensures filtering is only changed if different. However, > if we use glBindTexture directly rather than the > ResourceProvider::ScopedSamplerGL resource lock we'll be setting the filters on > every frame even if not really required. That, or we save them separately and > start duplicating provider functionalities and making the code more different > for each case. > > I think that's worth registering the external texture. Let me know if you think > otherwise. I want to get rid of the non-mailbox external texture feature because it's essentially a big thorn in the resource provider, that was meant as a crutch. We're getting rid of users currently, so I'd strongly prefer if we didn't add any. Please either create a resource through the RP, or skip the RP altogether. If you want to do the latter but share the filter-tracking feature, feel free to factor it out.
On 2013/03/21 18:26:29, piman wrote: > https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc > File cc/output/gl_renderer.cc (right): > > https://codereview.chromium.org/12642010/diff/35003/cc/output/gl_renderer.cc#... > cc/output/gl_renderer.cc:2278: GLC(context_, > context_->deleteTexture(on_demand_tile_raster_texture_)); > On 2013/03/21 18:14:16, Leandro Graciá Gil wrote: > > On 2013/03/21 17:20:22, danakj wrote: > > > On 2013/03/21 16:42:48, Leandro Graciá Gil wrote: > > > > On 2013/03/21 04:33:39, danakj wrote: > > > > > On 2013/03/21 03:35:50, Leandro Graciá Gil wrote: > > > > > > On 2013/03/21 02:34:42, danakj wrote: > > > > > > > You need to clean up with the ResourceProvider too > > > > > > > > > > > > Can I use ResourceProvider to clean external textures? Should this be > an > > > > > > external texture? > > > > > > > > > > ya, there's actually no need for this to be an external texture. You > could > > > > just > > > > > use CreateGLTexture() and then DeleteResource() here. And you'll need to > > > lock > > > > > the resource appropriately to get at the texture id when you want to use > > it. > > > > See > > > > > other uses of ResourceProvider::ScopedWriteLockGL in this file. > > > > > > > > Looking at ResourceProvider it seems that doing it that way associates a > > size > > > to > > > > the texture. We would need to destroy the resource and create a new one if > > the > > > > size changes, and apparently the current texture size isn't exposed > either. > > > > > > > > Also, I don't think for this case we want the resource provider to manage > > > > allocation or to put it in a texture pool. Perhaps an external texture is > > more > > > > convenient after all. I'm adding the code to delete the resource in the > > > > provider. > > > > > > I talked to piman@ and he suggested to just not register with the resource > > > provider at all. So what you've got minus registering it as an external > > texture. > > > > > > Basically resource provider is for managing resources that can be sent > between > > > compositors, but this one being internal to the GLRenderer does not need to > > > worry about that. > > > > Registering as an external texture makes things simpler when using it in the > > common rendering method for normal and on-demand tiles. We could manually bind > > the texture, but the method also sets texture filtering dynamically. > > > > Normally the provider ensures filtering is only changed if different. However, > > if we use glBindTexture directly rather than the > > ResourceProvider::ScopedSamplerGL resource lock we'll be setting the filters > on > > every frame even if not really required. That, or we save them separately and > > start duplicating provider functionalities and making the code more different > > for each case. > > > > I think that's worth registering the external texture. Let me know if you > think > > otherwise. > > I want to get rid of the non-mailbox external texture feature because it's > essentially a big thorn in the resource provider, that was meant as a crutch. > We're getting rid of users currently, so I'd strongly prefer if we didn't add > any. > Please either create a resource through the RP, or skip the RP altogether. > > If you want to do the latter but share the filter-tracking feature, feel free to > factor it out. Also, if the only concern is that resizing a resource in the RP requires deleting and recreating the texture IDs, it doesn't sound out-of-scope to add a ResizeResource.
Leandro, I think it would be productive to split into an edit to the quads/output/rendering patch first, then an edit to the tile manager / picture pile system.
On 2013/03/21 18:33:30, piman wrote: > Also, if the only concern is that resizing a resource in the RP requires > deleting and recreating the texture IDs, it doesn't sound out-of-scope to add a > ResizeResource. There's still the allocated flag and the texture pools. We don't want the resource lock to start doing anything strange or related to the texture allocation, so just adding ResizeResource is probably not a good option. We would need a texture whose allocation is handled externally, which brings external textures again. If we try to leave RP out then we can store the texture filter and check it. However, I've just noticed that ScopedSamplerGL makes this very complicated in GLRenderer::DrawTiledContentQuad as we would need to branch depending on using on-demand rasterization or not (one branch would call glBindTexture, the other would set the resource lock). If we branch then the scope is completely lost unless we take the rest of the function apart into a different one we call it from both cases. Neither of these approaches seem very nice. Any suggestions?
On Thu, Mar 21, 2013 at 12:45 PM, <leandrogracia@chromium.org> wrote: > On 2013/03/21 18:33:30, piman wrote: > >> Also, if the only concern is that resizing a resource in the RP requires >> deleting and recreating the texture IDs, it doesn't sound out-of-scope to >> add >> > a > >> ResizeResource. >> > > There's still the allocated flag and the texture pools. What about them? allocated is just internal tracking to check if we need to call texImage2D to allocate the texture image. On resize, all you need to do is make sure it's consistent with what you're doing (e.g. you can make it lazily resize the texture by simply setting allocated to false). texture pools is orthogonal to this, it's set as a TexParameter, you don't have to change it (but if you wanted to you can simply do it). > We don't want the > resource lock to start doing anything strange or related to the texture > allocation, so just adding ResizeResource is probably not a good option. You just have to make sure the resource isn't locked when you resize it. The conditions are probably very similar to DeleteResource, since it's virtually equivalent to DeleteResource+CreateResource. Only difference is wrt the exported flag, which you should check it's not set (you can't resize a texture that has been sent to the parent compositor). > We > would need a texture whose allocation is handled externally, which brings > external textures again. > I don't think you provided a compelling example of why it's bad. > > If we try to leave RP out then we can store the texture filter and check > it. > However, I've just noticed that ScopedSamplerGL makes this very > complicated in > GLRenderer::**DrawTiledContentQuad as we would need to branch depending > on using > on-demand rasterization or not (one branch would call glBindTexture, the > other > would set the resource lock). If we branch then the scope is completely > lost > unless we take the rest of the function apart into a different one we call > it > from both cases. > > Neither of these approaches seem very nice. Any suggestions? > We gave 3 suggestions: - Destroy+Create. no code to write on RP, but you take the tiny hit for the extra flushes (does it matter? you're in a slow path anyway). - add a ResizeResource to RP. That seems completely doable. - do it externally without interacting with the RP, with or without refactoring of the filter cache (again, does it matter to redundantly set the filter again in the slow path)? > > https://codereview.chromium.**org/12642010/<https://codereview.chromium.org/1... >
On 2013/03/21 19:59:08, piman wrote: > What about them? > allocated is just internal tracking to check if we need to call texImage2D > to allocate the texture image. On resize, all you need to do is make sure > it's consistent with what you're doing (e.g. you can make it lazily resize > the texture by simply setting allocated to false). > texture pools is orthogonal to this, it's set as a TexParameter, you don't > have to change it (but if you wanted to you can simply do it). If allocate is true, what can make RP call texImage2D? In this case since we call it manually during rasterization we might want to prevent RP doing so. > You just have to make sure the resource isn't locked when you resize it. > The conditions are probably very similar to DeleteResource, since it's > virtually equivalent to DeleteResource+CreateResource. Only difference is > wrt the exported flag, which you should check it's not set (you can't > resize a texture that has been sent to the parent compositor). What should be done if the flag is set? Perhaps deleting the old resource and creating a new one?
On Thu, Mar 21, 2013 at 3:43 PM, <leandrogracia@chromium.org> wrote: > On 2013/03/21 19:59:08, piman wrote: > >> What about them? >> allocated is just internal tracking to check if we need to call texImage2D >> to allocate the texture image. On resize, all you need to do is make sure >> it's consistent with what you're doing (e.g. you can make it lazily resize >> the texture by simply setting allocated to false). >> texture pools is orthogonal to this, it's set as a TexParameter, you don't >> have to change it (but if you wanted to you can simply do it). >> > > If allocate is true, what can make RP call texImage2D? In this case since > we > call it manually during rasterization we might want to prevent RP doing so. if allocate is true when you call resize, you can either: - keep it true and call texImage2D immediately - update the size and set it back to false. On the next LockForWrite (or other implicit allocation calls), it will be allocated at the right size if allocate is false, you just update the size. > > > You just have to make sure the resource isn't locked when you resize it. >> The conditions are probably very similar to DeleteResource, since it's >> virtually equivalent to DeleteResource+CreateResource. Only difference is >> wrt the exported flag, which you should check it's not set (you can't >> resize a texture that has been sent to the parent compositor). >> > > What should be done if the flag is set? Perhaps deleting the old resource > and > creating a new one? > Just asserting should be ok for now, since you're not planning to send this one to a parent compositor. > > https://codereview.chromium.**org/12642010/<https://codereview.chromium.org/1... >
Thank you very much for your feedback piman. I've followed your indications and implemented a ResizeResource method. Let me know if the patch looks good for you now. nduca: I could split the patch as suggested, but it seems the main pending issues have been resolved. If there are no further concerns I think we could prepare to land this.
Patch looks good from here, with some nits. vmpstr@, please have another pass as well. https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:176: const ManagedTileState::DrawingInfo::Mode mode = drawing_info.mode(); don't use const for non-pointer/reference local variables. it would have to be named kMode if you did want to use const, but it's really not a "constant" just a local cache. https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:200: nit: no blank lines between cases https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:219: nit: and here https://codereview.chromium.org/12642010/diff/69001/cc/quads/content_draw_qua... File cc/quads/content_draw_quad_base.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/quads/content_draw_qua... cc/quads/content_draw_quad_base.cc:25: const gfx::Rect& visible_rect = rect; nit: gfx::Rect visible_rect https://codereview.chromium.org/12642010/diff/69001/cc/quads/picture_draw_quad.h File cc/quads/picture_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/69001/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.h:53: static const PictureDrawQuad* MaterialCast(const DrawQuad*); this needs the same parameter name as in the .cc https://codereview.chromium.org/12642010/diff/69001/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.h:55: virtual ~PictureDrawQuad(); nit: move this just under Create() https://codereview.chromium.org/12642010/diff/69001/cc/quads/tile_draw_quad.h File cc/quads/tile_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/69001/cc/quads/tile_draw_quad.h... cc/quads/tile_draw_quad.h:41: virtual ~TileDrawQuad(); nit: move this just under Create() https://codereview.chromium.org/12642010/diff/69001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:78: SkRect::MakeXYWH(layer_rect_.width() / 2, 0, nit: 1 argument per line if they don't all fit on one line
Could you add a unittest for ResizeResource? https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... cc/resources/resource_provider.cc:274: DCHECK_LE(size.height(), max_texture_size_); Note: you're only handling GL resources. You should handle the other types too (where the max texture size doesn't apply). https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... cc/resources/resource_provider.cc:281: DCHECK(resource->pending_set_pixels || !resource->locked_for_write); I'm not sure about this if pending_set_pixels is set, you should double-check with epenner/reveman. https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... cc/resources/resource_provider.cc:282: DCHECK(!resource->exported); Check that it's not external (you shouldn't be resizing resources that are not managed by the RP). Also, check that you have no gl_pixel_buffer_id, nor any pixel_buffer, since those would be wrongly sized. https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... cc/resources/resource_provider.cc:286: resource->allocated = false; This is ok for GL resources (LazyAllocate will resize), but for bitmap resources, it should reallocate pixels (currently bitmaps aren't lazy-allocated).
epenner: I'm implementing a ResizeResource method in ResourceProvider and piman asked me to double-check something with you. Could you please take a look to his comment at line 281 in https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... and tell me what you think? Thanks!
Looks OK to me as well. https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:212: drawing_info.contents_swizzled(), I think contents_swizzled might not be set for picture_pile_mode tile. It's set later on in tile_manager run (after rasterizing)... However, you should be able to set that at any point in time https://codereview.chromium.org/12642010/diff/69001/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/69001/content/common/cc_message... content/common/cc_messages.cc:442: return false; nit: you break after other NOTIMPLEMENTED(), this should be consistent.
https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:176: const ManagedTileState::DrawingInfo::Mode mode = drawing_info.mode(); On 2013/03/22 18:02:41, danakj wrote: > don't use const for non-pointer/reference local variables. > > it would have to be named kMode if you did want to use const, but it's really > not a "constant" just a local cache. Done. https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:200: On 2013/03/22 18:02:41, danakj wrote: > nit: no blank lines between cases Done. https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:212: drawing_info.contents_swizzled(), On 2013/03/22 18:29:32, vmpstr wrote: > I think contents_swizzled might not be set for picture_pile_mode tile. It's set > later on in tile_manager run (after rasterizing)... However, you should be able > to set that at any point in time Good catch! Fixed. https://codereview.chromium.org/12642010/diff/69001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:219: On 2013/03/22 18:02:41, danakj wrote: > nit: and here Done. https://codereview.chromium.org/12642010/diff/69001/cc/quads/content_draw_qua... File cc/quads/content_draw_quad_base.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/quads/content_draw_qua... cc/quads/content_draw_quad_base.cc:25: const gfx::Rect& visible_rect = rect; On 2013/03/22 18:02:41, danakj wrote: > nit: gfx::Rect visible_rect Done. https://codereview.chromium.org/12642010/diff/69001/cc/quads/picture_draw_quad.h File cc/quads/picture_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/69001/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.h:53: static const PictureDrawQuad* MaterialCast(const DrawQuad*); On 2013/03/22 18:02:41, danakj wrote: > this needs the same parameter name as in the .cc Done. https://codereview.chromium.org/12642010/diff/69001/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.h:55: virtual ~PictureDrawQuad(); On 2013/03/22 18:02:41, danakj wrote: > nit: move this just under Create() Done. https://codereview.chromium.org/12642010/diff/69001/cc/quads/tile_draw_quad.h File cc/quads/tile_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/69001/cc/quads/tile_draw_quad.h... cc/quads/tile_draw_quad.h:41: virtual ~TileDrawQuad(); On 2013/03/22 18:02:41, danakj wrote: > nit: move this just under Create() Done. https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... cc/resources/resource_provider.cc:274: DCHECK_LE(size.height(), max_texture_size_); On 2013/03/22 18:03:03, piman wrote: > Note: you're only handling GL resources. You should handle the other types too > (where the max texture size doesn't apply). Done. https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... cc/resources/resource_provider.cc:282: DCHECK(!resource->exported); On 2013/03/22 18:03:03, piman wrote: > Check that it's not external (you shouldn't be resizing resources that are not > managed by the RP). > Also, check that you have no gl_pixel_buffer_id, nor any pixel_buffer, since > those would be wrongly sized. Done. https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... cc/resources/resource_provider.cc:286: resource->allocated = false; On 2013/03/22 18:03:03, piman wrote: > This is ok for GL resources (LazyAllocate will resize), but for bitmap > resources, it should reallocate pixels (currently bitmaps aren't > lazy-allocated). Done. https://codereview.chromium.org/12642010/diff/69001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:78: SkRect::MakeXYWH(layer_rect_.width() / 2, 0, On 2013/03/22 18:02:41, danakj wrote: > nit: 1 argument per line if they don't all fit on one line Done. https://codereview.chromium.org/12642010/diff/69001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_on_demand_raster.cc:78: SkRect::MakeXYWH(layer_rect_.width() / 2, 0, On 2013/03/22 18:02:41, danakj wrote: > nit: 1 argument per line if they don't all fit on one line Done. https://codereview.chromium.org/12642010/diff/69001/content/common/cc_message... File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/69001/content/common/cc_message... content/common/cc_messages.cc:442: return false; On 2013/03/22 18:29:32, vmpstr wrote: > nit: you break after other NOTIMPLEMENTED(), this should be consistent. This was actually a review suggestion from piman in patch #1.
Unit test for ResizeResource added.
LGTM+nits for the ResourceProvider+test changes. https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... cc/resources/resource_provider.cc:291: nit: no need for blank line. https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... cc/resources/resource_provider.cc:300: NOTREACHED(); nit: we usually don't add the default clause when switching on an enum, so that we get a compiler warning if adding an enum value. https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... cc/resources/resource_provider_unittest.cc:1176: default: nit: no need for default clause.
https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... cc/resources/resource_provider.cc:281: DCHECK(resource->pending_set_pixels || !resource->locked_for_write); I added reveman to review as well. I believe the pending flag is okay as it will still complete first before the resize. However! Async textures are immutable and can't be resized. This is similar to texStorage2DEXT which we might also want to use in here as it's nicer for GL implementations. Would it make sense to just delete the texture completely? That's effectively what happens on resize anyway, and is the only way to resize async or glTexStorage2D textures.
On 2013/03/22 22:30:59, epenner wrote: > https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... > File cc/resources/resource_provider.cc (right): > > https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... > cc/resources/resource_provider.cc:281: DCHECK(resource->pending_set_pixels || > !resource->locked_for_write); > I added reveman to review as well. > > I believe the pending flag is okay as it will still complete first before the > resize. However! Async textures are immutable and can't be resized. This is > similar to texStorage2DEXT which we might also want to use in here as it's nicer > for GL implementations. > > Would it make sense to just delete the texture completely? That's effectively > what happens on resize anyway, and is the only way to resize async or > glTexStorage2D textures. This is actually not for async textures, but to implement on-demand tile rasterization for out-of-memory situations. We're precisely trying to avoid deleting and creating the texture on every tile. By the way, isn't glTexStorage2D not part of OpenGL ES 2.x? If that's the case then the Android port won't be able to use it.
Also, if you do delete the texture id, I believe you can just reset pending state to false. Hmm, I suppose this could be done outside resource provider too, by just deleting resources. Is there any negatives to doing that instead?
On 2013/03/22 22:45:28, Leandro Graciá Gil wrote: > On 2013/03/22 22:30:59, epenner wrote: > > > https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... > > File cc/resources/resource_provider.cc (right): > > > > > https://codereview.chromium.org/12642010/diff/69001/cc/resources/resource_pro... > > cc/resources/resource_provider.cc:281: DCHECK(resource->pending_set_pixels || > > !resource->locked_for_write); > > I added reveman to review as well. > > > > I believe the pending flag is okay as it will still complete first before the > > resize. However! Async textures are immutable and can't be resized. This is > > similar to texStorage2DEXT which we might also want to use in here as it's > nicer > > for GL implementations. > > > > Would it make sense to just delete the texture completely? That's effectively > > what happens on resize anyway, and is the only way to resize async or > > glTexStorage2D textures. > > This is actually not for async textures, but to implement on-demand tile > rasterization for out-of-memory situations. We're precisely trying to avoid > deleting and creating the texture on every tile. > > By the way, isn't glTexStorage2D not part of OpenGL ES 2.x? If that's the case > then the Android port won't be able to use it. I forgot about texStorage2D. It's an extension to GLES2, but even if not supported by the driver, I think we emulate it at the command-buffers level. In any case, we can check if it was allocated by texStorage2D (maybe tag the resource in LazyAllocate), and in that case call deleteTexture+createTexture. Eric: what specifies that a texture is "async" wrt immutable? The fact that you called asyncTexImage2DCHROMIUM? It sounds like we can tag it too in that case.
https://codereview.chromium.org/12642010/diff/72004/cc/quads/picture_draw_quad.h File cc/quads/picture_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/72004/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.h:20: class CC_EXPORT PictureDrawQuad : public ContentDrawQuadBase { Oh, I should mention: Can you please add unit tests for this new quad type to draw_quad_unittest.cc? There's a few sets of tests, with 1 test per quad type.
> This is actually not for async textures, but to implement > on-demand tile rasterization for out-of-memory situations. > We're precisely trying to avoid deleting and creating the > texture on every tile. Sorry, but why are you trying to avoid deleting/creating textures? Resizing incurs the same performance cost as deleting/creating. Perhaps a driver could pad allocations and try to handle small resizes without reallocating, but I don't see that being very likely. > Eric: what specifies that a texture is "async" wrt immutable? > The fact that you called asyncTexImage2DCHROMIUM? It sounds > like we can tag it too in that case. Just calling either async call will make it immutable yes. So detecting that will suffice to flag the delete.
https://codereview.chromium.org/12642010/diff/72004/cc/resources/managed_tile... File cc/resources/managed_tile_state.h (right): https://codereview.chromium.org/12642010/diff/72004/cc/resources/managed_tile... cc/resources/managed_tile_state.h:70: void set_texture() { set_resource() and please change the enum to RESOURCE_MODE instead of TEXTURE_MODE. we shouldn't be referring to textures here as we don't know the type of resource. https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... cc/resources/resource_provider.cc:272: void ResourceProvider::ResizeResource(ResourceId id, gfx::Size size) { I don't understand why we need this function.
https://codereview.chromium.org/12642010/diff/72004/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/72004/cc/resources/tile_manager... cc/resources/tile_manager.cc:599: tile->drawing_info().set_texture(); This feels a bit awkward. I think something like set_no_rasterize_on_demand() would be better.
I've been thinking about all the details that need to be tracked, the different issues that might arise and the performance equivalence to deleting and creating a new texture. Considering that, I've reviewed the assumptions for introducing a ResizeResource method. While it made sense in the original design where we had a single texture that got resized, now that we create the resource on demand it should be perfectly possible to use the rasterized bitmap size to keep track of the texture size and destroy it when required, creating it again afterwards. With this we should prevent destroying the texture unless the size changes, and we should not need introducing a ResizeResource method. I should have considered this before and save a bit of everyone's time. Sorry for the inconveniences. Let me know if there are any other issues I should look at. https://codereview.chromium.org/12642010/diff/72004/cc/quads/picture_draw_quad.h File cc/quads/picture_draw_quad.h (right): https://codereview.chromium.org/12642010/diff/72004/cc/quads/picture_draw_qua... cc/quads/picture_draw_quad.h:20: class CC_EXPORT PictureDrawQuad : public ContentDrawQuadBase { On 2013/03/22 22:54:05, danakj wrote: > Oh, I should mention: Can you please add unit tests for this new quad type to > draw_quad_unittest.cc? There's a few sets of tests, with 1 test per quad type. Done. https://codereview.chromium.org/12642010/diff/72004/cc/resources/managed_tile... File cc/resources/managed_tile_state.h (right): https://codereview.chromium.org/12642010/diff/72004/cc/resources/managed_tile... cc/resources/managed_tile_state.h:70: void set_texture() { On 2013/03/22 23:56:50, David Reveman wrote: > set_resource() and please change the enum to RESOURCE_MODE instead of > TEXTURE_MODE. we shouldn't be referring to textures here as we don't know the > type of resource. Done. https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/12642010/diff/72004/cc/resources/resource_pro... cc/resources/resource_provider.cc:272: void ResourceProvider::ResizeResource(ResourceId id, gfx::Size size) { On 2013/03/22 23:56:50, David Reveman wrote: > I don't understand why we need this function. Considering all the details that need to be tracked and the problems this might introduce, I'm reviewing the assumptions that led to introducing this function. I'll make an overall comment in the next iteration of the patch. https://codereview.chromium.org/12642010/diff/72004/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/72004/cc/resources/tile_manager... cc/resources/tile_manager.cc:599: tile->drawing_info().set_texture(); On 2013/03/23 00:41:31, David Reveman wrote: > This feels a bit awkward. I think something like set_no_rasterize_on_demand() > would be better. Since there is another comment for this same method suggesting to use set_resource, I'm proposing "set_use_resource" to make clear both cases. Let me know what you think.
https://codereview.chromium.org/12642010/diff/100001/cc/quads/draw_quad_unitt... File cc/quads/draw_quad_unittest.cc (right): https://codereview.chromium.org/12642010/diff/100001/cc/quads/draw_quad_unitt... cc/quads/draw_quad_unittest.cc:731: TEST_F(DrawQuadIteratorTest, DebugBorderDrawQuad) { Thanks for the test! Can you do an iterator one as well? Should be a copy of this one more-or-less.
On 2013/03/25 16:47:59, danakj wrote: > https://codereview.chromium.org/12642010/diff/100001/cc/quads/draw_quad_unitt... > File cc/quads/draw_quad_unittest.cc (right): > > https://codereview.chromium.org/12642010/diff/100001/cc/quads/draw_quad_unitt... > cc/quads/draw_quad_unittest.cc:731: TEST_F(DrawQuadIteratorTest, > DebugBorderDrawQuad) { > Thanks for the test! Can you do an iterator one as well? Should be a copy of > this one more-or-less. Done.
Let me know if there is anything else that should be holding this patch. Thanks for the review and all the useful feedback until now.
Just FYI, you'll need to do a small rebase on top of https://codereview.chromium.org/12457031/ since PicturePileImpl::Raster signature changed.
On 2013/03/26 16:30:37, vmpstr wrote: > Just FYI, you'll need to do a small rebase on top of > https://codereview.chromium.org/12457031/ since PicturePileImpl::Raster > signature changed. Thanks for the heads up, rebase done. piman and other reviewers: anything else I should look at? Otherwise I'll push this to the commit queue later today.
On 2013/03/26 17:03:59, Leandro Graciá Gil wrote: > On 2013/03/26 16:30:37, vmpstr wrote: > > Just FYI, you'll need to do a small rebase on top of > > https://codereview.chromium.org/12457031/ since PicturePileImpl::Raster > > signature changed. > > Thanks for the heads up, rebase done. > > piman and other reviewers: anything else I should look at? Otherwise I'll push > this to the commit queue later today. I believe nat wanted to give this a look over, so please wait for that.
LGTM+nits for content/ and gl_renderer. I haven't looked at the rest in details as I'm not as familiar with the internals as others are. https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:1322: ResourceProvider::TextureUsageAny); nit: you could move this in the if block above, after the DeleteResource (saves an if). It guarantees that you'll create the resource. https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:1339: on_demand_tile_raster_bitmap_.getPixels())); You should be able to just use ResourceProvider::SetPixels https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages... content/common/cc_messages.cc:324: NOTIMPLEMENTED(); nit: NOTREACHED() or DCHECK. Not writing anything will confuse the reader (it expects the right number of quads), so make sure to assert on the writer side so that this can be tracked at the origin.
https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages... content/common/cc_messages.cc:324: NOTIMPLEMENTED(); On 2013/03/26 17:19:30, piman wrote: > nit: NOTREACHED() or DCHECK. > Not writing anything will confuse the reader (it expects the right number of > quads), so make sure to assert on the writer side so that this can be tracked at > the origin. Oh, I had requested this, because NOTREACHED() will also randomly DCHECK if we turn on implside painting. Didn't think about the number of quads. :/ I'll file a bug.
On Tue, Mar 26, 2013 at 10:28 AM, <danakj@chromium.org> wrote: > > https://codereview.chromium.**org/12642010/diff/9002/** > content/common/cc_messages.cc<https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc> > File content/common/cc_messages.cc (right): > > https://codereview.chromium.**org/12642010/diff/9002/** > content/common/cc_messages.cc#**newcode324<https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc#newcode324> > content/common/cc_messages.cc:**324: NOTIMPLEMENTED(); > On 2013/03/26 17:19:30, piman wrote: > >> nit: NOTREACHED() or DCHECK. >> Not writing anything will confuse the reader (it expects the right >> > number of > >> quads), so make sure to assert on the writer side so that this can be >> > tracked at > >> the origin. >> > > Oh, I had requested this, because NOTREACHED() will also randomly DCHECK > if we turn on implside painting. Didn't think about the number of quads. > :/ I'll file a bug. > Wait, what, why? I thought this was only for android webview ?!? > > https://codereview.chromium.**org/12642010/<https://codereview.chromium.org/1... >
On Tue, Mar 26, 2013 at 1:31 PM, Antoine Labour <piman@chromium.org> wrote: > On Tue, Mar 26, 2013 at 10:28 AM, <danakj@chromium.org> wrote: > >> >> https://codereview.chromium.**org/12642010/diff/9002/** >> content/common/cc_messages.cc<https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc> >> File content/common/cc_messages.cc (right): >> >> https://codereview.chromium.**org/12642010/diff/9002/** >> content/common/cc_messages.cc#**newcode324<https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc#newcode324> >> content/common/cc_messages.cc:**324: NOTIMPLEMENTED(); >> On 2013/03/26 17:19:30, piman wrote: >> >>> nit: NOTREACHED() or DCHECK. >>> Not writing anything will confuse the reader (it expects the right >>> >> number of >> >>> quads), so make sure to assert on the writer side so that this can be >>> >> tracked at >> >>> the origin. >>> >> >> Oh, I had requested this, because NOTREACHED() will also randomly DCHECK >> if we turn on implside painting. Didn't think about the number of quads. >> :/ I'll file a bug. >> > > Wait, what, why? I thought this was only for android webview ?!? > This is also the implside painting OOM strategy. On-remand raster if we can't afford the texture memory. > > >> >> https://codereview.chromium.**org/12642010/<https://codereview.chromium.org/1... >> > >
On Tue, Mar 26, 2013 at 10:28 AM, <danakj@chromium.org> wrote: > > https://codereview.chromium.**org/12642010/diff/9002/** > content/common/cc_messages.cc<https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc> > File content/common/cc_messages.cc (right): > > https://codereview.chromium.**org/12642010/diff/9002/** > content/common/cc_messages.cc#**newcode324<https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc#newcode324> > content/common/cc_messages.cc:**324: NOTIMPLEMENTED(); > On 2013/03/26 17:19:30, piman wrote: > >> nit: NOTREACHED() or DCHECK. >> Not writing anything will confuse the reader (it expects the right >> > number of > >> quads), so make sure to assert on the writer side so that this can be >> > tracked at > >> the origin. >> > > Oh, I had requested this, because NOTREACHED() will also randomly DCHECK > if we turn on implside painting. In any case, it's better to have an assert that can be traced than a renderer that stops updating without a good way to trace it. > Didn't think about the number of quads. > :/ I'll file a bug. > > https://codereview.chromium.**org/12642010/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:1322: ResourceProvider::TextureUsageAny); On 2013/03/26 17:19:30, piman wrote: > nit: you could move this in the if block above, after the DeleteResource (saves > an if). It guarantees that you'll create the resource. Done. https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:1339: on_demand_tile_raster_bitmap_.getPixels())); On 2013/03/26 17:19:30, piman wrote: > You should be able to just use ResourceProvider::SetPixels We want this to happen synchronously for this specific case rather than using the texture uploader. https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages.cc File content/common/cc_messages.cc (right): https://codereview.chromium.org/12642010/diff/9002/content/common/cc_messages... content/common/cc_messages.cc:324: NOTIMPLEMENTED(); On 2013/03/26 17:28:52, danakj wrote: > On 2013/03/26 17:19:30, piman wrote: > > nit: NOTREACHED() or DCHECK. > > Not writing anything will confuse the reader (it expects the right number of > > quads), so make sure to assert on the writer side so that this can be tracked > at > > the origin. > > Oh, I had requested this, because NOTREACHED() will also randomly DCHECK if we > turn on implside painting. Didn't think about the number of quads. :/ I'll file > a bug. Ok, in that case I'll change these back to NOTREACHED().
https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc#n... cc/output/gl_renderer.cc:1339: on_demand_tile_raster_bitmap_.getPixels())); On 2013/03/26 18:05:02, Leandro Graciá Gil wrote: > On 2013/03/26 17:19:30, piman wrote: > > You should be able to just use ResourceProvider::SetPixels > > We want this to happen synchronously for this specific case rather than using > the texture uploader. SetPixels is synchronous.
On 2013/03/26 18:22:38, piman wrote: > https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc > File cc/output/gl_renderer.cc (right): > > https://codereview.chromium.org/12642010/diff/9002/cc/output/gl_renderer.cc#n... > cc/output/gl_renderer.cc:1339: on_demand_tile_raster_bitmap_.getPixels())); > On 2013/03/26 18:05:02, Leandro Graciá Gil wrote: > > On 2013/03/26 17:19:30, piman wrote: > > > You should be able to just use ResourceProvider::SetPixels > > > > We want this to happen synchronously for this specific case rather than using > > the texture uploader. > > SetPixels is synchronous. My mistake then. Fixed.
lgtm with nit for cc/resources https://codereview.chromium.org/12642010/diff/9007/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/9007/cc/resources/tile_manager.... cc/resources/tile_manager.cc:597: !PlatformColor::SameComponentOrder(tile->format_)); nit: indent 4 spaces here
https://codereview.chromium.org/12642010/diff/9007/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/12642010/diff/9007/cc/resources/tile_manager.... cc/resources/tile_manager.cc:597: !PlatformColor::SameComponentOrder(tile->format_)); On 2013/03/26 18:32:53, David Reveman wrote: > nit: indent 4 spaces here Done.
lgtm
https://codereview.chromium.org/12642010/diff/126011/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/126011/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:185: mode == ManagedTileState::DrawingInfo::PICTURE_PILE_MODE) { I'm not sure I agree with setting had_incomplete_tile here. Arguably the tile is "complete" and whatever it draws on screen is at the ideal resolution. "had incomplete tile" implies that we need to keep vsync ticking because we're waiting on something to finish rastering. Resources that are using picture pile mode will never "finish" because there's no work to be done.
https://codereview.chromium.org/12642010/diff/126011/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12642010/diff/126011/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:185: mode == ManagedTileState::DrawingInfo::PICTURE_PILE_MODE) { On 2013/03/26 18:39:23, enne wrote: > I'm not sure I agree with setting had_incomplete_tile here. Arguably the tile > is "complete" and whatever it draws on screen is at the ideal resolution. > > "had incomplete tile" implies that we need to keep vsync ticking because we're > waiting on something to finish rastering. Resources that are using picture pile > mode will never "finish" because there's no work to be done. I see your point. I'm moving this out of the PICTURE_PILE_MODE case.
Really nicely done, Leandro. LGTM and thanks!
On 2013/03/26 19:59:41, nduca wrote: > Really nicely done, Leandro. LGTM and thanks! Thanks everyone for the review and all the feedback. Pushing into the CQ.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/12642010/46010
Failed to apply patch for cc/quads/content_draw_quad_base.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
A cc/quads/content_draw_quad_base.h
Copied cc/quads/tile_draw_quad.h -> cc/quads/content_draw_quad_base.h
patching file cc/quads/content_draw_quad_base.h
Hunk #2 FAILED at 14.
1 out of 2 hunks FAILED -- saving rejects to file
cc/quads/content_draw_quad_base.h.rej
Patch: NR cc/quads/tile_draw_quad.h->cc/quads/content_draw_quad_base.h
Index: cc/quads/content_draw_quad_base.h
diff --git a/cc/quads/tile_draw_quad.h b/cc/quads/content_draw_quad_base.h
similarity index 64%
copy from cc/quads/tile_draw_quad.h
copy to cc/quads/content_draw_quad_base.h
index
74c36980661354dd75f3f405b3befb7d1e365dc3..f91390b02bc1965f96007084fe12f89224c196de
100644
--- a/cc/quads/tile_draw_quad.h
+++ b/cc/quads/content_draw_quad_base.h
@@ -1,9 +1,9 @@
-// Copyright 2012 The Chromium Authors. All rights reserved.
+// Copyright 2013 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#ifndef CC_QUADS_TILE_DRAW_QUAD_H_
-#define CC_QUADS_TILE_DRAW_QUAD_H_
+#ifndef CC_QUADS_CONTENT_DRAW_QUAD_BASE_H_
+#define CC_QUADS_CONTENT_DRAW_QUAD_BASE_H_
#include "base/memory/scoped_ptr.h"
#include "cc/base/cc_export.h"
@@ -14,41 +14,35 @@
namespace cc {
-class CC_EXPORT TileDrawQuad : public DrawQuad {
+class CC_EXPORT ContentDrawQuadBase : public DrawQuad {
public:
- static scoped_ptr<TileDrawQuad> Create();
-
void SetNew(const SharedQuadState* shared_quad_state,
+ DrawQuad::Material material,
gfx::Rect rect,
gfx::Rect opaque_rect,
- unsigned resource_id,
const gfx::RectF& tex_coord_rect,
gfx::Size texture_size,
bool swizzle_contents);
void SetAll(const SharedQuadState* shared_quad_state,
+ DrawQuad::Material material,
gfx::Rect rect,
gfx::Rect opaque_rect,
gfx::Rect visible_rect,
bool needs_blending,
- unsigned resource_id,
const gfx::RectF& tex_coord_rect,
gfx::Size texture_size,
bool swizzle_contents);
- unsigned resource_id;
gfx::RectF tex_coord_rect;
gfx::Size texture_size;
bool swizzle_contents;
- virtual void IterateResources(const ResourceIteratorCallback& callback)
- OVERRIDE;
-
- static const TileDrawQuad* MaterialCast(const DrawQuad*);
- private:
- TileDrawQuad();
+ protected:
+ ContentDrawQuadBase();
+ virtual ~ContentDrawQuadBase();
};
}
-#endif // CC_QUADS_TILE_DRAW_QUAD_H_
+#endif // CC_QUADS_CONTENT_DRAW_QUAD_BASE_H_
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/12642010/58002
Message was sent while issue was closed.
Change committed as 190969 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
