1155498 - Use fd to keep android fence in FenceHandle

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ethan Lin[:ethlin]]

For now we use the android::fence in FenceHandle [1]. android::fence will close the file descriptor in it's destructor, so we have to dupe fd first when sending fence by IPC. If we keep the fd in FenceHandle directly, we can reduce some unnecessary duplication. But the ownership of the fd is a problem when we copy the FenceHandle.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/FenceUtilsGonk.h?from=fenceutilsgonk.h#41

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Part 2 - New FenceHandle in TextureHost

Comment 3

[Ethan Lin[:ethlin]]

Attachment: Part 3 - New FenceHandle in Compositor

Comment 4

[Ethan Lin[:ethlin]]

Attachment: Part 4 - New FenceHandle for TextureClient

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Part 5 - New FenceHandle in Decoder

Comment 6

[Ethan Lin[:ethlin]]

Originally, we use android::Fence in FenceHandle, but the android::Fence's destructor will auto close the FD. So we have to dup the FD when we want to transmit the FD to HwcComposer, OGL or IPC.
In these patches, I create a FdObj to replace the original android::Fence in FenceHandle, so we have more control over the FD. There is a public function named GetAndResetFd() to transmit the FD without dup. I also replace some android::Fence with FenceHandle, because most of the time we just need the FD but not android::Fence. Then I can remove some macro definition like '#if defined(MOZ_WIDGET_GONK)'.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

Rebase the patch.

Comment 8

[Ethan Lin[:ethlin]]

Attachment: Part 2 - New FenceHandle in TextureHost

Rebase the patch.

Comment 9

[Ethan Lin[:ethlin]]

Attachment: Part 3 - New FenceHandle in Compositor

Rebase the patch.

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8600789 [details] [diff] [review]
Part 3 - New FenceHandle in Compositor

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

::: widget/gonk/HwcComposer2D.h
@@ +26,5 @@
>  #include <list>
>  
>  #include <hardware/hwcomposer.h>
>  #if ANDROID_VERSION >= 17
>  #include <ui/Fence.h>

I think you can also remove this #include

Comment 11

[Chiajung Hung [:chiajung]]

Comment on attachment 8599807 [details] [diff] [review]
Part 4 - New FenceHandle for TextureClient

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

Looks good to me.

::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp
@@ +146,5 @@
>      if (index < 0) {
>      }
>  
> +    FenceHandle handle = client->GetAndResetReleaseFenceHandle();
> +    sp<Fence> fence = new Fence(handle.GetAndRestFd());

GetAndRes"e"tFd?

Comment 12

[Chiajung Hung [:chiajung]]

Comment on attachment 8599808 [details] [diff] [review]
Part 5 - New FenceHandle in Decoder

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

Looks good to me.

Comment 13

[Chiajung Hung [:chiajung]]

Comment on attachment 8599808 [details] [diff] [review]
Part 5 - New FenceHandle in Decoder

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

Looks good to me.

Comment 14

[Chiajung Hung [:chiajung]]

Comment on attachment 8600787 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

In this patch, I think you use FdObj to ref count the fd to avoid leak but the API of FenceHandle has no plain Get but only GetAndReset. Can we use UniquePtr and make it slightly clearer?

f+ for now. If you switch to UniquePtr implementation, please request feedback again.

Comment 15

[Ethan Lin[:ethlin]]

