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)
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)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•13 years ago
|
Priority: -- → P3
Updated•13 years ago
|
Assignee: nobody → margaret.leibovic
Comment 1•13 years ago
|
||
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.
Updated•13 years ago
|
Keywords: regression,
regressionwindow-wanted
Version: unspecified → Firefox 11
Reporter | ||
Comment 2•13 years ago
|
||
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.
Keywords: regressionwindow-wanted
Comment 3•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Thanks for taking a look. I'm also cc'ing kats, since he reviewed that change.
Comment 7•13 years ago
|
||
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•13 years ago
|
||
Kats, are you able to reproduce this now?
Comment 9•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Updated•13 years ago
|
tracking-fennec: --- → 11+
Comment 13•13 years ago
|
||
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
Comment 14•13 years ago
|
||
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
Comment 15•13 years ago
|
||
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
Comment 16•13 years ago
|
||
(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.
Updated•13 years ago
|
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
Comment 21•13 years ago
|
||
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.
Assignee | ||
Comment 22•13 years ago
|
||
(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.
Assignee | ||
Comment 24•13 years ago
|
||
(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...
Assignee | ||
Comment 25•13 years ago
|
||
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.
Assignee | ||
Comment 26•13 years ago
|
||
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?
Assignee | ||
Comment 27•13 years ago
|
||
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•13 years ago
|
||
:cwiiis, can you take this bug?
Assignee | ||
Comment 29•13 years ago
|
||
(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?
Assignee | ||
Comment 31•13 years ago
|
||
(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
Assignee | ||
Comment 33•13 years ago
|
||
ok, didn't realise this was also an MWC blocker - been looking at it this morning will take it.
Status: NEW → ASSIGNED
Assignee | ||
Comment 34•13 years ago
|
||
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.
Assignee | ||
Comment 35•13 years ago
|
||
I can confirm that this is the issue. Working on a patch.
Assignee | ||
Comment 36•13 years ago
|
||
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.
Assignee | ||
Comment 37•13 years ago
|
||
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.
Assignee | ||
Comment 38•13 years ago
|
||
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).
Assignee | ||
Comment 39•13 years ago
|
||
mbrubeck's suggestion of setting min width/height works perfectly.
Attachment #598895 -
Flags: review?(mbrubeck)
Assignee | ||
Updated•13 years ago
|
Assignee: chris.double → chrislord.net
Comment 40•13 years ago
|
||
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+
Comment 41•13 years ago
|
||
(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•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 43•13 years ago
|
||
Target Milestone: --- → Firefox 13
Samsung Galaxy S II, Galaxy Nexus
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•