Closed Bug 709813 Opened 13 years ago Closed 13 years ago

Video frames stop updating in DOM full screen mode on Android

Categories

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

11 Branch
ARM
Android
defect

Tracking

(fennec11+)

VERIFIED FIXED
Firefox 13
Tracking Status
fennec 11+ ---

People

(Reporter: andreea.pod, Assigned: cwiiis)

References

(Blocks 2 open bugs, )

Details

(Keywords: regression, Whiteboard: mwc-demo)

Attachments

(2 files)

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.
Priority: -- → P3
Assignee: nobody → 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.
Version: unspecified → Firefox 11
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.
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
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?
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.
Thanks for taking a look. I'm also cc'ing kats, since he reviewed that change.
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.
Kats, are you able to reproduce this now?
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.
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.
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.
tracking-fennec: --- → 11+
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.
Assignee: margaret.leibovic → nobody
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.
Summary: Video not playing in full screen mode → Full screen mode is broken
Let's keep bug titles descriptive please. I will try to look at this after I've finished bug 716107.
Summary: Full screen mode is broken → Video frames stop updating in DOM full screen mode on Android
(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.
Whiteboard: mwc-demo
(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.
Assignee: nobody → chris.double
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.
(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.
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.
(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...
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.
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?
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.
Blocks: 728251
Blocks: 728249
:cwiiis, can you take this bug?
(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...
(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?
(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.
Lifting to blocker for MWC-Demo
Severity: normal → blocker
ok, didn't realise this was also an MWC blocker - been looking at it this morning will take it.
Status: NEW → ASSIGNED
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.
I can confirm that this is the issue. Working on a patch.
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.
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.
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).
mbrubeck's suggestion of setting min width/height works perfectly.
Attachment #598895 - Flags: review?(mbrubeck)
Assignee: chris.double → chrislord.net
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...)
Attachment #598895 - Flags: review?(mbrubeck) → review+
(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?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Samsung Galaxy S II, Galaxy Nexus
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: