Closed Bug 951732 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla29
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
b2g-v1.4 --- verified

People

(Reporter: nkot, Assigned: sotaro)

References

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

Attached file 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
video of the issue  http://youtu.be/piZT3W_kMew
blocking-b2g: --- → 1.4?
Keywords: smoketest
Issue appears to occur on Youtube videos through Browser app. Using same build as Comment 0.
QA Contact: bzumwalt
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
Keywords: smoketest
The Camera app is also affected. We use video element at the viewfinder of Camera app.
Issue also occurs when rotating/changing orientation of phone in viewfinder of Camera app

[Same environmental variables as Comment 0]
Duplicate of this bug: 958058
Component: Gaia::Video → Graphics: Layers
Product: Firefox OS → Core
Assignee: nobody → sotaro.ikeda.g
Blocks: 946720
This bug seems regression of Bug 897452. I recognize it during debugging Bug 946720.
No longer blocks: 946720
Status: NEW → ASSIGNED
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 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.
(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)
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
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.
Youtube video playback was failed even on a ROM that created from default source. I am going to confirm again by using latest source.
(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.
By the patch the video playback problem is fixed. Confirmed it on master hamachi and master nexus-4.
Attachment #8360826 - Attachment is obsolete: true
Attachment #8361214 - Flags: review?(nical.bugzilla)
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"?
(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.
(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.
(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.
Flags: needinfo?(nical.bugzilla)
You convinced me :)
Flags: needinfo?(nical.bugzilla)
Attachment #8361214 - Flags: review?(nical.bugzilla) → review+
Fix build failure on linux.
Attachment #8361214 - Attachment is obsolete: true
Attachment #8362703 - Flags: review+
Fix mac build failure.
Attachment #8362703 - Attachment is obsolete: true
Blocks: 962101
No longer blocks: 962101
Committable patch.
Attachment #8362964 - Attachment is obsolete: true
Attachment #8364006 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/921523a69a9b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
blocking-b2g: 1.4? → 1.4+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.