Closed
Bug 984434
Opened 11 years ago
Closed 11 years ago
Change ImageBridgeChild::FlushAllImages() to use async ipc
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
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.
Assignee | ||
Comment 1•11 years ago
|
||
Confirmed the patch works on master hamachi.
Assignee | ||
Comment 2•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → sotaro.ikeda.g
Comment 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
Apply some comments. Applying the comments are still wip.
Attachment #8392260 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
ImageBridgeChild::RemoveTextureFromCompositable() is called in ImageClientSingle::UpdateImageInternal().
The RemoveTextureFromCompositable() is sync on gonk. It seems better to fix bug 988954 first.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8404917 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Change RemoveTextureFromCompositable() to async if possible.
Attachment #8405702 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #8407240 -
Attachment is patch: true
Attachment #8407240 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Comment 10•11 years ago
|
||
I confirmed that attachment 8407240 [details] [diff] [review] makes ImageBridge transaction async on b2g.
Assignee | ||
Comment 11•11 years ago
|
||
Fix OMXCodec shut down problem.
Attachment #8407240 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 12•11 years ago
|
||
Add IPC shutdown handling.
Attachment #8407696 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
Class renaming and some code clean up.
Attachment #8407812 -
Attachment is obsolete: true
Assignee | ||
Comment 14•11 years ago
|
||
Some code clean up and renaming.
Attachment #8407915 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
Add virtual to ~CompositableChild().
Attachment #8408442 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
Update AsyncTransactionTracker::WaitForever().
Attachment #8408446 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
Update ImageBridgeChild::RemoveTextureFromCompositableAsync().
Attachment #8408458 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8408717 -
Flags: review?(nical.bugzilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8408717 -
Flags: review?(nical.bugzilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8408717 -
Flags: review?(nical.bugzilla)
Comment 18•11 years ago
|
||
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)
Assignee | ||
Comment 19•11 years ago
|
||
Add missing AsyncTransactionTracker.h/.cpp
Assignee | ||
Comment 20•11 years ago
|
||
(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.
Assignee | ||
Comment 21•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8410324 -
Flags: feedback?(nical.bugzilla)
Attachment #8410324 -
Flags: feedback?(bjacob)
Comment 22•11 years ago
|
||
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 23•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 24•11 years ago
|
||
(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.
Comment 25•11 years ago
|
||
(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 :)
Assignee | ||
Comment 26•11 years ago
|
||
(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.
Assignee | ||
Comment 27•11 years ago
|
||
(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;
Assignee | ||
Comment 28•11 years ago
|
||
Apply the comments. 64bit sSerialCounter's atomicity part is temporary implementation.
Attachment #8410324 -
Attachment is obsolete: true
Assignee | ||
Comment 29•11 years ago
|
||
Update missing files.
Attachment #8412110 -
Attachment is obsolete: true
Assignee | ||
Comment 30•11 years ago
|
||
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.
Assignee | ||
Comment 31•11 years ago
|
||
Apply Bug 1000525's update and un-bitrot.
Attachment #8412111 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
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
Assignee | ||
Comment 33•11 years ago
|
||
> 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.
Assignee | ||
Comment 35•11 years ago
|
||
By applying Bug 1000525's patch + attachment 8414750 [details] [diff] [review], all major problem seems to be fixed.
Assignee | ||
Comment 37•11 years ago
|
||
Assignee | ||
Comment 38•11 years ago
|
||
Fix build failure on some platform.
Attachment #8418222 -
Attachment is obsolete: true
Assignee | ||
Comment 39•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8418309 -
Flags: review?(nical.bugzilla)
Comment 40•11 years ago
|
||
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+
Assignee | ||
Comment 41•11 years ago
|
||
Comment 42•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 43•11 years ago
|
||
Nominate to "b2g-1.4+". This bug blocks 1.4+ bug.
blocking-b2g: --- → 1.4?
Comment 45•11 years ago
|
||
This needs 1.4 uplift as successful fix for bug 1006957 (1.4+) depends on it.
blocking-b2g: 1.4? → 1.4+
Comment 46•11 years ago
|
||
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox30:
--- → wontfix
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
Comment 47•11 years ago
|
||
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)
Updated•11 years ago
|
Whiteboard: [sprd315886]
Comment 48•11 years ago
|
||
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)
Updated•11 years ago
|
Flags: needinfo?(styang)
Comment 49•11 years ago
|
||
video app work properly on today build with wip from bug 1016805.
Flags: needinfo?(kli)
Assignee | ||
Comment 50•11 years ago
|
||
Remove needinfo based on Comment 49.
Flags: needinfo?(sotaro.ikeda.g)
Updated•11 years ago
|
Flags: needinfo?(ttsai)
You need to log in
before you can comment on or make changes to this bug.
Description
•