Change at-raster image decode to "at raster task" |
||||
Issue descriptionOne wrinkle for oop rasterization is that at raster uploads require a bit of a dance to figure out which ops have images and then to do the decode+raster for them before serializing the ops themselves. vmpstr suggests that maybe at raster (which are currently "per op") could be moved to be "per raster task", decoded with normal decode tasks (like other images), but unlocked immediately after the raster task is done (unlike other images). This might cause a little bit of a higher watermark, in case where one tile has two large at raster images, because both of these images will be locked at the same time. vmpstr suggests looking at ooms after such a change lands.
,
Sep 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/92d549f9ab9342c069a1c4c335eeccabd5ed93d1 commit 92d549f9ab9342c069a1c4c335eeccabd5ed93d1 Author: Adrienne Walker <enne@chromium.org> Date: Thu Sep 21 23:59:46 2017 cc: Decode "at raster" images at raster task start Currently, at raster images are decoded lazily from the PlaybackImageProvider when they are encountered during the raster process. The lock on the raster image is held only for the duration of the op itself. To simplify code and to help with oop serialization, this patch now holds the lock for these images across the entire task. This will potentially increase the memory high watermark for cases where there are a lot of images. In practice, there are very few at raster images, and so this should not be too impactful. A followup to this patch is to assert that no image is encountered that has not been predecoded (either via task or via this at raster synchronous decode) and also to modify PlaybackImageProvider to not do any decoding or uploading. However, these are more risky changes and so have been deferred. Bug: 762732 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ifec303c16b4d35c789f313445614d7cd59955a61 Reviewed-on: https://chromium-review.googlesource.com/676276 Reviewed-by: Khushal <khushalsagar@chromium.org> Commit-Queue: enne <enne@chromium.org> Cr-Commit-Position: refs/heads/master@{#503617} [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/benchmarks/rasterize_and_record_benchmark_impl.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/paint/image_provider.h [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/paint/paint_op_buffer.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/paint/paint_op_buffer_unittest.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/raster/playback_image_provider.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/raster/playback_image_provider.h [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/raster/playback_image_provider_unittest.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/raster/raster_buffer_provider_unittest.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/raster/raster_source.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/test/skia_common.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/test/skia_common.h [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/tiles/image_controller.cc [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/tiles/image_controller.h [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/tiles/image_decode_cache.h [modify] https://crrev.com/92d549f9ab9342c069a1c4c335eeccabd5ed93d1/cc/tiles/tile_manager.cc
,
Oct 26 2017
,
Oct 29
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 29
The followup work from the commit log in comment #2 turned out to be impossible, as we couldn't cleanly figure out which images were really needed until raster time. The predecode image set includes false positives. |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Sep 20 2017