Closed Bug 1256693 Opened 5 years ago Closed 5 years ago

Cleanup ISurfaceAllocator


(Core :: Graphics: Layers, defect)

Not set



Tracking Status
firefox48 --- fixed


(Reporter: nical, Assigned: nical)



(2 files, 2 obsolete files)

ISurfaceAllocator has become a bit of a mess: It bundles a lot of not-entirely-related interfaces, not all of which are implemented by the implementing classes. ISurfaceAllocator also implements a lot of stuff (so it's not only an interface as the name suggests) that only makes sense for one of its child classes and just relies on the rest not calling into it.
Also there's code that expect the ISurfaceAllocator to be for example either a ShadowLayerForwarder or an ImageBridgeChild, which makes adding new ISurfaceAllocator implementations difficult.
Finally, I don't like the name. We don't have a lot of interfaces prefixed with "I" and it is not only used to allocate surface. I am not sure whether renaming it into something like "IPCAllocator" will be worth the code churn, though. I may just leave it as it is, not sure yet.

So my goal here is to split ISurfaceAllocator into several pure interfaces and have them implemented only where it makes sense.

For instance the GrallocAllocator an ShmemSectionAllocator interfaces will only be implemented on the child side (currently if you try to use them on the parent process, things get nasty).
These smaller interfaces will be accessible through virtual down-casting methods like AsShmemAllocator(), etc. an ISurfaceAllocator object may not implement all of the interfaces so users have to check that downcasting doesn't return null.
Attached patch (WIP) cleanup (obsolete) — Splinter Review
Attached patch Massive cleanup (obsolete) — Splinter Review
The patch ended up bigger than I hoped when I started, but I think the result is worth it.
It is now way harder to shoot yourself in the foot by calling an ISurfaceAllocator API that is only meant to be used with a certain implementation (for example ShadowLayerForwarder specifically) with another implementation (like ImageBridgeParent.
Examples of methods that were available in all ISurfaceAllocator but would cause bugs if used with the wrong type of ISurfaceAllocator:
- ParentPid
- Alloc/FreeShmemSection
- Alloc/DeallocGrallocBuffer
- GetMessageLoop
Some other methods would not cause bugs but were only useful for certain types of allocators.
I also removed some dead code along the way.

Most of the patch is moving code around and add some AsXYZ() casts here and there.
Attachment #8730797 - Flags: review?(sotaro.ikeda.g)
This is a quite the antisocial patch as it touches a lot of code but it's only a search and replace ISurfaceAllocator -> ClientIPCAllocator to statically ensure that we can't do something silly like pass an ImageBridgeParent as a parameter of TextureClient...
Attachment #8730750 - Attachment is obsolete: true
Attachment #8730815 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8730797 [details] [diff] [review]
Massive cleanup

Review of attachment 8730797 [details] [diff] [review]:

Nice! Looks good. I have one question.

::: gfx/layers/BufferTexture.cpp
@@ +127,5 @@
>                                    gfx::BackendType aMoz2DBackend,
>                                    int32_t aBufferSize,
>                                    TextureFlags aTextureFlags)
>  {
> +  if (!aAllocator || aAllocator->IsSameProcess() || !aAllocator->AsShmemAllocator()) {

Is it acceptable that "aAllocator->AsShmemAllocator() == false" here? If we allocate MemoryTextureData in child process, how does the MemoryTextureData work?

::: gfx/layers/ipc/ISurfaceAllocator.h
@@ +118,5 @@
> +  virtual ClientIPCAllocator* AsClientAllocator() override { return this; }
> +
> +  virtual MessageLoop * GetMessageLoop() const = 0;
> +
> +  virtual int32_t GetMaxTextureSize() const { return gfxPrefs::MaxTextureSize(); }  

// nit. Remove space.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8730815 [details] [diff] [review]
Make most client-side things interact with ClientIPCAllocator rather than ISurfaceAllocator

Looks good.
Attachment #8730815 - Flags: review?(sotaro.ikeda.g) → review+
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> > +  if (!aAllocator || aAllocator->IsSameProcess() || !aAllocator->AsShmemAllocator()) {
> Is it acceptable that "aAllocator->AsShmemAllocator() == false" here? If we
> allocate MemoryTextureData in child process, how does the MemoryTextureData
> work?

Good catch. What I should have done here is add the check in the else block below and return null rather than creating a SmemTextureData with an ISurfaceAllocator that can't do Shmems. In any case it would be dangerous to create a MemoryTextureData we are doing cross-process stuff.
Flags: needinfo?(nical.bugzilla)
Updated patch with review comments addressed.
Attachment #8730797 - Attachment is obsolete: true
Attachment #8730797 - Flags: review?(sotaro.ikeda.g)
Attachment #8731192 - Flags: review?(sotaro.ikeda.g)
Attachment #8731192 - Flags: review?(sotaro.ikeda.g) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.