Share shareable image surfaces instead of copying

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

(Blocks 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 unaffected, firefox58 fixed)

Details

(Whiteboard: [wr-mvp])

Attachments

(8 attachments, 45 obsolete attachments)

8.35 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
27.87 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
18.45 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.33 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.16 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.58 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.30 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.85 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
Assignee

Description

2 years ago
imagelib should allocate shared memory buffers for decoded images, and WebRenderImageLayer should opt to share those buffers instead of copying.
Assignee

Updated

2 years ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Assignee

Updated

2 years ago
Depends on: 1331938
Blocks: webrender
Assignee

Comment 1

2 years ago
Posted patch Switch to shared buffers, v1 (obsolete) — Splinter Review
Assignee

Comment 2

2 years ago
Posted patch Switch to shared buffers, v2 (obsolete) — Splinter Review
Rebase. Minor tweaks.
Attachment #8828868 - Attachment is obsolete: true
Assignee

Comment 3

2 years ago
Posted patch Switch to shared buffers, v3 (obsolete) — Splinter Review
Rebase and fix how the mContainer check was moved to the wrong place.
Attachment #8828917 - Attachment is obsolete: true
Assignee

Comment 4

2 years ago
Posted patch Switch to shared buffers, v4 (obsolete) — Splinter Review
Remove legacy code from earlier attempt at patch, minor refactor to make functions consistent within imgFrame.
Attachment #8829406 - Attachment is obsolete: true
Assignee

Comment 5

2 years ago
Posted patch Switch to shared buffers, v5 (obsolete) — Splinter Review
Rebase onto new patches in bug 1331938.
Attachment #8829407 - Attachment is obsolete: true
Assignee

Comment 6

2 years ago
Posted patch Switch to shared buffers, v6 (obsolete) — Splinter Review
Attachment #8829437 - Attachment is obsolete: true
Assignee

Comment 7

2 years ago
Posted patch Switch to shared buffers, v7 (obsolete) — Splinter Review
Rebase. Will follow up with breaking this patch down into individually landable/reviewable parts.
Attachment #8830814 - Attachment is obsolete: true
Assignee

Comment 8

2 years ago
Attachment #8832106 - Attachment is obsolete: true
Assignee

Updated

2 years ago
See Also: → 1330530
Assignee

Updated

2 years ago
Attachment #8832112 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Attachment #8832113 - Flags: review?(jmuizelaar)
Assignee

Comment 11

2 years ago
Add pref to control usage of shared vs volatile memory buffers in imagelib.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93decd84914e9c1a8af15df9963b9c2c20d7810b
Attachment #8832114 - Attachment is obsolete: true
Attachment #8832429 - Flags: review?(tnikkel)
Assignee

Comment 12

2 years ago
Hopefully fix Windows/Mac/Linux static analysis build failures.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eeec666958e18dce3e4795593b084e955fa0e01
Attachment #8832113 - Attachment is obsolete: true
Attachment #8832113 - Flags: review?(jmuizelaar)
Attachment #8832442 - Flags: review?(jmuizelaar)
Comment on attachment 8832112 [details] [diff] [review]
Part 1. Add shared image APIs to CompositorBridge, v1

Review of attachment 8832112 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +2299,5 @@
> +}
> +
> +mozilla::ipc::IPCResult
> +CompositorBridgeParentBase::RecvRemapSharedImage(const uint64_t& aId,
> +                                                 const ipc::SharedMemoryBasic::Handle& aHandle)

Drive-by comment: there's a potential security threat with these kinds of things, because one hijacked content process can spoof the aId belonging to another content process and basically replace images at will. We've had to guard against that sort of thing in the previous compositor APIs and we'll probably need to do it here as well. See bug 1258238 which has a more detailed explanation (which reminds me, I should poke seth on that...)
Comment on attachment 8832429 [details] [diff] [review]
Part 3. Decode images to shared surfaces for WebRender, v2

Review of attachment 8832429 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +4466,5 @@
>  
> +#ifdef MOZ_ENABLE_WEBRENDER
> +// Decodes images into shared memory to allow direct use in separate
> +// rendering processes.
> +pref("image.mem.shared", true);

Also please make sure this works for build with --enable-webrender but where gfx.webrender.enabled is false. You can set gfx.webrender.enabled to false and open a new window to have that window composited using not-webrender and exercise this code in that window. Sadly this codepath is not yet covered in our automated testing, but maybe I should add that.
Assignee

