Closed Bug 984434 Opened 6 years ago Closed 6 years ago

Change ImageBridgeChild::FlushAllImages() to use async ipc

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: [sprd315886])

Attachments

(2 files, 20 obsolete files)

37.38 KB, patch
nical
: review+
Details | Diff | Splinter Review
36.80 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
ImageBridgeChild::FlushAllImages() uses async ipc in the function. It seems better to use async ipc instead.
Confirmed the patch works on master hamachi.
Comment on attachment 8392260 [details] [diff] [review]
patch - Change ImageBridgeChild::FlushAllImages() to use async ipc

nical, can you do feedback to the patch? How do you think about the implementation?
Attachment #8392260 - Flags: feedback?(nical.bugzilla)
Assignee: nobody → sotaro.ikeda.g
Comment on attachment 8392260 [details] [diff] [review]
patch - Change ImageBridgeChild::FlushAllImages() to use async ipc

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

This looks good overall. We could improve it (maybe as a followup) by packing async transaction replies in an array that we send in one message to reduce the IPC traffic. There are a few other replies that I have been meaning to move to async (but haven't got the time to do yet) so even if right now we only have two replies in the array, it should become more important in the future.

If I understand correctly, this makes the thread that is talking to the ImageBridgeChild thread wait rather than making both of them wait. Does this give noticeable improvements? I'd rather make sure before pushing a complex change like this one. That said, this may be a useful tool for other use cases too.
One thing to watch out for, is: What happens during the destruction sequence, can there be race between the new replies and the destruction of some IPDL protocol? If so, will that cause memory leaks, crashes? For exemple are we sure that the ImageClient holding the closures can't be deleted before a reply comes back?

f+ if this makes things noticeably better.

::: gfx/layers/ipc/AsyncRequestStatus.h
@@ +33,5 @@
> +  {
> +    return mSerial;
> +  }
> +
> +  typedef void (*CompleteCallback)(void* aClosure);

Perhaps a task object would be nicer/safer here (see "base/task.h") because then we can use NewRunnableFunction to pass an arbitrary closure and we also get type safety instead of casting void*.

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +969,5 @@
>    HoldUntilTransaction(aTexture);
>  }
>  
> +void
> +ImageBridgeChild::RemoveTextureFromCompositableAsync(AsyncRequestStatus* aAsyncRequestStatus,

So, with this, will we still need the non-async version? If not I would just remove the non-async and call the async one RemoveTextureFromCompositable.

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +246,5 @@
>    return OtherProcess() == ipc::kInvalidProcessHandle;
>  }
>  
> +void
> +ImageBridgeParent::RemoveTextureReply(const OpRemoveTextureReply& aReply)

