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?

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

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

ImageBridgeChild::RemoveTextureFromCompositable() is called in ImageClientSingle::UpdateImageInternal(). The RemoveTextureFromCompositable() is sync on gonk. It seems better to fix bug 988954 first.

Change RemoveTextureFromCompositable() to async if possible.

Use async transaction for RemoveTextureFromCompositable() if TextureClient is recycled. By applying patch, youtube playback works smoothly. But OMXCodec has a problem.

I confirmed that attachment 8407240 [details] [diff] [review] makes ImageBridge transaction async on b2g.

Fix OMXCodec shut down problem.

Add IPC shutdown handling.

Class renaming and some code clean up.

Some code clean up and renaming.

Add virtual to ~CompositableChild().

Update AsyncTransactionTracker::WaitForever().

Update ImageBridgeChild::RemoveTextureFromCompositableAsync().

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.

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.

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.

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?

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

Update missing files.

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.

Fix fence delivery problem. Create a patch based on Bug 1000525's patch. But this patch a problem that AsyncTransactionTracker::WaitComplete() does not exit.

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

un-bitrot

By applying Bug 1000525's patch + attachment 8414750 [details] [diff] [review], all major problem seems to be fixed.

Un-bitrot.

Fix build failure on some platform.

https://tbpl.mozilla.org/?tree=Try&rev=28070544df95

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

https://hg.mozilla.org/integration/b2g-inbound/rev/f8ba182364b1

Nominate to "b2g-1.4+". This bug blocks 1.4+ bug.

A patch for b2g v1.4.

This needs 1.4 uplift as successful fix for bug 1006957 (1.4+) depends on it.

This commit caused dolphin video app always crash when generate thumbnail.

Hi! Sotaro, Do you have Dolphin device? We need your help with the issue in comment 47. Thanks -- Keven

video app work properly on today build with wip from bug 1016805.

Remove needinfo based on Comment 49.