Closed Bug 902929 Opened 11 years ago Closed 11 years ago

Regression: HTML5 video not visible after entering full-screen

Categories

(Core :: Graphics: Layers, defect)

26 Branch
ARM
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox24 --- unaffected
firefox25 + verified
firefox26 + verified
fennec 25+ ---

People

(Reporter: u421692, Assigned: nrc)

References

()

Details

(Keywords: regression, reproducible)

Attachments

(3 files, 1 obsolete file)

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.
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
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.
tracking-fennec: --- → ?
Component: General → Graphics, Panning and Zooming
Summary: HTML5 video can not be played after trying to play in Full Screen → HTML5 video not visible after entering full-screen
Summary: HTML5 video not visible after entering full-screen → Regression: HTML5 video not visible after entering full-screen
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
Component: Graphics, Panning and Zooming → Graphics: Layers
Product: Firefox for Android → Core
Version: Firefox 25 → 26 Branch
Blocks: 899435, 874721
Nick, sounds like you regressed Android. Mind having a look?
Assignee: nobody → ncameron
tracking-fennec: ? → 25+
(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.
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.
Flags: needinfo?(nical.bugzilla)
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).
Flags: needinfo?(nical.bugzilla)
Async video makes me sad.
Attachment #790590 - Flags: review?(nical.bugzilla)
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.
Attachment #790590 - Flags: review?(nical.bugzilla) → review-
(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.
>
(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.
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.
Attached patch assertionSplinter Review
Attachment #792476 - Flags: review?(nical.bugzilla)
needinfo for comment 12 and possibly patch re-review
Flags: needinfo?(nical.bugzilla)
Attachment #792476 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #790590 - Flags: review- → review?
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.
Attachment #790590 - Flags: review? → review?(nical.bugzilla)
Flags: needinfo?(nical.bugzilla)
(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.
Attachment #790590 - Flags: review?(nical.bugzilla) → review+
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>
Turns out I missed modifying one of the Detach calls. Carrying r=nical
Attachment #790590 - Attachment is obsolete: true
Attachment #793804 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fe7940a4a91a
https://hg.mozilla.org/mozilla-central/rev/a6452864ba27
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Are there plans to land this on aurora (just wondering as bug is already closed)?
This is still broken on Aurora.
Flags: needinfo?(ncameron)
No longer blocks: 899435
Flags: needinfo?(ncameron)
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
Attachment #793804 - Flags: approval-mozilla-aurora?
(In reply to Aaron Train [:aaronmt] from comment #27)
> This is still broken on Aurora.

Yes, needs uplift. Requested in previous comment.
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
Attachment #793804 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.