This is a bit confusing the way it reads "remove a texture reply" this is a function so we (or at least I) naturally tend to take the first word (Remove) as a verb, so maybe it should be something more like "ReplyTextureRemoved(...)"
Attachment #8392260 - Flags: feedback?(nical.bugzilla) → feedback+
Blocks: 988954
Blocks: 767484
(In reply to Nicolas Silva [:nical] from comment #3)
> This looks good overall. We could improve it (maybe as a followup) by
> packing async transaction replies in an array that we send in one message to
> reduce the IPC traffic. There are a few other replies that I have been
> meaning to move to async (but haven't got the time to do yet) so even if
> right now we only have two replies in the array, it should become more
> important in the future.
 
> If I understand correctly, this makes the thread that is talking to the
> ImageBridgeChild thread wait rather than making both of them wait.

Yes.

> Does this give noticeable improvements? I'd rather make sure before pushing a complex
> change like this one. That said, this may be a useful tool for other use
> cases too.

I do not expect noticeable improvement only this bug. This bug is a first step to make ImageBridge and LayerTransaction more asyn. I think that Bug 767484 and Bug 988954 could improve the performance. 

> One thing to watch out for, is: What happens during the destruction
> sequence, can there be race between the new replies and the destruction of
> some IPDL protocol? If so, will that cause memory leaks, crashes? For
> exemple are we sure that the ImageClient holding the closures can't be
> deleted before a reply comes back?

Good point. attachment 8392260 [details] [diff] [review] implements ongoing AsyncRequestStatus at CompositableForwarder. It seems better to move it to ***Child class. If so, the destruction could be handled easily.
Apply some comments. Applying the comments are still wip.
Attachment #8392260 - Attachment is obsolete: true
ImageBridgeChild::RemoveTextureFromCompositable() is called in ImageClientSingle::UpdateImageInternal().
The RemoveTextureFromCompositable() is sync on gonk. It seems better to fix bug 988954 first.
No longer blocks: 988954
Depends on: 988954
Change RemoveTextureFromCompositable() to async if possible.
Attachment #8405702 - Attachment is obsolete: true
Use async transaction for RemoveTextureFromCompositable() if TextureClient is recycled. By applying patch, youtube playback works smoothly. But OMXCodec has a problem.
Attachment #8407047 - Attachment is obsolete: true
Attachment #8407240 - Attachment is patch: true
Attachment #8407240 - Attachment mime type: text/x-patch → text/plain
I confirmed that attachment 8407240 [details] [diff] [review] makes ImageBridge transaction async on b2g.
Fix OMXCodec shut down problem.
Attachment #8407240 - Attachment is obsolete: true
Blocks: 988956, 988941
No longer depends on: 988954
Add IPC shutdown handling.
Attachment #8407696 - Attachment is obsolete: true
Class renaming and some code clean up.
Attachment #8407812 - Attachment is obsolete: true
Some code clean up and renaming.
Attachment #8407915 - Attachment is obsolete: true
Add virtual to ~CompositableChild().
Attachment #8408442 - Attachment is obsolete: true
Update AsyncTransactionTracker::WaitForever().
Attachment #8408446 - Attachment is obsolete: true
Update ImageBridgeChild::RemoveTextureFromCompositableAsync().
Attachment #8408458 - Attachment is obsolete: true
Attachment #8408717 - Flags: review?(nical.bugzilla)
Attachment #8408717 - Flags: review?(nical.bugzilla)
Attachment #8408717 - Flags: review?(nical.bugzilla)
Comment on attachment 8408717 [details] [diff] [review]
patch v12 - Change ImageBridgeChild::FlushAllImages() to use async ipc

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

Looks good. I am not found of some of the names.
In particular, it took me a moment to read and understand "Tracker" in AsyncTransactionTracker. I see it as a callback so perhaps Callback would be a better name? bjacob's input would be useful here, he is better at naming things than I am, and also you guys can sort names out face to face which is easier.

::: gfx/layers/client/CompositableClient.h
@@ +163,5 @@
>   *
>   * CompositableChild is owned by a CompositableClient.
>   */
>  class CompositableChild : public PCompositableChild
> +                        , public AsyncTransactionTrackersHolder

I don't see the definition of AsyncTransactionTrackersHolder, is it missing from your patch?

::: gfx/layers/client/ImageClient.h
@@ +68,5 @@
> +  virtual TemporaryRef<AsyncTransactionTracker> PrepareFlushAllImages() { return nullptr; }
> +
> +  /**
> +   * asynchronously remove all the textures used by the image client.
> +   * 

nit: trailing space

::: gfx/layers/ipc/CompositableForwarder.h
@@ +182,5 @@
>    }
>  
>    bool IsOnCompositorSide() const MOZ_OVERRIDE { return false; }
>  
> +  virtual bool IsOnImageBridgeChild() const { return false; }

nit: since it is implemented by the ImageBridgeChild, I suppose "IsImageBridgeChild" makes more sense.
Attachment #8408717 - Flags: feedback?(bjacob)
Add missing AsyncTransactionTracker.h/.cpp
(In reply to Nicolas Silva [:nical] from comment #18)
> Comment on attachment 8408717 [details] [diff] [review]
> patch v12 - Change ImageBridgeChild::FlushAllImages() to use async ipc
> 
> Review of attachment 8408717 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I am not found of some of the names.
> In particular, it took me a moment to read and understand "Tracker" in
> AsyncTransactionTracker. I see it as a callback so perhaps Callback would be
> a better name? bjacob's input would be useful here, he is better at naming
> things than I am, and also you guys can sort names out face to face which is
> easier.
> 
> ::: gfx/layers/client/CompositableClient.h
> @@ +163,5 @@
> >   *
> >   * CompositableChild is owned by a CompositableClient.
> >   */
> >  class CompositableChild : public PCompositableChild
> > +                        , public AsyncTransactionTrackersHolder
> 
> I don't see the definition of AsyncTransactionTrackersHolder, is it missing
> from your patch?

It is in AsyncTransactionTracker.h/.cpp. I quickly add attachment 8410239 [details] [diff] [review].

> ::: gfx/layers/client/ImageClient.h
> @@ +68,5 @@
> > +  virtual TemporaryRef<AsyncTransactionTracker> PrepareFlushAllImages() { return nullptr; }
> > +
> > +  /**
> > +   * asynchronously remove all the textures used by the image client.
> > +   * 
> 
> nit: trailing space

Will update in a next patch.

> 
> ::: gfx/layers/ipc/CompositableForwarder.h
> @@ +182,5 @@
> >    }
> >  
> >    bool IsOnCompositorSide() const MOZ_OVERRIDE { return false; }
> >  
> > +  virtual bool IsOnImageBridgeChild() const { return false; }
> 
> nit: since it is implemented by the ImageBridgeChild, I suppose
> "IsImageBridgeChild" makes more sense.

Will update in a next patch.
Apply the comments.
Attachment #8408717 - Attachment is obsolete: true
Attachment #8410239 - Attachment is obsolete: true
Attachment #8408717 - Flags: review?(nical.bugzilla)
Attachment #8408717 - Flags: feedback?(bjacob)
Attachment #8410324 - Flags: feedback?(nical.bugzilla)
Attachment #8410324 - Flags: feedback?(bjacob)
Comment on attachment 8410324 [details] [diff] [review]
patch v14 - Change ImageBridgeChild::FlushAllImages() to use async ipc

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

::: gfx/layers/ipc/AsyncTransactionTracker.cpp
@@ +23,5 @@
> +{
> +}
> +
> +void
> +AsyncTransactionTracker::WaitForever()

It would be nice if this method could assert that we are not in the IPDL thread (ImageBridgeChild or main-thread depending on the case, although I guess right now we only use it for ImageBridge), to prevent deadlocks.

::: gfx/layers/ipc/LayersMessages.ipdlh
@@ +331,5 @@
>    PTexture texture;
>  };
>  
> +struct OpRemoveTextureAsync {
> +  uint32_t requestId;

I prefer uint64_t for this kind of id that is generated with an increment. It's not much more expensive and it will make overflow an non-issue.
Attachment #8410324 - Flags: feedback?(nical.bugzilla) → feedback+
Comment on attachment 8410324 [details] [diff] [review]
patch v14 - Change ImageBridgeChild::FlushAllImages() to use async ipc

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

::: gfx/layers/ipc/AsyncTransactionTracker.h
@@ +19,5 @@
> +/**
> + * AsyncTransactionTracker tracks asynchronous transaction.
> + * It is typically used for asynchronous layer transaction handling.
> + */
> +class AsyncTransactionTracker

"Tracker" in AsyncTransactionTracker sounds fine to me: this sounds like "tracking a package", which is what this is.

@@ +34,5 @@
> +
> +  /**
> +   * Wait until asynchronous transaction complete.
> +   */
> +  void WaitForever();

My only suggestion is consider renaming WaitForever to WaitComplete or WaitUntilComplete?
Attachment #8410324 - Flags: feedback?(bjacob) → feedback+
Status: NEW → ASSIGNED
(In reply to Nicolas Silva [:nical] from comment #18)
> Comment on attachment 8408717 [details] [diff] [review]
> patch v12 - Change ImageBridgeChild::FlushAllImages() to use async ipc
> 
> Review of attachment 8408717 [details] [diff] [review]:

> In particular, it took me a moment to read and understand "Tracker" in
> AsyncTransactionTracker. I see it as a callback so perhaps Callback would be
> a better name? bjacob's input would be useful here, he is better at naming
> things than I am, and also you guys can sort names out face to face which is
> easier.

I and bjacob talked about AsyncTransactionTracker. As in comment 23, he also agreed to the name. I like the current name than callback. AsyncTransactionTracker does not directly expose the call back to it's user. It expose a capability to track async transaction complete.
(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> I and bjacob talked about AsyncTransactionTracker. As in comment 23, he also
> agreed to the name. I like the current name than callback.
> AsyncTransactionTracker does not directly expose the call back to it's user.
> It expose a capability to track async transaction complete.

Then it's all good :)
(In reply to Benoit Jacob [:bjacob] from comment #23)
> 
> @@ +34,5 @@
> > +
> > +  /**
> > +   * Wait until asynchronous transaction complete.
> > +   */
> > +  void WaitForever();
> 
> My only suggestion is consider renaming WaitForever to WaitComplete or
> WaitUntilComplete?

Will change to WaitComplete() in a next patch.
(In reply to Nicolas Silva [:nical] from comment #22)
> Comment on attachment 8410324 [details] [diff] [review]
> patch v14 - Change ImageBridgeChild::FlushAllImages() to use async ipc
> 
> Review of attachment 8410324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/AsyncTransactionTracker.cpp
> @@ +23,5 @@
> > +{
> > +}
> > +
> > +void
> > +AsyncTransactionTracker::WaitForever()
> 
> It would be nice if this method could assert that we are not in the IPDL
> thread (ImageBridgeChild or main-thread depending on the case, although I
> guess right now we only use it for ImageBridge), to prevent deadlocks.

I will update in a next patch.

> 
> ::: gfx/layers/ipc/LayersMessages.ipdlh
> @@ +331,5 @@
> >    PTexture texture;
> >  };
> >  
> > +struct OpRemoveTextureAsync {
> > +  uint32_t requestId;
> 
> I prefer uint64_t for this kind of id that is generated with an increment.
> It's not much more expensive and it will make overflow an non-issue.

I will update in a next patch. One problem is that AsyncTransactionTracker's ID allocation could happen on multi-threaded. Therefore, to allocate ID, the following atomic is used. But mozilla::Atomic does not support uint64_t.

>   static mozilla::Atomic<uint32_t> sSerialCounter;
Apply the comments. 64bit sSerialCounter's atomicity part is temporary implementation.
Attachment #8410324 - Attachment is obsolete: true
Update missing files.
Attachment #8412110 - Attachment is obsolete: true
attachment 8412111 [details] [diff] [review] works correctly on master hamachi. But on master nexus-5, video flickering problem happens. ImageBridgeChild::FlushAllImages() part works correctly, otherwise OMXCodec can not destruct correctly. But somehow, Fence seems not correctly delivered to OMXCodec.
Apply Bug 1000525's update and un-bitrot.
Attachment #8412111 - Attachment is obsolete: true
Fix fence delivery problem. Create a patch based on Bug 1000525's patch.

But this patch a problem that AsyncTransactionTracker::WaitComplete() does not exit.
Attachment #8414635 - Attachment is obsolete: true
> Fix fence delivery problem. Create a patch based on Bug 1000525's patch.
> 
> But this patch a problem that AsyncTransactionTracker::WaitComplete() does
> not exit.

The problem was in the Bug 1000525's patch part.
Depends on: 1000525
un-bitrot
Attachment #8414715 - Attachment is obsolete: true
By applying  Bug 1000525's patch + attachment 8414750 [details] [diff] [review], all major problem seems to be fixed.
Un-bitrot.
Attachment #8414750 - Attachment is obsolete: true
Fix build failure on some platform.
Attachment #8418222 - Attachment is obsolete: true
Blocks: 1006957
Attachment #8418309 - Flags: review?(nical.bugzilla)
Comment on attachment 8418309 [details] [diff] [review]
patch v20 - Change ImageBridgeChild::FlushAllImages() to use async ipc

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

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +362,5 @@
>    if (InImageBridgeChildThread()) {
> +    NS_ERROR("ImageBridgeChild::FlushAllImages() is called on ImageBridge thread.");
> +     return;
> +   }
> + 

nit: a few trailing spaces here and below
Attachment #8418309 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/f8ba182364b1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Nominate to "b2g-1.4+". This bug blocks 1.4+ bug.
blocking-b2g: --- → 1.4?
This needs 1.4 uplift as successful fix for bug 1006957 (1.4+) depends on it.
blocking-b2g: 1.4? → 1.4+
This commit caused dolphin video app always crash when generate thumbnail.
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(ming.li)
Flags: needinfo?(kli)
Flags: needinfo?(kkuo)
Whiteboard: [sprd315886]
Hi! Sotaro,

Do you have Dolphin device? We need your help with the issue in comment 47.
Thanks

--
Keven
Flags: needinfo?(kkuo) → needinfo?(sotaro.ikeda.g)
Flags: needinfo?(styang)
Blocks: 1016805
Flags: needinfo?(ming.li)
video app work properly on today build with wip from bug 1016805.
Flags: needinfo?(kli)
Remove needinfo based on Comment 49.
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(ttsai)
Depends on: 1010683
You need to log in before you can comment on or make changes to this bug.