Comment 15

2 years ago
Fix builds again.
Attachment #8832442 - Attachment is obsolete: true
Attachment #8832442 - Flags: review?(jmuizelaar)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> Drive-by comment: there's a potential security threat with these kinds of
> things, because one hijacked content process can spoof the aId belonging to
> another content process and basically replace images at will. We've had to
> guard against that sort of thing in the previous compositor APIs and we'll
> probably need to do it here as well. See bug 1258238 which has a more
> detailed explanation (which reminds me, I should poke seth on that...)

After some clarification on IRC I don't believe this is an issue with the patch as written. The implementation on the parent side is in the base class, which means the CrossProcessCompositorBridgeParent handling the requests never delegates it to a CompositorBridgeParent. Therefore the map is isolated per-process and one content process shouldn't be able to affect another one.
Assignee

Updated

2 years ago
Attachment #8832112 - Flags: review?(jmuizelaar)
Assignee

Comment 17

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> > Drive-by comment: there's a potential security threat with these kinds of
> > things, because one hijacked content process can spoof the aId belonging to
> > another content process and basically replace images at will. We've had to
> > guard against that sort of thing in the previous compositor APIs and we'll
> > probably need to do it here as well. See bug 1258238 which has a more
> > detailed explanation (which reminds me, I should poke seth on that...)
> 
> After some clarification on IRC I don't believe this is an issue with the
> patch as written. The implementation on the parent side is in the base
> class, which means the CrossProcessCompositorBridgeParent handling the
> requests never delegates it to a CompositorBridgeParent. Therefore the map
> is isolated per-process and one content process shouldn't be able to affect
> another one.

Reviewing the code again to figure out *why* I felt I didn't need to forward the messages from CrossProcessCompositorBridgeParent to another CompositorBridgeParent is because at this moment, CrossProcessCompositorBridgeParent::AllocPWebRenderBridgeParent [1] gives *itself* as the WebRenderBridgeParent's CompositorBridgeParent [2] object it should refer back to. I use this in the part 2 patch in WebRenderBridgeParent::ProcessWebrenderCommands to convert a shared image ID in the display list to its internal representation.

[1] https://hg.mozilla.org/projects/graphics/annotate/c3133d426341/gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp#l221
[2] https://hg.mozilla.org/projects/graphics/annotate/f5c3c0d9169f/gfx/layers/wr/WebRenderBridgeParent.cpp#l79

If that were to change, then we will need to revisit the IDs and storage.
Assignee

Comment 18

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Comment on attachment 8832429 [details] [diff] [review]
> Part 3. Decode images to shared surfaces for WebRender, v2
> 
> Review of attachment 8832429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/init/all.js
> @@ +4466,5 @@
> >  
> > +#ifdef MOZ_ENABLE_WEBRENDER
> > +// Decodes images into shared memory to allow direct use in separate
> > +// rendering processes.
> > +pref("image.mem.shared", true);
> 
> Also please make sure this works for build with --enable-webrender but where
> gfx.webrender.enabled is false. You can set gfx.webrender.enabled to false
> and open a new window to have that window composited using not-webrender and
> exercise this code in that window. Sadly this codepath is not yet covered in
> our automated testing, but maybe I should add that.

I will test the scenarios manually but it is designed to work every which way, although not necessarily as efficiently.

1) SourceSurfaceVolatileData + non-WebRender => current behaviour on mozilla-central
2) SourceSurfaceSharedData + non-WebRender => slightly less efficient memory allocations, but should otherwise work the same as #1; it will release the shareable handles as soon as it discovers this is the case on the RasterImage::Draw call
3) SourceSurfaceVolatileData + WebRender => current behaviour on graphics, will incur a copy for image layers
4) SourceSurfaceSharedData + WebRender => avoids copy when using image layers; it will also release the shareable handles if the image is kept in a painted layer (as deduced if we see a RasterImage::Draw call)
Assignee

Updated

2 years ago
Attachment #8832112 - Flags: review?(jmuizelaar)
Assignee

Comment 20

2 years ago
Comment on attachment 8832460 [details] [diff] [review]
Part 2. Add special handling for shared surfaces in image layers, v3

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f83ea01abeb4a7cedb661c2a75a4032ff79702e5

