Gecko rebuilds image keys every time something changes which forces webrender to do more work than it should and causes use texture cache unoptimally.

Before I delve into the meat of the topic I have a few renamings in progress to clarify a thing or two. The first one is renaming GetImageKey into something that doesn't look like a simple getter, since this implies expensive operations.

WebRenderCompositableHolder does more than just hold on to things. I settled on SyncImagePipelineManager which I think describes the role of this class more accurately.

Comment on attachment 8889492 [details] [diff] [review] Rename WebRenderCompositableHolder into AsyncImagePipelineManager Thank, it represents current implementation.

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3418fca78878 Rename GetImageKey into GenerateImageKey. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c82658a657 Rename WebRenderCompositableHolder into AsyncImagePipelineManager. r=sotaro

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=117375607&repo=mozilla-inbound

> backed out for bustage Unified builds strike again.

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6d0004a4d8 Backed out changeset f8c82658a657 https://hg.mozilla.org/integration/mozilla-inbound/rev/3330afeba84c Backed out changeset 3418fca78878 for bustage

Oops. This one got blindly renamed while search-n-replace'ing but the parameter is not the image pipeline manager, but a pipeline, so "aPipeline" makes more sense.

previous upload was missing the patch.

This mostly adds some missing variants of wr_api_update_image. Also removes the redundant wr_add_external_image_buffer which can be done with wr_add_external_image.

