1146214 - Refactor fence delivery to reduce ipc messages and code duplication

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ethan Lin[:ethlin]]

There is a complicate mechanism to handle fence delivery. I want to simplify the code to reduce code complexity and ipc messages.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Remove unused set fence functions

Remove the unused functions in TiledContentHost and TiledLayerBuffer.

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Part 2 - Remove FenceDeliveryTracker and related ipc messages

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.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: Part 3 - Combine fence delivery with SendUpdate

Since the texture will be used after SendUpdate message, we can just send the fence with SendUpdate.

Comment 4

[Ethan Lin[:ethlin]]

Attachment: Part 4 - Handle the fence delivery

Handle the fence delivery in CompositableTransactionManager.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Part 5 - Use CompositableTransactionManager to hold the release fence

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.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: Part 6 - Combine some functions to reduce duplicate code

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.

Comment 7

[Chiajung Hung [:chiajung]]

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.

Comment 8

[Chiajung Hung [:chiajung]]

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.

Comment 9

[Chiajung Hung [:chiajung]]

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.

Comment 10

[Chiajung Hung [:chiajung]]

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.

Comment 11

[Chiajung Hung [:chiajung]]

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.

Comment 12

[Ethan Lin[:ethlin]]

Attachment: Part 2 - Combine FenceHandle and FenceHandleFromChild

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.

Comment 13

[Ethan Lin[:ethlin]]

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.

Comment 14

[Ethan Lin[:ethlin]]

Attachment: Part 3 - Prepare fence colume in SharedBufferManager

This patch prepares a correspond fence with buffer in SharedBufferManager. We can set the previous release fence to here to prevent ipc problem.

Comment 15

[Ethan Lin[:ethlin]]

Attachment: Part 4 - implement the fence delivery

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.

Comment 16

[Ethan Lin[:ethlin]]

Attachment: Part 5 - Fix typo

This patch just fixes the typo of the function name.

Comment 17

[Ethan Lin[:ethlin]]

Attachment: Part 6 - Remove the reset unused functions

Remove the reset unused functions after refactoring.

Comment 18

[Chiajung Hung [:chiajung]]

Comment on attachment 8582249 [details] [diff] [review]
Part 2 - Combine FenceHandle and FenceHandleFromChild

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

Looks good to me.

Comment 19

[Chiajung Hung [:chiajung]]

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.

Comment 20

[Chiajung Hung [:chiajung]]

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.

Comment 21

[Chiajung Hung [:chiajung]]

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 22

[Chiajung Hung [:chiajung]]

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.

Comment 23

[Chiajung Hung [:chiajung]]

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.

Comment 24

[Peter Chang[:pchang]]

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?

Comment 25

[Peter Chang[:pchang]]

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.

Comment 26

[Ethan Lin[:ethlin]]

Attachment: Part 3 - Prepare fence column and function in SharedBufferManager

Apply the feedback to this patch.

Comment 27

[Ethan Lin[:ethlin]]

Attachment: Part 4 - implement the fence delivery

Apply the feedback to this patch.

Comment 28

[Ethan Lin[:ethlin]]

Attachment: Part 5 - Fix typo

Update the patch due to the change of Part4 patch

Comment 29

[Ethan Lin[:ethlin]]

Attachment: Part 6 - Remove the rest unused functions

Update the patch due to the change of Part4 patch.

Comment 30

[Ethan Lin[:ethlin]]

Attachment: Part 3.1 - Prepare fence column and function in SharedBufferManager

Correct the patch.

Comment 31

[Ethan Lin[:ethlin]]

Attachment: Part 4.1 - implement the fence delivery

Correct the patch.

Comment 32

[Ethan Lin[:ethlin]]

Attachment: Part 5.1 - Fix typo

Correct the patch.

Comment 33

[Ethan Lin[:ethlin]]

Attachment: Part 6.1 - Remove the rest unused functions

Correct the patch.

Comment 34

[Chiajung Hung [:chiajung]]

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)

Comment 35

[Chiajung Hung [:chiajung]]

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 36

[Chiajung Hung [:chiajung]]

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 37

[Ethan Lin[:ethlin]]

Attachment: 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.

Comment 38

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

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?

Comment 39

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

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.

Comment 40

[Ethan Lin[:ethlin]]

Attachment: Part 3.2 - Prepare fence column and function in SharedBufferManager

Apply the Jerry and Chiajung's feedback in this patch.

Comment 41

[Ethan Lin[:ethlin]]

Attachment: Part 4.2 - implement the fence delivery

Apply the Jerry and Chiajung's feedback in this patch.

Comment 42

[Ethan Lin[:ethlin]]

Attachment: Part 5.2 - Fix typo

Update the patch due to the change of Part 4.

Comment 43

[Ethan Lin[:ethlin]]

Attachment: Part 6.2 - Remove the rest unused functions

Update the patch due to the change of Part 4.

