902929 - Regression: HTML5 video not visible after entering full-screen

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[u421692]

Attachment: device-2013-08-08-175111.png

Environment:
Device: LG Nexus 4 (Android 4.2.2)
Build: Nightly 26.0a1 (2013-08-06) / Aurora 25.0a2 (2013-08-08)

1. Open "http://random.pavlov.net/silent.ogg".
2. Long tap on video and choose Full Screen.
3. Tap on back button.

Expected result:
At step 2: The video is played in full screen.
At step 3: The video is played in windowed mode.

Actual result:
At step 2: The screen is black, but the video controls are available.
At step 3: The screen is grey, but the video controls are available.
See attached screenshot.

Comment 1

[u421692]

Regression window in central:
good: Nightly 25.0a1 (2013-08-05)
bad: Nightly 26.0a1 (2013-08-06)

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=482b9d04974a&tochange=a0dd80f800e2

Comment 2

[Aaron Train [:aaronmt]]

Any way to narrow down on inbound, there's a huge window there. I can reproduce on my LG Nexus 4 (Android 4.3). Also, this might need to be moved to Core.

Comment 3

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=79fb01e0e3fa&tochange=620bbab02c0b

http://hg.mozilla.org/integration/mozilla-inbound/rev/c6962b015a6a

Bug 874721 and or bug 899435

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Nick, sounds like you regressed Android. Mind having a look?

Comment 5

