Closed Bug 1042696 Opened 5 years ago Closed 5 years ago

TextureClient has its mAllocator member set twice

Categories

(Core :: Graphics: Layers, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: nical, Assigned: ethlin, Mentored)

Details

Attachments

(3 files, 3 obsolete files)

once in the constructor and once in InitIPDLActor. It makes no sense for a TextureClient to change its allocator, so let's figure out what's going on here and fix it so that the allocator is only set once.
Mentor: nical.bugzilla
Assignee: nobody → etlin
There are two class member both named mAllocator. One is in TextureClient, which is for using the same thread to free TextureClient [2] (bug 1006957). Another one is for allocating buffer in sub-class [1], like GrallocTextureClientOGL. I think there are two problems:
1. Dupe class member. The BufferTextureClient is a sub-class of TextureClient, both TextureClient and BufferTextureClient have a member named mAllocator [3].
2. Multiple places set Allocator value. From InitIPDLActor and constructor, though they set separately to base-class and sub-class's member.

Here is my modification for the two issues.
1. Remove the TextureClient's mAllocator. Just use Actor Forwarder's message loop to release TextureClient.
2. Set GetAllocator() as a virtual function, so child can do it's implementation, like BufferTextureClient. 

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/GrallocTextureClient.cpp?#293
[2] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/CompositableClient.cpp?#68
[3] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.h?#606
Attachment #8538217 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8538217 [details] [diff] [review]
v1 - set Allocator by constructor

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

I would prefer that we keep mAllocator in TextureClient, remove it from BufferTextureClient (the duplication was definitely an oversight), but without making GetAllocator virtual since there shouldn't be any reason for it to behave differently from a TextureClient implementation to the other.
From the look of it, the assignment mAllocator = aForwarder in InitIPDLActor is not needed. There should be a MOZ_ASSERT(mAllocator == aForwarder) at the beginning of the method instead.
Attachment #8538217 - Flags: feedback?(nical.bugzilla) → feedback-
I think you mean we only set mAllocator by the constructor of TextureClient. Actually this was my first version. I think it also makes sense that we set mAllocator to TextureClient at constructor and destroy it by the same mAllocator's message loop. 
This patch includes following changes:
1. All TextureClient sub-class's Constructors have a aAllocator parameter to initial the mAllocator in TextureClient.
2. Remove all TexureClient sub-class's mAllocator (including BufferTexutureClient, )
3. In InitIPDLActor, Add a ASSERT to make sure mAllocator == aForwarder and remove assignment mAllocator = aForwarder.
Attachment #8538217 - Attachment is obsolete: true
Attachment #8539093 - Flags: review?(nical.bugzilla)
Comment on attachment 8539093 [details] [diff] [review]
v2 - retain the TextureClient mAllocator and add Assert

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

Nice
Attachment #8539093 - Flags: review?(nical.bugzilla) → review+
There was a windows platform compile error. This patch fix the problem.
Try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a9a75254224
Attachment #8539093 - Attachment is obsolete: true
Attachment #8539871 - Flags: review?(nical.bugzilla)
Attachment #8539871 - Flags: review?(nical.bugzilla) → review+
Change patch to 8 lines format.
Please land the attachment 8540098 [details] [diff] [review] to mozilla-central.

Try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a9a75254224
Keywords: checkin-needed
Attached patch fix_assert.patchSplinter Review
There is a case will enter the assertion. By Bug 1091777 fix, we will create another kind of ISurfaceAllocator(TextureClientRecycleAllocatorImp) as a TextureClient pool and set it to TextureClient constructor. The TextureClientRecycleAllocatorImp is nothing about IPDL, so we cannot send it to InitIPDLActor. I think in out case, we could just check the message loop is the same in InitIPDLActor. Because we want to make sure they are using the same message loop to construct / ipc / destroy.
The try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7b53b5cd7ac
Attachment #8540732 - Flags: review?(nical.bugzilla)
Attached file is the ISurfaceAllocator diagram. The TextureClientRecycleAllocatorImp inherit from ISurfaceAllocator and has a ISurfaceAllocator which is the original CompositableForwarder.
Attachment #8540732 - Flags: review?(nical.bugzilla) → review+
I folded the follow-up into the original patch since logically that made more sense (and will allow for easier bisection in the future).
https://hg.mozilla.org/integration/mozilla-inbound/rev/725c5cf6db6c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/725c5cf6db6c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.