New issue
Advanced search Search tips

Issue 665266 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Dec 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocking:
issue 597810
issue 670926



Sign in to add a comment

Async Image Updates on the compositor thread

Project Member Reported by khushals...@chromium.org, Nov 15 2016

Issue description

Currently changing the image content from the rastered tiles on the compositor thread requires a commit with new recordings replacing that image and the region for the image being invalidated by blink.

This is a common problem for allowing image decodes to not block activation of the pending tree, so a tile without the image content can be rastered and then be replaced with a re-rastered tile when the decode finishes. And to allow blimp to serialize and send a display list to the client without the images, on a low priority channel that does not block the compositor update. When the images arrive, we want the compositor to invalidate and re-raster tiles for which the image content is now available.

I think the changes we need for both these cases is:

1) Build a map for image -> region in the DiscardableImageMap as we build the RTree right now.

2) Allow skipping images during raster. This will be used for both images with deferred decode (checker-imaging) or images with no content available (blimp).

3) Merge invalidations on the pending tree for images skipped on the last frame. The merge step should basically take the set of images skipped in the last frame and present in the current frame and add the invalidations for them to pending tree's invalidation.
 
The changes above are the ones which I think will be common. We need other changes on the blimp side to update the set of images the compositor can use as they arrive.

And lots more for checker-imaging in cc. :D
Blocking: 597810
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc85c1aa0881047c4410cbedc1e4543bcf789a1f

commit fc85c1aa0881047c4410cbedc1e4543bcf789a1f
Author: khushalsagar <khushalsagar@chromium.org>
Date: Thu Nov 17 11:21:06 2016

cc: Build Image to Region map during image meta-data generation.

During the discardable image meta-data generation phase, we build an
Rtree to pull the set of images given a rect. This change adds a
reverse lookup map to be built at the same time for querying the region
for an image in a DisplayItemList.

BUG= 665266 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2496283004
Cr-Commit-Position: refs/heads/master@{#432841}

[modify] https://crrev.com/fc85c1aa0881047c4410cbedc1e4543bcf789a1f/cc/playback/discardable_image_map.cc
[modify] https://crrev.com/fc85c1aa0881047c4410cbedc1e4543bcf789a1f/cc/playback/discardable_image_map.h
[modify] https://crrev.com/fc85c1aa0881047c4410cbedc1e4543bcf789a1f/cc/playback/discardable_image_map_unittest.cc
[modify] https://crrev.com/fc85c1aa0881047c4410cbedc1e4543bcf789a1f/cc/playback/display_item_list.cc
[modify] https://crrev.com/fc85c1aa0881047c4410cbedc1e4543bcf789a1f/cc/playback/display_item_list.h

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/654417a5699ed0965a94977350266bf5ce206daa

commit 654417a5699ed0965a94977350266bf5ce206daa
Author: vmiura <vmiura@chromium.org>
Date: Sat Dec 03 21:10:01 2016

Revert of cc: Build Image to Region map during image meta-data generation. (patchset #6 id:100001 of https://codereview.chromium.org/2496283004/ )

Reason for revert:
Computing these image regions adds significant time to the critical path on image heavy content.  Let's re-land after addressing the performance.

BUG= 670926 

Original issue's description:
> cc: Build Image to Region map during image meta-data generation.
>
> During the discardable image meta-data generation phase, we build an
> Rtree to pull the set of images given a rect. This change adds a
> reverse lookup map to be built at the same time for querying the region
> for an image in a DisplayItemList.
>
> BUG= 665266 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
>
> Committed: https://crrev.com/fc85c1aa0881047c4410cbedc1e4543bcf789a1f
> Cr-Commit-Position: refs/heads/master@{#432841}

TBR=vmpstr@chromium.org,khushalsagar@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 665266 

Review-Url: https://codereview.chromium.org/2551583003
Cr-Commit-Position: refs/heads/master@{#436174}

[modify] https://crrev.com/654417a5699ed0965a94977350266bf5ce206daa/cc/playback/discardable_image_map.cc
[modify] https://crrev.com/654417a5699ed0965a94977350266bf5ce206daa/cc/playback/discardable_image_map.h
[modify] https://crrev.com/654417a5699ed0965a94977350266bf5ce206daa/cc/playback/discardable_image_map_unittest.cc
[modify] https://crrev.com/654417a5699ed0965a94977350266bf5ce206daa/cc/playback/display_item_list.cc
[modify] https://crrev.com/654417a5699ed0965a94977350266bf5ce206daa/cc/playback/display_item_list.h

Hmm it's kind of surprising that this is such a big regression ( issue 670926 ). Perhaps we need to have a simple region here (or a vector of rects)
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 6 2016

Labels: merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c006456e1c0057757977351133fd7bb2e5275b90

commit c006456e1c0057757977351133fd7bb2e5275b90
Author: Victor Miura <vmiura@chromium.org>
Date: Tue Dec 06 00:06:03 2016

Revert of cc: Build Image to Region map during image meta-data generation. (patchset #6 id:100001 of https://codereview.chromium.org/2496283004/ )

Reason for revert:
Computing these image regions adds significant time to the critical path on image heavy content.  Let's re-land after addressing the performance.

BUG= 670926 

Original issue's description:
> cc: Build Image to Region map during image meta-data generation.
>
> During the discardable image meta-data generation phase, we build an
> Rtree to pull the set of images given a rect. This change adds a
> reverse lookup map to be built at the same time for querying the region
> for an image in a DisplayItemList.
>
> BUG= 665266 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
>
> Committed: https://crrev.com/fc85c1aa0881047c4410cbedc1e4543bcf789a1f
> Cr-Commit-Position: refs/heads/master@{#432841}

TBR=vmpstr@chromium.org,khushalsagar@chromium.org
BUG= 665266 

Review-Url: https://codereview.chromium.org/2551583003
Cr-Commit-Position: refs/heads/master@{#436174}
(cherry picked from commit 654417a5699ed0965a94977350266bf5ce206daa)

Review URL: https://codereview.chromium.org/2552903002 .

Cr-Commit-Position: refs/branch-heads/2924@{#347}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/c006456e1c0057757977351133fd7bb2e5275b90/cc/playback/discardable_image_map.cc
[modify] https://crrev.com/c006456e1c0057757977351133fd7bb2e5275b90/cc/playback/discardable_image_map.h
[modify] https://crrev.com/c006456e1c0057757977351133fd7bb2e5275b90/cc/playback/discardable_image_map_unittest.cc
[modify] https://crrev.com/c006456e1c0057757977351133fd7bb2e5275b90/cc/playback/display_item_list.cc
[modify] https://crrev.com/c006456e1c0057757977351133fd7bb2e5275b90/cc/playback/display_item_list.h

Blocking: 670926
Status: WontFix (was: Assigned)
I'll close this bug for now. Vlad and I discussed the considerations for performing this on the impl thread with respect to the information required for cases where we can not checkerboard an image. For instance animated images and progressively loaded images.

Once we raster a frame with an image in these cases, we don't want to checker it in the subsequent frame and currently cc does not have enough information plumbed through to make these decisions. For progressively loaded images for instance, we see each update as a new SkImage in cc. So given that checker-imaging is the only use-case now, its worth investigating how these cases should be handled. And whether there are substantial gains with performing this completely on the impl thread as opposed to having blink request a decode and generating a new picture once its complete. Its better to address these in a doc before moving forward with this implementation.

Sign in to add a comment