[Nick Cameron [:nrc]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> Nick, sounds like you regressed Android. Mind having a look?

Looking into this, I can repro and with a debug build I get a helpful assertion so it should be easy enough to fix. But I need to re-root my device because Android 4.3, in order to get JimDB to work and I have been faffing around with that for a while. Sigh.

Comment 6

[Nick Cameron [:nrc]]

Nical: what kind of Attach/Detach stuff do we do for async video when we go to full screen on Android? We seem to be attaching an ImageHost to an ImageLayer where there is already an ImageHost attached. Annoyingly detaching the old ImageHost doesn't fix it, so there is more to this bug than just that. But I don't know where to start looking.

Comment 7

[Nicolas Silva [:nical]]

I have don't know what happens when android goes full screen, unfortunately. It's not a scenario that has stood out as one of the complicated cases (until now).

Comment 8

[Nick Cameron [:nrc]]

Attachment: rejig attaching compositable hosts to layers

Async video makes me sad.

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 790590 [details] [diff] [review]
rejig attaching compositable hosts to layers

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

::: gfx/layers/ipc/LayerTransactionParent.h
@@ +95,5 @@
>    virtual bool DeallocPCompositableParent(PCompositableParent* actor) MOZ_OVERRIDE;
>  
> +  void Attach(ShadowLayerParent* aLayerParent,
> +              CompositableParent* aCompositable,
> +              bool aIsAsyncVideo);

This version of Attach should never be called with async-video!
Async-video compositables get attached using AttachAsyncCompositable
ImageClientBridge should not even have a CompositableChild.

So we should not need the extra boolean parameter here.

The async-video setup should be:

* On the main thread:
The layer holds a reference to an ImageClientBridge which only job is to call AttachAsyncCompositable if needed. It does not have an IPDL actor.

* On the ImageBridgeChild thread (and the video state machine thread):
the ImageContainer has a reference to a regular ImageClient which has an IPDL actor.
The ImageClient cannot be used in OpAttachCompositable(PLayer PCompositable) because it is not managed by LayerTransactions (instead it is managed by CompositableTransactions which is a subset of the former, and does not have any message containing PLayer).


* On the compositor side:
The LayerComposite has a reference to a ImageHost that corresponds to the ImageClient of the ImageBridgeChild thread. The ImageClientBrdige does *not* have a counterpart on the compositor side, since it's only puropse is to send to the LayerComposite the ID of the ImageBridge's ImageClient.

I imagine that in the case of this bug for some reason the ImageClientBridge (on the main thread) gets a CompositableChild, which creates a CompositableParent and a CompositableHost on the compositor side *in addition* to the CompositableParent and CompositableHost of the ImageClient that lives on the ImageBridge thread, and creates a conflict.

We should add an assert that ImageClientBridge never has a CompositableChild.

Comment 10

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #9)
> Comment on attachment 790590 [details] [diff] [review]
> rejig attaching compositable hosts to layers
> 
> Review of attachment 790590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/LayerTransactionParent.h
> @@ +95,5 @@
> >    virtual bool DeallocPCompositableParent(PCompositableParent* actor) MOZ_OVERRIDE;
> >  
> > +  void Attach(ShadowLayerParent* aLayerParent,
> > +              CompositableParent* aCompositable,
> > +              bool aIsAsyncVideo);
> 
> This version of Attach should never be called with async-video!
> Async-video compositables get attached using AttachAsyncCompositable
> ImageClientBridge should not even have a CompositableChild.
> 

This is not true right now - we use the AttachAsyncCompositable message, but there is no AttachAsyncCompositable method on layers, both 'attach' messages take the same path once we hit LayerTransactionParent. See

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayerTransactionParent.cpp#l404


> So we should not need the extra boolean parameter here.
>

Comment 11

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #9)

> I imagine that in the case of this bug for some reason the ImageClientBridge
> (on the main thread) gets a CompositableChild, which creates a
> CompositableParent and a CompositableHost on the compositor side *in
> addition* to the CompositableParent and CompositableHost of the ImageClient
> that lives on the ImageBridge thread, and creates a conflict.
> 
> We should add an assert that ImageClientBridge never has a CompositableChild.

I confirmed that this is not happening.

Comment 12

[Nick Cameron [:nrc]]

So, the question is, what should be happening when the video goes full screen? What I observe happening is that we attach the same compositable parent (via the async attach mechanism and the compositable map) to a new layer.

My model for this (not verified) is that we create a new layer tree with a new ImageClientBridge which calls AttachCompositableAsync with the id for the old ImageBridge's ImageClient.

Is this correct? Because if so we need the attached patch to make attaching work properly. Otherwise I guess we need to change something.

Comment 13

[Nick Cameron [:nrc]]

Attachment: assertion

Comment 14

[Nick Cameron [:nrc]]

needinfo for comment 12 and possibly patch re-review

Comment 15

[Nicolas Silva [:nical]]

Comment on attachment 790590 [details] [diff] [review]
rejig attaching compositable hosts to layers

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

Oh! sorry I should have had an extra cup of coffee before doing that review. Flagging myself for a re-review.
Now I am still unsure about why we can't just detach the compositable before reattaching. Detach just clears out the compositor and device textures because the compositable could be attached to a layer using a different compositor (that happens for instance when dragging a tab containing a video to another window) and the widget (and gl context) could be destroyed before reattaching so we want to make sure we are not holding on to gl resources after Detaching.

Comment 16

[Nicolas Silva [:nical]]

Comment on attachment 790590 [details] [diff] [review]
rejig attaching compositable hosts to layers

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

::: gfx/layers/composite/CompositableHost.h
@@ +202,4 @@
>    }
> +  // Detach this compositable host from its layer.
> +  // If we are used for async video, then it is not safe to blindly detach since
> +  // we might be re-attached to a different layer. aLayer is the layer which the

Please elaborate on this. Why is it not safe? the point of detaching is partly to make sure we can re-attach to a different layer. If this is not safe we should make it safe.

Comment 17

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #16)
> Comment on attachment 790590 [details] [diff] [review]
> rejig attaching compositable hosts to layers
> 
> Review of attachment 790590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/CompositableHost.h
> @@ +202,4 @@
> >    }
> > +  // Detach this compositable host from its layer.
> > +  // If we are used for async video, then it is not safe to blindly detach since
> > +  // we might be re-attached to a different layer. aLayer is the layer which the
> 
> Please elaborate on this. Why is it not safe? the point of detaching is
> partly to make sure we can re-attach to a different layer. If this is not
> safe we should make it safe.