Proof of concept. This is not where I want to go but in the short term this lets me start using UpdateImage in places without colliding with Jerry's ongoing work. Once we have added transactions to webrender's API, I am thinking of simplifying this a lot (right now most of the code around AddImage and UpdateImage is more or less duplicated which isn't great).

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc8a44d772b Rename GetImageKey into GenerateImageKey. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/b580af52a231 Rename WebRenderCompositableHolder into AsyncImagePipelineManager. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/a85a338ca634 Fix unified build issue. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/b8847dab6f58 Changes to the ffi boundary. r=sotaro

https://hg.mozilla.org/mozilla-central/rev/fdc8a44d772b https://hg.mozilla.org/mozilla-central/rev/b580af52a231 https://hg.mozilla.org/mozilla-central/rev/a85a338ca634 https://hg.mozilla.org/mozilla-central/rev/b8847dab6f58

I am going to get back to this. In preamble, let's simplify the image key creating. we don't need to send callbacks around we just need to know the number of sub-textures (= number of image keys).

Comment on attachment 8910362 [details] [diff] [review] Simplify generating image keys for texture hosts Review of attachment 8910362 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, though there was one place that needed "return". ::: gfx/layers/wr/WebRenderTextureHost.cpp @@ +139,3 @@ > { > MOZ_ASSERT(mWrappedTextureHost); > + mWrappedTextureHost->NumSubTextures(); It needs to have "return".

> It needs to have "return". Eeek! good catch!

This adds the flexibility to update the image key instead of having to add new ones. ResourceUpdateQueue's add and update methods have the same signatures so I used a little function pointer trick which I am not super proud of but makes the code a lot less verbose and repetitive than my earlier prototype.

Most of the interesting stuff is here. This patch reorganizes the code a bit to simplify it and checks whether we can reuse the image keys instead of adding new ones every time.

I think that the name is more descriptive and less prone to confusion with the part where we push into the ResourceUpdateQueue.

Comment on attachment 8910847 [details] [diff] [review] Update the image keys whenever possible Review of attachment 8910847 [details] [diff] [review]: ----------------------------------------------------------------- :nical, can you answer the following comments? ::: gfx/layers/wr/AsyncImagePipelineManager.cpp @@ +215,5 @@ > + auto op = canUpdate ? TextureHost::UPDATE_IMAGE : TextureHost::ADD_IMAGE; > + Range<wr::ImageKey> keys(&aKeys[0], aKeys.Length()); > + wrTexture->PushResourceUpdates(aResources, op, keys, wrTexture->GetExternalImageKey()); > + > + return canUpdate; The function return false, if ImageKey is added. Is it as expected? If UpdateImageKeys() returns false, ApplyAsyncImages() does not add DisplayList item for the pipeline. It means image of pipeline is not rendered if Image is added. @@ +252,5 @@ > + } > + > + dSurf->Unmap(); > + > + return false; It always return false, is it intentional? It seems to cause a problem for non-external image rendering code path.

> Is it as expected? If UpdateImageKeys() returns false, ApplyAsyncImages() does not add DisplayList item for the pipeline. It means image of pipeline is not rendered if Image is added. This is intentional although it might not work properly yet (I am running into unrelated crash that prevent me from testing this thoroughly). The idea is that if we only update the image keys without creating /destroying any, we should be able to render with the previous display list just fine and not have to rebuild the scene in webrender. > It always return false, is it intentional? Oops, no. It should return false if we updated the image and true if created a new image key.

(In reply to Nicolas Silva [:nical] from comment #22) > > Is it as expected? If UpdateImageKeys() returns false, ApplyAsyncImages() does not add DisplayList item for the pipeline. It means image of pipeline is not rendered if Image is added. > > This is intentional although it might not work properly yet (I am running > into unrelated crash that prevent me from testing this thoroughly). The idea > is that if we only update the image keys without creating /destroying any, > we should be able to render with the previous display list just fine and not > have to rebuild the scene in webrender. In current implementation, when first ImageKey of Pipeline is generated by UpdateImageKeys() returns false, then ApplyAsyncImages() does not add it to DisplayList, then Image is not rendered by WebRender.

Comment on attachment 8910847 [details] [diff] [review] Update the image keys whenever possible Review of attachment 8910847 [details] [diff] [review]: ----------------------------------------------------------------- :nical, can you answer the following comments? ::: gfx/layers/wr/AsyncImagePipelineManager.cpp @@ +215,5 @@ > + auto op = canUpdate ? TextureHost::UPDATE_IMAGE : TextureHost::ADD_IMAGE; > + Range<wr::ImageKey> keys(&aKeys[0], aKeys.Length()); > + wrTexture->PushResourceUpdates(aResources, op, keys, wrTexture->GetExternalImageKey()); > + > + return canUpdate; The function return false, if ImageKey is added. Is it as expected? If UpdateImageKeys() returns false, ApplyAsyncImages() does not add DisplayList item for the pipeline. It means image of pipeline is not rendered if Image is added. @@ +252,5 @@ > + } > + > + dSurf->Unmap(); > + > + return false; It always return false, is it intentional? It seems to cause a problem for non-external image rendering code path. @@ +280,4 @@ > pipeline, > keys, > keysToDelete); > if (!updateDisplayList) { :nical, Is it implemented as you expected? UpdateImageKeys() returns canUpdate.

Sorry, only bottom part is new comment:(

passing booleans around caused a lot of confusion (for me at least) and some bugs that you spotted in the review. This version replaces the booleans by Maybe<TextureHost::ResourceUpdateOp> which makes the intent a lot clearer. > :nical, Is it implemented as you expected? UpdateImageKeys() returns canUpdate. That was another mistake of mine. Fixed in the new patch. > In current implementation, when first ImageKey of Pipeline is generated by UpdateImageKeys() returns false, then ApplyAsyncImages() does not add it to DisplayList, then Image is not rendered by WebRender. The first (successful) image of a pipeline should always cause the creation of a display list. we should be able to ensure that by always creating a display list if we add an image, and never updating if we didn't have an image previously (which wouldn't be possible). So when we update some image keys we know for sure that we have a display list already and it refers to these image keys.

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d11e27057a3 Simplify generating image keys for TextureHosts. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/fa524e77ef3e Make it possible to update TextureHost image keys. r=sotaro

In addition to simplifying the code (no need to buffer image key deletions for a frame any more, this should roughly divide by two the texture cache usage of video frames. I tested this with and without external images and it works well (modulo the problem of bug 1385314 which I still have).

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3f77db6b22 Backed out changeset fa524e77ef3e https://hg.mozilla.org/integration/mozilla-inbound/rev/5afcc2c8b6cd Backed out changeset 3d11e27057a3 for build bustage on OS X and Windows at gfx/layers/d3d11/TextureD3D11.cpp(1069). r=backout on a CLOSED TREE

Backed out for build bustage on OS X and Windows at gfx/layers/d3d11/TextureD3D11.cpp(1069): https://hg.mozilla.org/integration/mozilla-inbound/rev/5afcc2c8b6cde30476cda410726d00ba0ea61324 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3f77db6b22d578e48da2d0318fc62c74fd4d18 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fa524e77ef3e0496d052be067a87cc28db6659a0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132690096&repo=mozilla-inbound 13:07:44 INFO - z:/build/build/src/gfx/layers/d3d11/TextureD3D11.cpp(1069): error C2761: 'void mozilla::layers::DXGITextureHostD3D11::PushResourceUpdates(mozilla::wr::ResourceUpdateQueue &,mozilla::layers::TextureHost::ResourceUpdateOp,const mozilla::Range<mozilla::wr::ImageKey> &,const mozilla::wr::ExternalImageId &)': member function redeclaration not allowed 13:07:44 INFO - z:/build/build/src/gfx/layers/d3d11/TextureD3D11.cpp(1070): error C2447: '{': missing function header (old-style formal list?) 13:07:44 INFO - z:/build/build/src/gfx/layers/d3d11/TextureD3D11.cpp(1301): error C2761: 'void mozilla::layers::DXGIYCbCrTextureHostD3D11::PushResourceUpdates(mozilla::wr::ResourceUpdateQueue &,mozilla::layers::TextureHost::ResourceUpdateOp,const mozilla::Range<mozilla::wr::ImageKey> &,const mozilla::wr::ExternalImageId &)': member function redeclaration not allowed 13:07:44 INFO - z:/build/build/src/gfx/layers/d3d11/TextureD3D11.cpp(1302): error C2447: '{': missing function header (old-style formal list?) 13:07:44 INFO - z:/build/build/src/config/rules.mk:1063: recipe for target 'Unified_cpp_gfx_layers7.obj' failed 13:07:44 INFO - mozmake.EXE[5]: *** [Unified_cpp_gfx_layers7.obj] Error 2

Comment on attachment 8911132 [details] [diff] [review] Update the image keys whenever possible Looks good. Thanks!

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5bd03fc507 Simplify generating image keys for TextureHosts. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/a7da103dab00 Make it possible to update TextureHost image keys. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5aa364ea69 Update TextureHost image keys when posible. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/98c7b4d33f28 Rename TextureHost::PushExternalImage into PushDisplayItems. r=sotaro https://hg.mozilla.org/integration/mozilla-inbound/rev/594f4ea5d802 Delete image keys as soon as they are not used anymore. r=sotaro

https://hg.mozilla.org/mozilla-central/rev/8f5bd03fc507 https://hg.mozilla.org/mozilla-central/rev/a7da103dab00 https://hg.mozilla.org/mozilla-central/rev/5a5aa364ea69 https://hg.mozilla.org/mozilla-central/rev/98c7b4d33f28 https://hg.mozilla.org/mozilla-central/rev/594f4ea5d802