The SkImage can now be destroyed on any thread. See this comment:

https://bugs.chromium.org/p/chromium/issues/detail?id=905337#c24

Assigning to penghuang@.

We should be able to cache any promise images for SharedImages. As long as a SharedImage is alive, the metadata is static (color space, size, pixel format).

I believe that this is true for other external resources as well (e.g. anything with a mailbox), but I am less confident there. Technically, you can put any texture in a mailbox (ProduceTextureDirectCHROMIUM), so there is no guarantee of immutability based on mailbox ID.

I discussed with egdaniel@.
Seems we can not cache promise SkImage(at least we can not cache it for Vulkan). SKIA thinks the contents in a promise SkImage cannot be changed.

Yes Skia always assumes that an SkImage is immutable. Therefore for a given promise SkImage, we expect that every time fulfill is called we will be given a texture that has the exact same contents.

The reason being is that we may have made a copy of this texture (e.g. if we are doing mip maps and the original promise didn't have mips). We cache that copy and use it for future uses of the SkImage.

So it is not safe to fulfill a promise image with a backend texture. Then wrap that same backend texture in an SkSurface. Draw to the surface, and then fulfill the promise a second time with the same backend texture.

However, reading this issue, the first comment seems to be talking about caching long lived tile content. If this is long lived, not changing tile content then it should be cached in an SkImage.

How about fulfill the promise image with a different backend texture? Will Skia discard the cache and use the new backend?  

The will not work either. We make the decisions on how we are drawing way before we get to a point where we would call fulfill. Technically, by the contract, you could fulfill with a different backend texture, but if you want to guarantee correct rendering the new texture must have identical pixels to the original.

As Greg says, fulfilling with a different texture is OK if the contents are the same.

If the texture is recycled for use with new tile contents we can reuse the GrBackendTexture but must create a new promise SkImage.

FWIW I'm working on making it so that promise images are created with a thin wrapper around GrBackendTexture rather than GrBackendTexture itself. If this wrapper is held on to and use to repeatedly fulfill SkImages then we should avoid some internal cost of recreating GrTextures for each fulfillment. At first this will only avoid GrTexture recreation if it is used to refulfill the same SkImage but I plan to extend it so can avoid GrTexture recreation also in the case where the backend texture fulfills a different promise SkImage (new tile contents).

When we OOP raster a tile, we may permit partial updates to that tile.

https://cs.chromium.org/chromium/src/cc/raster/gpu_raster_buffer_provider.cc?rcl=c544b4fd47ba647a51fcebb3dd4e690554848377&l=400

Permitting partial updates is useful to things like highlight hover and cursor blink.

We could probably taint the tile to indicate that the contents have changed and we need a new promise image.

I dug deeper. I'm convinced that we only partial raster into tiles that we have returned to the renderer. The relevant call site on the display compositor side is DisplayResourceProvider::DeleteAndReturnUnusedResourcesToChild

This is what I would expect to be true: we don't want the display compositor drawing a tile while a child (renderer) is updating it's contents.

What this means is that we could have a simple cache of promise images:
- if no promise image for resource, create a new promise image
- if resource returned to child, remove associated promise image

I would expect this cache to be roughly the same size of the displayed quads. We tend to be pretty aggressive about returning resources to children to free up GPU mem.

It looks like SurfaceAggregator is driving the return of the tiles to the child in SurfaceAggregator::PrewalkTree. That method will call  DisplayResourceProvider::DeclareUsedResourcesFromChild. All unused resources are eligible to be returned to the child (e.g. tiles not present in the next frame).

+cc enne

enne: Can you confirm comment #10 to keep me honest?

w/o DDL we do similar caching in display resource provider: https://cs.chromium.org/chromium/src/components/viz/service/display/display_resource_provider.cc?sq=package:chromium&g=0&l=837

We keep around a resource id to SkImage map and clear that in DeleteAndReturnUnusedResourcesToChild.

To do similar things would require display resource provider calling some functions on SkiaOutputSurface. Though those functions would be similar use to SharedBitmapManager or ContextProvider.

Comment #10 sounds correct to me.  For gpu/oop raster, we only partial raster into tiles that have been returned for exactly that reason.  (For software raster we partial raster into local buffers.)

This sounds like a reasonable approach to me.  At worst, if a resource gets returned and then resent you'll recreate the promise image, but I'm not sure that there's currently any information about whether it's the same as before or not, so maybe that's fine.

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

commit 65bd1d204d66444f0bf202dae9aa5ce08907ef37
Author: Peng Huang <penghuang@chromium.org>
Date: Thu Jan 10 17:03:41 2019

Cache SkImage for SkiaRenderer + DDL code path.

Bug:  914474 
Change-Id: Id32bae52750096a86092ca845b07097bae0cd822
Reviewed-on: https://chromium-review.googlesource.com/c/1404057
Reviewed-by: weiliangc <weiliangc@chromium.org>
Commit-Queue: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621614}
[modify] https://crrev.com/65bd1d204d66444f0bf202dae9aa5ce08907ef37/components/viz/service/display/display_resource_provider.cc
[modify] https://crrev.com/65bd1d204d66444f0bf202dae9aa5ce08907ef37/components/viz/service/display/display_resource_provider.h
[modify] https://crrev.com/65bd1d204d66444f0bf202dae9aa5ce08907ef37/components/viz/service/display/display_resource_provider_unittest.cc
[modify] https://crrev.com/65bd1d204d66444f0bf202dae9aa5ce08907ef37/components/viz/service/display/skia_renderer.cc
[modify] https://crrev.com/65bd1d204d66444f0bf202dae9aa5ce08907ef37/components/viz/service/display/skia_renderer.h

I believe that this is fixed.