Closed
Bug 1256693
Opened 8 years ago
Closed 8 years ago
Cleanup ISurfaceAllocator
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: nical, Assigned: nical)
Details
Attachments
(2 files, 2 obsolete files)
76.99 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
73.53 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(nical.bugzilla)
Comment 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
(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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8731192 -
Flags: review?(sotaro.ikeda.g) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/93f4eafb8beb https://hg.mozilla.org/integration/mozilla-inbound/rev/7a74257343be
Comment 9•8 years ago
|
||
backout bugherder landing |
I had to back this out for apparently causing some leaks in media mochitests: https://treeherder.mozilla.org/logviewer.html#?job_id=24027348&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/975ac7ff948a https://hg.mozilla.org/integration/mozilla-inbound/rev/28d316c95986
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/232fda30a2ad https://hg.mozilla.org/integration/mozilla-inbound/rev/a85b0c803132
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/232fda30a2ad https://hg.mozilla.org/mozilla-central/rev/a85b0c803132
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/622952a12884
You need to log in
before you can comment on or make changes to this bug.
Description
•