709813 - Video frames stop updating in DOM full screen mode on Android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Andreea Pod]

Mozilla /5.0 (Android;Linux armv7l;rv:11.0a1) Gecko/20111212 Firefox/11.0a1 Fennec/11.0a1
Device: LG Optimus 2X (Android 2.2)

Steps to reproduce:
1. Go to http://random.pavlov.net/video.html 
2. Long tap on video to open the context menu
3. tap on Full Screen

Expected results:
video should play in full screen

Actual results:
White screen is displayed and video is not playing.

Comment 1

[:Margaret Leibovic]

Can you provide a regression range for this? I'm wondering if some change to our gecko video or dom full screen code caused this.

Comment 2

[Andreea Pod]

Not reproducible on:
Mozilla /5.0 (Android;Linux armv7l;rv:11.0a1) Gecko/20111123 Firefox/11.0a1 Fennec/11.0a1
http://hg.mozilla.org/projects/birch/rev/cd5725c23a13

Reproducible on:
Mozilla /5.0 (Android;Linux armv7l;rv:11.0a1) Gecko/20111124 Firefox/11.0a1 Fennec/11.0a1
http://hg.mozilla.org/projects/birch/rev/f8505fcb5808

Note:
- I don't know why I get error when  accessing the links above
- on the builds until 11/24 the video is working in full screen but is not centered.

Comment 3

[:Margaret Leibovic]

Thanks, Andreea. The links aren't working because the birch project branch has been reset, but you can view the changesets on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/cd5725c23a13
http://hg.mozilla.org/mozilla-central/rev/f8505fcb5808

Comment 4

[:Margaret Leibovic]

I suspect the layout/font inflation changes in that range may have broken DOM fullscreen mode, since if you try to put other kinds of elements into full screen mode, they also look busted: http://pearce.org.nz/full-screen/.

dbaron, do you know if any of your changes may have impacted this?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Which font inflation changes are in that range?  That range is entirely within:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=cd5725c23a13 which, for birch, would have included merging of http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9a0abc25480&tochange=0ea84b44a7f1 from mozilla-central.  Bug 703141 (landed on birch) seems more likely if that range (and my analysis of it) is correct.

Comment 6

[:Margaret Leibovic]

Thanks for taking a look. I'm also cc'ing kats, since he reviewed that change.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

So I'm unable to reproduce this problem since my devices crash whenever I try loading any of these pages. (bug 713009). Since I can't reproduce it, I can't really provide much useful information right now; I'd have to basically dive into the code to figure out what codepaths are getting executed and how going into full-screen mode touches the viewport.

Comment 8

[:Margaret Leibovic]

Kats, are you able to reproduce this now?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Yes, just tried it, can reproduce the problem on random.pavlov.net. From the logs it looks like the page size is coming out to (width,height)=(0,0) when in full-screen, so that might be the problem. I'll verify that, but in the meantime do you know how the scrollHeight of the body/html elements are affected when in full screen mode? I'm not really sure of how gecko implements the full screen stuff.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: computed css properties on full-screen video element

Ok, so after the element goes into full screen, it ends up with the attached CSS properties. There's a lot of properties where I don't know how it affects the rendered page, but two things stick out. One is that the page scrollHeight/scrollWidth drop to (0,0) as I mentioned before. As far as I can tell, this is different in Fennec compared to desktop Ffx 9.0.1 - on desktop the scrollHeight remains non-zero. I assume this is because the video element is the only thing on the page, and it has position:static so it won't count towards the page dimensions?

The other thing is that the video element's width and height attributes are 800x480, rather than being the size of device display. I know we use 800x480 as the default viewport so I imagine that's where it's coming from.

Comment 11

[:Margaret Leibovic]

Thanks for looking at this, Kats. I'm cc'ing Chris in case he can lend any insight into what gecko is doing when we enter full screen mode.

Comment 13

[:Margaret Leibovic]

I'm un-assigning myself because I don't have any idea how to fix this, and I think it's probably a layout issue that's outside my realm of knowledge.

