1281456 - Decouple TextureForwarder and CompositableForwarder

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[George Wright (:gw280) (needinfo me!)]

Currently, CompositableForwarder is a subclass of TextureForwarder for historical reasons, and e.g. ShadowLayerForwarder shouldn't really be a TextureForwarder.

We should really decouple these and clean up this area of code a little.

Comment 1

[Matt Woodrow (:mattwoodrow)]

I started looking into this a bit as a consequence of fixing bug 1303897.

Does this seems correct?

4 interfaces:
IPCChannel (IsIPCOpen, GetMessageLoop, ShmemAllocator?)
TextureForwarder (Create PTexture)
CompositableForwarder (Create PCompositable, UseTexture etc)
KnowsCompositor (Tied to a specific Compositor, has a TextureFactoryIdentifier)


5 IPDL protocols:

VideoBridge
  - IPCChannel
  - TextureForwarder
  - KnowsCompositor

ImageBridge
  - IPCChannel
  - TextureForwarder
  - KnowsCompositor
  - CompositableForwarder

CompositorBridge
 - IPCChannel
 - TextureForwarder

LayerTransaction
  - KnowsCompositor
  - CompositableForwarder

VRManager
  - IPCChannel
  - TextureForwarder
  - KnowsCompositor

I think IPCChannel and TextureForwarder can be combined since they are always used together (even though they're not necessarily the same thing)

I think given that we can have GetTextureForwarder() on CompositableForwarder and KnowsCompositor, but not the reverse (since there may be multiple).

CompositableForwarder could be a subclass of KnowsCompositor, but I'm not sure that makes much sense logically.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Actually, maybe IPCChannel should be on LayerTransaction too, since it can be closed independently of CompositorBridge.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Updated interfaces:

LayersIPCActor
LayersIPCChannel : public LayersIPCActor
TextureForwarder : public LayersIPCChannel
KnowsCompositor (name suggestions appreciated)
CompositableForwarder : public KnowsCompositor

ShadowLayerForwarder : public CompositableForwarder
CompositorBridge : public TextureForwarder
ImageBridge : public CompositableForwarder, public TextureForwarder
VideoBridge : public TextureForwarder
VRManager : public TextureForwarder

Some notes:

* We have ISurfaceAllocator that is meant to provide a shared interface for both client and host allocators and let us use the same code for both. There is literally on callsite in the entire codebase that can actually be called from either side [1], and then it is only ever referenced from the client side (where we cast it back using AsTextureForwarder).

I don't think that abstraction is adding value, so I've removed it from the client (should probably do the host side too).

* We have a lot of As***() accessors that only return a value if the concrete object implements the given interface. This leads to a lot of code that takes a low-level interface (like ClientIPCAllocator) and then tries to up-cast to a derived class. We then have code to handle if this returns nullptr, even though all the callers are passing objects that wouldn't return null. This of course could change at any time when people add new callers.

My changes get rid of these, and only have accessors that are infallible. You must get passed in the right interface for the functionality your code needs.

* VideoBridge, VRManager and ImageBridge all implement KnowsCompositor even though they are singletons and aren't really attached to a single compositor. My changes don't make this any better or worse, they just make it more obvious. We should fix this.


* ShadowLayerForwarder seems somewhat pointless. It uses the 'Shadow' terminology which is pre-layers refactoring and just serves to confuse people now. Most of the comments in it are horribly out of date. We should take the main functionality (building Edits) and move it into LayerTransactionChild (or ClientLayerManager?) and get rid of the extra class.



[1] http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/gfx/layers/client/TextureClient.cpp#1456

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: New TextureForwarder interfaces

See the previous comment for an explanation of most things.

This builds on OSX, doing try builds to find the remaining platform specific issues.

Comment 5

[Nicolas Silva [:nical]]

I like this plan a lot.

Comment 6

[George Wright (:gw280) (needinfo me!)]

This looks great! A couple of comments:

- What's the value of LayersIPCActor? It only exposes an IPCOpen() method, which I would argue is more a property of the IPC channel than the actor, no? Could we just move IPCOpen() into LayersIPCChannel and forget about LayersIPCActor?
- I mentioned this on IRC, but I want to put it here for posterity. I agree that TextureForwarder and LayersIPCChannel aren't really related, but at the moment they're always used together and so it kind of almost makes sense to lump them together. That being said, I'm not sure making TextureForwarder a subclass of LayersIPCChannel is particularly useful, and from a class arrangement perspective I'd much rather have an associated TextureForwarder by composition on the LayersIPCChannel rather than making them equivalent in the type system. I'll leave the final call up to you though.

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to George Wright (:gw280) (:gwright) from comment #6)
> This looks great! A couple of comments:
> 
> - What's the value of LayersIPCActor? It only exposes an IPCOpen() method,
> which I would argue is more a property of the IPC channel than the actor,
> no? Could we just move IPCOpen() into LayersIPCChannel and forget about
> LayersIPCActor?

The existing code has this function on all classes involved, including ShadowLayerForwarder, which definitely *isn't* a LayersIPCChannel (it forwards the call to LayerTransactionChild, which is a sub-protocol).

> - I mentioned this on IRC, but I want to put it here for posterity. I agree
> that TextureForwarder and LayersIPCChannel aren't really related, but at the
> moment they're always used together and so it kind of almost makes sense to
> lump them together. That being said, I'm not sure making TextureForwarder a
> subclass of LayersIPCChannel is particularly useful, and from a class
> arrangement perspective I'd much rather have an associated TextureForwarder
> by composition on the LayersIPCChannel rather than making them equivalent in
> the type system. I'll leave the final call up to you though.

I think it's easier since it avoids need to have AsLayersIPCChannel() calls all over the code (I had it this way earlier, it adds a lot of clutter).

I've got a comment above TextureForwarder explaining that the relationship isn't really inheritance and that we could break it in the future if we have a use case.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: New TextureForwarder interfaces

Mostly the same as the previous patch, but now compiles on all platforms and doesn't regress tests.

I fixed TextureClientPool to also take the max texture size, since we were previously querying this on CompositorBridgeChild (which wasn't a valid thing to do).

Comment 9

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8794144 [details] [diff] [review]
New TextureForwarder interfaces

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

::: gfx/layers/ipc/ShadowLayers.h
@@ +367,5 @@
>  
>    // Returns true if aSurface wraps a Shmem.
>    static bool IsShmem(SurfaceDescriptor* aSurface);
>  
> +  TextureForwarder* AsTextureForwarder() override { return GetCompositorBridgeChild(); }

I'd like a comment here showing that this is not ideal (like the comment above this method in the header file).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ead4d4eb90
Decouple TextureForwarder and CompositableForwarder. r=gw280

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7ead4d4eb90

Comment 12

[Andrei Vaida [:avaida]]

Matt, is this something manual QA should be looking at on Fx52?

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #12)
> Matt, is this something manual QA should be looking at on Fx52?

Nope, this is just internal refactoring.

Comment 14

[Andrei Vaida [:avaida]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> (In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #12)
> > Matt, is this something manual QA should be looking at on Fx52?
> 
> Nope, this is just internal refactoring.

Thank you for following up on this. Updating flags accordingly.