Closed
Bug 896250
Opened 12 years ago
Closed 12 years ago
Flash content, including YouTube, is clipped to half the expected height and width on Retina display
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox24 | --- | unaffected |
firefox25 | --- | fixed |
People
(Reporter: cpeterson, Assigned: milan)
References
()
Details
(Keywords: regression, reproducible)
Attachments
(3 files)
9.53 KB,
image/png
|
Details | |
1.06 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
The Flash animation at http://www.informationisbeautiful.net/play/snake-oil-supplements/ is half the expected width and height. This is a regression in Nightly 25 build 2013-07-20.
Here is the pushlog from build 2013-07-19 and 2013-07-20:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b2c82ae98db&tochange=0983f1a0961c
Reporter | ||
Comment 1•12 years ago
|
||
BenWa: could this Flash problem be a regression from bug 873378?
I bisected the mozilla-inbound TBPL builds and I believe the regression is from this range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b9fe5a05edd9&tochange=bdf71cb870ac
Comment 2•12 years ago
|
||
Ohh it's possible and quite interesting. If it is that would mean that we don't support drawing plugin layers without an active layer.
Is this with a retina macbook?
Flags: needinfo?(bgirard)
Reporter | ||
Comment 3•12 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #2)
> Is this with a retina macbook?
Yes, I forgot to mention I'm using a Retina MacBook. If I change the gfx.hidpi.enabled pref from 2 (default) to 0 (disabled), then Flash animations are rendered correctly.
Comment 4•12 years ago
|
||
I wonder if there's a bug. I'm at a conference right now and don't have a retina. Can you try breaking on this line:
http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2090
It has to be getting hit unexpectedly.
Reporter | ||
Comment 5•12 years ago
|
||
Sorry, I don't have a Mac build environment to debug this.
Can you recommend someone else to investigate? Clipped Flash content, including YouTube videos, on Retina displays seems like a pretty critical regression.
Flags: needinfo?(bgirard)
Summary: Flash animations are half the expected width and height → Flash content, including YouTube, is clipped to half the expected height and width on Retina display
Assignee | ||
Comment 6•12 years ago
|
||
Matt or Jeff, can you take a look?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bgirard)
Comment 7•12 years ago
|
||
I volunteer Joe as he has a Retina display
Flags: needinfo?(jmuizelaar) → needinfo?(joe)
Assignee | ||
Comment 8•12 years ago
|
||
I don't see the problem on my 10.8.3 retina.
Assignee | ||
Comment 9•12 years ago
|
||
And when I say 10.8.3, I of course mean 10.8.4.
Comment 10•12 years ago
|
||
Try forcing LAYER_NONE for nsPluginFrame. I suspect the bug is inacte plugins drawing in the Thebes layer don't honor the npapi content scale.
Reporter | ||
Comment 11•12 years ago
|
||
Milan: what version of Flash are you testing? I have Flash Player 11.8.800.94, the latest release.
Flags: needinfo?(milan)
Assignee | ||
Comment 12•12 years ago
|
||
I have 11.7.700.225 and was about to update, but ironically, the http://www.adobe.com/software/flash/about/ demonstrated the problem. So, yes, I can reproduce.
Flags: needinfo?(milan)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 13•12 years ago
|
||
BenWa, was there another bug for this?
Chris, this fixes the problem for me.
Assignee | ||
Updated•12 years ago
|
Attachment #779397 -
Attachment is patch: true
Attachment #779397 -
Attachment mime type: text/x-pascal → text/plain
Assignee | ||
Updated•12 years ago
|
Attachment #779397 -
Flags: review?(bgirard) → review?(joe)
Updated•12 years ago
|
Attachment #779397 -
Flags: review?(joe) → review+
Comment 14•12 years ago
|
||
We need to file a follow-up bug about the fact that plugins are 1/4-clipped as BenWa mentions in comment 10.
Assignee | ||
Comment 15•12 years ago
|
||
BenWa, I'll let you file the bug Joe mentions in Comment 14, I want to make sure it's correctly done :) We'll keep this bug for the recent regression that exposed that bug.
Keywords: checkin-needed
Comment 16•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1b4d5c51b7
Can we get a test for this regression?
Comment 17•12 years ago
|
||
FYI this bug appears to fix the problem, on Windows 8 at least, described by this bug report:
https://bugzilla.mozilla.org/show_bug.cgi?id=896198
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1b4d5c51b7
>
> Can we get a test for this regression?
Good question. We don't currently have any Flash tests, but I'll take a look at what could be done, as these would be useful.
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1b4d5c51b7
>
If the backout does fix Android orange, it's probably the bug 873378 that wasn't passing it, but because of the typo fixed here, it was never using 20 as the layer max, but 1 instead.
Comment 20•12 years ago
|
||
Backed out for Android reftest-2 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa3b74214e4
https://tbpl.mozilla.org/php/getParsedLog.php?id=25623153&tree=Mozilla-Inbound
Comment 22•12 years ago
|
||
master is currently broken for B2G until this patch lands (it was broken when bug 873378 landed).
If this isn't fixed really soon, then I'd like to land a #ifdef MOZ_WIDGET_GONK version of this patch that correct the return value for gonk
Comment 23•12 years ago
|
||
Comment 24•12 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #23)
> Created attachment 780679 [details] [diff] [review]
> Set layers.max-active to 1 for mobile, since 20 is failing reftest
>
> https://tbpl.mozilla.org/?tree=Try&rev=c7ed91cc82f2
Is this going to fix the choppy scrolling on Windows?
Comment 25•12 years ago
|
||
(In reply to Gary [:streetwolf] from comment #24)
> (In reply to Dave Hylands [:dhylands] from comment #23)
> > Created attachment 780679 [details] [diff] [review]
> > Set layers.max-active to 1 for mobile, since 20 is failing reftest
> >
> > https://tbpl.mozilla.org/?tree=Try&rev=c7ed91cc82f2
>
> Is this going to fix the choppy scrolling on Windows?
I don't know (I don't have a windows machine). With the tree the way it is right now, max-active layers is being set to 1 for everybody, which breaks b2g, and may cause other effects on other platforms - but I'm not sure.
With the "corrected" patch max-active layers would be -1 for all platforms except android, where it would be 20, except that this makes the android reftest fail.
The patch I added makes max-active layers be 1 for android (so reftest passes), and -1 for everybody else, so things go back to the way they were before bug 873378 landed.
Comment 26•12 years ago
|
||
When this patch first landed on inbound it did fix my scrolling problem on Windows 8 but unfortunately broke Android. On the Windows side is your new patch doing the same thing as the original patch? If so then it should fix scrolling on Windows.
Comment 27•12 years ago
|
||
If you have the patch on a Try server I'll gladly test it out for you on Windows 8.
Comment 28•12 years ago
|
||
Found your Try server and your patch does indeed fix the scrolling on Windows 8. Here's hoping it doesn't break Android again because scrolling on Windows without this patch is terrible.
Comment 29•12 years ago
|
||
Didn't realize that I need to select android reftests manually, so here's my try for that.
https://tbpl.mozilla.org/?tree=Try&rev=88bed1e45718
Updated•12 years ago
|
Attachment #780679 -
Flags: review?(joe)
Assignee | ||
Comment 30•12 years ago
|
||
Thanks all for chasing this, both BenWa and I are offline until Friday/Monday, which is why this dragged on.
Updated•12 years ago
|
Attachment #780679 -
Flags: review?(joe) → review+
Comment 31•12 years ago
|
||
So I'd like to go ahead and land these 2 patches.
Presumably, this bug should be left open until the layers stuff for android gets sorted out.
Comment 32•12 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/4f35e943f939
https://hg.mozilla.org/projects/birch/rev/da5fa63e90cb
Whiteboard: [fixed-in-birch]
Comment 33•12 years ago
|
||
I talked to joe on IRC and apparently there is a new bug for flash not working, so this bug can follow the normal wokflow.
Comment 34•12 years ago
|
||
Will this be making https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/ soon?
Comment 35•12 years ago
|
||
(In reply to Gary [:streetwolf] from comment #34)
> Will this be making
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> inbound-win32/ soon?
I landed on birch (which is essentially b2g-inbound), which RyanVM merges to m-c, assuming everything is green. Presumably m-i-win32 then syncs up with m-c periodically, so it should pick it up the next merge after this lands on m-c.
Comment 36•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/da5fa63e90cb
https://hg.mozilla.org/mozilla-central/rev/4f35e943f939
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 37•12 years ago
|
||
Unfortunately, I had to back this out again due to OSX debug asserts that appeared once birch was merged to m-c. Apparently this patch didn't get along well with something that landed on m-c after your last Try push :(
https://hg.mozilla.org/mozilla-central/rev/46d73e889cb4
https://tbpl.mozilla.org/php/getParsedLog.php?id=25752577&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=25750893&tree=Mozilla-Central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [fixed-in-birch]
Target Milestone: mozilla25 → ---
Comment 39•12 years ago
|
||
I don't get how this can cause the OSX asserts.
Comment 40•12 years ago
|
||
Pushing to try again from inbound rather than birch.
Comment 41•12 years ago
|
||
Oops - here's the link
https://tbpl.mozilla.org/?tree=Try&rev=6d37160d9abf
Comment 42•12 years ago
|
||
New build fixes the scrolling on Windows. Seeing all the problems there are under mobile why can't this fix make it's way to Windows? My understanding of the update process is woefully lacking.
This should now be fixed by the backout in bug 873378 comment 37 (with followup in bug 898713).
Comment 44•12 years ago
|
||
I can confirm that this is fixed in today's nightly
Comment 45•12 years ago
|
||
See Comment 10. I think the underlying issue is still there. i.e. That we don't draw inactive plugin layers correctly with Retina scaling. This is 'fixed' because in nearly all the cases plugin layers are active but may not always be.
Assignee | ||
Comment 46•12 years ago
|
||
BenWa, we can certainly morph this bug to cover the comment 10, if you think that is better than opening another bug.
Flags: needinfo?(bgirard)
Updated•12 years ago
|
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•