1042696 - TextureClient has its mAllocator member set twice

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

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.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: v1 - set Allocator by constructor

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

Comment 2

[Nicolas Silva [:nical]]

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.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: v2 - retain the TextureClient mAllocator and add Assert

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.

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8539093 [details] [diff] [review]
v2 - retain the TextureClient mAllocator and add Assert

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

Nice

Comment 5

[Ethan Lin[:ethlin]]

Attachment: v3 - fix windows platform complie error

There was a windows platform compile error. This patch fix the problem.
Try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a9a75254224

Comment 6

[Ethan Lin[:ethlin]]

Attachment: v4 - Set allocator to TextureClient from constructor. r=nical.

Comment 7

[Ethan Lin[:ethlin]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9790020b89

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G debug crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/daa2d1ac6b18

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4872152&repo=mozilla-inbound

Comment 10

[Ethan Lin[:ethlin]]

Attachment: fix_assert.patch

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

Comment 11

[Ethan Lin[:ethlin]]

Attachment: ISurfaceAllocator diagram

Attached file is the ISurfaceAllocator diagram. The TextureClientRecycleAllocatorImp inherit from ISurfaceAllocator and has a ISurfaceAllocator which is the original CompositableForwarder.

Comment 12

[Ethan Lin[:ethlin]]

Please land the attachment 8540098 [details] [diff] [review] and attachment 8540732 [details] [diff] [review] to mozilla-central.

The try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7b53b5cd7ac

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/725c5cf6db6c