984434 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

ImageBridgeChild::FlushAllImages() uses async ipc in the function. It seems better to use async ipc instead.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change ImageBridgeChild::FlushAllImages() to use async ipc

Confirmed the patch works on master hamachi.

Comment 2

[Sotaro Ikeda [:sotaro]]

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 3

[Nicolas Silva [:nical]]

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

Comment 4

[Sotaro Ikeda [:sotaro]]

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

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Apply some comments. Applying the comments are still wip.

Comment 6

[Sotaro Ikeda [:sotaro]]

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

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - Change ImageBridgeChild::FlushAllImages() to use async ipc (patch_984434_4

Change RemoveTextureFromCompositable() to async if possible.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch v5 - Change ImageBridgeChild::FlushAllImages() to use async ipc

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

Comment 10

[Sotaro Ikeda [:sotaro]]

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

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch v6 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Fix OMXCodec shut down problem.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch v7 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Add IPC shutdown handling.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch v8 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Class renaming and some code clean up.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch v9 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Some code clean up and renaming.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch v10 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Add virtual to ~CompositableChild().

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch v11 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Update AsyncTransactionTracker::WaitForever().

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch v12 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Update ImageBridgeChild::RemoveTextureFromCompositableAsync().

Comment 18

[Nicolas Silva [:nical]]

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.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch v13 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Add missing AsyncTransactionTracker.h/.cpp

Comment 20

[Sotaro Ikeda [:sotaro]]

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

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch v14 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Apply the comments.

Comment 22

[Nicolas Silva [:nical]]

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 23

[Benoit Jacob [:bjacob] (mostly away)]

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?

Comment 24

[Sotaro Ikeda [:sotaro]]

(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

[Nicolas Silva [:nical]]

(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 :)

Comment 26

[Sotaro Ikeda [:sotaro]]

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

Comment 27

[Sotaro Ikeda [:sotaro]]

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

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch v15 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Apply the comments. 64bit sSerialCounter's atomicity part is temporary implementation.

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: patch v15 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Update missing files.

Comment 30

[Sotaro Ikeda [:sotaro]]

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.

Comment 31

[Sotaro Ikeda [:sotaro]]

Attachment: patch v16 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Apply Bug 1000525's update and un-bitrot.

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch v17 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Fix fence delivery problem. Create a patch based on Bug 1000525's patch.

But this patch a problem that AsyncTransactionTracker::WaitComplete() does not exit.

Comment 33

[Sotaro Ikeda [:sotaro]]

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

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch v18 - Change ImageBridgeChild::FlushAllImages() to use async ipc

un-bitrot

Comment 35

[Sotaro Ikeda [:sotaro]]

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

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: patch v19 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Un-bitrot.

Comment 37

[Sotaro Ikeda [:sotaro]]

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

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch v20 - Change ImageBridgeChild::FlushAllImages() to use async ipc

Fix build failure on some platform.

Comment 39

[Sotaro Ikeda [:sotaro]]

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

Comment 40

[Nicolas Silva [:nical]]

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

Comment 41

[Sotaro Ikeda [:sotaro]]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f8ba182364b1

Comment 43

[Sotaro Ikeda [:sotaro]]

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

Comment 44

[Sotaro Ikeda [:sotaro]]

Attachment: patch v20 for b2g v1.4 - Change ImageBridgeChild::FlushAllImages() to use async ipc

A patch for b2g v1.4.

Comment 45

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/7340ad0624d2

Comment 47

[James Zhang (Spreadtrum)]

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

Comment 48

[Keven Kuo [:kkuo]]

Hi! Sotaro,

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

--
Keven

Comment 49

[Kai-Zhen Li [:seinlin][:kli]]

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

Comment 50

[Sotaro Ikeda [:sotaro]]

Remove needinfo based on Comment 49.