1331944 - Share shareable image surfaces instead of copying

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Switch to shared buffers, v1

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Switch to shared buffers, v2

Rebase. Minor tweaks.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Switch to shared buffers, v3

Rebase and fix how the mContainer check was moved to the wrong place.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Switch to shared buffers, v4

Remove legacy code from earlier attempt at patch, minor refactor to make functions consistent within imgFrame.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Switch to shared buffers, v5

Rebase onto new patches in bug 1331938.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Switch to shared buffers, v6

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Switch to shared buffers, v7

Rebase. Will follow up with breaking this patch down into individually landable/reviewable parts.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add shared image APIs to CompositorBridge, v1

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v1

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Decode images to shared surfaces for WebRender, v1

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Decode images to shared surfaces for WebRender, v2

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

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

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v2

Hopefully fix Windows/Mac/Linux static analysis build failures.

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

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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 14

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v3

Fix builds again.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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)

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Great, thanks!

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 21

[Timothy Nikkel (:tnikkel)]

(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.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v4

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.

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Decode images to shared surfaces for WebRender, v3 [carries r=tnikkel]

Handle imgFrame::GetFrame/GetFrameAtSize appropriately.

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v5

Rebase.

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add shared image APIs to CompositorBridge, v2

Rebase.

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v6

Rebase.

Comment 27

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v7

Rebase.

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add shared image APIs to CompositorBridge, v3

Rebase.

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v8

Rebase. Update new mask layer rendering added in bug 1341565 to also use the shared surfaces instead of using external images if possible.

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add shared image APIs to CompositorBridge, v4

Comment 31

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v9

Refactor due to changes from bug 1355678.

Comment 32

[Jeff Muizelaar [:jrmuizel]]

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 33

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 34

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add shared image APIs to CompositorBridge, v5

Rebase and update based on feedback.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 35

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v10

(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.

Comment 36

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 37

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add shared image APIs to CompositorBridge, v6

Comment 38

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add special handling for shared surfaces in image layers, v11

try (volatile memory): https://treeherder.mozilla.org/#/jobs?repo=try&revision=440dd7c1b52cf6730bb5f5212050073910e52f8d
try (shared memory): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4de57a53298753aaf1ec7877dbe3ece55dd810d

Comment 39

[Sotaro Ikeda [:sotaro]]

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.

Comment 40

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 41

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add SourceSurfaceSharedDataWrapper and SourceSurfaceSharedData::HandleLock, v1

Break out the changes to SourceSurfaceSharedData and add a new wrapper class intended to be a read-only surface wrapping a shared surface.

Comment 42

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2a. Add PSharedSurfaceBridge protocol and implementation., v1

Comment 43

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2b. Integrate PSharedSurfaceBridge with parent/GPU/child processes., v1

Comment 44

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Integrate PSharedSurfaceBridge with WebRender., v1

Includes ExternalBuffer support.

Comment 45

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 46

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2a. Add PSharedSurfaceBridge protocol and implementation., v2

Comment 47

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Integrate PSharedSurfaceBridge with WebRender., v2

Fix build errors and gtest/mochitest shutdown crashes.

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

Comment 48

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2a. Add PSharedSurfaceBridge protocol and implementation., v3

Comment 49

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Integrate PSharedSurfaceBridge with WebRender., v3

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

Comment 50

[Sotaro Ikeda [:sotaro]]

FYI,

In WebRenderTextureHost case, its lifetime is controlled by WebRenderCompositableHolder. The TextureHost is held by WebRenderCompositableHolder from WebRenderCompositableHolder::HoldExternalImage() until WebRenderCompositableHolder::Update().

Comment 51

[Sotaro Ikeda [:sotaro]]

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?

Comment 52

[Sotaro Ikeda [:sotaro]]

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.

Comment 53

[Sotaro Ikeda [:sotaro]]

(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 54

[Sotaro Ikeda [:sotaro]]

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 55

[Sotaro Ikeda [:sotaro]]

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.

Comment 56

[Sotaro Ikeda [:sotaro]]

(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.

Comment 57

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 58

[Andrew Osmond [:aosmond] (he/him)]

(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.

Comment 59

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add SourceSurfaceSharedDataWrapper and SourceSurfaceSharedData::HandleLock., v2

Comment 60

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add SharedSurfacesParent/Child to manage shared surfaces., v1

Comment 61

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Refactor mozilla::wr::LockExternalImage to make it easier to add new types., v1

Comment 62

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Add RenderSharedSurfaceTextureHost wrapper to integrate with external images., v1

Comment 63

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Integrate SharedSurfacesParent with the WebRender texture cache., v1

Comment 64

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. Expand SharedSurfacesChild to support sharing ImageContainers directly., v1

Comment 65

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 7. Handle shared surfaces in WebRenderBridgeParent::AddExternalImage., v1

Comment 66

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8. Add plumbing to use shared surfaces if available in WebRenderImageData., v1

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

Comment 67

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Refactor mozilla::wr::LockExternalImage to make it easier to add new types., v2

Comment 68

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Add RenderSharedSurfaceTextureHost wrapper to integrate with external images., v2

Fix some build errors...

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

Comment 69

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add SourceSurfaceSharedDataWrapper and SourceSurfaceSharedData::HandleLock., v3

Comment 70

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Add SharedSurfacesParent/Child to manage shared surfaces., v2

Comment 71

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Refactor mozilla::wr::LockExternalImage to make it easier to add new types., v3

Comment 72

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Add RenderSharedSurfaceTextureHost wrapper to integrate with external images., v3

Comment 73

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Integrate SharedSurfacesParent with the WebRender texture cache., v2

Comment 74

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. Expand SharedSurfacesChild to support sharing ImageContainers directly., v2

Comment 75

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 7. Handle shared surfaces in WebRenderBridgeParent::AddExternalImage., v2

Comment 76

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8. Add plumbing to use shared surfaces if available in WebRenderImageData., v2

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

Comment 77

[Nicolas Silva [:nical]]

(needinfo'ing myself as a reminder to keep a look at these patches)

Comment 78

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 79

[Pulsebot]

Pushed by [e-mail]:
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

Comment 80

[Cristina Coroiu [:ccoroiu]]

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

Comment 81

[Nicolas Silva [:nical]]

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.

Comment 82

[Nicolas Silva [:nical]]

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).

Comment 83

[Pulsebot]

Pushed by [e-mail]:
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

Comment 84

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/83cc1f67e9f7
https://hg.mozilla.org/mozilla-central/rev/40ec55507394
https://hg.mozilla.org/mozilla-central/rev/5bdfda0b0b3a
https://hg.mozilla.org/mozilla-central/rev/56e6ab347bae
https://hg.mozilla.org/mozilla-central/rev/50b1fcf13fba
https://hg.mozilla.org/mozilla-central/rev/b77eb1c48b73
https://hg.mozilla.org/mozilla-central/rev/ed385db85112
https://hg.mozilla.org/mozilla-central/rev/8bc9bb88a4d1
https://hg.mozilla.org/mozilla-central/rev/c5be64943e91
https://hg.mozilla.org/mozilla-central/rev/12ac8625bf4c
https://hg.mozilla.org/mozilla-central/rev/3a29cc4edf47
https://hg.mozilla.org/mozilla-central/rev/3741bc7ff417
https://hg.mozilla.org/mozilla-central/rev/8bb3b0587f93
https://hg.mozilla.org/mozilla-central/rev/d903cabfc500

Comment 85

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/83cc1f67e9f7
https://hg.mozilla.org/mozilla-central/rev/40ec55507394
https://hg.mozilla.org/mozilla-central/rev/5bdfda0b0b3a
https://hg.mozilla.org/mozilla-central/rev/56e6ab347bae
https://hg.mozilla.org/mozilla-central/rev/50b1fcf13fba
https://hg.mozilla.org/mozilla-central/rev/b77eb1c48b73
https://hg.mozilla.org/mozilla-central/rev/ed385db85112
https://hg.mozilla.org/mozilla-central/rev/8bc9bb88a4d1
https://hg.mozilla.org/mozilla-central/rev/c5be64943e91
https://hg.mozilla.org/mozilla-central/rev/12ac8625bf4c
https://hg.mozilla.org/mozilla-central/rev/3a29cc4edf47
https://hg.mozilla.org/mozilla-central/rev/3741bc7ff417
https://hg.mozilla.org/mozilla-central/rev/8bb3b0587f93
https://hg.mozilla.org/mozilla-central/rev/d903cabfc500