Builds seem to working now for all platforms.
Attachment #8832460 - Flags: review?(jmuizelaar)
(In reply to Andrew Osmond [:aosmond] from comment #18)
> 4) SourceSurfaceSharedData + WebRender => avoids copy when using image
> layers; it will also release the shareable handles if the image is kept in a
> painted layer (as deduced if we see a RasterImage::Draw call)

Some image consumers don't use Draw(). canvas.drawImage will use GetFrame I think. Border image might now also.
Attachment #8832429 - Flags: review?(tnikkel) → review+
Assignee

Comment 22

2 years ago
Add mechanism to allow off main thread callers to SourceSurfaceSharedData::Finalize and SourceSurfaceSharedData::FinishedSharing. Technically those calls worked off main thread but that meant the handle could get freed underneath WebRenderImageLayer::TryShareSurface. This is needed to handle the feedback from part 3, where canvas may get the image data through RasterImage::GetFrame/GetFrameAtSize where we need to free the handle in that case.
Attachment #8832460 - Attachment is obsolete: true
Attachment #8832460 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Attachment #8835084 - Flags: review?(jmuizelaar)
Assignee

Comment 23

2 years ago
Handle imgFrame::GetFrame/GetFrameAtSize appropriately.
Attachment #8832429 - Attachment is obsolete: true
Attachment #8835086 - Flags: review+
Assignee

Updated

2 years ago
Attachment #8835086 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Summary: Switch to shared buffers in imagelib → Share shareable image surfaces instead of copying
Assignee

Comment 24

2 years ago
Rebase.
Attachment #8835084 - Attachment is obsolete: true
Attachment #8835084 - Flags: review?(jmuizelaar)
Attachment #8839930 - Flags: review?(jmuizelaar)
Assignee

Comment 25

2 years ago
Rebase.
Attachment #8832112 - Attachment is obsolete: true
Attachment #8832112 - Flags: review?(jmuizelaar)
Attachment #8842951 - Flags: review?(jmuizelaar)
Assignee

Comment 26

2 years ago
Rebase.
Attachment #8839930 - Attachment is obsolete: true
Attachment #8839930 - Flags: review?(jmuizelaar)
Attachment #8842952 - Flags: review?(jmuizelaar)
Assignee

Comment 27

2 years ago
Rebase.
Attachment #8842952 - Attachment is obsolete: true
Attachment #8842952 - Flags: review?(jmuizelaar)
Attachment #8843272 - Flags: review?(jmuizelaar)
Assignee

Comment 28

2 years ago
Rebase.
Attachment #8842951 - Attachment is obsolete: true
Attachment #8842951 - Flags: review?(jmuizelaar)
Attachment #8846608 - Flags: review?(jmuizelaar)
Assignee

Comment 29

2 years ago
Rebase. Update new mask layer rendering added in bug 1341565 to also use the shared surfaces instead of using external images if possible.
Attachment #8843272 - Attachment is obsolete: true
Attachment #8843272 - Flags: review?(jmuizelaar)
Attachment #8846609 - Flags: review?(jmuizelaar)
Assignee

Comment 30

2 years ago
Attachment #8846608 - Attachment is obsolete: true
Attachment #8846608 - Flags: review?(jmuizelaar)
Attachment #8857503 - Flags: review?(jmuizelaar)
Assignee

Comment 31

2 years ago
Refactor due to changes from bug 1355678.
Attachment #8846609 - Attachment is obsolete: true
Attachment #8846609 - Flags: review?(jmuizelaar)
Attachment #8857504 - Flags: review?(jmuizelaar)
Comment on attachment 8857503 [details] [diff] [review]
Part 1. Add shared image APIs to CompositorBridge, v4

Review of attachment 8857503 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorBridgeChild.cpp
@@ +1161,5 @@
>  }
>  
> +uint64_t
> +CompositorBridgeChild::GenerateSharedImageId()
> +{

I believe we can use GetNextResourceId() instead of this now.

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +2387,5 @@
> +  if (NS_WARN_IF(!shmem->Map(len))) {
> +    return IPC_FAIL_NO_REASON(this);
> +  }
> +
> +  shmem->Protect(static_cast<char*>(shmem->memory()), len, RightsRead);

It seems like Map() should at least optionally take permissions so that we don't have to mprotect after we map. Want to fix this in the shared memory code?
Comment on attachment 8857504 [details] [diff] [review]
Part 2. Add special handling for shared surfaces in image layers, v9

Review of attachment 8857504 [details] [diff] [review]:
-----------------------------------------------------------------

It would be good to know why this needs to interact with the compositor bridge and instead of the webrender bridge.

::: gfx/layers/wr/WebRenderImageLayer.cpp
@@ +96,5 @@
> +  RefPtr<CompositorBridgeChild> mBridge;
> +  uint64_t mId;
> +};
> +
> +typedef AutoTArray<WrImageLayerUserDataEntry, 1> WrImageLayerUserData;

It seems like we don't need support for multiple bridges.
Attachment #8857504 - Flags: review?(jmuizelaar) → review-
Assignee

Updated

2 years ago
Depends on: 1356289
Assignee

Comment 34

2 years ago
Rebase and update based on feedback.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> > Drive-by comment: there's a potential security threat with these kinds of
> > things, because one hijacked content process can spoof the aId belonging to
> > another content process and basically replace images at will. We've had to
> > guard against that sort of thing in the previous compositor APIs and we'll
> > probably need to do it here as well. See bug 1258238 which has a more
> > detailed explanation (which reminds me, I should poke seth on that...)
> 
> After some clarification on IRC I don't believe this is an issue with the
> patch as written. The implementation on the parent side is in the base
> class, which means the CrossProcessCompositorBridgeParent handling the
> requests never delegates it to a CompositorBridgeParent. Therefore the map
> is isolated per-process and one content process shouldn't be able to affect
> another one.

In any event, good news, Remap got eliminated in this patch :).

(In reply to Jeff Muizelaar [:jrmuizel] from comment #32)
> Comment on attachment 8857503 [details] [diff] [review]
> Part 1. Add shared image APIs to CompositorBridge, v4
> 
> Review of attachment 8857503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorBridgeChild.cpp
> @@ +1161,5 @@
> >  }
> >  
> > +uint64_t
> > +CompositorBridgeChild::GenerateSharedImageId()
> > +{
> 
> I believe we can use GetNextResourceId() instead of this now.
> 

Currently GetNextResourceId lives in WebRenderBridgeChild. If we can move that to CompositorBridgeChild then I can switch over easily. For now I updated the code to mirror CompositorBridgeChild::GetNextExternalImageId which also got moved. 

> ::: gfx/layers/ipc/CompositorBridgeParent.cpp
> @@ +2387,5 @@
> > +  if (NS_WARN_IF(!shmem->Map(len))) {
> > +    return IPC_FAIL_NO_REASON(this);
> > +  }
> > +
> > +  shmem->Protect(static_cast<char*>(shmem->memory()), len, RightsRead);
> 
> It seems like Map() should at least optionally take permissions so that we
> don't have to mprotect after we map. Want to fix this in the shared memory
> code?

Done in bug 1356289.
Attachment #8857503 - Attachment is obsolete: true
Attachment #8857503 - Flags: review?(jmuizelaar)
Attachment #8859280 - Flags: review?(jmuizelaar)
Assignee

Comment 35

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #33)
> Comment on attachment 8857504 [details] [diff] [review]
> Part 2. Add special handling for shared surfaces in image layers, v9
> 
> Review of attachment 8857504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would be good to know why this needs to interact with the compositor
> bridge and instead of the webrender bridge.
> 
> ::: gfx/layers/wr/WebRenderImageLayer.cpp
> @@ +96,5 @@
> > +  RefPtr<CompositorBridgeChild> mBridge;
> > +  uint64_t mId;
> > +};
> > +
> > +typedef AutoTArray<WrImageLayerUserDataEntry, 1> WrImageLayerUserData;
> 
> It seems like we don't need support for multiple bridges.

Removed. This was left over from when I used WebRenderBridgeChild/Parent to store the state information, because there can be multiple of those (e.g. one for each tab/window at the time of writing). However since they can live in the same content process, it made no sense to share the image twice, so I switched it to use CompositorBridgeChild/Parent, but didn't remove the logic to handle multiple bridges.
Attachment #8857504 - Attachment is obsolete: true
Attachment #8859281 - Flags: review?(jmuizelaar)
See Also: → 1355702
Comment on attachment 8859281 [details] [diff] [review]
Part 2. Add special handling for shared surfaces in image layers, v10