Comment 14

[:Margaret Leibovic]

I think this should be a higher priority. I know sites aren't really using DOM fullscreen mode yet, but it's pretty busted looking. Also, the fact that full screen videos are broken is bad, since that's a feature that we're trying to support.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Let's keep bug titles descriptive please. I will try to look at this after I've finished bug 716107.

Comment 16

[:Margaret Leibovic]

(In reply to Chris Pearce (:cpearce) from comment #15)
> Let's keep bug titles descriptive please.

I apologize for the bad job updating the summary - I wanted to expand this bug to include the fact that fullscreen mode is also broken for non-video elements, but I guess that could be filed as a separate bug.

Also, on the videos I've tested, you can't even see a frame of the video - the entire screen is just filled with a background texture.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kartikaya Gupta (:kats) from comment #10)
> Ok, so after the element goes into full screen, it ends up with the attached
> CSS properties. There's a lot of properties where I don't know how it
> affects the rendered page, but two things stick out. One is that the page
> scrollHeight/scrollWidth drop to (0,0) as I mentioned before.

That shouldn't be happening and is almost certainly related to the bug.

Comment 21

[cajbir (:cajbir)]

A bisect based on Margaret's range shows the following commit where it first breaks:

http://hg.mozilla.org/mozilla-central/rev/3f0f2ffed076

That's the first patch in a series but fullscreen remains broken after all the patches in bug 703141 that were landed.

Comment 22

[Chris Lord [:cwiiis]]

(In reply to Chris Double (:doublec) from comment #21)
> A bisect based on Margaret's range shows the following commit where it first
> breaks:
> 
> http://hg.mozilla.org/mozilla-central/rev/3f0f2ffed076
> 
> That's the first patch in a series but fullscreen remains broken after all
> the patches in bug 703141 that were landed.

That was a pretty huge patch... The likely culprit is that before that patch, the browser was the root node of our xul document and occupied the entire space in it - after that patch, the browser is housed in a vbox and is sized explicitly (rather than stretching into available space) so that we can 'fake' a displayport...

Why this causes full-screen to entirely fail, I don't know though - how exactly does full-screen mode work on mobile? I'll look into this.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think if you monitored the Viewport:Change events, you'd probably see some with zero width and height, and figuring out why those got dispatched would probably lead to the problem.

Comment 24

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #23)
> I think if you monitored the Viewport:Change events, you'd probably see some
> with zero width and height, and figuring out why those got dispatched would
> probably lead to the problem.

this sounds like a good hint... Though it's proving to be a bit tricky to debug...

Video performance is pretty good on my Flyer (why? No gralloc, single-core...), but I get a crash on full-screening with "java.lang.NoSuchMethodError: android.view.View.setSystemUiVisibility" (will look at this, this is on gingerbread) - performance is so bad on my Galaxy Nexus (dual-core, has gralloc...?!) that it's hard to do anything...

Comment 25

[Chris Lord [:cwiiis]]

Filed bug 728181 for the crash, with patch.

Note, I'm not seeing and viewports with zero width/height generated by Java when entering fullscreen. Still investigating.

Comment 26

[Chris Lord [:cwiiis]]

This is the sequence of events I get when going into fullscreen:

D/GeckoLayerController( 2345): setViewportSize: v=RectF(0.0, 0.0, 600.0, 1024.0) p=(600.0,1024.0) z=1.0 o=0.0,0.0
D/GeckoPanZoomController( 2345): generating valid viewport using v=RectF(0.0, 0.0, 600.0, 1024.0) p=(600.0,1024.0) z=1.0 o=0.0,0.0
D/GeckoPanZoomController( 2345): generated valid viewport as v=RectF(0.0, 0.0, 600.0, 1024.0) p=(600.0,1024.0) z=1.0 o=0.0,0.0
D/GeckoLayerController( 2345): setViewportMetrics: v=RectF(0.0, 0.0, 600.0, 1024.0) p=(600.0,1024.0) z=1.0 o=0.0,0.0
D/GeckoPanZoomController( 2345): generating valid viewport using v=RectF(0.0, 0.0, 600.0, 1024.0) p=(600.0,1024.0) z=1.0 o=0.0,0.0
D/GeckoPanZoomController( 2345): generated valid viewport as v=RectF(0.0, 0.0, 600.0, 1024.0) p=(600.0,1024.0) z=1.0 o=0.0,0.0
D/GeckoSoftwareLayerClient( 2345): ### beginDrawing(1280, 2048, 256, 256, {"x":0,"y":0,"width":600,"height":992,"offsetX":0,"offsetY":0,"pageWidth":0,"pageHeight":0,"zoom":1,"backgroundColor":"transparent"}, false)
D/GeckoSoftwareLayerClient( 2345): ### endDrawing(0, 0, 1280, 2048)
D/GeckoLayerController( 2345): setViewportMetrics: v=RectF(0.0, 0.0, 600.0, 1024.0) p=(0.0,0.0) z=1.0 o=0.0,0.0
D/GeckoPanZoomController( 2345): generating valid viewport using v=RectF(0.0, 0.0, 600.0, 1024.0) p=(0.0,0.0) z=1.0 o=0.0,0.0
D/GeckoPanZoomController( 2345): generated valid viewport as v=RectF(0.0, 0.0, 600.0, 1024.0) p=(0.0,0.0) z=1.0 o=0.0,0.0
D/GeckoLayerController( 2345): setViewportMetrics: v=RectF(0.0, 0.0, 600.0, 1024.0) p=(0.0,0.0) z=1.0 o=0.0,0.0
I/GeckoSoftwareLayerClient( 2345): zerdatime 770366476 - endDrawing
D/GeckoEvent( 2345): ### Viewport: v=RectF(0.0, 0.0, 600.0, 1024.0) p=(0.0,0.0) z=1.0 o=0.0,0.0
E/GeckoConsole( 2345): ### Viewport: { "x" : 0.0, "y" : 0.0, "width" : 600, "height" : 1024, "pageWidth" : 0.0, "pageHeight" : 0.0, "offsetX" : 0.0, "offsetY" : 0.0, "zoom" : 1.0 }

So as soon as we're in full-screen mode, pageWidth and pageHeight are zero. Given that zoom is 1.0, this in turn means that scrollWidth and scrollHeight of both the body and document element are zero (as kats has stated)... Which perhaps is to be expected? Will try just adding the screen size into the max-check... Is there a reason not to do that?

Comment 27

[Chris Lord [:cwiiis]]

If I make sure that the unzoomed page size can never be smaller than the window size, I end up with just a blank white page... Though I guess if the document scroll-size is zero, this is unsurprising (it'd just be painting the xul document background).

I guess we need to look at what's handling fullscreen and what it's doing... I suppose the way we've implemented displayport buffering has broken some assumption it makes.

Comment 28

[Erin Lancaster [:elan]]

:cwiiis, can you take this bug?

Comment 29

[Chris Lord [:cwiiis]]

(In reply to Erin Lancaster from comment #28)
> :cwiiis, can you take this bug?

I don't want to commit to taking this just yet, as we're concentrating on getting Maple to a good state by Wednesday. Just to keep this updated though, I can't see anything obvious under mobile/android that would cause this...

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Lord [:cwiiis] from comment #26)
> So as soon as we're in full-screen mode, pageWidth and pageHeight are zero.

I don't know this Java code, but setting zero pageWidth and pageHeight is surely incorrect?

Comment 31

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #30)
> (In reply to Chris Lord [:cwiiis] from comment #26)
> > So as soon as we're in full-screen mode, pageWidth and pageHeight are zero.
> 
> I don't know this Java code, but setting zero pageWidth and pageHeight is
> surely incorrect?

The pageWidth and pageHeight come from this snippet in browser.js:

      let pageWidth = this._viewport.width, pageHeight = this._viewport.height;
      if (doc instanceof SVGDocument) {
        let rect = doc.rootElement.getBoundingClientRect();
        // we need to add rect.left and rect.top twice so that the SVG is drawn
        // centered on the page; if we add it only once then the SVG will be
        // on the bottom-right of the page and if we don't add it at all then
        // we end up with a cropped SVG (see bug 712065)
        pageWidth = Math.ceil(rect.left + rect.width + rect.left);
        pageHeight = Math.ceil(rect.top + rect.height + rect.top);
      } else {
        let body = doc.body || { scrollWidth: pageWidth, scrollHeight: pageHeight };
        let html = doc.documentElement || { scrollWidth: pageWidth, scrollHeight: pageHeight };
        pageWidth = Math.max(body.scrollWidth, html.scrollWidth);
        pageHeight = Math.max(body.scrollHeight, html.scrollHeight);
      }

      /* Transform the page width and height based on the zoom factor. */
      pageWidth *= this._viewport.zoom;
      pageHeight *= this._viewport.zoom;

i.e. they're either the body or document element's scrollWidth/scrollHeight. These shouldn't be zero - forcing them not to be doesn't help, as there's nothing drawn there anyway.

Comment 32

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Lifting to blocker for MWC-Demo

Comment 33

[Chris Lord [:cwiiis]]

ok, didn't realise this was also an MWC blocker - been looking at it this morning will take it.

Comment 34

[Chris Lord [:cwiiis]]

Been reading through the fullscreen code. My current thinking is that the way we layout the document is causing the CSS rule that gets set on fullscreen elements to break:

http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#251

The mechanism for switching to Android fullscreen is certainly fine. We also probably need to have separate viewport handling for when there is a fullscreen element on the document. WIP.

Comment 35

[Chris Lord [:cwiiis]]

I can confirm that this is the issue. Working on a patch.

Comment 36

[Chris Lord [:cwiiis]]

hmm, it would appear that this width/height thing is only indicative of another issue though... Applying the same rules to an element not in fullscreen mode works fine.

Comment 37

[Chris Lord [:cwiiis]]

The problem is this fullscreen rule in ua.css is applying to the browser element, as well as whatever has been set fullscreen - this is causing the width/height set in setBrowserSize in browser.js to be overridden, temporarily.

Comment 38

[Chris Lord [:cwiiis]]

I'm drawing a blank on how to fix this cleanly (I've tried several things) - Cc'ing cpearce for comment.

The problem appears to be that the fullscreen ua style rule is applying to the browser element and overriding the width and height we set on it with 100% (which ends up being zero, the way we have our application laid out, I think).

Comment 39

[Chris Lord [:cwiiis]]

Attachment: Fix fullscreen mode by using minWidth/Height

mbrubeck's suggestion of setting min width/height works perfectly.

Comment 40

[Matt Brubeck (:mbrubeck)]

Comment on attachment 598895 [details] [diff] [review]
Fix fullscreen mode by using minWidth/Height

Appending " !important!" to the rules might work too.  (I'm not sure which is better, since either could be broken by future changes to ua.css...)

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Lord [:cwiiis] from comment #38)
> The problem appears to be that the fullscreen ua style rule is applying to
> the browser element and overriding the width and height we set on it with
> 100% (which ends up being zero, the way we have our application laid out, I
> think).

This is intentional. By setting the fullscreen element, and its containing frames' elements (browser, iframes etc) to width and height 100%, we ensure that the fullscreen element completely fills its containing frame, and its containing frames completely fill their containing frames. That way the fullscreen element encompases the entire screen.

It sounds like the problem is that your browser element has width/height 0 in fullscreen mode somehow?

Comment 42

[Joe Drew (not getting mail)]

https://hg.mozilla.org/projects/maple/rev/9ed5ea2c7357

Comment 43

[Ed Morley [:emorley]]

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

Comment 44

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Samsung Galaxy S II, Galaxy Nexus