1276811 - [webvr] Bug 1276811 - Enable TextureClient to be used without CompositableForwarder

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[:kip (Kearwood Gilbert)]

TextureForwarder is a base class that fits between CompositableForwarder and ISurfaceAllocator, enabling the texture forwarding functionality to be shared by subclasses that do not wish to bring in the layerization and compositor dependencies from CompositableForwarder.

This is used the WebVR 1.0 implementation in Bug 1250244, which implements IPC for texture forwarding outside of the compositor.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Bug 1276811 - Enable TextureClient to be used without CompositableForwarder,

- TextureForwarder is a base class that fits between
  CompositableForwarder and ISurfaceAllocator, enabling
  the texture forwarding functionality to be shared
  by subclasses that do not wish to bring in the
  layerization and compositor dependencies in
  CompositableForwarder.

Review commit: https://reviewboard.mozilla.org/r/56458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56458/

Comment 2

[Nicolas Silva [:nical]]

(In reply to :kip (Kearwood Gilbert) from comment #0)
> TextureForwarder is a base class that fits between CompositableForwarder and
> ISurfaceAllocator, enabling the texture forwarding functionality to be
> shared by subclasses that do not wish to bring in the layerization and
> compositor dependencies from CompositableForwarder.

Was this coordinated with George (bug 1176011) or is it a pure coincidence?

Anyway, most of this patch does exactly the same as the TextureForarder from bug 1176011 already r+'ed which is good. Could you you rebase your patch on top of Georges's? There should not be much left of it after that.

Comment 3

[:kip (Kearwood Gilbert)]

(In reply to Nicolas Silva [:nical] from comment #2)
> (In reply to :kip (Kearwood Gilbert) from comment #0)
> > TextureForwarder is a base class that fits between CompositableForwarder and
> > ISurfaceAllocator, enabling the texture forwarding functionality to be
> > shared by subclasses that do not wish to bring in the layerization and
> > compositor dependencies from CompositableForwarder.
> 
> Was this coordinated with George (bug 1176011) or is it a pure coincidence?
> 
> Anyway, most of this patch does exactly the same as the TextureForarder from
> bug 1176011 already r+'ed which is good. Could you you rebase your patch on
> top of Georges's? There should not be much left of it after that.

This was a coincidence :-)

I am rebasing and testing my patchset over the Bug 1176011 patch.

The remaining changes after rebasing are mostly concerned with allowing TextureClient to be used without a CompositableForwarder.  I'll update and rename this bug to match.

Comment 4

[:kip (Kearwood Gilbert)]

I have rebased this over Bug 1176011, which adds TextureForwarder.

The remaining patch enables TextureClient to be used with TextureForwarder rather than CompositableForwarder.

Comment 5

[:kip (Kearwood Gilbert)]

Attachment: Bug 1276811 - Enable TextureClient to be used without CompositableForwarder

Bug 1276811 - Enable TextureClient to be used without CompositableForwarder
- Refactoring to make TextureClient use the higher-level 
  TextureForwarder interface.

(If desired, I'll move this patch to reviewboard once Bug 1176011 lands)

Comment 6

[:kip (Kearwood Gilbert)]

Comment on attachment 8758077 [details]
Bug 1276811 - Enable TextureClient to be used without CompositableForwarder,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56458/diff/1-2/

Comment 7

[:kip (Kearwood Gilbert)]

Comment on attachment 8758077 [details]
Bug 1276811 - Enable TextureClient to be used without CompositableForwarder,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56458/diff/2-3/

Comment 8

[:kip (Kearwood Gilbert)]

Comment on attachment 8758077 [details]
Bug 1276811 - Enable TextureClient to be used without CompositableForwarder,

https://reviewboard.mozilla.org/r/56458/#review57958

nical had r+'ed this before I moved the patch to MozReview.

Comment 9

[:kip (Kearwood Gilbert)]

Pushed to try through mozreview.  If passes, will commit to unblock Bug 1250244

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8758077 [details]
Bug 1276811 - Enable TextureClient to be used without CompositableForwarder,

https://reviewboard.mozilla.org/r/56458/#review58078

There's some brokenness that landed with the initial TextureForwarder patch. The fix I landed yesterday got backed out so I'll try to get something asap, in the mean time there are some really buggy assumptions about TextureForwarder and CompositableForwarder. Your patch doesn't make it strictly worse I think but whatever I'll come up with to fix the situation will probably involve changing parts of it.

::: gfx/layers/client/TextureClient.cpp:868
(Diff revision 3)
>      ClearRecycleCallback();
>    }
>  }
>  
>  bool
> -TextureClient::InitIPDLActor(CompositableForwarder* aForwarder)
> +TextureClient::InitIPDLActor(TextureForwarder* aForwarder)

There's something hat will need to be figured out here. Right now ShadowLayerForwarder (our CompositableForwarder) inherits TextureForwarder, but it is confusing because the TextureForwarder we want is in fact CompositorBridgeChild (which is not a CompositableForwarder). At some point we need to clean this up and make CompositableForwarder not inherit TextureForwarder.

If a texture is used in a transaction (with layers and/or with ImageBridge) it needs to have a CompositableForwarder (I assume it won't need a CompositableForwarder for the WebVR usecase)

We probably need a separate InitIPDLActor method. One that takes a CompositableForwarder, and one that takes a TextureForwarder.

Comment 11

[:kip (Kearwood Gilbert)]

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

Comment 12

[:kip (Kearwood Gilbert)]

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

Comment 13

[:kip (Kearwood Gilbert)]

Comment on attachment 8758077 [details]
Bug 1276811 - Enable TextureClient to be used without CompositableForwarder,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56458/diff/3-4/

Comment 14

[:kip (Kearwood Gilbert)]

(In reply to Nicolas Silva [:nical] from comment #10)

Some functions have been moved from TextureForwarder back to CompositableForwarder, such as GetCompositorBackendType, so I had to update the patch once again.

This time, I have left the InitIPDLActor method alone, and added an overloaded version that takes a TextureForwarder* and a LayersBackend.

This has passed a try run.  Could you verify that the updated patch is okay with you before I commit?

Comment 15

[Nicolas Silva [:nical]]

(In reply to :kip (Kearwood Gilbert) from comment #14)
> Could you verify that the updated patch is okay with you before I commit?

Looks good!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/039447407fcc
Enable TextureClient to be used without CompositableForwarder,r=nical

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/039447407fcc