951732 - [B2G][Video] When tapping on playing video it starts flickering

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Natalya Kot [:nkot]]

Attachment: logcat

Description:
Video starts flickering when tapping it while playing.

Repro Steps:
1) Updated Buri to BuildID: 20131218040201
2) Open Video app
3) Select any video to play
4) Tap on the video while playing

Actual:
 Video starts flickering

Expected:
 Video will continue play normally, video menu UI will appear or hide 

Environmental Variables:
Device: Buri Master M-C Mozilla RIL
BuildID: 20131218040201
Gaia: e2f0e09e980b1cb3275a0bb033931cb48f9d521c
Gecko: 862cb6a1cc88
Version: 29.0a1
Firmware Version: V1.2_20131115

Notes: doesn't happen on v1.3 

Repro frequency: 100%
See attached logcat

Comment 1

[Natalya Kot [:nkot]]

video of the issue  http://youtu.be/piZT3W_kMew

Comment 2

[Brogan Zumwalt [Inactive]]

Issue appears to occur on Youtube videos through Browser app. Using same build as Comment 0.

Comment 3

[Brogan Zumwalt [Inactive]]

Regression Window:

Works:
Build ID: 20131212040203
Gecko: http://hg.mozilla.org/mozilla-central/rev/1ad9af3a2ab8
Gaia: 8952898bbc98dd31e25b647203791cf129867ff1
Platform Version: 29.0a1
Firmware Version: V1.2_US_20131115


Doesn't Work:
Build ID: 20131213040203
Gecko: http://hg.mozilla.org/mozilla-central/rev/8b5875dc7e31
Gaia: 390b313a254a947d12e3cdbcde19d7d1619ff63c
Platform Version: 29.0a1
Firmware Version: V1.2_US_20131115

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

The Camera app is also affected. We use video element at the viewfinder of Camera app.

Comment 5

[Brogan Zumwalt [Inactive]]

Issue also occurs when rotating/changing orientation of phone in viewfinder of Camera app

[Same environmental variables as Comment 0]

Comment 7

[Sotaro Ikeda [:sotaro]]

This bug seems regression of Bug 897452. I recognize it during debugging Bug 946720.

Comment 8

[Nicolas Silva [:nical]]

I think that this is caused by the the SendRemoveTexture message being potentially sent before the transaction instead of afterwards.

ContentClient gets around this by storing the textures to remove in an array that it clears out after the transaction.

Comment 9

[Sotaro Ikeda [:sotaro]]

Comment 8 is not enough. By removing TOpRemoveTexture message at Bug 897452, Some TextureClient's semantics seems to be broken. In current master, there is no way to explicitly clear a CompositableHost's TextureHost from CompositableClient. It is necessary at the following functions.
   + ImageClientSingle::FlushAllImages()
   + ImageClientBuffered::FlushAllImages()

To complement the above omission, PTextureClient::SendRemoveTexture() triggers to disable TextureHost's data. When the function is called TextureHost/TextureSource's data becomes invalid. But it conflicts with Bug 946720. In thebes layer's canse, TextureHost/TextureSource needs to be valid longer than TextureClient.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> Comment 8 is not enough. By removing TOpRemoveTexture message at Bug 897452,
> Some TextureClient's semantics seems to be broken. In current master, there
> is no way to explicitly clear a CompositableHost's TextureHost from
> CompositableClient. It is necessary at the following functions.
>    + ImageClientSingle::FlushAllImages()
>    + ImageClientBuffered::FlushAllImages()

It might be better to handle this case by adding an argument to PTextureChild::RemoveTextureSync() like the following.

>  PTextureChild::RemoveTextureSync(bool aForgetSharedData)

Comment 11

[Sotaro Ikeda [:sotaro]]

Oh, TEXTURE_DEALLOCATE_DEFERRED is already present. Comment 10 is not necessary. It is just the following part's code's problem.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.cpp#731

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - keep TextureClient until Transaction complete

