Closed Bug 1146214 Opened 5 years ago Closed 4 years ago

Refactor fence delivery to reduce ipc messages and code duplication

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 36 obsolete files)

4.74 KB, patch
sotaro
: review+
nical
: review+
Details | Diff | Splinter Review
12.43 KB, patch
sotaro
: review+
nical
: review+
Details | Diff | Splinter Review
27.99 KB, patch
sotaro
: review+
nical
: review+
Details | Diff | Splinter Review
20.25 KB, patch
sotaro
: review+
nical
: review+
Details | Diff | Splinter Review
11.31 KB, patch
sotaro
: review+
nical
: review+
Details | Diff | Splinter Review
There is a complicate mechanism to handle fence delivery. I want to simplify the code to reduce code complexity and ipc messages.
Remove the unused functions in TiledContentHost and TiledLayerBuffer.
Attachment #8581403 - Flags: feedback?(hshih)
Attachment #8581403 - Flags: feedback?(chung)
Remove FenceDeliveryTracker and related async messages. On the client side, TextureClient holds the fence, and we will track the TextureClient, so we don't need the fence tracker. On the parent side, I will use CompositableTransactionManager to keep the fence.
Attachment #8581406 - Flags: feedback?(hshih)
Attachment #8581406 - Flags: feedback?(chung)
Since the texture will be used after SendUpdate message, we can just send the fence with SendUpdate.
Attachment #8581407 - Flags: feedback?(hshih)
Attachment #8581407 - Flags: feedback?(chung)
Handle the fence delivery in CompositableTransactionManager.
Attachment #8581408 - Flags: feedback?(hshih)
Attachment #8581408 - Flags: feedback?(chung)
We need to keep the release fence before content process gets it. I keep the fence in the CompositableTransactionParent and update it when receiving Update.
Attachment #8581410 - Flags: feedback?(hshih)
Attachment #8581410 - Flags: feedback?(chung)
Attachment #8581403 - Flags: feedback?(chung) → feedback+
In SendFence functions, there are some duplicated codes both in LayerTransactionParent and CompositableTransactionManager. I try to extract the part of FenceHandle to reduce the duplicated codes.
Attachment #8581415 - Flags: feedback?(hshih)
Attachment #8581415 - Flags: feedback?(chung)
Comment on attachment 8581406 [details] [diff] [review]
Part 2 - Remove FenceDeliveryTracker and related ipc messages

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

We want to unify most of the code path and there seems some more things to clean up.

And I would prefer new infrastructure patch first then removal of obsolete.

::: gfx/layers/ipc/LayersMessages.ipdlh
@@ +381,5 @@
>  };
>  
> +struct OpDeliverFenceFromChild {
> +  PTexture texture;
> +  FenceHandleFromChild fence;

FenceHandleFromChild is basically identical to FenceHandle, and OpDeliverFenceFromChild is the same with OpDeliverFence.

Why you still keep them as the purpose of these patches is refactor and cleanup.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +864,5 @@
>    }
>    mShadowManager->SendForceComposite();
>  }
>  
>  void ShadowLayerForwarder::SendPendingAsyncMessges()

Message.
Attachment #8581406 - Flags: feedback?(chung) → feedback-
Comment on attachment 8581407 [details] [diff] [review]
Part 3 - Combine fence delivery with SendUpdate

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

I think you need some more small clean up in this part.

And those change worth another check, so I leave f- for now.

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +121,3 @@
>    FenceHandle handle = aTexture->GetAcquireFenceHandle();
>    if (handle.IsValid()) {
> +    mTxn->AddEdit(OpDeliverFenceFromChild(nullptr, aTexture->GetIPDLActor(), FenceHandleFromChild(handle)));

To do this in this way for all platform, you will need a union like
union MaybeFenceFromChild {
  FenceFromChild;
  null_t;
}

and insert the fence here.

BTW, in our design, we want make fence a part of OpUseTexture, not a indepedent edit. So you should make OpUseTexture:
struct OpUseTexture {
  PCompositable compositable;
  PTexture texture;
  MaybeFence fence;
};

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +398,3 @@
>    FenceHandle handle = aTexture->GetAcquireFenceHandle();
>    if (handle.IsValid()) {
> +    mTxn->AddEdit(OpDeliverFenceFromChild(nullptr, aTexture->GetIPDLActor(), FenceHandleFromChild(handle)));

ditto.
Attachment #8581407 - Flags: feedback?(chung) → feedback-
Comment on attachment 8581408 [details] [diff] [review]
Part 4 - Handle the fence delivery

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

part 3 & 4 maybe merged into 1 to make the logic clearer.
f- for now for the same reason.

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +198,5 @@
> +        }
> +#endif
> +      }
> +      break;
> +    }

ditto.
Attachment #8581408 - Flags: feedback?(chung) → feedback-
Comment on attachment 8581410 [details] [diff] [review]
Part 5 - Use CompositableTransactionManager to hold the release fence

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

More documentation needed.

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +316,5 @@
> +    return false;
> +  }
> +  mReleaseFences.push_back(aReleaseFence);
> +  return true;
> +}

Based on our discussion before, I suspect you will have another patch to remove these right?

Before that, I think you should leave some comment to explain why we need this, and how this better than Trackers.
Currently, all these functions is for the same purpose as far as I can tell. Can we merge them?

::: gfx/layers/ipc/CompositableTransactionParent.h
@@ +71,5 @@
>    virtual void ReplyRemoveTexture(const OpReplyRemoveTexture& aReply) {}
>  
>    std::vector<AsyncParentMessageData> mPendingAsyncMessage;
> +  std::list<FenceHandle> mReleaseFences;
> +  std::list<FenceHandle> mPrevReleaseFences;

Why you need list?
I think vector is enough.

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +259,5 @@
>      layer_manager()->BeginTransaction();
>    }
>  
> +  // Backup release fence before handling composite messages
> +  BackupReleaseFence();