Review of attachment 8859281 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderImageLayer.cpp
@@ +349,5 @@
>    }
>  
>    WrImageKey key = GetImageKey();
> +  if (sharedImageId) {
> +    WrBridge()->AddWebRenderParentCommand(OpAddSharedImage(sharedImageId, key));

As I said offline I think this is close enough to retaining the image that we might as well just do that before landing.
Attachment #8859281 - Flags: review?(jmuizelaar) → review-
Assignee

Updated

2 years ago
Depends on: 1358014
Assignee

Comment 37

2 years ago
Attachment #8859280 - Attachment is obsolete: true
Attachment #8859280 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Attachment #8862843 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Attachment #8862844 - Flags: review?(jmuizelaar)
Comment on attachment 8862844 [details] [diff] [review]
Part 2. Add special handling for shared surfaces in image layers, v11

Review of attachment 8862844 [details] [diff] [review]:
-----------------------------------------------------------------

Some comment when I checked the patches.

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +481,5 @@
> +        mActiveKeys.Put(wr::AsUint64(key), key);
> +
> +        wr::ImageDescriptor descriptor(data->mSize, data->mStride, data->mFormat);
> +        auto slice = Range<uint8_t>(data->GetData(), data->GetDataLength());
> +        mApi->AddImage(key, descriptor, slice);

This api cause image copy twice in webrender. It might be better to add support of ExternalBuffer as a next step in a different bug.

::: gfx/layers/wr/WebRenderImageLayer.cpp
@@ +111,5 @@
> +
> +  void UpdateBridge(CompositorBridgeChild* aBridge)
> +  {
> +    Unused << NS_WARN_IF(ReleaseId());
> +    mBridge = aBridge;

Doesn't it cause to leak SharedImageData in WebRenderBridgeParent, since mBridge->SendRemoveSharedImage(mId) is called only to latest mBridge?

@@ +186,5 @@
> +}
> +
> +bool
> +WebRenderImageLayer::TryShared(Image* aImage)
> +{

Don't we need to handle a case that ImageKey for ExternalImageId(for CompositableClient) is allocated already?

It seems helpful to make clear if ImageKey allocation of shared image and ExternalImageId(for CompositableClient) does not conflict.

::: gfx/layers/wr/WebRenderImageLayer.h
@@ +54,5 @@
>      WebRenderImageLayer* mLayer;
>    };
>  
>    wr::MaybeExternalImageId mExternalImageId;
> +  uint64_t mSharedImageId;

Can't we use explicit type hare instead of uint64_t to make clear if mSharedImageId is valid? I converted mExternalImageId from uint64_t to wr::MaybeExternalImageId Bug 1357644.

MaybeExternalImageId might be a good choice, since in future, we might want to add support of ExternalBuffer to reduce image copy in webrender.
Assignee

Comment 40

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #39)
> Comment on attachment 8862844 [details] [diff] [review]
> Part 2. Add special handling for shared surfaces in image layers, v11
> 
> Review of attachment 8862844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some comment when I checked the patches.
> 
> ::: gfx/layers/wr/WebRenderBridgeParent.cpp
> @@ +481,5 @@
> > +        mActiveKeys.Put(wr::AsUint64(key), key);
> > +
> > +        wr::ImageDescriptor descriptor(data->mSize, data->mStride, data->mFormat);
> > +        auto slice = Range<uint8_t>(data->GetData(), data->GetDataLength());
> > +        mApi->AddImage(key, descriptor, slice);
> 
> This api cause image copy twice in webrender. It might be better to add
> support of ExternalBuffer as a next step in a different bug.
> 

Yes, that is the plan. I will add comments and open a bug :). By default images are not yet allocated to a shared buffer so it shouldn't have a performance impact if the above is worse than external images.

> ::: gfx/layers/wr/WebRenderImageLayer.cpp
> @@ +111,5 @@
> > +
> > +  void UpdateBridge(CompositorBridgeChild* aBridge)
> > +  {
> > +    Unused << NS_WARN_IF(ReleaseId());
> > +    mBridge = aBridge;
> 
> Doesn't it cause to leak SharedImageData in WebRenderBridgeParent, since
> mBridge->SendRemoveSharedImage(mId) is called only to latest mBridge?
> 

Discussed offline. This is complicated. To summarize, we need to either move the sharing of the ID and handle to a singleton which the CompositorBridgeChild/Parent share, or we consider moving the add/remove IPDL APIs to another protocol class which will always map 1:1 between the process which decoded the image and the process which needs to render it.

> @@ +186,5 @@
> > +}
> > +
> > +bool
> > +WebRenderImageLayer::TryShared(Image* aImage)
> > +{
> 
> Don't we need to handle a case that ImageKey for ExternalImageId(for
> CompositableClient) is allocated already?
> 
> It seems helpful to make clear if ImageKey allocation of shared image and
> ExternalImageId(for CompositableClient) does not conflict.
> 