(In reply to Boris Chiou [:boris] from comment #10)
> Comment on attachment 8600789 [details] [diff] [review]
> Part 3 - New FenceHandle in Compositor
> 
> Review of attachment 8600789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.h
> @@ +26,5 @@
> >  #include <list>
> >  
> >  #include <hardware/hwcomposer.h>
> >  #if ANDROID_VERSION >= 17
> >  #include <ui/Fence.h>
> 
> I think you can also remove this #include

OK, I will remove this.

Comment 16

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

Remove FenceUtilsGonk, we can just use FenceUtils after removing android::fence from FenceHandle.

Comment 17

[Ethan Lin[:ethlin]]

Attachment: Part 3 - New FenceHandle in Compositor

I apply boris's feedback to remove unused #include.

Comment 18

[Ethan Lin[:ethlin]]

Attachment: Part 4 - New FenceHandle for TextureClient

Fix typo.

Comment 19

[Ethan Lin[:ethlin]]

Attachment: Part 5 - New FenceHandle in Decoder

Fix the compile error in some platforms.

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8602465 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

::: gfx/layers/ipc/FenceUtils.cpp
@@ +20,5 @@
> +void
> +ParamTraits<FenceHandle>::Write(Message* aMsg,
> +                                const paramType& aParam)
> +{
> +  FenceHandle handle = aParam;

There is the const semantic for aParam here, but we still change its member indirectly.

@@ +56,5 @@
> +
> +void
> +FenceHandle::Merge(const FenceHandle& aFenceHandle)
> +{
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17

Could you check the line 8 in this file? Why we need two same guards?

@@ +63,5 @@
> +  }
> +
> +  if (!IsValid()) {
> +    mFence = aFenceHandle.mFence;
> +  } else {

How about skip the dup op if we want to merge with itself?

::: gfx/layers/ipc/FenceUtils.h
@@ +7,5 @@
>  
>  #ifndef IPC_FencerUtils_h
>  #define IPC_FencerUtils_h
>  
> +#include <unistd.h>

Does the windows platform have this header? What is this header used for?

@@ +41,5 @@
> +
> +  FenceHandle(int aFenceFd);
> +
> +  bool operator==(const FenceHandle& aOther) const {
> +    return mFence.get() == aOther.mFence.get();

If we have two FenceHandle instances with the same fd, what's the result of this operator? Do you want to check the fd number or the fd address?

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8600788 [details] [diff] [review]
Part 2 - New FenceHandle in TextureHost

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

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +268,2 @@
>  #endif
>    return FenceHandle();

How about return GetAndResetReleaseFence() and remove the #ifdef in this block?
Since we already handle the invalid fd FenceHandle class.

Comment 22

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8602469 [details] [diff] [review]
Part 4 - New FenceHandle for TextureClient

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

::: widget/gonk/nativewindow/GonkNativeWindowLL.cpp
@@ +158,5 @@
>          return;
>      }
>  
> +    FenceHandle handle = client->GetAndResetReleaseFenceHandle();
> +    sp<Fence> fence = new Fence(handle.mFence.GetAndResetFd());

Can we access the private member mFence here?
Call handle.GetAndResetFd() instead.

Comment 23

[Ethan Lin[:ethlin]]

Comment on attachment 8600788 [details] [diff] [review]
Part 2 - New FenceHandle in TextureHost

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

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +268,2 @@
>  #endif
>    return FenceHandle();

OK, I think we can just use GetAndResetReleaseFence().

Comment 24

[Ethan Lin[:ethlin]]

Comment on attachment 8602469 [details] [diff] [review]
Part 4 - New FenceHandle for TextureClient

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

::: widget/gonk/nativewindow/GonkNativeWindowLL.cpp
@@ +158,5 @@
>          return;
>      }
>  
> +    FenceHandle handle = client->GetAndResetReleaseFenceHandle();
> +    sp<Fence> fence = new Fence(handle.mFence.GetAndResetFd());

It will cause a compile error in the LL platform. I will fix it.

Comment 25

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

Comment 26

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

Comment 27

[Ethan Lin[:ethlin]]

Attachment: Part 2 - New FenceHandle in TextureHost

Comment 28

[Ethan Lin[:ethlin]]

Attachment: Part 4 - New FenceHandle for TextureClient

Comment 29

[Nicolas Silva [:nical]]

This is out of my comfort zone and there are other more competent reviewers so the review I gave was a bit superficial. Sotaro's black belt in android-fence-jutsu will compensate.

Comment 30

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8602652 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

Overall looks great! There are some problems need to address. sorry set to review-.

::: gfx/layers/ipc/FenceUtils.cpp
@@ +49,5 @@
> +{
> +}
> +
> +FenceHandle::FenceHandle(int aFenceFd)
> +  : mFence(new FdObj(aFenceFd))

Each FenceHandle seems to have different FdObj. From it, it seems that there could be multiple FdObjs for one Fencefd. It seems very dangerous from sanity check point of view. One FenceFd's owner should be one object. Incorrect/invalid fd usage is one of the very difficult to fix bugs. Therefore gecko code always have to ensure correct fd usage.

::: gfx/layers/ipc/FenceUtils.h
@@ +14,4 @@
>  namespace mozilla {
>  namespace layers {
>  
> +class FdObj {

This name seems too generic to be under layers namespace. And FdObj is used only within FenceHandle. It might be better to move it within FenceHandle.

@@ +19,5 @@
> +  friend class FenceHandle;
> +public:
> +  FdObj()
> +    : mFd(-1) {}
> +  FdObj(int aFd)

One-argument constructors normally need to be marked as explicit.

@@ +36,5 @@
> +struct FenceHandle {
> +public:
> +  FenceHandle();
> +
> +  FenceHandle(int aFenceFd);

One-argument constructors normally need to be marked as explicit.

Comment 31

[Ethan Lin[:ethlin]]

Comment on attachment 8602652 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

::: gfx/layers/ipc/FenceUtils.cpp
@@ +49,5 @@
> +{
> +}
> +
> +FenceHandle::FenceHandle(int aFenceFd)
> +  : mFence(new FdObj(aFenceFd))

Right, the original android fence also has this problem. We can set same Fencefd to different android fences in different FenceHandle. 
To solve this problem, I think we can use a static map object to record Fencefd to FbObj. If the constructor's Fd is already in the map, then we just use the previous FdObj without creating a new one. Do you think it makes sense? Or I don't need to fix this in this bug because it's not a new problem by the patch?

::: gfx/layers/ipc/FenceUtils.h
@@ +14,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +class FdObj {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(FdObj)

OK, I will move it to FenceHandle.

@@ +19,5 @@
> +  friend class FenceHandle;
> +public:
> +  FdObj()
> +    : mFd(-1) {}
> +  FdObj(int aFd)

OK!

@@ +36,5 @@
> +struct FenceHandle {
> +public:
> +  FenceHandle();
> +
> +  FenceHandle(int aFenceFd);

OK!

Comment 32

[Sotaro Ikeda [:sotaro]]

(In reply to Ethan Lin[:elin] from comment #31)
> Comment on attachment 8602652 [details] [diff] [review]
> Part 1 - Change to FenceHandle. Use fd object to replace android object
> 
> Review of attachment 8602652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/FenceUtils.cpp
> @@ +49,5 @@
> > +{
> > +}
> > +
> > +FenceHandle::FenceHandle(int aFenceFd)
> > +  : mFence(new FdObj(aFenceFd))
> 
> Right, the original android fence also has this problem. We can set same
> Fencefd to different android fences in different FenceHandle. 
> To solve this problem, I think we can use a static map object to record
> Fencefd to FbObj. If the constructor's Fd is already in the map, then we
> just use the previous FdObj without creating a new one. Do you think it
> makes sense? Or I don't need to fix this in this bug because it's not a new
> problem by the patch?

My comment was not clear enough. I am going to explain more. I do not think it is necessary to use a map.

What I wanted to say is the following.
- The code should ensure that one fd's owner should be one object.
  If we wanted to pass fd within multiple users,
  the fd should be wrapped by reference counted object and share the object within the users.
  Or explicitly pass the owner ship of the object like unique_ptr.

And I do not think android have this problem. In android framework works like the following.
  - If fence fd is not wrapped by fd, create android::Fence instance and wrap the fd by it.
  - Within same process, to pass the fd, instead pass the Fence instance. Do not pass raw fd.
     One Fence object is always used for the same fd.     
  - The Fence object is reference counted object, its lifetime is controlled by the reference.
  - It a fd is needed out side of the Fence object, create new fd by dup.

As explained above, one fd is always wrapped by one android::Fence object. Between different Fence object, they always wrap different fd.

In your patch, FenceHandle is used to pass around, FenceHandle is not reference counted object and it is easily copied to pass around. FenceHandle owns one FdObj. It is not clear why it is used like that. If FdObj is reference counted, it expects that is owned by multiple users. But is is not used like that.

Each FenceHandle owns different FdObj instances. There is not explicit semantics how one fd is passed and shared safely between multiple FenceHandles. In current code, it seems mostly ensured just by caller sides convention. I wanted to address this point. Does the question becomes clear?

Comment 33

[Sotaro Ikeda [:sotaro]]

My comments from another point of views are the followings.
- Usage of FdObj is not clear for me.
  If FdObj is reference counted, it expects that is owned by multiple users.
- Why does each FenceHandle creates each FdObj?
- What does FdObj represent?
- From fd usage consistency and safety point of view,
  fd ownership handling needs to be more explicit and consistent.

Comment 34

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> And I do not think android have this problem. In android framework works
> like the following.

But android's how to handle fd is not consistent enough.

Comment 35

[Sotaro Ikeda [:sotaro]]

elin, and I prefer to keep to keep 1-1 relationship between fence fd and wrapping object(like android::Fence) as much as possible to mitigate the debugging easier. fence fd is one of the major fd that is going to be affected by invalid fd bugs, because fence fds are created and destructed a lot. Even when invalid fd usage is caused by other module, fence fd often appear as invalid fd usage at first. Is there a problem to do it? At first I expected FdObj do it. But it seems not use for it.

Comment 36

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8602652 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

::: gfx/layers/ipc/FenceUtils.cpp
@@ +21,5 @@
> +ParamTraits<FenceHandle>::Write(Message* aMsg,
> +                                const paramType& aParam)
> +{
> +  FenceHandle handle = aParam;
> +  if (handle.IsValid()) {

I wanted to deny invalid FenceHandle in this function. Silent ignorance could be a risk of unexpected bug. Therefore the caller should send FenceHandle only when it is valid.

Comment 37

[Ethan Lin[:ethlin]]

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> As explained above, one fd is always wrapped by one android::Fence object.
> Between different Fence object, they always wrap different fd.
> 
> In your patch, FenceHandle is used to pass around, FenceHandle is not
> reference counted object and it is easily copied to pass around. FenceHandle
> owns one FdObj. It is not clear why it is used like that. If FdObj is
> reference counted, it expects that is owned by multiple users. But is is not
> used like that.
> 
> Each FenceHandle owns different FdObj instances. There is not explicit
> semantics how one fd is passed and shared safely between multiple
> FenceHandles. In current code, it seems mostly ensured just by caller sides
> convention. I wanted to address this point. Does the question becomes clear?

sotaro, I should say the original FenceHandle is almost the same. The original FenceHandle wrap a  sp<android::Fence> and we create a android::Fence in the constructor. And we also store/copy the FenceHandle in functions to let the sp<android::Fence> ref count increase. The difference is that, in my patches, I directly send the FenceFd to the FenceHandle on the caller side but not creating a RefPtr<FdObj> first. I think I can let the caller side pass the RefPtr<FdObj> to FenceHandle. Will that be better?

(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> elin, and I prefer to keep to keep 1-1 relationship between fence fd and
> wrapping object(like android::Fence) as much as possible to mitigate the
> debugging easier. fence fd is one of the major fd that is going to be
> affected by invalid fd bugs, because fence fds are created and destructed a
> lot. Even when invalid fd usage is caused by other module, fence fd often
> appear as invalid fd usage at first. Is there a problem to do it? At first I
> expected FdObj do it. But it seems not use for it.

I am not sure which part let you feel the fence fd and wrapping object is not 1-1? The FenceHandle wrapping the RefPtr<FdObj> is just like the original FenceHandle wrappign sp<android::Fence>. Could you point the issue more explicitly? Thanks.

Comment 38

[Sotaro Ikeda [:sotaro]]

(In reply to Ethan Lin[:elin] from comment #37)
> (In reply to Sotaro Ikeda [:sotaro] from comment #32)
> > As explained above, one fd is always wrapped by one android::Fence object.
> > Between different Fence object, they always wrap different fd.
> > 
> > In your patch, FenceHandle is used to pass around, FenceHandle is not
> > reference counted object and it is easily copied to pass around. FenceHandle
> > owns one FdObj. It is not clear why it is used like that. If FdObj is
> > reference counted, it expects that is owned by multiple users. But is is not
> > used like that.
> > 
> > Each FenceHandle owns different FdObj instances. There is not explicit
> > semantics how one fd is passed and shared safely between multiple
> > FenceHandles. In current code, it seems mostly ensured just by caller sides
> > convention. I wanted to address this point. Does the question becomes clear?
> 
> sotaro, I should say the original FenceHandle is almost the same. The
> original FenceHandle wrap a  sp<android::Fence> and we create a
> android::Fence in the constructor. And we also store/copy the FenceHandle in
> functions to let the sp<android::Fence> ref count increase. The difference
> is that, in my patches, I directly send the FenceFd to the FenceHandle on
> the caller side but not creating a RefPtr<FdObj> first. I think I can let
> the caller side pass the RefPtr<FdObj> to FenceHandle. Will that be better?

Yes, as in comment 35.

> 
> (In reply to Sotaro Ikeda [:sotaro] from comment #35)
> > elin, and I prefer to keep to keep 1-1 relationship between fence fd and
> > wrapping object(like android::Fence) as much as possible to mitigate the
> > debugging easier. fence fd is one of the major fd that is going to be
> > affected by invalid fd bugs, because fence fds are created and destructed a
> > lot. Even when invalid fd usage is caused by other module, fence fd often
> > appear as invalid fd usage at first. Is there a problem to do it? At first I
> > expected FdObj do it. But it seems not use for it.
> 
> I am not sure which part let you feel the fence fd and wrapping object is
> not 1-1? The FenceHandle wrapping the RefPtr<FdObj> is just like the
> original FenceHandle wrappign sp<android::Fence>. Could you point the issue
> more explicitly? Thanks.

In android, once fd is wrapped by android::Fence, fd's life time becomes same to android::Fence. But FdObj case, it is not. Every time fd is passed between FenceHandle, FdObj is recreated.

Comment 39

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

I change the input/output of FenceHandle to RefPtr<FdObj>. It should be clearer for the caller side.

Comment 40

[Ethan Lin[:ethlin]]

Attachment: Part 2 - New FenceHandle in TextureHost

Comment 41

[Ethan Lin[:ethlin]]

Attachment: Part 3 - New FenceHandle in Compositor

Comment 42

[Ethan Lin[:ethlin]]

Attachment: Part 4 - New FenceHandle for TextureClient

Comment 43

[Ethan Lin[:ethlin]]

Attachment: Part 5 - New FenceHandle in Decoder

Comment 44

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8604527 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

::: gfx/layers/ipc/FenceUtils.cpp
@@ +77,5 @@
> +}
> +
> +TemporaryRef<FenceHandle::FdObj>
> +FenceHandle::GetAndResetFd()
> +{

This function seems very confusing. It looks similar to FdObj::GetAndResetFd(). For me, the necessity is not clear.
If we want to clear a reference, the following is enough.
 > mFenceHandle = FenceHandle();

Can you explain about it?

::: gfx/layers/ipc/FenceUtils.h
@@ +42,5 @@
> +  };
> +
> +  FenceHandle();
> +
> +  explicit FenceHandle(const RefPtr<FdObj>& aFdObj);

gecko normally uses a raw pointer for reference counted object.

@@ +57,5 @@
> +  void Merge(const FenceHandle& aFenceHandle);
> +
> +  TemporaryRef<FdObj> GetAndResetFd();
> +
> +  TemporaryRef<FdObj> GetDupFd();

If we use nsRefPtr, here becomes already_AddRefed

@@ +60,5 @@
> +
> +  TemporaryRef<FdObj> GetDupFd();
> +
> +private:
> +  RefPtr<FdObj> mFence;

Please use nsRefPtr here. Currently, we prefer using nsRefPtr if there is no explicit reason to use RefPtr.

Comment 45

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8604527 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

::: gfx/layers/ipc/FenceUtils.cpp
@@ +77,5 @@
> +}
> +
> +TemporaryRef<FenceHandle::FdObj>
> +FenceHandle::GetAndResetFd()
> +{

Sorry, this part is OK for me. In some cases, it makes code simple. In some case, the code looks a bit wired. But it is OK.

> RefPtr<FenceHandle::FdObj> fence = mAcquireFence.GetAndResetFd();
> int fenceFd = fence->GetAndResetFd();

Comment 46

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8604529 [details] [diff] [review]
Part 3 - New FenceHandle in Compositor

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

review+ if comments are addressed.

::: widget/gonk/HwcComposer2D.cpp
@@ +854,5 @@
>              continue;
>          }
> +        FenceHandle fence = texture->GetAndResetAcquireFence();
> +        if (fence.IsValid()) {
> +            RefPtr<FenceHandle::FdObj> fdObj = fence.GetAndResetFd();

Please use nsRefPtr here.

@@ +867,5 @@
>      for (uint32_t j=0; j < (mList->numHwLayers - 1); j++) {
>          if (mList->hwLayers[j].releaseFenceFd >= 0) {
>              int fd = mList->hwLayers[j].releaseFenceFd;
>              mList->hwLayers[j].releaseFenceFd = -1;
> +            RefPtr<FenceHandle::FdObj> fdObj = new FenceHandle::FdObj(fd);

Please use nsRefPtr here.

Comment 47

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8604530 [details] [diff] [review]
Part 4 - New FenceHandle for TextureClient

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

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +193,5 @@
>  #if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
>              int fenceFd = mEGL->fDupNativeFenceFDANDROID(mEGL->Display(), sync);
>              if (fenceFd != -1) {
>                  mEGL->fDestroySync(mEGL->Display(), sync);
> +                RefPtr<FenceHandle::FdObj> fence = new FenceHandle::FdObj(fenceFd);

Please use nsRefPtr here.

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ +100,5 @@
>    }
>  
>  #if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
>     if (mReleaseFenceHandle.IsValid()) {
> +     RefPtr<FenceHandle::FdObj> fdObj = mReleaseFenceHandle.GetAndResetFd();

Please use nsRefPtr here.

@@ +144,3 @@
>    int32_t rv = mGraphicBuffer->lockAsync(usage,
>                                           reinterpret_cast<void**>(&mMappedBuffer),
> +                                         mReleaseFenceHandle.GetAndResetFd());

lockAsync request fd, but the above seems to pass FenceHandle::FdObj.

::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp
@@ +146,5 @@
>      if (index < 0) {
>      }
>  
> +    FenceHandle handle = client->GetAndResetReleaseFenceHandle();
> +    RefPtr<FenceHandle::FdObj> fdObj = handle.GetAndResetFd();

Please use nsRefPtr here.

::: widget/gonk/nativewindow/GonkNativeWindowKK.cpp
@@ +146,5 @@
>      if (index < 0) {
>      }
>  
> +    FenceHandle handle = client->GetAndResetReleaseFenceHandle();
> +    RefPtr<FenceHandle::FdObj> fdObj = handle.GetAndResetFd();

Please use nsRefPtr here.

::: widget/gonk/nativewindow/GonkNativeWindowLL.cpp
@@ +158,5 @@
>          return;
>      }
>  
> +    FenceHandle handle = client->GetAndResetReleaseFenceHandle();
> +    RefPtr<FenceHandle::FdObj> fdObj = handle.GetAndResetFd();

Please use nsRefPtr here.

Comment 48

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8604533 [details] [diff] [review]
Part 5 - New FenceHandle in Decoder

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

Review+ if the comments are addressed.

::: dom/media/omx/MediaCodecReader.cpp
@@ +819,2 @@
>  #if MOZ_WIDGET_GONK && ANDROID_VERSION >= 17
> +      RefPtr<FenceHandle::FdObj> fdObj = releasingItems[i].mReleaseFence.GetAndResetFd();

Please use nsRefPtr here.

::: dom/media/omx/OmxDecoder.cpp
@@ +878,5 @@
>    for (int i = 0; i < size; i++) {
>      MediaBuffer *buffer;
>      buffer = releasingVideoBuffers[i].mMediaBuffer;
>  #if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
> +    RefPtr<FenceHandle::FdObj> fdObj = releasingVideoBuffers.editItemAt(i).mReleaseFenceHandle.GetAndResetFd();

Please use nsRefPtr here.

Comment 49

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8604527 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

It might better to change the following functions names like GetAndResetFdObj. During the review, I feel that they looks like handling raw fd.
- FenceHandle::GetAndResetFd()
- FenceHandle::GetDupFd()

Comment 50

[Ethan Lin[:ethlin]]

Comment on attachment 8604527 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

::: gfx/layers/ipc/FenceUtils.h
@@ +42,5 @@
> +  };
> +
> +  FenceHandle();
> +
> +  explicit FenceHandle(const RefPtr<FdObj>& aFdObj);

OK, I will use a raw pointer.

@@ +57,5 @@
> +  void Merge(const FenceHandle& aFenceHandle);
> +
> +  TemporaryRef<FdObj> GetAndResetFd();
> +
> +  TemporaryRef<FdObj> GetDupFd();

Ok, I will do it.

@@ +60,5 @@
> +
> +  TemporaryRef<FdObj> GetDupFd();
> +
> +private:
> +  RefPtr<FdObj> mFence;

ditto.

Comment 51

[Ethan Lin[:ethlin]]

(In reply to Sotaro Ikeda [:sotaro] from comment #49)
> Comment on attachment 8604527 [details] [diff] [review]
> Part 1 - Change to FenceHandle. Use fd object to replace android object
> 
> Review of attachment 8604527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It might better to change the following functions names like
> GetAndResetFdObj. During the review, I feel that they looks like handling
> raw fd.
> - FenceHandle::GetAndResetFd()
> - FenceHandle::GetDupFd()

OK, I will add 'Obj' as the suffix.

Comment 52

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

Use the nsRefPtr to replace RefPtr and apply sotaro's comments.

Comment 53

[Ethan Lin[:ethlin]]

Attachment: Part 2 - New FenceHandle in TextureHost. r=sotaro

Use nsRefPtr to replace RefPtr.

Comment 54

[Ethan Lin[:ethlin]]

Attachment: Part 3 - New FenceHandle in Compositor. r=sotaro

Use nsRefPtr to replace RefPtr.

Comment 55

[Ethan Lin[:ethlin]]

Attachment: Part 4 - New FenceHandle for TextureClient

Use nsRefPtr to replace RefPtr and fix the wrong use of GetAndResetFd.

Comment 56

[Ethan Lin[:ethlin]]

Attachment: Part 5 - New FenceHandle in Decoder. r=sotaro

Use nsRefPtr to replace RefPtr.

Comment 57

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

Add a function TransferToAnotherFenceHandle to handle the fence transfer.

Comment 58

[Ethan Lin[:ethlin]]

Attachment: Part 2 - New FenceHandle in TextureHost. . (carry r+: sotaro)

Comment 59

[Ethan Lin[:ethlin]]

Attachment: Part 3 - New FenceHandle in Compositor. (carry r+:sotaro)

Comment 60

[Ethan Lin[:ethlin]]

Attachment: Part 4 - New FenceHandle for TextureClient

Comment 61

[Ethan Lin[:ethlin]]

Attachment: Part 5 - New FenceHandle in Decoder. (carry r+: sotaro)

Comment 62

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

Fix a wrong comment of nsRefPtr.

Comment 63

[Ethan Lin[:ethlin]]

Attachment: Part 4 - New FenceHandle for TextureClient

Comment 64

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object

This is the correct patch.

Comment 65

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8605133 [details] [diff] [review]
Part 1 - Change to FenceHandle. Use fd object to replace android object

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

Good!

Comment 66

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Change to FenceHandle. Use fd object to replace android object. (carry r+: sotaro)

Fix compile error.

Comment 67

[Ethan Lin[:ethlin]]

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

Comment 68

[Ethan Lin[:ethlin]]

(In reply to Ethan Lin[:elin] from comment #67)
> try server result:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb8722a24355

The test 'dt1' should be nothing to do with this patch, because there are many similar failures in other tries.

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abe9f6d9f540
https://hg.mozilla.org/integration/mozilla-inbound/rev/a449c033cadc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa993396952
https://hg.mozilla.org/integration/mozilla-inbound/rev/0984c5672f13
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2e6d38b6aa

Comment 70

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abe9f6d9f540
https://hg.mozilla.org/mozilla-central/rev/a449c033cadc
https://hg.mozilla.org/mozilla-central/rev/1aa993396952
https://hg.mozilla.org/mozilla-central/rev/0984c5672f13
https://hg.mozilla.org/mozilla-central/rev/cf2e6d38b6aa