Explain why we need backup and why backup here.
Attachment #8581410 - Flags: feedback?(chung) → feedback+
Comment on attachment 8581415 [details] [diff] [review]
Part 6 - Combine some functions to reduce duplicate code

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

Looks good to me.

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +323,5 @@
>  
>  void
> +ImageBridgeParent::SendFenceHandleToTrackerIfPresent(const FenceHandle& fence,
> +                                                     uint64_t aDestHolderId,
> +                                                     uint64_t aTransactionId)

ImageBridgeParent tracks all fence passed by, that means it is FenceTracker. Rename this to ImageBridgeParent::TrackFence make it clearer.
Especially that ImageBridgeParent is an IPC actor, it has a bunch of SendXXX IPC functions, and this one has nothing to do with IPC, a name of SendXXX make it a little confusing.
Attachment #8581415 - Flags: feedback?(chung) → feedback+
Component: General → Graphics: Layers
Product: Firefox OS → Core
Merge the FenceHandle and FenceHandleFromChild. I send the process type when writing the message, and then we can know if the message is in-process.
Attachment #8581406 - Attachment is obsolete: true
Attachment #8581406 - Flags: feedback?(hshih)
Attachment #8582249 - Flags: feedback?(hshih)
Attachment #8582249 - Flags: feedback?(chung)
Fence is used to check if the buffer is accessible, so a fence's life time should be shorter than the corresponding buffer's life time. It should be safe to put the previous fence with the buffer. I will put the previous release fence to the SharedBufferManager before sending the fence to client for replacing the FenceDeliveryTracker.
This patch prepares a correspond fence with buffer in SharedBufferManager. We can set the previous release fence to here to prevent ipc problem.
Attachment #8581407 - Attachment is obsolete: true
Attachment #8581407 - Flags: feedback?(hshih)
Attachment #8582251 - Flags: feedback?(hshih)
Attachment #8582251 - Flags: feedback?(chung)
For client side, I merge the ipc message OpDeliverFenceFromChild to OpUseTexture, and I remove the FenceDeliveryTracker because the TextureClient will hold the fence and the RemoveTextureFromCompositableTracker will hold the TextureClient.
For parent side, GrallocTextureHost will move the fence to SharedBufferManager when CompositableTransactionManager try to send the fence to client. Since the SharedBufferManager holds the fence, we don't need the FenceDeliveryTracker to track the fence.
Attachment #8581408 - Attachment is obsolete: true
Attachment #8581408 - Flags: feedback?(hshih)
Attachment #8582256 - Flags: feedback?(hshih)
Attachment #8582256 - Flags: feedback?(chung)
Attached patch Part 5 - Fix typo (obsolete) — Splinter Review
This patch just fixes the typo of the function name.
Attachment #8581410 - Attachment is obsolete: true
Attachment #8581410 - Flags: feedback?(hshih)
Attachment #8582258 - Flags: feedback?(hshih)
Attachment #8582258 - Flags: feedback?(chung)
Attachment #8582251 - Attachment description: Patch 3 - Prepare fence colume in SharedBufferManager → Part 3 - Prepare fence colume in SharedBufferManager
Remove the reset unused functions after refactoring.
Attachment #8581415 - Attachment is obsolete: true
Attachment #8581415 - Flags: feedback?(hshih)
Attachment #8582259 - Flags: feedback?(hshih)
Attachment #8582259 - Flags: feedback?(chung)
Comment on attachment 8582249 [details] [diff] [review]
Part 2 - Combine FenceHandle and FenceHandleFromChild

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

Looks good to me.
Attachment #8582249 - Flags: feedback?(chung) → feedback+
Comment on attachment 8582251 [details] [diff] [review]
Part 3 - Prepare fence colume in SharedBufferManager

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

