1155495 - refactor the part of android fence in TextureHostOGL

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ethan Lin[:ethlin]]

All functions and members in TextureHostOGL is for android fence. Each time using these functions will call AsHostOGL() to cast TextureHost to TextureHostOGL and check if the platform is WIDGE_GONK [1]. And there should be only GrallocTextureHost uses these members and functions.
I think there may be some way to refactor it to make the code more clear.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayerTransactionParent.cpp#926

Comment 1

[Peter Chang[:pchang]]

Jeremy, I think this is a good practice for you to study the gfx code flow related to textureclient/host

Comment 2

[Jeremy Chen ([e-mail])]

Thank you, Peter. I'll study this once I finished my work in hand.

Comment 3

[Jeremy Chen ([e-mail])]

Attachment: Bug 1155495 - part1: Remove TextureHostOGL class and integrate the platform specific API into TextureHost class. (v1)

Remove TextureHostOGL class and integrate the Android platform specific version API into TextureHost class.

Comment 4

[Jeremy Chen ([e-mail])]

Attachment: Bug 1155495 - part2: Remove unnecessary class inheritance and casting. (v1)

From part1 patch, we've removed TextureHostOGL class. So, we can Remove unnecessary class inheritance and class casting in the implementations.

Comment 5

[Jeremy Chen ([e-mail])]

Remove feedback request for now, cause I've got some feedback from Ethan.

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1155495 - part1: Remove TextureHostOGL and integrate the platform specific API into TextureHost. (v2)

Remove TextureHostOGL and integrate the Android platform specific version API into TextureHost.

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: Bug 1155495 - part2: Remove unnecessary class inheritance and casting. (v2)

From part1 patch, we've removed TextureHostOGL. So, we can Remove unnecessary class multi-inheritance and class casting in the implementations.

Comment 8

[Ethan Lin[:ethlin]]

Comment on attachment 8607441 [details] [diff] [review]
Bug 1155495 - part2: Remove unnecessary class inheritance and casting. (v2)

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

It looks good to me.

Comment 9

[Ethan Lin[:ethlin]]

Comment on attachment 8607437 [details] [diff] [review]
Bug 1155495 - part1: Remove TextureHostOGL and integrate the platform specific API into TextureHost. (v2)

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

f+ if the comments are addressed.

::: gfx/layers/composite/TextureHost.cpp
@@ +165,5 @@
> +  return true;
> +}
> +
> +FenceHandle
> +TextureHost::GetAndResetReleaseFence()

Since there is already a function named GetAndResetReleaseFenceHandle, you may combine GetAndResetReleaseFenceHandle and GetAndResetReleaseFence in the TextureHost and remove the GetAndResetReleaseFenceHandle in GrallocTextureHost.

::: gfx/layers/composite/TextureHost.h
@@ +521,5 @@
> +  /**
> +   * Store a fence that will signal when the current buffer is no longer being read.
> +   * Similar to android's GLConsumer::setReleaseFence()
> +   */
> +  virtual bool SetReleaseFence(const FenceHandle& aReleaseFence);

We should unify the function name. Maybe this function name should be SetReleaseFenceHandle. And it doesn't need to be virtual.

@@ +526,5 @@
> +
> +  /**
> +   * Return a releaseFence's Fence and clear a reference to the Fence.
> +   */
> +  virtual FenceHandle GetAndResetReleaseFence();

Combine with GetAndResetReleaseFenceHandle and remove virtual.

@@ +528,5 @@
> +   * Return a releaseFence's Fence and clear a reference to the Fence.
> +   */
> +  virtual FenceHandle GetAndResetReleaseFence();
> +
> +  virtual void SetAcquireFence(const FenceHandle& aAcquireFence);

Ditto.

@@ +533,5 @@
> +
> +  /**
> +   * Return a acquireFence's Fence and clear a reference to the Fence.
> +   */
> +  virtual FenceHandle GetAndResetAcquireFence();

Ditto.

@@ +535,5 @@
> +   * Return a acquireFence's Fence and clear a reference to the Fence.
> +   */
> +  virtual FenceHandle GetAndResetAcquireFence();
> +
> +  virtual void WaitAcquireFenceSyncComplete();

Remove virtual.

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Bug 1155495 - part1: (v3) Remove TextureHostOGL and integrate the platform specific API into TextureHost.

Addressed Ethan's comment.

Comment 11

[Jeremy Chen ([e-mail])]

