Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14230007:
cc: Do GatherPixelRefs from skia at record time (Closed)
|
Created:
7 years, 8 months ago by vmpstr Modified:
7 years, 8 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptioncc: Do GatherPixelRefs from skia at record time
This change gather lazy pixel refs from skia at record time and
stores them in a 512x512 grid. When GatherPixelRefs is invoked,
the refs are looked up in this grid (cells that intersect our
required rect contribute their pixel refs to the final result)
BUG=231977
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196451
Patch Set 1 #
Total comments: 6
Patch Set 2 : update to hash_map #
Total comments: 2
Patch Set 3 : changed kRegionSize to width/height #Patch Set 4 : changed gather to iterators #
Total comments: 3
Patch Set 5 : unittests + enne's review #
Total comments: 31
Patch Set 6 : reviews #Patch Set 7 : reviews #Patch Set 8 : updated unittests #
Total comments: 31
Patch Set 9 : more reviews #
Total comments: 21
Patch Set 10 : reviews #
Total comments: 4
Patch Set 11 : review #
Total comments: 6
Patch Set 12 : nits #Patch Set 13 : compile fix #
Messages
Total messages: 38 (0 generated)
I'm working on adding unit tests for this, but please take a look.
https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode67 cc/resources/picture.h:67: typedef std::vector<PositionPixelRefs> PositionPixelRefsVector; how about using: base::hash_map<std::pair<int, int>, std::list<skia::LazyPixelRef*> > PixelRefMap; that would give us the pixel refs for a tile in constant time on the cc thread.
> https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode67 > cc/resources/picture.h:67: typedef std::vector<PositionPixelRefs> > PositionPixelRefsVector; > how about using: > base::hash_map<std::pair<int, int>, > std::list<skia::LazyPixelRef*> > PixelRefMap; > > that would give us the pixel refs for a tile in constant time on the cc thread. I'm not sure how that would work. Pixel refs occupy a rect, not a single point. When we have a rect we're drawing, I'm not clear which point we want to grab out of PixelRefMap (right now I intersect that rect with the grid cell which in turn intersects with the pixel ref).
On 2013/04/16 18:53:07, vmpstr wrote: > > > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode67 > > cc/resources/picture.h:67: typedef std::vector<PositionPixelRefs> > > PositionPixelRefsVector; > > how about using: > > base::hash_map<std::pair<int, int>, > > std::list<skia::LazyPixelRef*> > PixelRefMap; > > > > that would give us the pixel refs for a tile in constant time on the cc > thread. > > I'm not sure how that would work. Pixel refs occupy a rect, not a single point. > When we have a rect we're drawing, I'm not clear which point we want to grab out > of PixelRefMap (right now I intersect that rect with the grid cell which in turn > intersects with the pixel ref). If the grid cell size is constant then hashing of the top left corner is enough to identify each grid cell. I'm assuming the current PositionPixelRefsVector is a vector of grid cells with associated pixel refs. Is that correct? If that's not the case then what I'm suggesting might not work.
https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.cc#newco... cc/resources/picture.cc:223: gfx::Rect rect(x, y, extent.width(), extent.height()); Can we get this information from Skia, rather than trying to guess what rect this image takes up? https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode67 cc/resources/picture.h:67: typedef std::vector<PositionPixelRefs> PositionPixelRefsVector; On 2013/04/16 18:38:35, David Reveman wrote: > how about using: > base::hash_map<std::pair<int, int>, > std::list<skia::LazyPixelRef*> > PixelRefMap; > > that would give us the pixel refs for a tile in constant time on the cc thread. For a recorded tile, you mean? I think the original code is good, since a given "get me the pixel refs for this rect" call doesn't seem like it should pick up every image in the whole recorded tile. (Unless that's what you're suggesting, and I'm misunderstanding?) https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode90 cc/resources/picture.h:90: PositionPixelRefsVector lazy_pixel_refs_; These are safe to store using raw pointers because the SkPicture and any clones have refcounted pointers to them?
On 2013/04/16 22:03:36, enne wrote: > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.cc > File cc/resources/picture.cc (right): > > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.cc#newco... > cc/resources/picture.cc:223: gfx::Rect rect(x, y, extent.width(), > extent.height()); > Can we get this information from Skia, rather than trying to guess what rect > this image takes up? I looked and I couldn't find an easy way to say where SkPixelRef's boundaries are. This isn't really calculating the image rect, but rather splitting up layer_rect_ into 512x512 tiles. The extent calculation here is just to make sure that the right and bottom rects don't go past layer_rect_ boundaries. > > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h > File cc/resources/picture.h (right): > > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode67 > cc/resources/picture.h:67: typedef std::vector<PositionPixelRefs> > PositionPixelRefsVector; > On 2013/04/16 18:38:35, David Reveman wrote: > > how about using: > > base::hash_map<std::pair<int, int>, > > std::list<skia::LazyPixelRef*> > PixelRefMap; > > > > that would give us the pixel refs for a tile in constant time on the cc > thread. > > For a recorded tile, you mean? I think the original code is good, since a given > "get me the pixel refs for this rect" call doesn't seem like it should pick up > every image in the whole recorded tile. (Unless that's what you're suggesting, > and I'm misunderstanding?) I've talked to David offline, and the difference between our approaches is that in the original code, I would iterate over the whole layer rect looking for grid cells that intersect the given rect. The proposal is to iterate over the given rect instead (since it should be smaller), and only look up the grid cells that we know would intersect it (we know this since they are in a regular grid). I agree that it's a more efficient approach, so I'm working on a revised CL. > > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode90 > cc/resources/picture.h:90: PositionPixelRefsVector lazy_pixel_refs_; > These are safe to store using raw pointers because the SkPicture and any clones > have refcounted pointers to them? I believe so?... According to skia docs, SkPixelRef (from which LazyPixelRef is derived) is thread safe: "This class can be shared/accessed between multiple threads." It derives from SkFlattenable, which derives from SkRefCnt.
On 2013/04/16 22:40:43, vmpstr wrote: > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.cc#newco... > > cc/resources/picture.cc:223: gfx::Rect rect(x, y, extent.width(), > > extent.height()); > > Can we get this information from Skia, rather than trying to guess what rect > > this image takes up? > > I looked and I couldn't find an easy way to say where SkPixelRef's boundaries > are. This isn't really calculating the image rect, but rather splitting up > layer_rect_ into 512x512 tiles. The extent calculation here is just to make sure > that the right and bottom rects don't go past layer_rect_ boundaries. Yeah, I understand that. It's just seems a bit of an awkward way to get at that information and I was wondering if there was a better way to handle this. This is a reasonable step to get us towards a rect => pixel ref mapping. I think I was hoping that ideally either Skia could give us this information directly or alternatively that we could do something like analysis canvas or a draw filter at record time, so that we could store directly which images were drawn where. > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode90 > > cc/resources/picture.h:90: PositionPixelRefsVector lazy_pixel_refs_; > > These are safe to store using raw pointers because the SkPicture and any > clones > > have refcounted pointers to them? > > I believe so?... According to skia docs, SkPixelRef (from which LazyPixelRef is > derived) is thread safe: "This class can be shared/accessed between multiple > threads." It derives from SkFlattenable, which derives from SkRefCnt. I was more asking about the ownership model here, in terms of which pictures are the ones keeping these pixel refs alive.
On 2013/04/16 23:03:50, enne wrote: > Yeah, I understand that. It's just seems a bit of an awkward way to get at that > information and I was wondering if there was a better way to handle this. This > is a reasonable step to get us towards a rect => pixel ref mapping. > > I think I was hoping that ideally either Skia could give us this information > directly or alternatively that we could do something like analysis canvas or a > draw filter at record time, so that we could store directly which images were > drawn where. > I think (or maybe hope?) that the long term goal is for Skia to actually do this type of gather while recording and give us the pixel refs for a rect in near-zero time. I agree that this feels a bit crude, since we're guessing how fine or coarse of a grid we'll need, but I think this is a good enough step forward? > I was more asking about the ownership model here, in terms of which pictures are > the ones keeping these pixel refs alive. Ah, sorry I misunderstood. Yes, the SkPicture (or its recording) has SkBitmap array, which is copied at clone time. However, each of the SkBitmaps contains a refcounted pointer to the pixel refs. So, any new clone will (should?) ensure that the pixel refs remain alive.
On 2013/04/16 23:03:50, enne wrote: > On 2013/04/16 22:40:43, vmpstr wrote: > > > > https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.cc#newco... > > > cc/resources/picture.cc:223: gfx::Rect rect(x, y, extent.width(), > > > extent.height()); > > > Can we get this information from Skia, rather than trying to guess what rect > > > this image takes up? > > > > I looked and I couldn't find an easy way to say where SkPixelRef's boundaries > > are. This isn't really calculating the image rect, but rather splitting up > > layer_rect_ into 512x512 tiles. The extent calculation here is just to make > sure > > that the right and bottom rects don't go past layer_rect_ boundaries. > > Yeah, I understand that. It's just seems a bit of an awkward way to get at that > information and I was wondering if there was a better way to handle this. This > is a reasonable step to get us towards a rect => pixel ref mapping. > > I think I was hoping that ideally either Skia could give us this information > directly or alternatively that we could do something like analysis canvas or a > draw filter at record time, so that we could store directly which images were > drawn where. Yes, the plan is to have skia do this as part of recording. In the meantime we're trying to do something as close to that as possible. The important part is being able to guarantee tile rect => pixel refs performance on cc thread that is good enough that the tile manager can consider it free. SkDrawFilter would be nice to use but we currently don't have access to the CTM in a clean when using it.
https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.cc#newco... cc/resources/picture.cc:223: gfx::Rect rect(x, y, extent.width(), extent.height()); On 2013/04/16 22:03:36, enne wrote: > Can we get this information from Skia, rather than trying to guess what rect > this image takes up? this is not related to the image size. it's the grid size used for storing pixel refs. we should probably use PicturePileBase::tile_grid_info_.fTileInterval here as that's probably what will be used if we move this to skia. https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/1/cc/resources/picture.h#newcode67 cc/resources/picture.h:67: typedef std::vector<PositionPixelRefs> PositionPixelRefsVector; we're trying to get the pixel refs for a playback tile in close to constant time on cc thread. guaranteeing constant time for this seem hard but I think having it not depend on SkPicture size (neither bytes or area) or number of pixel refs in the recording is good enough. the performance of using a vector would depend on SkPicture size (area).
The updated CL is up. PTAL.
https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc#n... cc/resources/picture.cc:26: const gfx::Size kRegionSize(512, 512); only POD is allowed for objects with static storage. you would have to make this int kRegionWidth and int kRegionHeight.. https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc#n... cc/resources/picture.cc:165: std::list<skia::LazyPixelRef*>& pixel_ref_list) { How about adding a Picture::PixelRefIterator instead of having this function that returns a list? I think that would be cleaner to use in the tile manager. It could even be allowed to iterate over the same pixel ref twice as the tile manager needs to handle that properly anyhow.
On 2013/04/17 00:42:56, David Reveman wrote: > https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc > File cc/resources/picture.cc (right): > > https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc#n... > cc/resources/picture.cc:26: const gfx::Size kRegionSize(512, 512); > only POD is allowed for objects with static storage. you would have to make this > int kRegionWidth and int kRegionHeight.. Fixed. > > https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc#n... > cc/resources/picture.cc:165: std::list<skia::LazyPixelRef*>& pixel_ref_list) { > How about adding a Picture::PixelRefIterator instead of having this function > that returns a list? > > I think that would be cleaner to use in the tile manager. It could even be > allowed to iterate over the same pixel ref twice as the tile manager needs to > handle that properly anyhow. I'm not sure if that would be cleaner. The problem is that we gather pixels from a picture pile, and each pile gathers from possibly several pictures. For tile manager to use an iterator, it would have to be a picture pile iterator, which would need to iterate over each picture and gather a list anyway. I'm all for making this iterator, but I think it's somewhat orthogonal to this change. Wdyt?
On 2013/04/17 16:20:28, vmpstr wrote: > On 2013/04/17 00:42:56, David Reveman wrote: > > https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc > > File cc/resources/picture.cc (right): > > > > > https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc#n... > > cc/resources/picture.cc:26: const gfx::Size kRegionSize(512, 512); > > only POD is allowed for objects with static storage. you would have to make > this > > int kRegionWidth and int kRegionHeight.. > > Fixed. > > > > > > https://codereview.chromium.org/14230007/diff/12001/cc/resources/picture.cc#n... > > cc/resources/picture.cc:165: std::list<skia::LazyPixelRef*>& pixel_ref_list) { > > How about adding a Picture::PixelRefIterator instead of having this function > > that returns a list? > > > > I think that would be cleaner to use in the tile manager. It could even be > > allowed to iterate over the same pixel ref twice as the tile manager needs to > > handle that properly anyhow. > > I'm not sure if that would be cleaner. The problem is that we gather pixels from > a picture pile, and each pile gathers from possibly several pictures. For tile > manager to use an iterator, it would have to be a picture pile iterator, which > would need to iterate over each picture and gather a list anyway. I'm all for > making this iterator, but I think it's somewhat orthogonal to this change. Wdyt? Right, it would be PicturePileImpl::PixelRefIterator but it should be possible to implement it without any temporary lists. We use iterator classes for similar things in the compositor (ie. PictureLayerTilingSet::CoverageIterator) and I would prefer if we followed that convention here.
I changed the implementation to use the iterator style. As soon as a couple of other patches land that add picture and picture pile impl unittests, I'll add unittests for them, so feel free to wait until that is done. Otherwise, PTAL.
I like the direction of this patch. As you say, unit tests for the iterator are definitely needed. Some drive-by comments: https://codereview.chromium.org/14230007/diff/22001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/22001/cc/resources/picture.cc#n... cc/resources/picture.cc:213: Picture::LazyPixelRefsIterator::LazyPixelRefsIterator() What's the default ctor for? https://codereview.chromium.org/14230007/diff/22001/cc/resources/picture.cc#n... cc/resources/picture.cc:222: Picture::LazyPixelRefsIterator::LazyPixelRefsIterator( To be consistent with other code, could you call this PixelRefIterator? Generally data structures seem to be named in the style of ValueIterator or ValueMap, but not ValuesMap or ValuesIterator. https://codereview.chromium.org/14230007/diff/22001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/14230007/diff/22001/cc/resources/tile_manager... cc/resources/tile_manager.cc:648: while (pixel_ref_iter) { subjective style nit: use a for loop for iterators
I added unittests, please take a look. On 2013/04/22 16:47:26, enne wrote: > I like the direction of this patch. As you say, unit tests for the iterator are > definitely needed. > > Some drive-by comments: > > https://codereview.chromium.org/14230007/diff/22001/cc/resources/picture.cc > File cc/resources/picture.cc (right): > > https://codereview.chromium.org/14230007/diff/22001/cc/resources/picture.cc#n... > cc/resources/picture.cc:213: > Picture::LazyPixelRefsIterator::LazyPixelRefsIterator() > What's the default ctor for? PicturePileImpl iterator has Picture iterator as a member. In order to keep most of the work in operator++, I keep this uninitialized in the constructor, which is why I need the Picture iterator default ctor. > > https://codereview.chromium.org/14230007/diff/22001/cc/resources/picture.cc#n... > cc/resources/picture.cc:222: > Picture::LazyPixelRefsIterator::LazyPixelRefsIterator( > To be consistent with other code, could you call this PixelRefIterator? > Generally data structures seem to be named in the style of ValueIterator or > ValueMap, but not ValuesMap or ValuesIterator. > Changed to LazyPixelRefIterator. I'm guessing you just meant not to use the plural. If you meant that I don't need "Lazy", then please let me know. > https://codereview.chromium.org/14230007/diff/22001/cc/resources/tile_manager.cc > File cc/resources/tile_manager.cc (right): > > https://codereview.chromium.org/14230007/diff/22001/cc/resources/tile_manager... > cc/resources/tile_manager.cc:648: while (pixel_ref_iter) { > subjective style nit: use a for loop for iterators Changed. Please take a look at the indentation, since I'm not sure how to style this correctly.
Thanks! I have three more test requests for you: (1) Can you add a test for a Picture::LazyPixelRefIterator where the layer rect does not intersect the Picture's layer rect? (2) Can you add some tests where pictures are drawn exactly on tile boundaries with 1px width or 1px height to make sure there's not an off-by-one error in your math? (3) Can you add a test to PicturePileImpl::LazyPixelRefIterator where a pile has more than one picture and possibly where only the non-base picture has the pixel ref. Also for that case, test that a layer rect inside the base picture but outside the non-base picture doesn't come up with any pixel refs. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:115: const PixelRefsMap& lazy_pixel_refs) : PixelRefsMap => PixelRefMap? https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:231: layer_rect.y() - layer_rect_.y(), I think you mean 0? ;) Is the reason for this because of the translation applied at recording/raster time? https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu... Do you think it would be more clear if all pictures were left in layer coordinates, rather than being translated to some origin? https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:296: GatherPixelRefsFromSkia(rect, lazy_pixel_refs); If only Skia could just give us bounding rectangles directly... https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:313: Picture::LazyPixelRefIterator::LazyPixelRefIterator() Ok, this makes sense. Thanks for the default ctor test. :) https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:346: std::max(min_point_.x(), picture->min_lazy_pixel_cell_.x()), I think this is wrong. lazy_pixel_cell is in layer space. min_point has been recentered to have layer_rect.origin() at (0, 0). Can you add a test where this would be caught? (This increases my belief that we should not have that translation in record/raster, since it's a confusing complication about what space things are in.) https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... File cc/resources/picture_pile_impl_unittest.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... cc/resources/picture_pile_impl_unittest.cc:372: // Lazy pixel refs are found in the following cells: Can you use different bitmaps per cell? https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... cc/resources/picture_pile_impl_unittest.cc:506: SkBitmap lazy_bitmap; You do this a bunch. Maybe you could wrap the boilerplate of creating an SkBitmap with a lazy pixel ref in some helper function or class. https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager... cc/resources/tile_manager.cc:644: for (PicturePileImpl::LazyPixelRefIterator pixel_ref_iter( Looks fine to me. I think I generally indent 5 spaces for the other two statements to match up with the parenthesis in "for (", but I think what you've written is legal style. If you're not already using it, I highly recommend clang-format. It's not always right, but it normally does a good job even with these sorts of constructs.
https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:38: const int kRegionHeight = 512; Can we avoid these constants and use SkTileGridPicture::TileGridInfo instead? https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:314: : picture_(0), nit: indent 4 spaces https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:389: if (iter == picture_->lazy_pixel_refs_.end() || iter->second.empty()) can "iter->second.empty()" ever be true? https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:393: current_list_ = iter->second; Can we avoid copying this list while iterating? looks like making current_list_ a reference and adding std::list<skia::LazyPixelRef*>::iterator would allow you to do this. Being able to iterate over pixel refs without performing heap allocations would be nice.. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.h#ne... cc/resources/picture.h:38: std::list<skia::LazyPixelRef*> > PixelRefsMap; I think adding a "typedef std::pair<int, int> PixelRefsMapKey" would make the use of this map a bit cleaner. See PictureLayerTiling for an example. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:305: picture_list_ = map_iterator->second; Lets try to avoid copying a list here as well. https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager... cc/resources/tile_manager.cc:638: void TileManager::GatherPixelRefsForTile(Tile* tile) { How about we get rid of this function completely now that iterating over pixel refs is cheap? For the time being you probably need a |decoded_pixel_refs| set in ManagedTileState to make sure we don't get stuck trying to decode an image forever but that can go away once I land cancellation and reprioritization. https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager... cc/resources/tile_manager.cc:656: rendering_stats_instrumentation_->AddImageGathering(duration); These stats should now be collected in Picture::GatherAllPixelRefs() instead, right?
For this test: "(2) Can you add some tests where pictures are drawn exactly on tile boundaries with 1px width or 1px height to make sure there's not an off-by-one error in your math?" We're kind of a the mercy of skia to get the right thing in rect that we pass in. I added a test for single pixel rect query to ensure that the iterator calculations are correct, given that the pixel refs from skia are where I expect. I've added the other requested tests, and addressed the comments made. PTAL. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:38: const int kRegionHeight = 512; On 2013/04/22 19:56:06, David Reveman wrote: > Can we avoid these constants and use SkTileGridPicture::TileGridInfo instead? Done. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:115: const PixelRefsMap& lazy_pixel_refs) : On 2013/04/22 19:10:56, enne wrote: > PixelRefsMap => PixelRefMap? Done. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:231: layer_rect.y() - layer_rect_.y(), On 2013/04/22 19:10:56, enne wrote: > I think you mean 0? ;) No, it's actually a different rect... I renamed the argument one to "query_rect" to avoid confusion. > > Is the reason for this because of the translation applied at recording/raster > time? > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu... > > Do you think it would be more clear if all pictures were left in layer > coordinates, rather than being translated to some origin? Yes, that's the reason. If we get rid of the translates, then we have to tell skia that we need a picture and recording canvas of size layer+offset. I'm sure that would work fine (since memory isn't actually allocated here), but it feels like it is a worse solution than what we have now. I think the translates are fairly clean... Wdyt? https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:296: GatherPixelRefsFromSkia(rect, lazy_pixel_refs); On 2013/04/22 19:10:56, enne wrote: > If only Skia could just give us bounding rectangles directly... Yep. Maybe they will in the future :D https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:314: : picture_(0), On 2013/04/22 19:56:06, David Reveman wrote: > nit: indent 4 spaces Done. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:346: std::max(min_point_.x(), picture->min_lazy_pixel_cell_.x()), On 2013/04/22 19:10:56, enne wrote: > I think this is wrong. lazy_pixel_cell is in layer space. min_point has been > recentered to have layer_rect.origin() at (0, 0). Can you add a test where this > would be caught? This is actually in layer space, since I add layer_rect back in. The way I think about it is that all operations on cc::Picture are in layer space, and only when we want to deal with SkPicture, we translate to have origin at 0,0. The non-intersecting tests that I've added should catch these problems. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:389: if (iter == picture_->lazy_pixel_refs_.end() || iter->second.empty()) On 2013/04/22 19:56:06, David Reveman wrote: > can "iter->second.empty()" ever be true? Nope, I removed it. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.cc#n... cc/resources/picture.cc:393: current_list_ = iter->second; On 2013/04/22 19:56:06, David Reveman wrote: > Can we avoid copying this list while iterating? looks like making current_list_ > a reference and adding std::list<skia::LazyPixelRef*>::iterator would allow you > to do this. Being able to iterate over pixel refs without performing heap > allocations would be nice.. Done. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture.h#ne... cc/resources/picture.h:38: std::list<skia::LazyPixelRef*> > PixelRefsMap; On 2013/04/22 19:56:06, David Reveman wrote: > I think adding a "typedef std::pair<int, int> PixelRefsMapKey" would make the > use of this map a bit cleaner. See PictureLayerTiling for an example. Done. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:305: picture_list_ = map_iterator->second; On 2013/04/22 19:56:06, David Reveman wrote: > Lets try to avoid copying a list here as well. Done. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... File cc/resources/picture_pile_impl_unittest.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... cc/resources/picture_pile_impl_unittest.cc:372: // Lazy pixel refs are found in the following cells: On 2013/04/22 19:10:56, enne wrote: > Can you use different bitmaps per cell? Done. https://codereview.chromium.org/14230007/diff/29001/cc/resources/picture_pile... cc/resources/picture_pile_impl_unittest.cc:506: SkBitmap lazy_bitmap; On 2013/04/22 19:10:56, enne wrote: > You do this a bunch. Maybe you could wrap the boilerplate of creating an > SkBitmap with a lazy pixel ref in some helper function or class. Done. https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager... cc/resources/tile_manager.cc:638: void TileManager::GatherPixelRefsForTile(Tile* tile) { On 2013/04/22 19:56:06, David Reveman wrote: > How about we get rid of this function completely now that iterating over pixel > refs is cheap? > > For the time being you probably need a |decoded_pixel_refs| set in > ManagedTileState to make sure we don't get stuck trying to decode an image > forever but that can go away once I land cancellation and reprioritization. Let's leave that as a follow-up. I would prefer this patch doesn't extend too far past picture/picture_pile_impl https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager... cc/resources/tile_manager.cc:656: rendering_stats_instrumentation_->AddImageGathering(duration); On 2013/04/22 19:56:06, David Reveman wrote: > These stats should now be collected in Picture::GatherAllPixelRefs() instead, > right? I'm not sure. It depends what we want to gather. It's still useful to know how long it took to look-up the pixel refs (to ensure that it is, in fact, quick).
The complication of test #2 makes sense. Maybe I'm just lost in the number of tests, but I don't see what you added for #1 and #3 in the diff. Could you point me at it? I have a few more nitpicky comments, but this is looking great. Thanks for all the tests. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:36: int ToMultiple(int value, int multiple) { Should this be moved to cc/base/util.h as RoundDown, equivalent to RoundUp? https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:201: cell_size_ = gfx::Size( For what it's worth, this isn't going to line up with the tile grid. You're going to be off-by-one, and so will hit four tile grid cells for every one cell here. That said, I don't think it really matters. Honestly, any resolution that is about the average size of images will probably work out ok. Maybe you'll decode something you don't need, but most of the time some other high priority tile will need it and it won't be a waste. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:235: const gfx::Rect& query_rect, const gfx::Rect& => gfx::Rect https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:239: picture_.get(), SkRect::MakeXYWH(query_rect.x() - layer_rect_.x(), Haha. Sorry for misreading earlier. Calling it query_rect helps a lot. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:304: gfx::Size extent(std::min(cell_size_.width(), layer_rect_.right() - x), I think these lines would be more clearly put as: gfx::Rect rect(gfx::Point(x, y), cell_size_); rect.intersect(layer_rect_); https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:337: gfx::Rect rect, rect => query_rect? https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:360: // Limit the points to knows pixel ref boundaries. knows => known? https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture_pile... File cc/resources/picture_pile_impl.h (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture_pile... cc/resources/picture_pile_impl.h:62: float contents_scale, style nit: incorrect indentation
https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/14230007/diff/29001/cc/resources/tile_manager... cc/resources/tile_manager.cc:656: rendering_stats_instrumentation_->AddImageGathering(duration); a trace event is enough to check that this is quick. it only makes sense to collect stats for things that have a very variable cost based on page contents. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:201: cell_size_ = gfx::Size( I would vote for getting this to line up properly in this patch so we only hit one cell when using an identity transform. I think it's worth getting the common case as fast as possible on the cc thread as we'll be iterating over a large set of tiles and all their pixel refs inside AssignGpuMemoryToTiles in the near future. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:236: std::list<skia::LazyPixelRef*>& pixel_ref_list) { nit: while you're here. I think the style guide recommends passing return values by pointer rather than reference. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:381: current_list_iterator_ != current_list_->end()) { I think this would be cleaner if you used a sentinel node here instead of allowing current_list_ to be NULL. you can get rid of |current_pixel_ref_| and |current_list_iterator_| would be the current position instead of being one step ahead, which I find awkward. should also make this a bit more efficient. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:410: picture_->lazy_pixel_refs_.find(PixelRefMapKey(current_x_, current_y_)); nit: think it might be a bit cleaner to put "PixelRefMapKey key(current_x_, current_y_)" on a separate line. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.h#ne... cc/resources/picture.h:71: class CC_EXPORT LazyPixelRefIterator { Seem to be some inconsistency in when the term "lazy" is used. Some variables use it but others don't. I would prefer to use PixelRefIterator and remove usage of term lazy everywhere except skia::LazyPixelRef of course. Wdyt? https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture_pile... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:272: PicturePileImpl::LazyPixelRefIterator::operator++() { please consider using a sentinel node and get rid of |current_pixel_ref_| here too. https://codereview.chromium.org/14230007/diff/39002/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/tile_manager... cc/resources/tile_manager.cc:650: managed_tile_state.pending_pixel_refs.push_back(*pixel_ref_iter); please add a TODO about removing GatherPixelRefsForTile and |pending_pixel_refs|. https://codereview.chromium.org/14230007/diff/39002/cc/resources/tile_manager... cc/resources/tile_manager.cc:651: ++pixel_ref_iter; you're already incrementing iterator in the "for" statement.
Addressed most comments. PTAL. @enne: the tests you asked for are found in the following: Test 1 (non intersecting query rect) is at the end of PictureTest::PixelRefIteratorNonZeroLayer Test 2 is PictureTest::PixelRefIteratorOnePixelQuery (which also tests my adjustment to use -1 for width and height) Test 3 is PicturePileImplTest::PixelRefIteratorLazyRefsBaseNonLazy (one large bitmap that covers the whole layer with no lazy refs, and small bitmaps with lazy refs). https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:36: int ToMultiple(int value, int multiple) { On 2013/04/23 01:59:39, enne wrote: > Should this be moved to cc/base/util.h as RoundDown, equivalent to RoundUp? Done. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:201: cell_size_ = gfx::Size( On 2013/04/23 01:59:39, enne wrote: > For what it's worth, this isn't going to line up with the tile grid. You're > going to be off-by-one, and so will hit four tile grid cells for every one cell > here. > > That said, I don't think it really matters. Honestly, any resolution that is > about the average size of images will probably work out ok. Maybe you'll decode > something you don't need, but most of the time some other high priority tile > will need it and it won't be a waste. I think I fixed this. Right now, when we do gather pixels from skia, we do it for a rect x,y w by h. When computing the indecies, we can just use x,y w-1 by h-1 rect. That way, if the only overlap is the right/bottom boundary, then we will only check one index (but still capture refs in w by h). If the overlap is bigger than one pixel, then we need to (correctly) check more indecies. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:235: const gfx::Rect& query_rect, On 2013/04/23 01:59:39, enne wrote: > const gfx::Rect& => gfx::Rect Done. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:236: std::list<skia::LazyPixelRef*>& pixel_ref_list) { On 2013/04/23 14:33:24, David Reveman wrote: > nit: while you're here. I think the style guide recommends passing return values > by pointer rather than reference. Done. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:239: picture_.get(), SkRect::MakeXYWH(query_rect.x() - layer_rect_.x(), On 2013/04/23 01:59:39, enne wrote: > Haha. Sorry for misreading earlier. Calling it query_rect helps a lot. Yeah, I vote in general to avoid naming completely different things with names that differ only by an underscore :P https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:304: gfx::Size extent(std::min(cell_size_.width(), layer_rect_.right() - x), On 2013/04/23 01:59:39, enne wrote: > I think these lines would be more clearly put as: > > gfx::Rect rect(gfx::Point(x, y), cell_size_); > rect.intersect(layer_rect_); Done. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:337: gfx::Rect rect, On 2013/04/23 01:59:39, enne wrote: > rect => query_rect? Done. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:360: // Limit the points to knows pixel ref boundaries. On 2013/04/23 01:59:39, enne wrote: > knows => known? Oops. Fixed. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:381: current_list_iterator_ != current_list_->end()) { On 2013/04/23 14:33:24, David Reveman wrote: > I think this would be cleaner if you used a sentinel node here instead of > allowing current_list_ to be NULL. you can get rid of |current_pixel_ref_| and > |current_list_iterator_| would be the current position instead of being one step > ahead, which I find awkward. should also make this a bit more efficient. Done. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.cc#n... cc/resources/picture.cc:410: picture_->lazy_pixel_refs_.find(PixelRefMapKey(current_x_, current_y_)); On 2013/04/23 14:33:24, David Reveman wrote: > nit: think it might be a bit cleaner to put "PixelRefMapKey key(current_x_, > current_y_)" on a separate line. Done. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture.h#ne... cc/resources/picture.h:71: class CC_EXPORT LazyPixelRefIterator { On 2013/04/23 14:33:24, David Reveman wrote: > Seem to be some inconsistency in when the term "lazy" is used. Some variables > use it but others don't. I would prefer to use PixelRefIterator and remove usage > of term lazy everywhere except skia::LazyPixelRef of course. Wdyt? Agreed. I renamed it. https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture_pile... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:272: PicturePileImpl::LazyPixelRefIterator::operator++() { On 2013/04/23 14:33:24, David Reveman wrote: > please consider using a sentinel node and get rid of |current_pixel_ref_| here > too. This one I left as is. It's a bit harder in this case. First off, there are 3 iterators in play (picture::pixelref, tile, picture list). Then, having a sentinel object for things like picture::pixelref iterator is a little bit awkward, since the iterator itself knows when its done, not by comparing against .end as STL containers have it. I mean, I can try to fix it up, but I'm fairly confident it would be more messy than what this is now. Wdyt? https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture_pile... File cc/resources/picture_pile_impl.h (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/picture_pile... cc/resources/picture_pile_impl.h:62: float contents_scale, On 2013/04/23 01:59:39, enne wrote: > style nit: incorrect indentation Fixed. https://codereview.chromium.org/14230007/diff/39002/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/14230007/diff/39002/cc/resources/tile_manager... cc/resources/tile_manager.cc:650: managed_tile_state.pending_pixel_refs.push_back(*pixel_ref_iter); On 2013/04/23 14:33:24, David Reveman wrote: > please add a TODO about removing GatherPixelRefsForTile and > |pending_pixel_refs|. Done. https://codereview.chromium.org/14230007/diff/39002/cc/resources/tile_manager... cc/resources/tile_manager.cc:651: ++pixel_ref_iter; On 2013/04/23 14:33:24, David Reveman wrote: > you're already incrementing iterator in the "for" statement. That's a pretty big whoops. Thanks for catching it.
https://codereview.chromium.org/14230007/diff/54001/cc/base/util.h File cc/base/util.h (right): https://codereview.chromium.org/14230007/diff/54001/cc/base/util.h#newcode14 cc/base/util.h:14: template <typename T> T RoundDown(T value, T multiple) { nit: make the parameter names consistent with RoundUp https://codereview.chromium.org/14230007/diff/54001/cc/base/util.h#newcode16 cc/base/util.h:16: : (value / multiple) * multiple; RoundUp doesn't seem to handle negative numbers correctly so it's inconsistent that RoundDown does. I think you should fix RoundUp too if you need this. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... cc/resources/picture.cc:331: Picture::PixelRefIterator::PixelRefIterator(const PixelRefIterator& other) { why do you need this ctor? https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... cc/resources/picture.cc:387: if (current_y_ > max_point_.y()) { is this ever true? is the loop below not already catching it? https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... cc/resources/picture.cc:402: // does not intersect the layer rect. can this special condition be handled in the ctor instead of in every iteration of this loop? https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h#ne... cc/resources/picture.h:39: std::list<skia::LazyPixelRef*> > PixelRefMap; Sorry for changing my mind but as the set of lazy pixel refs is static how about we store them in a std::vector<skia::LazyPixelRef*> instead? That will be more compact, a bit more efficient to iterate over and clean up some of the iterator logic below. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h#ne... cc/resources/picture.h:88: std::list<skia::LazyPixelRef*>::const_iterator current_list_iterator_; if we use the std::vector idea I mentioned above this becomes something like: const std::vector<skia::LazyPixelRef*> empty_vector_; const std::vector<skia::LazyPixelRef*>* pixel_refs_; unsigned current_position_; https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture_pile... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:272: PicturePileImpl::PixelRefIterator::operator++() { I feel like this could be cleaner. It's hard to follow the flow of this function and I don't like that we have a current_pixel_ref_ and keep the iterators one step ahead of the current position. Maybe splitting it up into functions by doing something like this: bool PixelRefIterator::AdvanceToValidTile() { DCHECK(tile_iterator_); do { PictureListMap::const_iterator map_iter = picture_list_map_.find(tile_iterator_.index()); if (map_iter != picture_list_map_.end()) { picture_list_ = map_iter->second; picture_list_iterator_ = picture_list_->begin(); return true; } } while (++tile_iterator_); return false; } bool PixelRefIterator::AdvanceToValidPicture() { do { if (picture_list_iterator_ != picture_list_->end()) { pixel_ref_iterator_ = Picture::PixelRefIterator( layer_rect_, *picture_list_iterator_); return true; } ++tile_iterator_; } while (AdvanceToValidTile()); return false; } PixelRefIterator& PixelRefIterator::operator++() { ++pixel_ref_iterator_; do { if (pixel_ref_iterator_) return *this; ++picture_list_iterator_; } while (AdvanceToValidPicture()); return *this; } and we do something like the following in ctor rather than ++(*this): PixelRefIterator::PixelRefIterator() { if (AdvanceToValidTile()) { while (AdvanceToValidPicture()) { if (pixel_ref_iterator_) break; ++picture_list_iterator_; } } } Wdyt? https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture_pile... File cc/resources/picture_pile_impl.h (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture_pile... cc/resources/picture_pile_impl.h:75: Picture::PixelRefIterator picture_refs_iterator_; picture_refs_iterator_ is a confusing name. should this be picture_pixel_ref_iterator_ or just pixel_ref_iterator?
On 2013/04/23 20:02:23, vmpstr wrote: > Test 3 is PicturePileImplTest::PixelRefIteratorLazyRefsBaseNonLazy (one large > bitmap that covers the whole layer with no lazy refs, and small bitmaps with > lazy refs). Unless I'm misreading, this only has one picture. Multiple bitmaps, but only one picture. Given that you have this iterator logic, I wanted one test where the pile has a picture list with more than one picture in it.
PTAL. @enne: I completely misunderstood what you meant by the test. That was my bad. I've added a PicturePileImplTest.PixelRefIteratorMultiplePictures to cover the test you mentioned. https://codereview.chromium.org/14230007/diff/54001/cc/base/util.h File cc/base/util.h (right): https://codereview.chromium.org/14230007/diff/54001/cc/base/util.h#newcode14 cc/base/util.h:14: template <typename T> T RoundDown(T value, T multiple) { On 2013/04/24 14:10:52, David Reveman wrote: > nit: make the parameter names consistent with RoundUp Done. https://codereview.chromium.org/14230007/diff/54001/cc/base/util.h#newcode16 cc/base/util.h:16: : (value / multiple) * multiple; On 2013/04/24 14:10:52, David Reveman wrote: > RoundUp doesn't seem to handle negative numbers correctly so it's inconsistent > that RoundDown does. I think you should fix RoundUp too if you need this. Done. I also checked and the current uses of RoundUp only deal with positive numbers, so this doesn't affect existing code. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... cc/resources/picture.cc:331: Picture::PixelRefIterator::PixelRefIterator(const PixelRefIterator& other) { On 2013/04/24 14:10:52, David Reveman wrote: > why do you need this ctor? If the "other" iterator is pointing to the sentinel list, I want to ensure that we start pointing to our sentinel list, instead of "other" sentinel list. Hence, I have to do a different operator= (below), and a copy ctor as well. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... cc/resources/picture.cc:387: if (current_y_ > max_point_.y()) { On 2013/04/24 14:10:52, David Reveman wrote: > is this ever true? is the loop below not already catching it? This can be true. The loop below (the if) would only hit if x > max.x. If x <= max.x, then it would go through the whole loop of looking for the pixel ref. I think it's better (ie easier to reason about it) if we check it here. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... cc/resources/picture.cc:402: // does not intersect the layer rect. On 2013/04/24 14:10:52, David Reveman wrote: > can this special condition be handled in the ctor instead of in every iteration > of this loop? Done. I added an intersect check in the ctor. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h#ne... cc/resources/picture.h:39: std::list<skia::LazyPixelRef*> > PixelRefMap; On 2013/04/24 14:10:52, David Reveman wrote: > Sorry for changing my mind but as the set of lazy pixel refs is static how about > we store them in a std::vector<skia::LazyPixelRef*> instead? That will be more > compact, a bit more efficient to iterate over and clean up some of the iterator > logic below. Changed to vector. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h#ne... cc/resources/picture.h:88: std::list<skia::LazyPixelRef*>::const_iterator current_list_iterator_; On 2013/04/24 14:10:52, David Reveman wrote: > if we use the std::vector idea I mentioned above this becomes something like: > const std::vector<skia::LazyPixelRef*> empty_vector_; > const std::vector<skia::LazyPixelRef*>* pixel_refs_; > unsigned current_position_; I kept the current way.. With position, we'd have to index into a vector with each access. I think it's better, in terms of efficiency, to keep an iterator instead (although under the covers it's probably the same thing). https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture_pile... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:272: PicturePileImpl::PixelRefIterator::operator++() { On 2013/04/24 14:10:52, David Reveman wrote: > I feel like this could be cleaner. It's hard to follow the flow of this function > and I don't like that we have a current_pixel_ref_ and keep the iterators one > step ahead of the current position. Maybe splitting it up into functions by > doing something like this: > I played around with it, and settled to something that is very similar to your proposal. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture_pile... File cc/resources/picture_pile_impl.h (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture_pile... cc/resources/picture_pile_impl.h:75: Picture::PixelRefIterator picture_refs_iterator_; On 2013/04/24 14:10:52, David Reveman wrote: > picture_refs_iterator_ is a confusing name. should this be > picture_pixel_ref_iterator_ or just pixel_ref_iterator? Done.
Thanks for the extra test. lgtm if it also looks good to reveman.
https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... cc/resources/picture.cc:331: Picture::PixelRefIterator::PixelRefIterator(const PixelRefIterator& other) { If you use an index to iterate over each vector I think you can avoid all this with global base::LazyInstance<std::vector<skia::LazyPixelRef*> > as sentinel value. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... cc/resources/picture.cc:387: if (current_y_ > max_point_.y()) { hm, I still can't see how a valid call to operator++() can cause this to happen as you perform the "current_y_ > max_point_.y()" check whenever you increment current_y. It's a bug if operator++() is called after reaching the end and if you want to protect against that you should use a DCHECK instead. https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h#ne... cc/resources/picture.h:88: std::list<skia::LazyPixelRef*>::const_iterator current_list_iterator_; index or iterator would be the same for std::vector in terms of performance. the benefit of using an index is that you can set it to -1 and use an empty vector as sentinel value. https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:301: if (!tile_iterator_) nit: can this happen? remove or change to DCHECK if not. https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:306: picture_list_iterator_ != picture_list_->end(); nit: indent looks a bit off here.
On 2013/04/24 23:01:27, David Reveman wrote: > https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc > File cc/resources/picture.cc (right): > > https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... > cc/resources/picture.cc:331: Picture::PixelRefIterator::PixelRefIterator(const > PixelRefIterator& other) { > If you use an index to iterate over each vector I think you can avoid all this > with global base::LazyInstance<std::vector<skia::LazyPixelRef*> > as sentinel > value. > > https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.cc#n... > cc/resources/picture.cc:387: if (current_y_ > max_point_.y()) { > hm, I still can't see how a valid call to operator++() can cause this to happen > as you perform the "current_y_ > max_point_.y()" check whenever you increment > current_y. It's a bug if operator++() is called after reaching the end and if > you want to protect against that you should use a DCHECK instead. > This was being triggered in tests that test invalid operator increment, and in ctor (++(*this)). I've made the increment conditional, removed the tests, and made this into a DCHECK. > https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h > File cc/resources/picture.h (right): > > https://codereview.chromium.org/14230007/diff/54001/cc/resources/picture.h#ne... > cc/resources/picture.h:88: std::list<skia::LazyPixelRef*>::const_iterator > current_list_iterator_; > index or iterator would be the same for std::vector in terms of performance. the > benefit of using an index is that you can set it to -1 and use an empty vector > as sentinel value. > I've reworked this a bit. PTAL. > https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... > File cc/resources/picture_pile_impl.cc (right): > > https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... > cc/resources/picture_pile_impl.cc:301: if (!tile_iterator_) > nit: can this happen? remove or change to DCHECK if not. > > https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... > cc/resources/picture_pile_impl.cc:306: picture_list_iterator_ != > picture_list_->end(); > nit: indent looks a bit off here.
https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:301: if (!tile_iterator_) On 2013/04/24 23:01:27, David Reveman wrote: > nit: can this happen? remove or change to DCHECK if not. Only when incrementing an iterator that is invalid... I've made this into a DCHECK and removed the tests that increment invalid iterators https://codereview.chromium.org/14230007/diff/62001/cc/resources/picture_pile... cc/resources/picture_pile_impl.cc:306: picture_list_iterator_ != picture_list_->end(); On 2013/04/24 23:01:27, David Reveman wrote: > nit: indent looks a bit off here. I'm not really sure how to indent this :) I've added a space.
https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.cc#n... cc/resources/picture.cc:322: Picture::PixelRefIterator::empty_pixel_refs_; I'm not sure if this is the right spot for this within a file, so please let me know.
lgtm with nits https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.cc#n... cc/resources/picture.cc:322: Picture::PixelRefIterator::empty_pixel_refs_; On 2013/04/25 00:01:10, vmpstr wrote: > I'm not sure if this is the right spot for this within a file, so please let me > know. I would just stick it in an anonymous namespace at the top instead of having it be part of Picture::PixelRefIterator. This is fine if you prefer it though. https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... cc/resources/picture.h:78: DCHECK(current_index_ < current_pixel_refs_->size()); nit: use DCHECK_LT https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... cc/resources/picture.h:79: return (*current_pixel_refs_)[current_index_]; current_pixel_refs_->at(index) might be a bit cleaner but this is fine if you prefer it https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... cc/resources/picture.h:83: DCHECK(current_index_ < current_pixel_refs_->size()); nit: use DCHECK_LT https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... cc/resources/picture.h:96: unsigned int current_index_; nit: just "unsigned" is preferred
Thanks for the review! On 2013/04/25 00:36:28, David Reveman wrote: > lgtm with nits > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.cc > File cc/resources/picture.cc (right): > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.cc#n... > cc/resources/picture.cc:322: Picture::PixelRefIterator::empty_pixel_refs_; > On 2013/04/25 00:01:10, vmpstr wrote: > > I'm not sure if this is the right spot for this within a file, so please let > me > > know. > > I would just stick it in an anonymous namespace at the top instead of having it > be part of Picture::PixelRefIterator. This is fine if you prefer it though. > Isn't it non-POD? I'll keep it where it is, so it's clear what class is using it. > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h > File cc/resources/picture.h (right): > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... > cc/resources/picture.h:78: DCHECK(current_index_ < current_pixel_refs_->size()); > nit: use DCHECK_LT Done. > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... > cc/resources/picture.h:79: return (*current_pixel_refs_)[current_index_]; > current_pixel_refs_->at(index) might be a bit cleaner but this is fine if you > prefer it > std::vector::at apparently does bound checking and throws an exception, which might be undesirable. > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... > cc/resources/picture.h:83: DCHECK(current_index_ < current_pixel_refs_->size()); > nit: use DCHECK_LT > Done. > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... > cc/resources/picture.h:96: unsigned int current_index_; > nit: just "unsigned" is preferred Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14230007/73002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
On 2013/04/25 00:51:00, vmpstr wrote: > Thanks for the review! > > On 2013/04/25 00:36:28, David Reveman wrote: > > lgtm with nits > > > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.cc > > File cc/resources/picture.cc (right): > > > > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.cc#n... > > cc/resources/picture.cc:322: Picture::PixelRefIterator::empty_pixel_refs_; > > On 2013/04/25 00:01:10, vmpstr wrote: > > > I'm not sure if this is the right spot for this within a file, so please let > > me > > > know. > > > > I would just stick it in an anonymous namespace at the top instead of having > it > > be part of Picture::PixelRefIterator. This is fine if you prefer it though. > > > > Isn't it non-POD? I'll keep it where it is, so it's clear what class is using > it. LazyInstance is a POD-struct. > > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h > > File cc/resources/picture.h (right): > > > > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... > > cc/resources/picture.h:78: DCHECK(current_index_ < > current_pixel_refs_->size()); > > nit: use DCHECK_LT > > Done. > > > > > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... > > cc/resources/picture.h:79: return (*current_pixel_refs_)[current_index_]; > > current_pixel_refs_->at(index) might be a bit cleaner but this is fine if you > > prefer it > > > > std::vector::at apparently does bound checking and throws an exception, which > might be undesirable. good call. > > > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... > > cc/resources/picture.h:83: DCHECK(current_index_ < > current_pixel_refs_->size()); > > nit: use DCHECK_LT > > > > Done. > > > > https://codereview.chromium.org/14230007/diff/67001/cc/resources/picture.h#ne... > > cc/resources/picture.h:96: unsigned int current_index_; > > nit: just "unsigned" is preferred > > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14230007/87001
Message was sent while issue was closed.
Change committed as 196451 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
