I see two issues.

1. cc's image decode controller cache is applied in ImageHijackCanvas::onDrawImage. In this case, the image is also used in SkCanvas::drawRect. The image is decoded and cached in skia when calling drawRect and image decode controller cache is not used.

2. When multiple tile worker threads want to decode the same image at the same time, threads block each other and the image is decoded multiple times.

That is odd.

1. I believe we had intended to pass flags to Skia to specifically tell it not to cache. Maybe we missed a spot.

2. I believe we changed things so that we would have one pass that decodes the images and a second pass that rasters the tiles.

Are we sure this isn't multiple images or an animated image?


I built ToT and ran a trace of reddit.com, which is attached. I may have seen what you mean. Look at CompositorTileWorker2/6 and CompositorTileWorker4/8 at 6300 ms. They are both decoding an image that is the same size. One seems to be blocked on the other to finish decoding. Both request frame 0.

They have different IDs, however. They also have different source and target sizes.

Deleted: trace_multiple_decode.json.gz 8.9 MB

trace_multiple_decode.json.gz 8.9 MB Download

> Are we sure this isn't multiple images or an animated image?

The website only contains one png image of this size. So it should be the same image.

> They have different IDs, however. They also have different source and target sizes.

I can only find the ID in CompositorTileWorker2/6. Seems no ID in CompositorTileWorker4/8?


I attached two test cases also showing this issue and corresponding traces. 

img.html: shows the same image at different sizes.
img2.html: shows image as background.

Look at CompositorTileWorker at 2240ms in the traces. Image is decoded multiple times.

Deleted: test_case.zip 9.0 KB

test_case.zip 9.0 KB Download

Deleted: trace_img.json.gz 1.5 MB

trace_img.json.gz 1.5 MB Download

Deleted: trace_img2.json.gz 1.6 MB

trace_img2.json.gz 1.6 MB Download

I see. Thank you for reducing it to a simplified example.
I believe I understand the concern now.

img.html has 2 copies of the image at different scales. Ideally, it would decode the image once and scale it twice. It looks like maybe we are decoding it and scaling it twice instead. That is the concern, right?

What is odd is img2.html. The image isn't different scales. It should decode once and reuse the cached version.


I'll investigate. Thank you!

I think this is a drawback in our image decode controllers. The problem is that the two images end up generating two different cache keys, since they are scaled to a different level. The process for actually decoding is to create a task that does a decode and scale.

The first thing the task does is to get the original decode (or look one up in the cache if available). Then we use the original decode to produce a scaled version.

Since this is running on multiple threads, both image cache keys begin work at the same time. The first one starts the decode of the original. When the second task starts, it doesn't find the decode in the cache yet (since it hasn't finished it), so it starts the decode as well. Unfortunately the decoders are serialized, so it blocks until the first task is done. Hence we get two decodes and two scales.

I think the solution here is to decouple the tasks into decode and scale so that the second scale can depend on the first decode as well. 

I agree 100%. In fact, while I was half-way through reading the first paragraph of your response I immediately thought "we probably need a decode phase and a scale phase (or something equivalent)".

This would explain the case where a non-scaled image is in the background as well. All 4 threads do not see a key in the cache and start a decode.



Out of curiosity, could we instead have something like an atomic compare-and-exchange for cache key entries? Could one thread request if a key exists and if it doesn't, claim ownership (cmpxchg)? That would allow us to not decode twice. However, it would mean 1.) false sharing on the key, and 2.) having a cache entry be in a not-yet-done state. #2 perhaps also means a lock and waiting. I'm not a fan of this. But I am mentioning it so we explore multiple ideas.


Having a decode phase and a scale phase doesn't have the false sharing or lock/waiting problem. I definitely prefer it.

Yeah I think that's an alternative worth investigating. The two phase approach suffers from lifetime ambiguity. That is, the original decode would have to be locked and it's unclear who would unlock it. It's probably not too bad, but I'm just mentioning this because this approach would also have some awkwardness.

I kind of mildly prefer switching this into two tasks to allow for possibility of cancelling tasks, but it's probably a bit more work to do.

Could the system which manages phases also manage lifetimes?

For example:
- Gather all the images to decode
- Lock them, issue decode tasks
- Once complete, issue scale tasks
- Once complete, unlock

Are locks atomic ref counts? So a tile could still hold a lock of a decoded image it uses?


A follow-up might be if we could not wait for all the decodes to complete before we begin issuing the scale tasks. I would love to not add a synchronization point.

There are some details that make it difficult to keep a lock for a long time without impacting peak memory.

For example, suppose you have a large image which is scaled to two small scales. The image is large enough that the original decode doesn't fit into the cache normally. What happens now is that we would process this decode "at raster" which means we lock it only for the duration of when it's needed and unlock it immediately after. With the two phase approach, this means it should be unlocked after both scales.

We don't want the tile to hold on to the lock of the original decode, because that means the originally locked image would remain locked for quite a bit longer (there may be multiple times that that depend on the scaled versions).

I think though what you're saying is that the scale tasks refs when created and unrefs when it's done. If that's the case, then yeah I think that would work.

#cblume, Have you replaced issue #1 with www.reddit.com?

I have not tried with reddit.com. However, I have thought about the impact of this in general.

Perhaps not many sites use repeated backgrounds or have the same image multiple times on a page. In those cases, the impact might be low. It is still important for us to fix, however.

BUT the impact is VERY HIGH when a site uses an atlas, which I bet reddit.com does. Atlases are a good thing and will cause this problem. For that reason, this is an important issue to address.

I am making this available since I won't be able to get to it soon.
I'll happily take it on if we do it next quarter.

Thank you again for finding, reporting, and reducing this bug!

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. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Can you triage this, vmpstr?

This is a known drawback of system when the initial image is at-raster. This case should be infrequent.. This also only impacts software, since gpu would use one raster thread so at raster images would appear sequentially.

I think it's still worth pursuing, but it isn't currently a priority. I'm happy to take a look when I have time.