Attachment: Bug 1155495 - part2: (v3) Remove unnecessary class inheritance and casting.

Fix function names.

Comment 12

[Jeremy Chen ([e-mail])]

In part1 patch, I remove TextureHostOGL and integrate the platform specific API into TextureHost. GetAndResetReleaseFenceHandle and GetAndResetReleaseFence are combined to one unified version in the TextureHost. Function names with "Fence" are fixed with "FenceHandle", which is now agree with those in TextureClient.

In part2 patch, we no longer need multi-inheritance and AsHostOGL() for GrallocTextureHost to use the platform specific API anymore.

Hi Sotaro, could you review these two patches?

Comment 13

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8607999 [details] [diff] [review]
Bug 1155495 - part1: (v3) Remove TextureHostOGL and integrate the platform specific API into TextureHost.

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

The implementation looks good. But TextureHost::WaitAcquireFenceHandleSyncComplete() is OpenGL specific. So, it is not clear for me that is OK to move the implementation from TextureHostOGL to TextureHost. Then, I forward the review to nical.

Comment 14

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8607999 [details] [diff] [review]
Bug 1155495 - part1: (v3) Remove TextureHostOGL and integrate the platform specific API into TextureHost.

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

::: gfx/layers/composite/TextureHost.cpp
@@ +180,5 @@
> +  return FenceHandle(fdObj);
> +}
> +
> +void
> +TextureHost::WaitAcquireFenceHandleSyncComplete()

Can this function's implementation be moved into FenceHandle? If it is done, we could remove dependence to EGL from TextureHost.

Comment 15

[Nicolas Silva [:nical]]

Comment on attachment 8607999 [details] [diff] [review]
Bug 1155495 - part1: (v3) Remove TextureHostOGL and integrate the platform specific API into TextureHost.

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

I like the idea of removing TextureHostOGL but let's not put EGL specific code in TextureHost. r+ if you implement sotaro's suggestion of abstracting away the EGL fence logic under FenceHandle or some kind of abstract fence.

Comment 16

[Jeremy Chen ([e-mail])]

If we put EGL fence logic into FenceHandle, would it affect the generality of FenceHandle? To remove dependence to EGL from TextureHost, could we set WaitAcquireFenceHandleSyncComplete() to virtual in TextureHost, and let GrallocTextureHost implement it? In that case, we could keep the generality of FenceHandle. We also could keep TextureHost away from platform specific code, and leave it to be implemented in any platform specific class.

Comment 17

[Nicolas Silva [:nical]]

(In reply to Jeremy Chen [:jeremychen] from comment #16)
> To remove dependence to EGL from TextureHost, could we set
> WaitAcquireFenceHandleSyncComplete() to virtual in TextureHost, and let
> GrallocTextureHost implement it? In that case, we could keep the generality
> of FenceHandle.

Sounds good to me.

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #17)
> (In reply to Jeremy Chen [:jeremychen] from comment #16)
> > To remove dependence to EGL from TextureHost, could we set
> > WaitAcquireFenceHandleSyncComplete() to virtual in TextureHost, and let
> > GrallocTextureHost implement it? In that case, we could keep the generality
> > of FenceHandle.
> 
> Sounds good to me.

me too.

Comment 19

[Jeremy Chen ([e-mail])]

Attachment: Bug 1155495 - part1: (v4) Remove TextureHostOGL and integrate the platform specific API into TextureHost.

Remove dependence to EGL from TextureHost. Set WaitAcquireFenceHandleSyncComplete() to virtual in TextureHost, and let GrallocTextureHost implement it.

Comment 20

[Jeremy Chen ([e-mail])]

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

It looks positive except for Linux debug reftest R(R1), which has nothing to do with this patch but always timeout lately.

Comment 21

[Jeremy Chen ([e-mail])]

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

Linux debug reftest R(R1) still timeout, like other similar jobs.

Comment 22

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] from comment #21)
> try result:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed51d0c3b74f
> 
> Linux debug reftest R(R1) still timeout, like other similar jobs.

Don't know why all B2G Desktop Linux x64 opt tests are showing exception now, while they were passed successfully before. Resend patches to try and wait for positive results. Clear checkin-needed keyword first.

Comment 23

[Jeremy Chen ([e-mail])]

try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=255b0df97ec0

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e8e84f21e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d45d284f81

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b8e8e84f21e1
https://hg.mozilla.org/mozilla-central/rev/a8d45d284f81