Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11453014:
Implement the logic to kick off image decoding jobs for TileManager (Closed)
|
Created:
8 years ago by qinmin Modified:
8 years ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement the logic to kick off image decoding jobs for TileManager
BUG=163980
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172813
Patch Set 1 #
Total comments: 44
Patch Set 2 : addressing comments #
Total comments: 16
Patch Set 3 : addressing comments #Patch Set 4 : adding more null checks #
Total comments: 10
Patch Set 5 : using list instead of vector for faster deletion #Patch Set 6 : moving pending_pixel_refs to the persistent section #
Total comments: 16
Patch Set 7 : Fix crashes #Patch Set 8 : addressing comments #
Total comments: 14
Patch Set 9 : fixing nits #Patch Set 10 : adding trace events for Decode and GatherPixelRefs() call #Patch Set 11 : bugfix #Patch Set 12 : bug fix #Patch Set 13 : fix for win_rel trybot #
Messages
Total messages: 29 (0 generated)
PTAL, hopefully I understood everything correctly.
https://codereview.chromium.org/11453014/diff/1/cc/image.h File cc/image.h (right): https://codereview.chromium.org/11453014/diff/1/cc/image.h#newcode16 cc/image.h:16: class CC_EXPORT Image : public base::RefCountedThreadSafe<Image> { Does this need thread safe ref counting? https://codereview.chromium.org/11453014/diff/1/cc/image.h#newcode18 cc/image.h:18: static scoped_refptr<Image> Create(SkPixelRef* pixel_ref); nit: maybe a blank line between static and normal function declarations. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode165 cc/tile_manager.cc:165: decoding_progress_.clear(); ManageTiles() might be called while images are being decoded. I assume it would be incorrect to clear all image decoding progress in that case. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode211 cc/tile_manager.cc:211: decoding_progress_[*it] = NOT_STARTED; The pixel refs for a tile should never change, right? It seems unnecessary to gather them every time ManagedTiles() is called. Looking at the skia implementation, GatherPixelRefs can be a bit costly. It would be nice to not make this call until we know that the tile is going to be rasterized. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode350 cc/tile_manager.cc:350: } I think we're starting to do too much in this function. Could we add an utility function to make this easier to read? Maybe DispatchMoreImageDecodingTasks. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode354 cc/tile_manager.cc:354: bool TileManager::HasUndecodedImages(Tile* tile, Could we just remove SkPixelRefs from managed_state.pixel_refs as they are started? It looks to me like we could replace decoding_progress_ with a managed_state.pending_image_decoding_tasks member if we remove SkPixelRefs as we post decoding tasks. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode382 cc/tile_manager.cc:382: scoped_refptr<Image> image(Image::Create(pixel_ref)); Doesn't look like image need to be ref counted. Can you make this scoped_ptr<> or just use Image* and delete the image in OnImageDecoded(). https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode391 cc/tile_manager.cc:391: base::Unretained(this), Each decoding task is associated with a tile and decoding the tile region of pixel ref doesn't mean it's decoded for another tile region, right? Maybe you should add scoped_refptr<Tile> to OnImageDecoded parameters. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode122 cc/tile_manager.h:122: void SpawnImageDecodingTask(SkPixelRef*); Maybe DispatchOneImageDecodingTask/OnImageDecodingTaskCompleted to be consistent other functions in this class? https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode136 cc/tile_manager.h:136: typedef base::hash_map<uint32_t, scoped_refptr<Tile> > TileMap; where is TileMap used?
In general this seems good. I just want to see the potential thrashing problem documented either in the code or a bug. https://codereview.chromium.org/11453014/diff/1/cc/image.cc File cc/image.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/image.cc#newcode15 cc/image.cc:15: Image::Image(const skia::RefPtr<SkPixelRef>& pixel_ref) : I don't really know skia::RefPtr but it looks strange that a RefPtr needs const&. https://codereview.chromium.org/11453014/diff/1/cc/image.cc#newcode22 cc/image.cc:22: static_cast<skia::LazyPixelRef*>(pixel_ref_.get())->Decode(); Nat can give more comments about this interface. But what I think is that Decode() is an intermediate step. Right now Decode() is implemented to decode and then release resource. So you're not guaranteed that after Decode() the image will stay there. It's temporary in the sense that we're sloppy now but we'll eventually do: PrepareToDecode(); DispatchDecodeJobs(); DispatchRasterJob(); ReleaseDecodeResources(); This way we don't get random thrashing and redecoding. If we see this as an intermediate step then it's fine for me as long as there's comments for what we want to do later, or a bug filed to document this. https://codereview.chromium.org/11453014/diff/1/cc/image.cc#newcode27 cc/image.cc:27: return static_cast<skia::LazyPixelRef*>(pixel_ref_.get())->PrepareToDecode( I think WebKit code for PrepareToDecode leaks resource but it least it won't crash.. https://codereview.chromium.org/11453014/diff/1/cc/image.h File cc/image.h (right): https://codereview.chromium.org/11453014/diff/1/cc/image.h#newcode19 cc/image.h:19: void Decode(); Need some comments here. Decode() is to populate the cache with the decoded image. It is not guaranteed that the entry will stay there, since it can get evicted if there's too much decode. https://codereview.chromium.org/11453014/diff/1/cc/picture.cc File cc/picture.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/picture.cc#newcode95 cc/picture.cc:95: SkAutoDataUnref pixel_refs(SkPictureUtils::GatherPixelRefs( Not all pixelrefs are sklazypixelref. You might have to check for the url of these pixelrefs, those are lazy have urls of "lazy". Example: https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source... https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode388 cc/tile_manager.cc:388: base::Bind(&Image::Decode, This is fine now but just be aware that after this is done the cache is not guaranteed to stick. We can get more stats later to see how many re-decodings are happening. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode120 cc/tile_manager.h:120: bool HasUndecodedImages(Tile*, std::vector<SkPixelRef*>&); This seems to be called over and over again, does this do caching?
https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode354 cc/tile_manager.cc:354: bool TileManager::HasUndecodedImages(Tile* tile, I guess we still need a map with all the pixel refs currently being decoded so we don't call PrepareToDecode on a pixel ref currently being decoded. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode382 cc/tile_manager.cc:382: scoped_refptr<Image> image(Image::Create(pixel_ref)); Would it make sense to add a ScopedPtrHashMap<SkPixelRef*, Image> to hold images during decode and track what pixel refs are currently in use?
Lets not land this until we guarantee we wont starve due to cache evitction. E.g. if we decode, then we get back to kickign the raster, and its not decoded, then kick the raster job anyway. E.g. no double jeaopardy.
https://codereview.chromium.org/11453014/diff/1/cc/image.cc File cc/image.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/image.cc#newcode22 cc/image.cc:22: static_cast<skia::LazyPixelRef*>(pixel_ref_.get())->Decode(); I think we should make redecoding work. E.g. this should work: DispatchOneTile image1.PrepareToDecode()->False DispatchDecodeTask(image1) DecodeTask(image1) TileMgr::DidDecode(image1) DispatchTileThatUsesImage1 ImageDecodingStore::DiscardCacheFor(image1) RasterizeImage1Tile drawpicture internally locks picture and discovers no cache entry redecodes inline We can track how many inline redecodes we get, and see if we get it often. Iff and only iff we get them, we can discuss mitigation. IMO, we can avoid (possibly completely) in a lot of cases by opting prioritizing pending tiles over new decoding work in the dispatching logic. Similarly, we could also track on the dispatch side whether kicking another decode would cause an evict, and if so, handle it. This is a more heuristic way to deal with the problem you're worrying about Alpha, but it should be considerably simpler to maintain and less prone livelocks. https://codereview.chromium.org/11453014/diff/1/cc/picture_pile_impl.cc File cc/picture_pile_impl.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/picture_pile_impl.cc#newco... cc/picture_pile_impl.cc:58: std::vector<SkPixelRef*> PicturePileImpl::GatherPixelRefs( style nit, should we pass in the vector that we're gathering into? Returning vectors by value dupes them. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode18 cc/tile_manager.h:18: struct hash<SkPixelRef*> { Can we make SkLazyPixelRef have an id() field on it that we allocate in the same way as image id? IDs are super super useful for debugging and it sidesteps this stuff. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode82 cc/tile_manager.h:82: std::vector<SkPixelRef*> pixel_refs; hmm.... isn't it sufficient to track num_pending_decode_jobs here? When you pre-dispatch the tile, you bump this each time you enqueue a decode job or find a decode job pending for that pixel ref id. Then, all you need to know on the decode-ack side is to decrement this. When the counter reaches zero, the tile can be dispatched for raster. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode140 cc/tile_manager.h:140: PixelRefDecodingProgress decoding_progress_; All you need is IsDecodeTaskPending. Finished is not useful to you because it might already be evicted. pixel_ref_ids_with_pending_decode_task: map of ids. or hash map. But thats it.
also, whats the purpose of Image?
https://codereview.chromium.org/11453014/diff/1/cc/image.cc File cc/image.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/image.cc#newcode15 cc/image.cc:15: Image::Image(const skia::RefPtr<SkPixelRef>& pixel_ref) : Somehow this is commonly used in cc/, I am not sure why this is on purpose or just convention. On 2012/12/05 20:24:28, Alpha wrote: > I don't really know skia::RefPtr but it looks strange that a RefPtr needs > const&. https://codereview.chromium.org/11453014/diff/1/cc/image.cc#newcode22 cc/image.cc:22: static_cast<skia::LazyPixelRef*>(pixel_ref_.get())->Decode(); So we need a freeCachedPixelRef() in lazy_pixel_ref.h to allow the image decoder to purge the cache for a particular pixelref. But then we are facing the problem that the cache could grow out of control. On 2012/12/06 08:46:53, nduca wrote: > I think we should make redecoding work. E.g. this should work: > DispatchOneTile > image1.PrepareToDecode()->False > DispatchDecodeTask(image1) > > DecodeTask(image1) > > TileMgr::DidDecode(image1) > DispatchTileThatUsesImage1 > > ImageDecodingStore::DiscardCacheFor(image1) > > RasterizeImage1Tile > drawpicture > internally locks picture and discovers no cache entry > redecodes inline > > We can track how many inline redecodes we get, and see if we get it often. Iff > and only iff we get them, we can discuss mitigation. IMO, we can avoid (possibly > completely) in a lot of cases by opting prioritizing pending tiles over new > decoding work in the dispatching logic. Similarly, we could also track on the > dispatch side whether kicking another decode would cause an evict, and if so, > handle it. This is a more heuristic way to deal with the problem you're worrying > about Alpha, but it should be considerably simpler to maintain and less prone > livelocks. https://codereview.chromium.org/11453014/diff/1/cc/image.h File cc/image.h (right): https://codereview.chromium.org/11453014/diff/1/cc/image.h#newcode16 cc/image.h:16: class CC_EXPORT Image : public base::RefCountedThreadSafe<Image> { Not necessarily, just to ensure when passing it to raster threads it won't get deleted. removed ref counting. On 2012/12/05 19:48:46, David Reveman wrote: > Does this need thread safe ref counting? https://codereview.chromium.org/11453014/diff/1/cc/image.h#newcode18 cc/image.h:18: static scoped_refptr<Image> Create(SkPixelRef* pixel_ref); On 2012/12/05 19:48:46, David Reveman wrote: > nit: maybe a blank line between static and normal function declarations. Done. https://codereview.chromium.org/11453014/diff/1/cc/image.h#newcode19 cc/image.h:19: void Decode(); On 2012/12/05 20:24:28, Alpha wrote: > Need some comments here. > > Decode() is to populate the cache with the decoded image. It is not guaranteed > that the entry will stay there, since it can get evicted if there's too much > decode. Done. https://codereview.chromium.org/11453014/diff/1/cc/picture.cc File cc/picture.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/picture.cc#newcode95 cc/picture.cc:95: SkAutoDataUnref pixel_refs(SkPictureUtils::GatherPixelRefs( Good catch, fixed. On 2012/12/05 20:24:28, Alpha wrote: > Not all pixelrefs are sklazypixelref. You might have to check for the url of > these pixelrefs, those are lazy have urls of "lazy". > > Example: > https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source... https://codereview.chromium.org/11453014/diff/1/cc/picture_pile_impl.cc File cc/picture_pile_impl.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/picture_pile_impl.cc#newco... cc/picture_pile_impl.cc:58: std::vector<SkPixelRef*> PicturePileImpl::GatherPixelRefs( On 2012/12/06 08:46:53, nduca wrote: > style nit, should we pass in the vector that we're gathering into? Returning > vectors by value dupes them. Done. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode165 cc/tile_manager.cc:165: decoding_progress_.clear(); Since this variable is changed to keep track of all pending decoding tasks, removed this line here. On 2012/12/05 19:48:46, David Reveman wrote: > ManageTiles() might be called while images are being decoded. I assume it would > be incorrect to clear all image decoding progress in that case. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode211 cc/tile_manager.cc:211: decoding_progress_[*it] = NOT_STARTED; Changed this to only get the skpixelref information for tiles in NOW_BIN. Delayed it for other tiles until we start to kick off image decoding jobs. On 2012/12/05 19:48:46, David Reveman wrote: > The pixel refs for a tile should never change, right? It seems unnecessary to > gather them every time ManagedTiles() is called. Looking at the skia > implementation, GatherPixelRefs can be a bit costly. It would be nice to not > make this call until we know that the tile is going to be rasterized. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode350 cc/tile_manager.cc:350: } On 2012/12/05 19:48:46, David Reveman wrote: > I think we're starting to do too much in this function. Could we add an utility > function to make this easier to read? Maybe DispatchMoreImageDecodingTasks. Done. Now all the jobs waiting on image decoding goes to tiles_waiting_for_image_decoding_. When one image decoding task finishes, we check that queue first, before we check the remaining task in the tiles_that_need_to_be_rasterized_ queue. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode354 cc/tile_manager.cc:354: bool TileManager::HasUndecodedImages(Tile* tile, On 2012/12/05 19:48:46, David Reveman wrote: > Could we just remove SkPixelRefs from managed_state.pixel_refs as they are > started? It looks to me like we could replace decoding_progress_ with a > managed_state.pending_image_decoding_tasks member if we remove SkPixelRefs as we > post decoding tasks. we cannot remove SkPixelRef if the decoding task just started. Since if a decoding job finishes, we need that information to check whether all decoding tasks have finished. But we can remove a SkPixelRef from the pixel_refs when an image decoding is done. And we still need the decoding_progress_ as a new tile can figure out whether an image is currently in decoding, or has finished decoding. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode382 cc/tile_manager.cc:382: scoped_refptr<Image> image(Image::Create(pixel_ref)); Removed the image class. However, I am not sure whether skPixelRef* is safe to be posted on to the raster thread. Should we use scoped_refptr to get hold of a tile, or a picture before posting the task? On 2012/12/05 20:28:35, David Reveman wrote: > Would it make sense to add a ScopedPtrHashMap<SkPixelRef*, Image> to hold images > during decode and track what pixel refs are currently in use? https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode388 cc/tile_manager.cc:388: base::Bind(&Image::Decode, Yes, redecoding can be problematic if the cache got cleared before we put the tile onto rasterization. On 2012/12/05 20:24:28, Alpha wrote: > This is fine now but just be aware that after this is done the cache is not > guaranteed to stick. We can get more stats later to see how many re-decodings > are happening. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.cc#newcode391 cc/tile_manager.cc:391: base::Unretained(this), I think when we kick off the image decoder, we always decode the whole picture. Although image decoder can cache part of the picture. This is just to make the implementation easier. Otherwise, we have to have knowledge of whether another tile is decoding the same image, and what is the decoding region. So prepareToDecode() can return whether the particular region is cached, but decode() always decode the whole image. Also, this makes the life of image decoder much easier. Otherwise, 4 tiles may lauch 4 decoding tasks on the same image with different rects. On 2012/12/05 19:48:46, David Reveman wrote: > Each decoding task is associated with a tile and decoding the tile region of > pixel ref doesn't mean it's decoded for another tile region, right? Maybe you > should add scoped_refptr<Tile> to OnImageDecoded parameters. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode18 cc/tile_manager.h:18: struct hash<SkPixelRef*> { SkPixelRef has getGenerationId() call. But the reverse lookup is not there. I added a map in tilemanager to do this. On 2012/12/06 08:46:53, nduca wrote: > Can we make SkLazyPixelRef have an id() field on it that we allocate in the same > way as image id? IDs are super super useful for debugging and it sidesteps this > stuff. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode82 cc/tile_manager.h:82: std::vector<SkPixelRef*> pixel_refs; Then we need a map in TileManager to map from skpixelref to all tiles. Otherwise, when an image decoding job finishes, I have no ideas how many tiles this job would affect as it can impact more than 1 tiles. Currently I can loop over all tiles to find those affected tiles. On 2012/12/06 08:46:53, nduca wrote: > hmm.... isn't it sufficient to track num_pending_decode_jobs here? When you > pre-dispatch the tile, you bump this each time you enqueue a decode job or find > a decode job pending for that pixel ref id. > > Then, all you need to know on the decode-ack side is to decrement this. When the > counter reaches zero, the tile can be dispatched for raster. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode120 cc/tile_manager.h:120: bool HasUndecodedImages(Tile*, std::vector<SkPixelRef*>&); No, this just check if the tile has undecoded images. If so, we will wait for all the decoding jobs to finish before kicking off the raster task. Also, this function returns a list of skpixelRefs that we need to kick off the decoding job. On 2012/12/05 20:24:28, Alpha wrote: > This seems to be called over and over again, does this do caching? https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode122 cc/tile_manager.h:122: void SpawnImageDecodingTask(SkPixelRef*); On 2012/12/05 19:48:46, David Reveman wrote: > Maybe DispatchOneImageDecodingTask/OnImageDecodingTaskCompleted to be consistent > other functions in this class? Done. https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode136 cc/tile_manager.h:136: typedef base::hash_map<uint32_t, scoped_refptr<Tile> > TileMap; This is originally wrote to map SkPixelRef* to tiles, but i think i forgot to remove it. On 2012/12/05 19:48:46, David Reveman wrote: > where is TileMap used? https://codereview.chromium.org/11453014/diff/1/cc/tile_manager.h#newcode140 cc/tile_manager.h:140: PixelRefDecodingProgress decoding_progress_; changed it to pending_decode_tasks_ hashmap here. On 2012/12/06 08:46:53, nduca wrote: > All you need is IsDecodeTaskPending. Finished is not useful to you because it > might already be evicted. > > pixel_ref_ids_with_pending_decode_task: map of ids. or hash map. But thats it.
It seems unpredictable whether we call PrepareToDecode() to check if a pixel ref is decoded or not. Is there a better way to handle that? The current approach seems pretty racy. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/picture.cc File cc/picture.cc (right): https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/picture.cc#newc... cc/picture.cc:109: if (memcmp((*refs)->getURI(), labelLazyDecoded, should this be strcmp? memcmp is allowed to read past a '\0'. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/picture_pile_im... File cc/picture_pile_impl.cc (right): https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/picture_pile_im... cc/picture_pile_impl.cc:80: result.clear(); don't have to change this but I would have just put |result| in this scope instead of calling clear. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc File cc/tile_manager.cc (right): https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:295: // we will get the information later when decoding is about to start. Why not lazily get information for all types? https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:425: bool TileManager::HasUndecodedImages(Tile* tile, Would it be cleaner to change this function to DispatchImageDecodingTasksForTile? It could still return true if there was images that needed decoding but you wouldn't need the temporary |unstarted| vector. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:431: bool has_removed_pending_tasks = false; I think this nested loop is both hard to read and inefficient. can you please do this without nesting the loops. e.g. put pixel refs that should be remove in temporary list. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:455: TRACE_EVENT0("cc", "TileManager::SpawnImageDecodingTask"); TileManager::DispatchOneImageDecodingTask https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:463: // as mutable. SkPixelRef is owned by the SkPicture, right? Keeping the PicturePileImpl alive should guarantee that the SkPicture is alive so you should probably pass a pile ref to OnImageDecodingTaskCompleted. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:475: while (true) { this is similar to the function above but you have three nested loops here. please remove the outer most loop.
lgtm with @reveman for final review
https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/picture.cc File cc/picture.cc (right): https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/picture.cc#newc... cc/picture.cc:109: if (memcmp((*refs)->getURI(), labelLazyDecoded, ok, using strncmp instead On 2012/12/07 21:14:54, David Reveman wrote: > should this be strcmp? memcmp is allowed to read past a '\0'. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/picture_pile_im... File cc/picture_pile_impl.cc (right): https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/picture_pile_im... cc/picture_pile_impl.cc:80: result.clear(); On 2012/12/07 21:14:54, David Reveman wrote: > don't have to change this but I would have just put |result| in this scope > instead of calling clear. Done. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc File cc/tile_manager.cc (right): https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:295: // we will get the information later when decoding is about to start. So if an image covers several tiles(A,B,C,D) in NOW_BIN, and tile A kicks off for image decoding first, while B, C, D are not scheduled due to task limit. When A's decoding job finishes, B, C and D will have no idea that the same picture they are trying to decode just finishes. This is just to make a tradeoff between the cost of calling GetImageInformationForTile() and the cost of the above problem. On 2012/12/07 21:14:54, David Reveman wrote: > Why not lazily get information for all types? https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:425: bool TileManager::HasUndecodedImages(Tile* tile, ok, combined HasUndecodedImages() and DispatchMoreImageDecodingTasks into HasDispatchedImageDecodingTasks() On 2012/12/07 21:14:54, David Reveman wrote: > Would it be cleaner to change this function to > DispatchImageDecodingTasksForTile? It could still return true if there was > images that needed decoding but you wouldn't need the temporary |unstarted| > vector. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:431: bool has_removed_pending_tasks = false; On 2012/12/07 21:14:54, David Reveman wrote: > I think this nested loop is both hard to read and inefficient. can you please do > this without nesting the loops. e.g. put pixel refs that should be remove in > temporary list. Done. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:455: TRACE_EVENT0("cc", "TileManager::SpawnImageDecodingTask"); On 2012/12/07 21:14:54, David Reveman wrote: > TileManager::DispatchOneImageDecodingTask Done. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:463: // as mutable. ok, just holding a scoped_refptr of the tile until the reply function is called. On 2012/12/07 21:14:54, David Reveman wrote: > SkPixelRef is owned by the SkPicture, right? Keeping the PicturePileImpl alive > should guarantee that the SkPicture is alive so you should probably pass a pile > ref to OnImageDecodingTaskCompleted. https://chromiumcodereview.appspot.com/11453014/diff/10001/cc/tile_manager.cc... cc/tile_manager.cc:475: while (true) { On 2012/12/07 21:14:54, David Reveman wrote: > this is similar to the function above but you have three nested loops here. > please remove the outer most loop. Done.
When I tried the patch on Mac, the renderer always crash when calling into SkPictureUtils::GatherPixelRefs(). And there are no stacktraces showing up. Any idea about this, Mike?
On 2012/12/09 18:39:28, qinmin wrote: > When I tried the patch on Mac, the renderer always crash when calling into > SkPictureUtils::GatherPixelRefs(). And there are no stacktraces showing up. > Any idea about this, Mike? Never mind, the return value from SkPictureUtils::GatherPixelRefs() could contain null SkPixelRef* pointers. Adding more null checks fixes this. Now the patch works fine on my Mac.
There seems to be a problem that all the returned SkPixelRefs* from SkPictureUtils::GatherPixelRefs() are not lazy. Alpha, can you comment on this? On 2012/12/09 20:48:02, qinmin wrote: > On 2012/12/09 18:39:28, qinmin wrote: > > When I tried the patch on Mac, the renderer always crash when calling into > > SkPictureUtils::GatherPixelRefs(). And there are no stacktraces showing up. > > Any idea about this, Mike? > > Never mind, the return value from SkPictureUtils::GatherPixelRefs() could > contain null SkPixelRef* pointers. Adding more null checks fixes this. > Now the patch works fine on my Mac.
https://codereview.chromium.org/11453014/diff/19001/cc/picture.cc File cc/picture.cc (right): https://codereview.chromium.org/11453014/diff/19001/cc/picture.cc#newcode99 cc/picture.cc:99: std::vector<SkPixelRef*>& result) { should this be a std::vector<LazyPixelRef*> and we do the cast from SkPixelRef* to LazyPixelRef* here rather than in the tile manager? https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:418: continue; I'm confused. Doesn't this function have to return true if one of the pixel refs have a pending decode task? https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:433: pending_pixel_refs.erase(pending_pixel_refs.begin() + (*it)); removing elements from the middle vector is not very efficient. if we need to be able to remove elements, should we be using a different data structure? https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:489: tiles_waiting_for_image_decoding_.begin() + (*it)); again removing elements from the middle of vector. can we avoid this somehow? https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.h#newcode61 cc/tile_manager.h:61: std::vector<SkPixelRef*> pending_pixel_refs; Should these be in the persistent state group above?
https://codereview.chromium.org/11453014/diff/19001/cc/picture.cc File cc/picture.cc (right): https://codereview.chromium.org/11453014/diff/19001/cc/picture.cc#newcode99 cc/picture.cc:99: std::vector<SkPixelRef*>& result) { On 2012/12/10 17:58:12, David Reveman wrote: > should this be a std::vector<LazyPixelRef*> and we do the cast from SkPixelRef* > to LazyPixelRef* here rather than in the tile manager? Done. https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:418: continue; it should return false in that case, suppose the particular rect of this image hasn't been decoded. On 2012/12/10 17:58:12, David Reveman wrote: > I'm confused. Doesn't this function have to return true if one of the pixel refs > have a pending decode task? https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:433: pending_pixel_refs.erase(pending_pixel_refs.begin() + (*it)); Switched to list instead On 2012/12/10 17:58:12, David Reveman wrote: > removing elements from the middle vector is not very efficient. if we need to be > able to remove elements, should we be using a different data structure? https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:489: tiles_waiting_for_image_decoding_.begin() + (*it)); using list instead On 2012/12/10 17:58:12, David Reveman wrote: > again removing elements from the middle of vector. can we avoid this somehow? https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/19001/cc/tile_manager.h#newcode61 cc/tile_manager.h:61: std::vector<SkPixelRef*> pending_pixel_refs; On 2012/12/10 17:58:12, David Reveman wrote: > Should these be in the persistent state group above? Done.
Almost there. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:262: mts.has_image_decoding_info = false; Why do you clear this here? Shouldn't we just initialize has_image_decoding_info when the managed tile is constructed and be done. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:380: void TileManager::GetImageInformationForTile(Tile* tile) { I don't think having this in a separate function is worth much now that you only call it from one place. Just move this to HasImageDecodingTasks. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:420: tiles_have_image_decoding_tasks_.erase(it); do you need a temporary "to be removed" list here? https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:435: DispatchOneRasterTask(thread, tile); hm, thread might not be the correct thread to dispatch this to after HasImageDecodingTasks has been called. You should probably change this so it's similar to the above loop. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:440: bool TileManager::HasImageDecodingTasks(Tile* tile) { Can you change this to something like: void DispatchImageDecodingTasksForTile(Tile* tile) so it's clear that this function will dispatch tasks. We don't need the return value anymore, right? Can just check pending_pixel_refs.empty() directly in DispatchMoreTasks. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.h#newcode58 cc/tile_manager.h:58: std::list<skia::LazyPixelRef*> pending_pixel_refs; nit: can you make it obvious that these two members are related. maybe change has_image_decoding_info to need_to_gather_pixel_refs or just add a comment. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.h#newcode124 cc/tile_manager.h:124: // Tiles that are has image decoding tasks. I guess the proper name for the list is tiles_that_need_to_be_rasterized_but_have_image_decoding_tasks_ but that's awkwardly long. Can you just make it clear in this comment that these are tiles that need to be rasterized? https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.h#newcode125 cc/tile_manager.h:125: TileList tiles_have_image_decoding_tasks_; nit: tiles_that_have_image_decoding_tasks_ or tiles_with_image_decoding_tasks_.
https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:262: mts.has_image_decoding_info = false; moved it to the ctor On 2012/12/11 01:35:42, David Reveman wrote: > Why do you clear this here? Shouldn't we just initialize has_image_decoding_info > when the managed tile is constructed and be done. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:380: void TileManager::GetImageInformationForTile(Tile* tile) { On 2012/12/11 01:35:42, David Reveman wrote: > I don't think having this in a separate function is worth much now that you only > call it from one place. Just move this to HasImageDecodingTasks. Done. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:420: tiles_have_image_decoding_tasks_.erase(it); Changed the code to erase(it++). On 2012/12/11 01:35:42, David Reveman wrote: > do you need a temporary "to be removed" list here? https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:435: DispatchOneRasterTask(thread, tile); You are right, somehow i missed that when rewriting this On 2012/12/11 01:35:42, David Reveman wrote: > hm, thread might not be the correct thread to dispatch this to after > HasImageDecodingTasks has been called. You should probably change this so it's > similar to the above loop. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:440: bool TileManager::HasImageDecodingTasks(Tile* tile) { Done. On 2012/12/11 01:35:42, David Reveman wrote: > Can you change this to something like: > void DispatchImageDecodingTasksForTile(Tile* tile) > so it's clear that this function will dispatch tasks. We don't need the return > value anymore, right? Can just check pending_pixel_refs.empty() directly in > DispatchMoreTasks. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.h#newcode58 cc/tile_manager.h:58: std::list<skia::LazyPixelRef*> pending_pixel_refs; On 2012/12/11 01:35:42, David Reveman wrote: > nit: can you make it obvious that these two members are related. maybe change > has_image_decoding_info to need_to_gather_pixel_refs or just add a comment. Done. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.h#newcode124 cc/tile_manager.h:124: // Tiles that are has image decoding tasks. On 2012/12/11 01:35:42, David Reveman wrote: > I guess the proper name for the list is > tiles_that_need_to_be_rasterized_but_have_image_decoding_tasks_ but that's > awkwardly long. Can you just make it clear in this comment that these are tiles > that need to be rasterized? Done. https://codereview.chromium.org/11453014/diff/26002/cc/tile_manager.h#newcode125 cc/tile_manager.h:125: TileList tiles_have_image_decoding_tasks_; On 2012/12/11 01:35:42, David Reveman wrote: > nit: tiles_that_have_image_decoding_tasks_ or tiles_with_image_decoding_tasks_. Done.
Lets add something for this new metric into the rendering_stats structure. Eg decodingTime and numDecodeJobs. This could be a followup patch but lets do it promptly. And expose it on tools/perf/smoothness_benchmark.py
lgtm with nits https://codereview.chromium.org/11453014/diff/25002/cc/picture.cc File cc/picture.cc (right): https://codereview.chromium.org/11453014/diff/25002/cc/picture.cc#newcode118 cc/picture.cc:118: (*refs)->getURI(), labelLazyDecoded, sizeof(labelLazyDecoded))) { I don't think this makes a difference but sizeof(labelLazyDecoded) will include '\0'. You probably want to use strcmp or make sure you pass 4 as n. https://codereview.chromium.org/11453014/diff/25002/cc/picture_pile_impl.h File cc/picture_pile_impl.h (right): https://codereview.chromium.org/11453014/diff/25002/cc/picture_pile_impl.h#ne... cc/picture_pile_impl.h:8: #include <map> nit: add #include <list> https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:424: else { nit: curly braces around both IF and ELSE required https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:442: RasterThread* thread = 0; nit: |thread| is not needed in this scope. please move it to the scope below where it's used. https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:454: else { nit: curly braces around both IF and ELSE required https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.h#newcode123 cc/tile_manager.h:123: // Tiles that are has image decoding tasks. These tiles needs to be rasterized nit: s/that are has image decoding tasks/with image decoding tasks/ https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.h#newcode127 cc/tile_manager.h:127: typedef base::hash_map<uint32_t, skia::LazyPixelRef*> PixelRefMap; nit: add "base/hash_tables.h" to includes
https://codereview.chromium.org/11453014/diff/25002/cc/picture.cc File cc/picture.cc (right): https://codereview.chromium.org/11453014/diff/25002/cc/picture.cc#newcode118 cc/picture.cc:118: (*refs)->getURI(), labelLazyDecoded, sizeof(labelLazyDecoded))) { ok, passing n=4 On 2012/12/11 05:07:18, David Reveman wrote: > I don't think this makes a difference but sizeof(labelLazyDecoded) will include > '\0'. You probably want to use strcmp or make sure you pass 4 as n. https://codereview.chromium.org/11453014/diff/25002/cc/picture_pile_impl.h File cc/picture_pile_impl.h (right): https://codereview.chromium.org/11453014/diff/25002/cc/picture_pile_impl.h#ne... cc/picture_pile_impl.h:8: #include <map> On 2012/12/11 05:07:18, David Reveman wrote: > nit: add #include <list> Done. https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:424: else { On 2012/12/11 05:07:18, David Reveman wrote: > nit: curly braces around both IF and ELSE required Done. https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:442: RasterThread* thread = 0; On 2012/12/11 05:07:18, David Reveman wrote: > nit: |thread| is not needed in this scope. please move it to the scope below > where it's used. Done. https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:454: else { On 2012/12/11 05:07:18, David Reveman wrote: > nit: curly braces around both IF and ELSE required Done. https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.h#newcode123 cc/tile_manager.h:123: // Tiles that are has image decoding tasks. These tiles needs to be rasterized done, also fixed needs->need On 2012/12/11 05:07:18, David Reveman wrote: > nit: s/that are has image decoding tasks/with image decoding tasks/ https://codereview.chromium.org/11453014/diff/25002/cc/tile_manager.h#newcode127 cc/tile_manager.h:127: typedef base::hash_map<uint32_t, skia::LazyPixelRef*> PixelRefMap; On 2012/12/11 05:07:18, David Reveman wrote: > nit: add "base/hash_tables.h" to includes Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/11453014/40001
Presubmit check for 11453014-40001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
skia
Presubmit checks took 1.3s to calculate.
Mike, would you please take a look at this Change? I need OWNERs approval for skia/ On 2012/12/12 22:07:24, I haz the power (commit-bot) wrote: > Presubmit check for 11453014-40001 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for files in these directories: > skia > > Presubmit checks took 1.3s to calculate.
(blind review) lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/11453014/40001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/11453014/49002
Message was sent while issue was closed.
Change committed as 172813 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