I can add comments to clarify this. It first attempts to share the underlying buffer of the surface, if a shared surface -- if it is not selected, mSharedImageId is cleared and external image can use mImageKey as it pleases. If the image changes and it is a shared surface, when we call WebRenderLayer::UpdateImageKey, mSharedImageId is still cleared and thus doesn't match the new ID. It will check if we have an old key (the external key) and then release it.

> ::: gfx/layers/wr/WebRenderImageLayer.h
> @@ +54,5 @@
> >      WebRenderImageLayer* mLayer;
> >    };
> >  
> >    wr::MaybeExternalImageId mExternalImageId;
> > +  uint64_t mSharedImageId;
> 
> Can't we use explicit type hare instead of uint64_t to make clear if
> mSharedImageId is valid? I converted mExternalImageId from uint64_t to
> wr::MaybeExternalImageId Bug 1357644.
> 
> MaybeExternalImageId might be a good choice, since in future, we might want
> to add support of ExternalBuffer to reduce image copy in webrender.

Good idea, will do.
Assignee

Updated

2 years ago
Attachment #8862843 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Attachment #8862844 - Flags: review?(jmuizelaar)
Assignee

Comment 41

2 years ago
Break out the changes to SourceSurfaceSharedData and add a new wrapper class intended to be a read-only surface wrapping a shared surface.
Attachment #8862843 - Attachment is obsolete: true
Attachment #8862844 - Attachment is obsolete: true
Assignee

Comment 44

2 years ago
Includes ExternalBuffer support.
Assignee

Updated

2 years ago
Attachment #8864930 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Attachment #8865512 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

2 years ago
Attachment #8864933 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

2 years ago
Attachment #8865513 - Flags: review?(jmuizelaar)
FYI,

In WebRenderTextureHost case, its lifetime is controlled by WebRenderCompositableHolder. The TextureHost is held by WebRenderCompositableHolder from WebRenderCompositableHolder::HoldExternalImage() until WebRenderCompositableHolder::Update().
Comment on attachment 8865513 [details] [diff] [review]
Part 3. Integrate PSharedSurfaceBridge with WebRender., v3