Well, currently we only Detach when we CleanupResources, so basically when we are destroying our compositables. We do not detach before re-attaching, that is part of the problem here. (I'm not sure we should either, we should just regard Attach as possibly re-attach in the async case, which is what this patch does).

So, the bad sequence is:

1 compositable attached to layer A
2 compositable re-attached to layer B
3 layer A dies and calls Detach on compositable
  --> compositable is detached from layer B
4 layer B renders but its compositable is detached
  --> sadness.

This patch fixes this by checking at step 3 which layer is being detached and only detaching if we are detaching from the currently attached layer.

Comment 18

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6356b67f6c87
https://hg.mozilla.org/integration/mozilla-inbound/rev/315c9f0af9bb

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out under suspicion of breaking test_media_sniffer.html <https://tbpl.mozilla.org/php/getParsedLog.php?id=26799920&tree=Mozilla-Inbound>:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b89d05e87140
https://hg.mozilla.org/integration/mozilla-inbound/rev/662edd7ed81c

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like Android and B2G are where it breaks:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26800872&tree=Mozilla-Inbound


INFO -  08-21 03:36:49.106   694   694 I GeckoDump: 757 INFO TEST-PASS | /tests/content/media/test/test_media_sniffer.html | The media loads when served with application/octet-stream.
20:37:57     INFO -  08-21 03:36:49.146   694   694 I GeckoDump: 758 INFO TEST-PASS | /tests/content/media/test/test_media_sniffer.html | [finished gizmo.mp4-3] Length of array should match number of running tests
20:37:57     INFO -  08-21 03:36:50.777   694   694 I Gecko   :
20:37:57     INFO -  08-21 03:36:50.777   694   694 I Gecko   : ###!!! [Child][SyncChannel] Error: Channel error: cannot send/recv
20:37:57     INFO -  08-21 03:36:50.777   694   694 I Gecko   :
20:37:57     INFO -  08-21 03:36:50.847    33    33 I ServiceManager: service 'media.resource_manager' died
20:37:57     INFO -  08-21 03:36:50.897    40   936 E OMXNodeInstance: !!! Observer died. Quickly, do something, ... anything...
20:37:57     INFO - Return code: 0
20:37:57     INFO - TinderboxPrint: mochitest<br/><em class="testfail">T-FAIL</em>

Comment 21

[Nick Cameron [:nrc]]

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

Comment 22

[Nick Cameron [:nrc]]

Attachment: rejig attaching compositable hosts to layers

Turns out I missed modifying one of the Detach calls. Carrying r=nical

Comment 23

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6452864ba27
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7940a4a91a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fe7940a4a91a
https://hg.mozilla.org/mozilla-central/rev/a6452864ba27

Comment 26

[Frank Wein [:mcsmurf]]

Are there plans to land this on aurora (just wondering as bug is already closed)?

Comment 27

[Aaron Train [:aaronmt]]

This is still broken on Aurora.

Comment 28

[Nick Cameron [:nrc]]

Comment on attachment 793804 [details] [diff] [review]
rejig attaching compositable hosts to layers

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 874721
User impact if declined: Unable to view full screen video on Android - crashes.
Testing completed (on m-c, etc.): Been on m-c for over two weeks
Risk to taking this patch (and alternatives if risky): Medium risk - it is not a small patch. But it is purely for fixing an existing bug and has not caused problems in a pretty long time in nightlies. No alternatives exist (well, I guess we could disable full-screen video, but that kind of sucks and I'm not sure how easy it is).
String or IDL/UUID changes made by this patch: none

Comment 29

[Nick Cameron [:nrc]]

(In reply to Aaron Train [:aaronmt] from comment #27)
> This is still broken on Aurora.

Yes, needs uplift. Requested in previous comment.

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 793804 [details] [diff] [review]
rejig attaching compositable hosts to layers

medium-risk is OK for Aurora given this is an important regression

Comment 31

[Nick Cameron [:nrc]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/178978aaff0c