Closed Bug 1383786 Opened 3 years ago Closed 2 years ago

Refactor ImageKey management

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

(Whiteboard: [wr-mvp])

Attachments

(10 files, 3 obsolete files)

8.71 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
46.14 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
2.82 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
973 bytes, patch
sotaro
: review+
Details | Diff | Splinter Review
9.48 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
17.66 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
26.95 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
20.03 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
15.15 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
7.30 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
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.
Attachment #8889490 - Flags: review?(sotaro.ikeda.g)
WebRenderCompositableHolder does more than just hold on to things. I settled on SyncImagePipelineManager which I think describes the role of this class more accurately.
Attachment #8889492 - Flags: review?(sotaro.ikeda.g)
Attachment #8889490 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8889492 [details] [diff] [review]
Rename WebRenderCompositableHolder into AsyncImagePipelineManager

Thank, it represents current implementation.
Attachment #8889492 - Flags: review?(sotaro.ikeda.g) → review+
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
Keywords: leave-open
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=117375607&repo=mozilla-inbound
Flags: needinfo?(nical.bugzilla)
> backed out for bustage

Unified builds strike again.
Flags: needinfo?(nical.bugzilla)
Attached patch Fix unified build issue (obsolete) — Splinter Review
Attachment #8889814 - Flags: review?(sotaro.ikeda.g)
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.
Attachment #8889815 - Flags: review?(sotaro.ikeda.g)
Attached patch unified-buildSplinter Review
previous upload was missing the patch.
Attachment #8889814 - Attachment is obsolete: true
Attachment #8889814 - Flags: review?(sotaro.ikeda.g)
Attachment #8889848 - Flags: review?(sotaro.ikeda.g)
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.
Attachment #8889880 - Flags: review?(sotaro.ikeda.g)
Attachment #8889815 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8889848 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8889880 - Flags: review?(sotaro.ikeda.g) → review+
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
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
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).
Attachment #8910362 - Flags: review?(sotaro.ikeda.g)
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".
Attachment #8910362 - Flags: review?(sotaro.ikeda.g) → review+
> 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.
Attachment #8910846 - Flags: review?(sotaro.ikeda.g)
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.
Attachment #8890830 - Attachment is obsolete: true
Attachment #8910847 - Flags: review?(sotaro.ikeda.g)
I think that the name is more descriptive and less prone to confusion with the part where we push into the ResourceUpdateQueue.
Attachment #8910848 - Flags: review?(sotaro.ikeda.g)
Attachment #8910848 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8910846 - Flags: review?(sotaro.ikeda.g) → review+
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.
Attachment #8910847 - Flags: review?(sotaro.ikeda.g)
> 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.
Attachment #8910847 - Attachment is obsolete: true
Attachment #8911132 - Flags: review?(sotaro.ikeda.g)
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).
Attachment #8911161 - Flags: review?(sotaro.ikeda.g)
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
Flags: needinfo?(nical.bugzilla)
Attachment #8911161 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8911132 [details] [diff] [review]
Update the image keys whenever possible

Looks good. Thanks!
Attachment #8911132 - Flags: review?(sotaro.ikeda.g) → review+
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
Depends on: 1403439
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: needinfo?(nical.bugzilla)
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1407069
Depends on: 1412755
Duplicate of this bug: 1397752
You need to log in before you can comment on or make changes to this bug.