Review of attachment 8865513 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/webrender_bindings/RendererOGL.cpp
@@ +21,5 @@
>  {
>    RendererOGL* renderer = reinterpret_cast<RendererOGL*>(aObj);
> +
> +  RefPtr<gfx::DataSourceSurface> surface =
> +    layers::SharedSurfaceBridgeParent::Get(aId);

How do we ensure that id for SharedSurface is valid until end of its usage on Render thread?
Attachment #8864933 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8865512 [details] [diff] [review]
Part 2a. Add PSharedSurfaceBridge protocol and implementation., v3

Review of attachment 8865512 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/PSharedSurfaceBridge.ipdl
@@ +21,5 @@
> +{
> +parent:
> +  async Add(ExternalImageId aId, SurfaceDescriptorShared aDesc);
> +  async Remove(ExternalImageId aId);
> +  sync Flush();

Can you add a comment why sync IPC is necessary?

::: gfx/layers/ipc/SharedSurfaceBridgeParent.cpp
@@ +19,5 @@
> +
> +/* static */ already_AddRefed<DataSourceSurface>
> +SharedSurfaceBridgeParent::Get(const wr::ExternalImageId& aId)
> +{
> +  StaticMutexAutoLock lock(sMutex);

Could we avoid hold mutex in this function? It might affect to performance.
Attachment #8865512 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #52)
> Comment on attachment 8865512 [details] [diff] [review]
> Part 2a. Add PSharedSurfaceBridge protocol and implementation., v3
> ::: gfx/layers/ipc/SharedSurfaceBridgeParent.cpp
> @@ +19,5 @@
> > +
> > +/* static */ already_AddRefed<DataSourceSurface>
> > +SharedSurfaceBridgeParent::Get(const wr::ExternalImageId& aId)
> > +{
> > +  StaticMutexAutoLock lock(sMutex);
> 
> Could we avoid hold mutex in this function? It might affect to performance.

It is OK. RenderThread::GetRenderTexture() also holds mutex.
Comment on attachment 8865513 [details] [diff] [review]
Part 3. Integrate PSharedSurfaceBridge with WebRender., v3

Review of attachment 8865513 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/webrender_bindings/RendererOGL.cpp
@@ +21,5 @@
>  {
>    RendererOGL* renderer = reinterpret_cast<RendererOGL*>(aObj);
> +
> +  RefPtr<gfx::DataSourceSurface> surface =
> +    layers::SharedSurfaceBridgeParent::Get(aId);

The patch uses same Id for a different storage. If we failed to get a object from id, how could we know if it is caused by SharedSurfaceBridgeParent or RenderTextureHost?
Comment on attachment 8865512 [details] [diff] [review]
Part 2a. Add PSharedSurfaceBridge protocol and implementation., v3

Review of attachment 8865512 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/SharedSurfaceBridgeParent.cpp
@@ +184,5 @@
> +  RefPtr<SourceSurfaceSharedDataWrapper> surface =
> +    new SourceSurfaceSharedDataWrapper();
> +  if (NS_WARN_IF(!surface->Init(aDesc.size(), aDesc.stride(),
> +                                aDesc.format(), aDesc.handle(),
> +                                OtherPid()))) {

If it happens, how could client side know the problem? If client side allocate ImageKey to this buffer, webrender could cause crash.
(In reply to Sotaro Ikeda [:sotaro] from comment #55)
> 
> If it happens, how could client side know the problem? If client side
> allocate ImageKey to this buffer, webrender could cause crash.

We could address this problem later in another bug.
Assignee

Updated

2 years ago
Attachment #8865513 - Flags: review?(jmuizelaar)
Assignee

Updated

2 years ago
Attachment #8864930 - Flags: review?(jmuizelaar)
Assignee

Comment 57

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #51)
> Comment on attachment 8865513 [details] [diff] [review]
> Part 3. Integrate PSharedSurfaceBridge with WebRender., v3
> 
> Review of attachment 8865513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/webrender_bindings/RendererOGL.cpp
> @@ +21,5 @@
> >  {
> >    RendererOGL* renderer = reinterpret_cast<RendererOGL*>(aObj);
> > +
> > +  RefPtr<gfx::DataSourceSurface> surface =
> > +    layers::SharedSurfaceBridgeParent::Get(aId);
> 
> How do we ensure that id for SharedSurface is valid until end of its usage
> on Render thread?

Ah, I see what you are saying. It could get added over the bridge, handled in WebRenderBridge::ProcessWebRenderCommands, freed over the bridge all on the compositor thread, before the renderer thread even gets to run. I hadn't considered that case, I will fix it in my next update.
Assignee

Comment 58

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #53)
> (In reply to Sotaro Ikeda [:sotaro] from comment #52)
> > Comment on attachment 8865512 [details] [diff] [review]
> > Part 2a. Add PSharedSurfaceBridge protocol and implementation., v3
> > ::: gfx/layers/ipc/SharedSurfaceBridgeParent.cpp
> > @@ +19,5 @@
> > > +
> > > +/* static */ already_AddRefed<DataSourceSurface>
> > > +SharedSurfaceBridgeParent::Get(const wr::ExternalImageId& aId)
> > > +{
> > > +  StaticMutexAutoLock lock(sMutex);
> > 
> > Could we avoid hold mutex in this function? It might affect to performance.
> 
> It is OK. RenderThread::GetRenderTexture() also holds mutex.

Still, it is a good point. I will avoid freeing buffers while holding the lock, particular in SharedSurfaceBridgeParent's destructor which could free N surfaces. munmap is not necessarily cheap.
Assignee

Updated

2 years ago
Depends on: 1365927
Priority: P3 → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
Assignee

Comment 59

2 years ago
Attachment #8864930 - Attachment is obsolete: true
Attachment #8864933 - Attachment is obsolete: true
Attachment #8865512 - Attachment is obsolete: true
Attachment #8865513 - Attachment is obsolete: true
Assignee

Comment 70

2 years ago
Attachment #8917490 - Attachment is obsolete: true
Attachment #8921888 - Flags: review?(jmuizelaar)
(needinfo'ing myself as a reminder to keep a look at these patches)
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8921887 [details] [diff] [review]
Part 1. Add SourceSurfaceSharedDataWrapper and SourceSurfaceSharedData::HandleLock., v3

Review of attachment 8921887 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/SourceSurfaceSharedData.cpp
@@ +24,5 @@
> +  mFormat = aFormat;
> +  mCreatorPid = aCreatorPid;
> +
> +  size_t len = GetAlignedDataLength();
> +  mBuf = new SharedMemoryBasic();

You can use MakeAndAddRef<SharedMemoryBasic>();

::: gfx/layers/SourceSurfaceSharedData.h
@@ +15,5 @@
>  
> +class SourceSurfaceSharedData;
> +
> +class SourceSurfaceSharedDataWrapper final : public DataSourceSurface
> +{

This needs a story about why it exists.
Attachment #8921887 - Flags: review?(jmuizelaar) → review+
Attachment #8921888 - Flags: review?(jmuizelaar) → review+
Attachment #8921889 - Flags: review?(jmuizelaar) → review+
Attachment #8921890 - Flags: review?(jmuizelaar) → review+
Attachment #8921891 - Flags: review?(jmuizelaar) → review+
Attachment #8921892 - Flags: review?(jmuizelaar) → review+
Attachment #8921894 - Flags: review?(jmuizelaar) → review+
Attachment #8921896 - Flags: review?(jmuizelaar) → review+

Comment 79

2 years ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e1da430d9f9
Part 1. Add SourceSurfaceSharedDataWrapper and SourceSurfaceSharedData::HandleLock. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/83cc1f67e9f7
Part 2. Add SharedSurfacesParent/Child to manage shared surfaces. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ec55507394
Part 3. Refactor mozilla::wr::LockExternalImage to make it easier to add new types. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bdfda0b0b3a
Part 4. Add RenderSharedSurfaceTextureHost wrapper to integrate with external images. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e6ab347bae
Part 5. Integrate SharedSurfacesParent with the WebRender texture cache. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/50b1fcf13fba
Part 6. Expand SharedSurfacesChild to support sharing ImageContainers directly. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77eb1c48b73
Part 7. Handle shared surfaces in WebRenderBridgeParent::AddExternalImage. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c56a10468d7
Part 8. Add plumbing to use shared surfaces if available in WebRenderImageData. r=jrmuizel
Backed out changeset 7e1da430d9f9::7c56a10468d7 (bug 1331944) for bustage on Android at gfx/layers/ipc/SharedSurfacesParent.cpp r=backout on a CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7c56a10468d707c64c2dafd19696cb4c68d4a20e

https://hg.mozilla.org/integration/mozilla-inbound/rev/719f1c2189d28fa84d6010d21883cc56df09a9d9
Flags: needinfo?(aosmond)
Comment on attachment 8921890 [details] [diff] [review]
Part 4. Add RenderSharedSurfaceTextureHost wrapper to integrate with external images., v3

Review of attachment 8921890 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/webrender_bindings/RenderSharedSurfaceTextureHost.h
@@ +14,5 @@
> +}
> +
> +namespace wr {
> +
> +class RenderSharedSurfaceTextureHost final : public RenderTextureHost

A comment briefly explaining why this class exists would be nice.
Overall it looks good. I have mixed feelings about having two systems (TextureClient and SharedSurface) that do similar things, but that's fine as long as we have a good understanding of the advantages and limitations of each systems and when to choose one or the other. It would be great if you could add some documentation about that part on the SharedSurface side (what does it do that isn't in TextureClient), and more generally document the methods (For example it takes a bit of digging to see the difference between SharedSurfaceParent's Release and Remove methods).
Flags: needinfo?(nical.bugzilla)

Comment 83

2 years ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed385db85112
Part 1. Add SourceSurfaceSharedDataWrapper and SourceSurfaceSharedData::HandleLock. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc9bb88a4d1
Part 2. Add SharedSurfacesParent/Child to manage shared surfaces. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be64943e91
Part 3. Refactor mozilla::wr::LockExternalImage to make it easier to add new types. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/12ac8625bf4c
Part 4. Add RenderSharedSurfaceTextureHost wrapper to integrate with external images. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a29cc4edf47
Part 5. Integrate SharedSurfacesParent with the WebRender texture cache. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3741bc7ff417
Part 6. Expand SharedSurfacesChild to support sharing ImageContainers directly. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb3b0587f93
Part 7. Handle shared surfaces in WebRenderBridgeParent::AddExternalImage. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d903cabfc500
Part 8. Add plumbing to use shared surfaces if available in WebRenderImageData. r=jrmuizel
Assignee

Updated

2 years ago
Flags: needinfo?(aosmond)
You need to log in before you can comment on or make changes to this bug.