This fix the flickering problem. But buffer handling around OMXCodec still not correct. Especially, youtube playback has a problem. On master hamachi, youtube playback does not start. On nexus 4, youtube playback start but the browser process crash when browser app set to background.

Comment 13

[Sotaro Ikeda [:sotaro]]

Youtube video playback was failed even on a ROM that created from default source. I am going to confirm again by using latest source.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> Created attachment 8360826 [details] [diff] [review]
> patch - keep TextureClient until Transaction complete
> 
> This fix the flickering problem. But buffer handling around OMXCodec still
> not correct. Especially, youtube playback has a problem. On master hamachi,
> youtube playback does not start.

This happens even on default ROM. It seems like different problem. So only remaining problem is the following.
- All gralloc buffers are not returned to OMXCodec during the codec shutdown.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - keep TextureClient until Transaction complete

By the patch the video playback problem is fixed. Confirmed it on master hamachi and master nexus-4.

Comment 16

[Nicolas Silva [:nical]]

Comment on attachment 8361214 [details] [diff] [review]
patch v2 - keep TextureClient until Transaction complete

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

Overall looks good, I'd just want to confirm with you about the FlushAllImage case. Also some nits about the names, would the ones I propose above work for you Sotaro?

::: gfx/layers/client/ImageClient.cpp
@@ +102,5 @@
>  void
>  ImageClientSingle::FlushAllImages(bool aExceptFront)
>  {
>    if (!aExceptFront && mFrontBuffer) {
> +    GetForwarder()->AddForceRemovingTexture(mFrontBuffer);

I thought we needed to remove the texture right away in the case of FlushAllImages (instead of removing them after the next transaction)?

::: gfx/layers/ipc/CompositableForwarder.h
@@ +167,5 @@
>    /**
> +   * Forcibly remove texture data from TextureClient
> +   * after a tansaction with Compositor.
> +   */
> +  virtual void AddForceRemovingTexture(TextureClient* aClient)

I would prefer to rename this into "RemoveAfterTransaction" to better reflect what it is about to do. Especially since we want to move away from using ForceRemove manually, and that we'll rely on ref counting exclusively (with the exception of FlushAllImages), so we'll eventually just keep the textures in the array and let the ref count do the work.

@@ +178,5 @@
> +  /**
> +   * Forcibly remove texture data from TextureClient
> +   * This function needs to be called after a tansaction with Compositor.
> +   */
> +  virtual void ForceRemoveTexturesIfNecessary()

I'd prefer to rename this into "RemovePendingTextures". Soon we won't be calling ForceRemove, we'll just clear the array and let reference counting do the job.

@@ +243,5 @@
>  
>  protected:
>    TextureFactoryIdentifier mTextureFactoryIdentifier;
>    bool mMultiProcess;
> +  nsTArray<RefPtr<TextureClient> > mForceRemovingTextures;

How about naming it "mTexturesToRemove"?

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #16)
> Comment on attachment 8361214 [details] [diff] [review]
> patch v2 - keep TextureClient until Transaction complete
> 
> Review of attachment 8361214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good, I'd just want to confirm with you about the
> FlushAllImage case. Also some nits about the names, would the ones I propose
> above work for you Sotaro?
> 
> ::: gfx/layers/client/ImageClient.cpp
> @@ +102,5 @@
> >  void
> >  ImageClientSingle::FlushAllImages(bool aExceptFront)
> >  {
> >    if (!aExceptFront && mFrontBuffer) {
> > +    GetForwarder()->AddForceRemovingTexture(mFrontBuffer);
> 
> I thought we needed to remove the texture right away in the case of
> FlushAllImages (instead of removing them after the next transaction)?

ImageBridgeChild::FlushAllImagesNow() wait end of transaction synchronously. Even after the patch, the textures are removed synchronously. There is no need to remove the texture right away in ImageClientSingle::FlushAllImages().

> 
> ::: gfx/layers/ipc/CompositableForwarder.h
> @@ +167,5 @@
> >    /**
> > +   * Forcibly remove texture data from TextureClient
> > +   * after a tansaction with Compositor.
> > +   */
> > +  virtual void AddForceRemovingTexture(TextureClient* aClient)
> 
> I would prefer to rename this into "RemoveAfterTransaction" to better
> reflect what it is about to do. Especially since we want to move away from
> using ForceRemove manually, and that we'll rely on ref counting exclusively
> (with the exception of FlushAllImages), so we'll eventually just keep the
> textures in the array and let the ref count do the work.

Yeah, it is a better name. I'll change it in next patch.

> 
> @@ +178,5 @@
> > +  /**
> > +   * Forcibly remove texture data from TextureClient
> > +   * This function needs to be called after a tansaction with Compositor.
> > +   */
> > +  virtual void ForceRemoveTexturesIfNecessary()
> 
> I'd prefer to rename this into "RemovePendingTextures". Soon we won't be
> calling ForceRemove, we'll just clear the array and let reference counting
> do the job.

I'll change in a next patch.

> 
> @@ +243,5 @@
> >  
> >  protected:
> >    TextureFactoryIdentifier mTextureFactoryIdentifier;
> >    bool mMultiProcess;
> > +  nsTArray<RefPtr<TextureClient> > mForceRemovingTextures;
> 
> How about naming it "mTexturesToRemove"?

Change it in a next patch.

Comment 18

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> ImageBridgeChild::FlushAllImagesNow() wait end of transaction synchronously.
> Even after the patch, the textures are removed synchronously. There is no
> need to remove the texture right away in ImageClientSingle::FlushAllImages().

My main worry is, what if there is no next transaction? There isn't always one, especially around shutdown which is when FlushAllImages is typically called.
Or if we know for sure that there will be one, then we should have a way to assert that in case that invariant breaks somehow (otherwise it's going to cause us to block forever which isn't great because it doesn't provide with nice information when debugging).

Why can't we just call ForceRemove right away in the case of FlushAllImages? There is no risk of flickering since it is only happening when we shut down and have nothing more to display.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #18)
> My main worry is, what if there is no next transaction? There isn't always
> one, especially around shutdown which is when FlushAllImages is typically
> called.

ImageClientSingle::FlushAllImages() is called only from ImageBridgeChild::FlushAllImagesNow(). In ImageBridgeChild::FlushAllImagesNow(), ImageBridgeChild::EndTransaction() is always called. And the patch handles the case of no transaction in ImageBridgeChild::EndTransaction(). So, no next transaction is handled by the patch.

> Why can't we just call ForceRemove right away in the case of FlushAllImages?
> There is no risk of flickering since it is only happening when we shut down
> and have nothing more to display.

In current gecko's implementation, ImageClientSingle::FlushAllImages() uses TextureClient::ForceRemove(). From the following reasons, it should be handled by CompositableClient/Host as same as before PTexture.
- Fence object need to be delivered to TextureClient even when ImageClientSingle::FlushAllImages() is called.
- When TextureClient is rendered to multiple targets, ImageClientSingle::FlushAllImages() should affect to the TextureClient, but not to other CompositableClient. In current gecko, ImageClientSingle::FlushAllImages() disable the all Compositable's rendering that using same TexutreClient.

Comment 20

[Nicolas Silva [:nical]]

You convinced me :)

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - keep TextureClient until Transaction complete(r+nical)

Fix build failure on linux.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - keep TextureClient until Transaction complete(r+nical)

Fix mac build failure.

Comment 23

[Sotaro Ikeda [:sotaro]]

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

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch v5 - Keep TextureClient until Transaction complete(r=nical)

Committable patch.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/921523a69a9b

Comment 27

[William Hsu [:whsu]]

Verified it.
Thanks for the help!

* Build information:
 - Gaia      4c3b2f57f4229c5f36f0d8fd399e65f4db88f104
 - Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/3aaca223b673
 - BuildID   20140330160202
 - Version   30.0a2