Comment 44

[Ethan Lin[:ethlin]]

Attachment: Part 4.2 - implement the fence delivery

This patch is the correct one.

Comment 45

[Ethan Lin[:ethlin]]

Attachment: Part 5.2 - Fix typo

This patch is the correct one.

Comment 46

[Nicolas Silva [:nical]]

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 47

[Sotaro Ikeda [:sotaro]]

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?

Comment 48

[Sotaro Ikeda [:sotaro]]

(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 49

[Sotaro Ikeda [:sotaro]]

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 50

[Sotaro Ikeda [:sotaro]]

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).

Comment 51

[Ethan Lin[:ethlin]]

> 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.

Comment 52

[Ethan Lin[:ethlin]]

Attachment: Fence_flow_of_LayerTransaction.svg

Update the diagram for content's fence.

Comment 53

[Ethan Lin[:ethlin]]

Attachment: Part 4.3 - implement the fence delivery

Apply comment46 in this patch.

Comment 54

[Ethan Lin[:ethlin]]

(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.

Comment 55

[Ethan Lin[:ethlin]]

(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?

Comment 56

[Ethan Lin[:ethlin]]

(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.

Comment 57

[Nicolas Silva [:nical]]

(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.

Comment 58

[Sotaro Ikeda [:sotaro]]

(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.

Comment 59

[Sotaro Ikeda [:sotaro]]

(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.

Comment 60

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Remove unused set fence functions

Rebase the patch.

Comment 61

[Ethan Lin[:ethlin]]

Attachment: Part 1.2 - Remove unused set fence functions

Rebase the patch.

Comment 62

[Ethan Lin[:ethlin]]

Attachment: Part 2.2 - Combine FenceHandle and FenceHandleFromChild

I add a function to dup the fence when receiving a in-process fence from IPC.

Comment 63

[Ethan Lin[:ethlin]]

Attachment: Part 3.3 - Prepare fence column and function in SharedBufferManager

Rebase the patch.

Comment 64

[Ethan Lin[:ethlin]]

Attachment: Part 4.4 - implement the fence delivery

I set the CompositorOGL release fence to TextureHost when setting HWC release fence. So the final release fence will be holded by TextureHost.

Comment 65

[Ethan Lin[:ethlin]]

Attachment: Part 5.3 - Fix typo

Rebase the patch.

Comment 66

[Ethan Lin[:ethlin]]

Attachment: Part 5.4 - Fix typo

Rebase the patch.

Comment 67

[Ethan Lin[:ethlin]]

Attachment: Part 6.3 - Remove the rest unused functions

Rebase the patch.

Comment 68

[Ethan Lin[:ethlin]]

(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.

Comment 69

[Ethan Lin[:ethlin]]

(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.

Comment 70

[Sotaro Ikeda [:sotaro]]

(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.

Comment 71

[Ethan Lin[:ethlin]]

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.

Comment 72

[Ethan Lin[:ethlin]]

Attachment: Part 1.3 - Remove unused set fence functions

Rebase the patch.

Comment 73

[Ethan Lin[:ethlin]]

Attachment: Part 2.3 - Combine FenceHandle and FenceHandleFromChild

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.

Comment 74

[Ethan Lin[:ethlin]]

Attachment: Part 3.4 - Remove reply fence action and combine messages

Remove some unused messages after Part2 and combine the OpDeliverFenceFromChild to OpUseTexture.

Comment 75

[Ethan Lin[:ethlin]]

Attachment: Part 4.5 - Remove the rest unused functions

Remove the reset unused functions after refactoring.

Comment 76

[Ethan Lin[:ethlin]]

Attachment: Part 5.4 - Rename some functions

Rename some functions after refactoring.

Comment 77

[Sotaro Ikeda [:sotaro]]

(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?

Comment 78

[Ethan Lin[:ethlin]]

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

Comment 79

[Sotaro Ikeda [:sotaro]]

(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.

Comment 80

[Nicolas Silva [:nical]]

Comment on attachment 8590706 [details] [diff] [review]
Part 4.5 - Remove the rest unused functions

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

yay!

Comment 81

[Ethan Lin[:ethlin]]

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

Comment 82

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ed7d03ebae
https://hg.mozilla.org/integration/mozilla-inbound/rev/0af24986e843
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff2d1f3a2c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef36caa16cf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/893e3ebffee0

Comment 83

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ed7d03ebae
https://hg.mozilla.org/integration/mozilla-inbound/rev/0af24986e843
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff2d1f3a2c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef36caa16cf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/893e3ebffee0

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4ed7d03ebae
https://hg.mozilla.org/mozilla-central/rev/0af24986e843
https://hg.mozilla.org/mozilla-central/rev/7ff2d1f3a2c2
https://hg.mozilla.org/mozilla-central/rev/ef36caa16cf3
https://hg.mozilla.org/mozilla-central/rev/893e3ebffee0