Some small fix needed. f- for now.

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +332,5 @@
>  }
>  
> +
> +/*static*/ void
> +SharedBufferManagerParent::BackupReleaseFence(ProcessId id,

The purpose of SomeFunc+SomeFuncImpl in this class is to make sure IPC runs on IPC thread, in this case, you have no IPC inside, you should use a Mutex to protect mBufferSets directly.

@@ +369,5 @@
> +
> +  if (handle.type() == MaybeMagicGrallocBufferHandle::TGrallocBufferRef) {
> +    key = handle.get_GrallocBufferRef().mKey;
> +  } else if (handle.type() == MaybeMagicGrallocBufferHandle::TMagicGrallocBufferHandle) {
> +    key = handle.get_MagicGrallocBufferHandle().mRef.mKey;

While backup Fence, you should not allow NewSurfaceDescriptorGralloc.

In the BufferManager design, NewSurfaceDescriptorGralloc is only legal for first alloc message (where we really serialize GraphicBuffer). All other SurfaceDescriptor should contains only GrallocBufferRef.
Please ASSERT here.

::: gfx/layers/ipc/SharedBufferManagerParent.h
@@ +100,5 @@
>    static std::map<base::ProcessId, SharedBufferManagerParent*> sManagers;
>  
>  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> +
> +  struct BufferSet {

BufferSet makes me think it is a sub-class of Set.
GrallocItem or something else maybe better

@@ +109,3 @@
>    /**
> +   * Buffers and previous release fence owned by this
> +   * SharedBufferManager pair. Hold previous ReleaseFence to 

clear the tailing space.

@@ +109,4 @@
>    /**
> +   * Buffers and previous release fence owned by this
> +   * SharedBufferManager pair. Hold previous ReleaseFence to 
> +   * prevent Fence delivery failure via gecko IPC.

Why Fence delivery fail if we not hold onto the sp?
The problem should be we may release the origin Fence before the receiver can add ref to it.
Attachment #8582251 - Flags: feedback?(chung) → feedback-
Comment on attachment 8582256 [details] [diff] [review]
Part 4 - implement the fence delivery

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

Some minor check needed. f- for now.

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +299,5 @@
> +  FenceHandle fence;
> +  if (aCompositableHost && aCompositableHost->GetCompositor()) {
> +    fence = aCompositableHost->GetCompositor()->GetReleaseFence();
> +  }
> +  aTexture->SetReleaseFenceHandle(fence);

I feel something wrong in this function.

You can return fence directly, rather than Set + GetAndClear.
As you always Clear mCurrentFence by GetAndReset, the Set function always set the new Fence as mCurrentFence (no way to hit the merge case in it).

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +399,5 @@
>  }
>  
> +ImageBridgeParent::SendFenceHandleToTrackerIfPresent(const FenceHandle& fence,
> +                                                     uint64_t aDestHolderId,
> +                                                     uint64_t aTransactionId)

Please rename this as you do not Send anything to anywhere.

@@ +408,4 @@
>  }
>  
>  /*static*/ void
>  ImageBridgeParent::SendFenceHandleToTrackerIfPresent(base::ProcessId aChildProcessId,

ditto.

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ +29,5 @@
>  , mMappedBuffer(nullptr)
>  , mMediaBuffer(nullptr)
>  , mIsOpaque(gfx::IsOpaque(aFormat))
>  {
> +  AddFlags(TextureFlags::DEALLOCATE_CLIENT);

Is this valid?

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +277,5 @@
> +    base::ProcessId owner;
> +    if (handle.type() == MaybeMagicGrallocBufferHandle::TGrallocBufferRef) {
> +      owner = handle.get_GrallocBufferRef().mOwner;
> +    } else {
> +      owner = handle.get_MagicGrallocBufferHandle().mRef.mOwner;

Please ASSERT here.
Attachment #8582256 - Flags: feedback?(chung) → feedback-
Comment on attachment 8582256 [details] [diff] [review]
Part 4 - implement the fence delivery

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

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +424,4 @@
>  }
>  
>  /*static*/ void
> +ImageBridgeParent::SendPendingAsyncMessages(base::ProcessId aChildProcessId)

Move the type fix to next part.

@@ +429,5 @@
>    ImageBridgeParent* imageBridge = ImageBridgeParent::GetInstance(aChildProcessId);
>    if (!imageBridge) {
>      return;
>    }
> +  imageBridge->SendPendingAsyncMessages();

ditto.
Comment on attachment 8582258 [details] [diff] [review]
Part 5 - Fix typo

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

Looks good to me. Except the typo in ImageBridge seems fixed in last part, too.
Attachment #8582258 - Flags: feedback?(chung) → feedback+
Comment on attachment 8582259 [details] [diff] [review]
Part 6 - Remove the reset unused functions

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

Rest? Reset? Please fix the commit string.
Attachment #8582259 - Flags: feedback?(chung) → feedback+
Attachment #8581403 - Flags: feedback?(hshih) → feedback+
Attachment #8582249 - Flags: feedback?(hshih) → feedback+
Comment on attachment 8582256 [details] [diff] [review]
Part 4 - implement the fence delivery

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

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +281,5 @@
> +      owner = handle.get_MagicGrallocBufferHandle().mRef.mOwner;
> +    }
> +    SharedBufferManagerParent::BackupReleaseFence(owner, mGrallocHandle,
> +                                                  FenceHandle(mReleaseFence));
> +  }

What happen when the handle.type() is null?
Suggest to add moz_assert or moz_crash for it.

::: gfx/layers/opengl/GrallocTextureHost.h
@@ +51,5 @@
>  
>    virtual bool BindTextureSource(CompositableTextureSourceRef& aTextureSource) MOZ_OVERRIDE;
>  
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
> +  virtual android::sp<android::Fence> GetAndResetReleaseFence() MOZ_OVERRIDE;

How about rename this to PopReleaseFence() which implies get and clear the releasefence?
Attachment #8582256 - Flags: feedback- → feedback?(chung)
Attachment #8582256 - Flags: feedback?(chung)
Comment on attachment 8582251 [details] [diff] [review]
Part 3 - Prepare fence colume in SharedBufferManager

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

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +335,5 @@
> +/*static*/ void
> +SharedBufferManagerParent::BackupReleaseFence(ProcessId id,
> +                                              mozilla::layers::SurfaceDescriptor aDesc,
> +                                              const FenceHandle& fence)
> +{

Missing #ifdef  MOZ_HAVE_SURFACEDESCRIPTORGRALLOC

@@ +359,5 @@
> +  }
> +
> +#ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> +  int64_t key = -1;
> +  MaybeMagicGrallocBufferHandle handle;

Please return here directly if type is not gralloc.

::: gfx/layers/ipc/SharedBufferManagerParent.h
@@ +62,5 @@
>    /**
>     * Break the buffer's sharing state, decrease buffer reference for both side
>     */
>    static void DropGrallocBuffer(ProcessId id, mozilla::layers::SurfaceDescriptor aDesc);
>  

Please add description about this function and which thread will call this.
Apply the feedback to this patch.
Attachment #8582251 - Attachment is obsolete: true
Attachment #8582251 - Flags: feedback?(hshih)
Attachment #8583506 - Flags: feedback?(hshih)
Attachment #8583506 - Flags: feedback?(chung)
Apply the feedback to this patch.
Attachment #8582256 - Attachment is obsolete: true
Attachment #8582256 - Flags: feedback?(hshih)
Attachment #8583507 - Flags: feedback?(hshih)
Attachment #8583507 - Flags: feedback?(chung)
Attached patch Part 5 - Fix typo (obsolete) — Splinter Review
Update the patch due to the change of Part4 patch
Attachment #8582258 - Attachment is obsolete: true
Attachment #8582258 - Flags: feedback?(hshih)
Update the patch due to the change of Part4 patch.
Attachment #8582259 - Attachment is obsolete: true
Attachment #8582259 - Flags: feedback?(hshih)
Correct the patch.
Attachment #8583506 - Attachment is obsolete: true
Attachment #8583506 - Flags: feedback?(hshih)
Attachment #8583506 - Flags: feedback?(chung)
Attachment #8583511 - Flags: feedback?(hshih)
Attachment #8583511 - Flags: feedback?(chung)
Correct the patch.
Attachment #8583507 - Attachment is obsolete: true
Attachment #8583507 - Flags: feedback?(hshih)
Attachment #8583507 - Flags: feedback?(chung)
Attachment #8583512 - Flags: feedback?(hshih)
Attachment #8583512 - Flags: feedback?(chung)
Attached patch Part 5.1 - Fix typo (obsolete) — Splinter Review
Correct the patch.
Attachment #8583509 - Attachment is obsolete: true
Correct the patch.
Attachment #8583510 - Attachment is obsolete: true
Comment on attachment 8583511 [details] [diff] [review]
Part 3.1 - Prepare fence column and function in SharedBufferManager

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

Some minor suggestion.

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +339,5 @@
> +  mgr->KeepReleaseFenceImpl(aDesc, fence);
> +}
> +
> +/*static*/ void
> +SharedBufferManagerParent::KeepReleaseFence(ProcessId id,

Drop this argument and get it from SurfaceDescriptor directly, otherwise you will have to extract the pid from SurfaceDescriptor or GrallocHandle from caller, too.

And this way, you have to examine the validity of SurfaceDescriptor here and ASSERT here directly, which make debug easier. (assert on the caller thread is easier to get its backtrace)
Attachment #8583511 - Flags: feedback?(chung) → feedback+
Comment on attachment 8583512 [details] [diff] [review]
Part 4.1 - implement the fence delivery

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

I am still not sure whether AddFlags(TextureFlags::DEALLOCATE_CLIENT) in GrallocTextureClient good or not.
f+ for now, I will check related code. Maybe others can tell whether it is good or not.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +268,5 @@
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
> +android::sp<android::Fence>
> +GrallocTextureHostOGL::RemoveReleaseFence()
> +{
> +  // Hold previous ReleaseFence in SharedBufferManager to prevent Fence delivery

Copy the comment from SharedBufferManagerParent or explain why we must hold the Fence until client side recv it.
Comment on attachment 8583512 [details] [diff] [review]
Part 4.1 - implement the fence delivery

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

I am still not sure whether AddFlags(TextureFlags::DEALLOCATE_CLIENT) in GrallocTextureClient good or not.
f+ for now, I will check related code. Maybe others can tell whether it is good or not.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +268,5 @@
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
> +android::sp<android::Fence>
> +GrallocTextureHostOGL::RemoveReleaseFence()
> +{
> +  // Hold previous ReleaseFence in SharedBufferManager to prevent Fence delivery

Copy the comment from SharedBufferManagerParent or explain why we must hold the Fence until client side recv it.
Attachment #8583512 - Flags: feedback?(chung) → feedback+
Attached image Fence_flow_of_LayerTransaction.svg (obsolete) —
The current fence flow is as the attached file.

I think there are some advantages:
1. Reduce the ipc messages between b2g and content.
2. Reduce the allocation and deallocation of FenceDeliveryTracker.
3. Simplify the logic of the fence transmission. 
4. Hide the control of fence life-time in the TextureHost. LayerTransaction and ImageBridge don’t need to handle the fence life-time.
Comment on attachment 8583511 [details] [diff] [review]
Part 3.1 - Prepare fence column and function in SharedBufferManager

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

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +353,5 @@
> +  if (!mgr) {
> +    return;
> +  }
> +
> +  if (PlatformThread::CurrentId() == mgr->mThread->thread_id()) {

How about just check the thread module in debug build?

MOZ_ASSERT(PlatformThread::CurrentId() != mgr->mThread->thread_id())

@@ +369,5 @@
> +void
> +SharedBufferManagerParent::KeepReleaseFenceImpl(mozilla::layers::SurfaceDescriptor aDesc,
> +                                                const FenceHandle& fence)
> +{
> +#ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC

add an assert here to check the thread model.

::: gfx/layers/ipc/SharedBufferManagerParent.h
@@ +97,5 @@
> +
> +  // dispatched function
> +  static void KeepReleaseFenceSync(SharedBufferManagerParent* mgr,
> +                                   mozilla::layers::SurfaceDescriptor aDesc,
> +                                   const FenceHandle& fence);

KeepReleaseFence is used for Gonk only. Should we move all related functions into |MOZ_HAVE_SURFACEDESCRIPTORGRALLOC| section?
Attachment #8583511 - Flags: feedback?(hshih) → feedback+
Comment on attachment 8583512 [details] [diff] [review]
Part 4.1 - implement the fence delivery

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

I'm not sure the usage of TextureFlags::DEALLOCATE_CLIENT flag that is correct or not, but these patches look good.

::: gfx/layers/ipc/ImageBridgeParent.h
@@ +122,5 @@
>  
>    static void ReplyRemoveTexture(base::ProcessId aChildProcessId,
>                                   const OpReplyRemoveTexture& aReply);
>  
> +  void PendDeliverFenceMessage(const FenceHandle& fence,

AppendXXXMessage?

@@ +126,5 @@
> +  void PendDeliverFenceMessage(const FenceHandle& fence,
> +                               uint64_t aDestHolderId,
> +                               uint64_t aTransactionId);
> +
> +  static void PendDeliverFenceMessage(base::ProcessId aChildProcessId,

ditto

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ +29,5 @@
>  , mMappedBuffer(nullptr)
>  , mMediaBuffer(nullptr)
>  , mIsOpaque(gfx::IsOpaque(aFormat))
>  {
> +  AddFlags(TextureFlags::DEALLOCATE_CLIENT);

Need layer expert to review this.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +279,5 @@
> +    MOZ_ASSERT(handle.type() == MaybeMagicGrallocBufferHandle::TGrallocBufferRef);
> +
> +    owner = handle.get_GrallocBufferRef().mOwner;
> +
> +    SharedBufferManagerParent::KeepReleaseFence(owner, mGrallocHandle,

Could we move this |KeepReleaseFeuce| to the last node we try to pass to ipc? It's hard to know we still keep the fence in |RemoveReleaseFence|.

::: gfx/layers/opengl/TextureHostOGL.h
@@ +143,5 @@
>  
>    /**
>     * Store a fence that will signal when the current buffer is no longer being read.
>     * Similar to android's GLConsumer::setReleaseFence()
>     */

Please update the comment for this function. |Store| and |merge| is not the same meaning.

@@ -170,5 @@
> -   * Hold previous ReleaseFence to prevent Fence delivery failure via gecko IPC.
> -   * Fence is a kernel object and its lifetime is managed by a reference count.
> -   * Until the Fence is delivered to client side, need to hold Fence on host side.
> -   */
> -  android::sp<android::Fence> mPrevReleaseFence;

To reviewer:
We hold the release fence at SharedBufferManager.
Attachment #8583512 - Flags: feedback?(hshih) → feedback+
Apply the Jerry and Chiajung's feedback in this patch.
Attachment #8583511 - Attachment is obsolete: true
Apply the Jerry and Chiajung's feedback in this patch.
Attachment #8583512 - Attachment is obsolete: true
Attached patch Part 5.2 - Fix typo (obsolete) — Splinter Review
Update the patch due to the change of Part 4.
Update the patch due to the change of Part 4.
Attachment #8583514 - Attachment is obsolete: true
Attachment #8583516 - Attachment is obsolete: true
Attachment #8581403 - Flags: review?(sotaro.ikeda.g)
Attachment #8581403 - Flags: review?(nical.bugzilla)
Attachment #8582249 - Flags: review?(sotaro.ikeda.g)
Attachment #8582249 - Flags: review?(nical.bugzilla)
Attachment #8584239 - Flags: review?(sotaro.ikeda.g)
Attachment #8584239 - Flags: review?(nical.bugzilla)
Attachment #8584240 - Flags: review?(sotaro.ikeda.g)
Attachment #8584240 - Flags: review?(nical.bugzilla)
Attachment #8584241 - Flags: review?(sotaro.ikeda.g)
Attachment #8584241 - Flags: review?(nical.bugzilla)
Attachment #8584242 - Flags: review?(sotaro.ikeda.g)
Attachment #8584242 - Flags: review?(nical.bugzilla)
This patch is the correct one.
Attachment #8584240 - Attachment is obsolete: true
Attachment #8584240 - Flags: review?(sotaro.ikeda.g)
Attachment #8584240 - Flags: review?(nical.bugzilla)
Attachment #8584246 - Flags: review?(sotaro.ikeda.g)
Attachment #8584246 - Flags: review?(nical.bugzilla)
Attached patch Part 5.2 - Fix typo (obsolete) — Splinter Review
This patch is the correct one.
Attachment #8584247 - Flags: review?(sotaro.ikeda.g)
Attachment #8584247 - Flags: review?(nical.bugzilla)
Attachment #8584241 - Attachment is obsolete: true
Attachment #8584241 - Flags: review?(sotaro.ikeda.g)
Attachment #8584241 - Flags: review?(nical.bugzilla)
Comment on attachment 8584246 [details] [diff] [review]
Part 4.2 - implement the fence delivery

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

This is a massive changeset in a part of layers that I don't understand as well as I should so it'll take me some time to digest it. So far I like it. This is removing more code than it is adding and the simplification looks worthwhile.

::: gfx/layers/composite/TextureHost.cpp
@@ +159,5 @@
> +{
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
> +  TextureHostOGL* hostOGL = this->AsHostOGL();
> +  if (hostOGL) {
> +    return hostOGL->MergeReleaseFence(fence.mFence);

nit: I would prefer to have the OpenGL TextureHost classes implement MergeReleaseFenceHandle(FenceHandle), rather than doing the dispatch here.

@@ +160,5 @@
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
> +  TextureHostOGL* hostOGL = this->AsHostOGL();
> +  if (hostOGL) {
> +    return hostOGL->MergeReleaseFence(fence.mFence);
> +  }

wouldn't it make sense to return false if (!hostOGL)?
adding a boolean value that is not used tends to give the false impression that error cases are handled properly, It'd be better if users of this function checked the return type, if only with something like:
DebugOnly<bool> status = texture->MergeReleaseFenceHandle(fence);
MOZ_ASSERT(status);
Or even crash release builds since I don't know how good the test coverage is with debug builds on b2g (I know it used to be pretty bad a little while ago).
Comment on attachment 8582249 [details] [diff] [review]
Part 2 - Combine FenceHandle and FenceHandleFromChild

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

::: gfx/layers/ipc/FenceUtilsGonk.cpp
@@ +93,5 @@
>      // the right thing and dup's the fd.  If it's shared cross-thread,
>      // SCM_RIGHTS doesn't dup the fd.  That's surprising, but we just
>      // deal with it here.  NB: only the "default" (master) process can
>      // alloc gralloc buffers.
> +    bool sameProcess = (XRE_GetProcessType() == processType);

If malformed client send processType as GeckoProcessType_Default, b2g process seems to leak fd. How could it be prevented?
Flags: needinfo?(etlin)
(In reply to Ethan Lin[:Ethan] from comment #37)
> Created attachment 8583548 [details]
> Fence_flow_of_LayerTransaction.svg
> 
> The current fence flow is as the attached file.
> 
> I think there are some advantages:
> 1. Reduce the ipc messages between b2g and content.
> 2. Reduce the allocation and deallocation of FenceDeliveryTracker.
> 3. Simplify the logic of the fence transmission. 
> 4. Hide the control of fence life-time in the TextureHost. LayerTransaction
> and ImageBridge don’t need to handle the fence life-time.

Ethan, in this diagram, how fence's life time is ensured if fence is delivered from content to b2g process?
Comment on attachment 8584246 [details] [diff] [review]
Part 4.2 - implement the fence delivery

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

::: gfx/layers/opengl/TextureHostOGL.h
@@ -170,5 @@
> -   * Hold previous ReleaseFence to prevent Fence delivery failure via gecko IPC.
> -   * Fence is a kernel object and its lifetime is managed by a reference count.
> -   * Until the Fence is delivered to client side, need to hold Fence on host side.
> -   */
> -  android::sp<android::Fence> mPrevReleaseFence;

This is dead code. Actually, it does nothing about preventing Fence delivery failure via gecko IPC.
Comment on attachment 8584239 [details] [diff] [review]
Part 3.2 - Prepare fence column and function in SharedBufferManager

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

::: gfx/layers/ipc/SharedBufferManagerParent.h
@@ +118,5 @@
>  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> +
> +  struct GrallocItem {
> +    android::sp<android::GraphicBuffer> buffer;
> +    FenceHandle fence;

This hold only one fence. If one TextureHost is rendered to multiple ImageHosts, in this case, multiple OpRemoveTextureAsyncs could be handled to one TextureHost concurrently. Because OpRemoveTextureAsync is a operation from ImageClient to ImageHost.

In this situation, how a life times of the Fences are ensured? The Fences are related to only on TexureHost(gralloc buffer).
> Ethan, in this diagram, how fence's life time is ensured if fence is
> delivered from content to b2g process?
Hi Sotaro,
The TextureClient holds the acquire fence, and the TrackerHolder will hold the TextureClient until receiving the OpReplyRemoveTexture. So the acquire fence life time is ensured by the RemoveTextureFromCompositableTracker.
Attached image Fence_flow_of_LayerTransaction.svg (obsolete) —
Update the diagram for content's fence.
Attachment #8583548 - Attachment is obsolete: true
Apply comment46 in this patch.
Attachment #8584246 - Attachment is obsolete: true
Attachment #8584246 - Flags: review?(sotaro.ikeda.g)
Attachment #8584246 - Flags: review?(nical.bugzilla)
Attachment #8585934 - Flags: review?(sotaro.ikeda.g)
Attachment #8585934 - Flags: review?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #49)
> Comment on attachment 8584246 [details] [diff] [review]
> Part 4.2 - implement the fence delivery
> 
> Review of attachment 8584246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/TextureHostOGL.h
> @@ -170,5 @@
> > -   * Hold previous ReleaseFence to prevent Fence delivery failure via gecko IPC.
> > -   * Fence is a kernel object and its lifetime is managed by a reference count.
> > -   * Until the Fence is delivered to client side, need to hold Fence on host side.
> > -   */
> > -  android::sp<android::Fence> mPrevReleaseFence;
> 
> This is dead code. Actually, it does nothing about preventing Fence delivery
> failure via gecko IPC.

Right. The actual item to keep fence life-time is tracker now.
(In reply to Sotaro Ikeda [:sotaro] from comment #50)
> Comment on attachment 8584239 [details] [diff] [review]
> Part 3.2 - Prepare fence column and function in SharedBufferManager
> 
> Review of attachment 8584239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/SharedBufferManagerParent.h
> @@ +118,5 @@
> >  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> > +
> > +  struct GrallocItem {
> > +    android::sp<android::GraphicBuffer> buffer;
> > +    FenceHandle fence;
> 
> This hold only one fence. If one TextureHost is rendered to multiple
> ImageHosts, in this case, multiple OpRemoveTextureAsyncs could be handled to
> one TextureHost concurrently. Because OpRemoveTextureAsync is a operation
> from ImageClient to ImageHost.
> 
> In this situation, how a life times of the Fences are ensured? The Fences
> are related to only on TexureHost(gralloc buffer).

May I ask in which case we will have multiple ImageHosts to hold the same TextureHost? Or this is just a possible problem in the future?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #47)
> Comment on attachment 8582249 [details] [diff] [review]
> Part 2 - Combine FenceHandle and FenceHandleFromChild
> 
> Review of attachment 8582249 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/FenceUtilsGonk.cpp
> @@ +93,5 @@
> >      // the right thing and dup's the fd.  If it's shared cross-thread,
> >      // SCM_RIGHTS doesn't dup the fd.  That's surprising, but we just
> >      // deal with it here.  NB: only the "default" (master) process can
> >      // alloc gralloc buffers.
> > +    bool sameProcess = (XRE_GetProcessType() == processType);
> 
> If malformed client send processType as GeckoProcessType_Default, b2g
> process seems to leak fd. How could it be prevented?

I will think if there is other way to prevent the leakage of fd in this case.
(In reply to Ethan Lin[:Ethan] from comment #55)
> 
> May I ask in which case we will have multiple ImageHosts to hold the same
> TextureHost? Or this is just a possible problem in the future?

We have had this for a while, even though it's not the most commonly used feature on the web. You can have any number of ImageLayer reading from the same ImageContainer, and they will share the same TextureHost (but have an ImageHost per layer). For instance you can set the source of video element to be the output of another one from javascript and it will typically do that.
(In reply to Ethan Lin[:Ethan] from comment #55)
> > 
> > In this situation, how a life times of the Fences are ensured? The Fences
> > are related to only on TexureHost(gralloc buffer).
> 
> May I ask in which case we will have multiple ImageHosts to hold the same
> TextureHost? Or this is just a possible problem in the future?

There are two main use cases. One is for video, another is for image animation.

If we are going to use WebRTC more, video's use case becomes more common. Recent fix of Bug 1147435 is also caused by multiple ImageHosts use case.

Image animation example is Bug 1015984. In the bug, attachment 8428732 [details] is attached as a sample applicaiton.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Ethan Lin[:Ethan] from comment #56)
> (In reply to Sotaro Ikeda [:sotaro] from comment #47)
> > Comment on attachment 8582249 [details] [diff] [review]
> > Part 2 - Combine FenceHandle and FenceHandleFromChild
> > 
> > Review of attachment 8582249 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/ipc/FenceUtilsGonk.cpp
> > @@ +93,5 @@
> > >      // the right thing and dup's the fd.  If it's shared cross-thread,
> > >      // SCM_RIGHTS doesn't dup the fd.  That's surprising, but we just
> > >      // deal with it here.  NB: only the "default" (master) process can
> > >      // alloc gralloc buffers.
> > > +    bool sameProcess = (XRE_GetProcessType() == processType);
> > 
> > If malformed client send processType as GeckoProcessType_Default, b2g
> > process seems to leak fd. How could it be prevented?
> 
> I will think if there is other way to prevent the leakage of fd in this case.

FenceHandleFromChild creation was intentional from the security reason. My first implementation used only FenceHandle.
Depends on: 1149492
Rebase the patch.
Attachment #8581403 - Attachment is obsolete: true
Attachment #8581403 - Flags: review?(sotaro.ikeda.g)
Attachment #8581403 - Flags: review?(nical.bugzilla)
Attachment #8587093 - Flags: review?(sotaro.ikeda.g)
Attachment #8587093 - Flags: review?(nical.bugzilla)
Rebase the patch.
Attachment #8587093 - Attachment is obsolete: true
Attachment #8587093 - Flags: review?(sotaro.ikeda.g)
Attachment #8587093 - Flags: review?(nical.bugzilla)
Attachment #8587094 - Flags: review?(sotaro.ikeda.g)
Attachment #8587094 - Flags: review?(nical.bugzilla)
I add a function to dup the fence when receiving a in-process fence from IPC.
Attachment #8582249 - Attachment is obsolete: true
Attachment #8582249 - Flags: review?(sotaro.ikeda.g)
Attachment #8582249 - Flags: review?(nical.bugzilla)
Attachment #8587095 - Flags: review?(sotaro.ikeda.g)
Attachment #8587095 - Flags: review?(nical.bugzilla)
Rebase the patch.
Attachment #8584239 - Attachment is obsolete: true
Attachment #8584239 - Flags: review?(sotaro.ikeda.g)
Attachment #8584239 - Flags: review?(nical.bugzilla)
Attachment #8587096 - Flags: review?(sotaro.ikeda.g)
Attachment #8587096 - Flags: review?(nical.bugzilla)
I set the CompositorOGL release fence to TextureHost when setting HWC release fence. So the final release fence will be holded by TextureHost.
Attachment #8585934 - Attachment is obsolete: true
Attachment #8585934 - Flags: review?(sotaro.ikeda.g)
Attachment #8585934 - Flags: review?(nical.bugzilla)
Attachment #8587099 - Flags: review?(sotaro.ikeda.g)
Attachment #8587099 - Flags: review?(nical.bugzilla)
Attached patch Part 5.3 - Fix typo (obsolete) — Splinter Review
Rebase the patch.
Attachment #8584247 - Attachment is obsolete: true
Attachment #8584247 - Flags: review?(sotaro.ikeda.g)
Attachment #8584247 - Flags: review?(nical.bugzilla)
Attachment #8587100 - Flags: review?(sotaro.ikeda.g)
Attachment #8587100 - Flags: review?(nical.bugzilla)
Attached patch Part 5.4 - Fix typo (obsolete) — Splinter Review
Rebase the patch.
Attachment #8587100 - Attachment is obsolete: true
Attachment #8587100 - Flags: review?(sotaro.ikeda.g)
Attachment #8587100 - Flags: review?(nical.bugzilla)
Attachment #8587101 - Flags: review?(sotaro.ikeda.g)
Attachment #8587101 - Flags: review?(nical.bugzilla)
Rebase the patch.
Attachment #8584242 - Attachment is obsolete: true
Attachment #8584242 - Flags: review?(sotaro.ikeda.g)
Attachment #8584242 - Flags: review?(nical.bugzilla)
Attachment #8587102 - Flags: review?(sotaro.ikeda.g)
Attachment #8587102 - Flags: review?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #59)
> (In reply to Ethan Lin[:Ethan] from comment #56)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #47)
> > > Comment on attachment 8582249 [details] [diff] [review]
> > > Part 2 - Combine FenceHandle and FenceHandleFromChild
> > > 
> > > Review of attachment 8582249 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: gfx/layers/ipc/FenceUtilsGonk.cpp
> > > @@ +93,5 @@
> > > >      // the right thing and dup's the fd.  If it's shared cross-thread,
> > > >      // SCM_RIGHTS doesn't dup the fd.  That's surprising, but we just
> > > >      // deal with it here.  NB: only the "default" (master) process can
> > > >      // alloc gralloc buffers.
> > > > +    bool sameProcess = (XRE_GetProcessType() == processType);
> > > 
> > > If malformed client send processType as GeckoProcessType_Default, b2g
> > > process seems to leak fd. How could it be prevented?
> > 
> > I will think if there is other way to prevent the leakage of fd in this case.
> 
> FenceHandleFromChild creation was intentional from the security reason. My
> first implementation used only FenceHandle.

In the new patch, I dup the fence by ipc actor when it gets fence. If Bug 1149492 fixed, then we can remove the manual duplication.
Flags: needinfo?(etlin)
(In reply to Sotaro Ikeda [:sotaro] from comment #50)
> Comment on attachment 8584239 [details] [diff] [review]
> Part 3.2 - Prepare fence column and function in SharedBufferManager
> 
> Review of attachment 8584239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/SharedBufferManagerParent.h
> @@ +118,5 @@
> >  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> > +
> > +  struct GrallocItem {
> > +    android::sp<android::GraphicBuffer> buffer;
> > +    FenceHandle fence;
> 
> This hold only one fence. If one TextureHost is rendered to multiple
> ImageHosts, in this case, multiple OpRemoveTextureAsyncs could be handled to
> one TextureHost concurrently. Because OpRemoveTextureAsync is a operation
> from ImageClient to ImageHost.
> 
> In this situation, how a life times of the Fences are ensured? The Fences
> are related to only on TexureHost(gralloc buffer).

All OpRemoveTextureAsync messages are processed by Compositor thread. In my patch, the final release fence will be held by TextureHost. So if we receive two OpRemoveTextureAsync in a transaction, for first message, we will do the following things:
1. keep the fence in SharedBufferManager
2. clear the fence in TextureHost
3. send the release fence to the corresponding TextureClient

For the second message, due to the clearance of fence in the first message, we will do nothing. Because the fence and TextureHost is always 1 to 1, so the fence life time should be safe in the multiple ImageHost case.
(In reply to Ethan Lin[:Ethan] from comment #69)
> 
> For the second message, due to the clearance of fence in the first message,
> we will do nothing. Because the fence and TextureHost is always 1 to 1, so
> the fence life time should be safe in the multiple ImageHost case.

Ethan, can you explain more about it? A relationship between fence and TextureHost is not 1 to 1. TextureHost hold new fence for each Composition.
Flags: needinfo?(etlin)
Sotaro, I think there is a fence life time problem of different transactions when we have multiple ImageHost to one TextureHost. I will modify the patch to handle this case. Thanks for your suggestion.
Flags: needinfo?(etlin)
Rebase the patch.
Attachment #8585922 - Attachment is obsolete: true
Attachment #8587094 - Attachment is obsolete: true
Attachment #8587095 - Attachment is obsolete: true
Attachment #8587096 - Attachment is obsolete: true
Attachment #8587099 - Attachment is obsolete: true
Attachment #8587101 - Attachment is obsolete: true
Attachment #8587102 - Attachment is obsolete: true
Attachment #8587094 - Flags: review?(sotaro.ikeda.g)
Attachment #8587094 - Flags: review?(nical.bugzilla)
Attachment #8587095 - Flags: review?(sotaro.ikeda.g)
Attachment #8587095 - Flags: review?(nical.bugzilla)
Attachment #8587096 - Flags: review?(sotaro.ikeda.g)
Attachment #8587096 - Flags: review?(nical.bugzilla)
Attachment #8587099 - Flags: review?(sotaro.ikeda.g)
Attachment #8587099 - Flags: review?(nical.bugzilla)
Attachment #8587101 - Flags: review?(sotaro.ikeda.g)
Attachment #8587101 - Flags: review?(nical.bugzilla)
Attachment #8587102 - Flags: review?(sotaro.ikeda.g)
Attachment #8587102 - Flags: review?(nical.bugzilla)
Attachment #8590694 - Flags: review?(sotaro.ikeda.g)
Attachment #8590694 - Flags: review?(nical.bugzilla)
I dupe the FD when writing FenceHandle to IPC message. Then we don't need to keep fence until delivery and don't need to check it's a cross-process or in-process message. So I can remove the FenceHandleFromChild.
Attachment #8590698 - Flags: review?(sotaro.ikeda.g)
Attachment #8590698 - Flags: review?(nical.bugzilla)
Remove some unused messages after Part2 and combine the OpDeliverFenceFromChild to OpUseTexture.
Attachment #8590705 - Flags: review?(sotaro.ikeda.g)
Attachment #8590705 - Flags: review?(nical.bugzilla)
Remove the reset unused functions after refactoring.
Attachment #8590706 - Flags: review?(sotaro.ikeda.g)
Attachment #8590706 - Flags: review?(nical.bugzilla)
Rename some functions after refactoring.
Attachment #8590710 - Flags: review?(sotaro.ikeda.g)
Attachment #8590710 - Flags: review?(nical.bugzilla)
No longer depends on: 1149492
(In reply to Ethan Lin[:Ethan] from comment #71)
> Sotaro, I think there is a fence life time problem of different transactions
> when we have multiple ImageHost to one TextureHost. I will modify the patch
> to handle this case. Thanks for your suggestion.

Ethan, how is the case handled? Can you explain about it?
Flags: needinfo?(etlin)
Sotaro, I dupe the fd when writing the IPC message, so we don't need to hold the fence anymore. Which means the fd will be held by the IPC message. For the case of multiple ImageHost, it's just the same. When we send each ImageHost's release fence in different transactions, the IPC message will hold the duped fd(s) for each ImageHost until the other side gets the fd. The parameter 'auto_close'[1] in FileDescriptor will help us close the fd after transmission.

[1] https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/file_descriptor_posix.h#21
Flags: needinfo?(etlin)
(In reply to Ethan Lin[:elin] from comment #78)
> Sotaro, I dupe the fd when writing the IPC message, so we don't need to hold
> the fence anymore. Which means the fd will be held by the IPC message. For
> the case of multiple ImageHost, it's just the same. When we send each
> ImageHost's release fence in different transactions, the IPC message will
> hold the duped fd(s) for each ImageHost until the other side gets the fd.
> The parameter 'auto_close'[1] in FileDescriptor will help us close the fd
> after transmission.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/
> file_descriptor_posix.h#21

Nice!, I did not know about it.
Attachment #8590694 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8590698 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8590706 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8590705 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8590710 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8590694 - Flags: review?(nical.bugzilla) → review+
Attachment #8590698 - Flags: review?(nical.bugzilla) → review+
Attachment #8590705 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8590706 [details] [diff] [review]
Part 4.5 - Remove the rest unused functions

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

yay!
Attachment #8590706 - Flags: review?(nical.bugzilla) → review+
Attachment #8590710 - Flags: review?(nical.bugzilla) → review+
Blocks: fence
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.