Last Comment Bug 829284 - Unity plugin doesn't display in HiDPI mode
: Unity plugin doesn't display in HiDPI mode
Status: RESOLVED FIXED
: qawanted
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla21
Assigned To: Steven Michaud [:smichaud] (Retired)
: juan becerra [:juanb]
: Benjamin Smedberg [:bsmedberg]
Mentors:
http://files.unity3d.com/webplayer_te...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-10 14:03 PST by Steven Michaud [:smichaud] (Retired)
Modified: 2014-01-10 10:40 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
20+


Attachments
Provisional fix (15.13 KB, patch)
2013-01-17 10:36 PST, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Splinter Review
Fix rev2 (20.34 KB, patch)
2013-01-23 13:20 PST, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Splinter Review
Fix rev3 (20.42 KB, patch)
2013-01-23 14:05 PST, Steven Michaud [:smichaud] (Retired)
b56girard: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Steven Michaud [:smichaud] (Retired) 2013-01-10 14:03:44 PST
At least with the following testcase, the Unity plugin doesn't display in HiDPI mode (on a Retina display with support for HiDPI mode turned on, as it is by default):

http://files.unity3d.com/webplayer_test/input3x/InputTest3x.html

The display is just blank.

One workaround is to turn off HiDPI mode (by going to about:config and setting gfx.hidpi.enabled to -1).
Comment 1 Steven Michaud [:smichaud] (Retired) 2013-01-10 14:07:25 PST
This bug starts happening with the 2012-10-03 mozilla-central nightly, which gives the following regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=85dd8e346102&tochange=635fcc11d2b1

Presumably the bug was caused or triggered by my patch for bug 785667.
Comment 2 Steven Michaud [:smichaud] (Retired) 2013-01-10 14:14:22 PST
This bug's diagnosis is complicated by the fact that it's partially hidden by a much more serious bug -- bug 828954.

That bug happens in the FF 18 release.  It started happening with the 2012-12-18 mozilla-central nightly (caused by a mistake in my patch for bug 804606), and was (indirectly) fixed by the patch for bug 825620 (which first appeared in the 2013-01-05 mozilla-central nightly).
Comment 3 Martin Best (:mbest) 2013-01-15 11:06:49 PST
Will this be fixed in 18.0.1?
Comment 4 Benjamin Smedberg [:bsmedberg] 2013-01-15 11:13:45 PST
No, this will not be fixed in 18.0.1
Comment 5 Steven Michaud [:smichaud] (Retired) 2013-01-15 12:51:16 PST
I've got a hackish fix for this, but am still looking for a better alternative.

So yes, it's probably best not to try to get this into FF 18.0.1.

We should probably relnote that the Unity plugin won't work properly in FF 18.0.1 on a Retina display unless HiDPI mode is turned off.
Comment 6 Martin Best (:mbest) 2013-01-15 19:56:30 PST
Ok, thanks for the info, I'd like to see if we can get a solution into 19, if at all possible.  Let's find the solution first though and then we can see what the release drivers think about the risk.
Comment 7 Steven Michaud [:smichaud] (Retired) 2013-01-17 10:36:49 PST
Created attachment 703396 [details] [diff] [review]
Provisional fix

Here's a fix for this bug.  It's not the "hackish fix" I mentioned in comment #5 (which I would have used a quirk to limit to the Unity plugin), but something more comprehensive.

And here's a tryserver build made with it:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-d63fe54f11e3/try-macosx64/firefox-21.0a1.en-US.mac.dmg

This build works fine in what testing I've done with it.  But it causes a number of test failures, which I need to investigate (and either change the patch or the tests).  Nonetheless I'd like people to test it, and report there results here.

Turns out the reason the Unity plugin fails to paint is that it draws the wrong conclusions about how big the plugin is in HiDPI mode, and where it should paint in the CALayer it passes to the browser.  The Unity plugin actually ignores the NPWindow size information passed to the plugin in calls to NPP_SetWindow() (which is correct).  Instead it gets this information by calling -[CALayer frame] on the CALayer it passes to the browser after the browser has set its size in nsCARenderer::SetBounds() (in gfx/2d/QuartzSupport.mm):

http://hg.mozilla.org/mozilla-central/annotate/712eca11a04e/gfx/2d/QuartzSupport.mm#l697

Thanks to the hacks I've been using here to support HiDPI mode, the "frame" of the plugin's CALayer is twice the "true" size (vertically and horizontally) of the plugin's NPWindow.

I spent a few days banging my head against this problem, trying to come up with a workable alternative.  Yesterday I came up with something that seems obvious in retrospect -- instead of hacking the plugin's CALayer directly to support HiDPI mode, we should create our own "wrapper" CALayer, make the plugin's CALayer a sublayer of that, and only hack "our" CALayer.  This allows the plugin's CALayer's "frame" to be "correct" -- the same size as the NPWindow information we pass via NPP_SetWindow(), and the same size as the plugin CALayer's "bounds".
Comment 8 Noah Richards 2013-01-17 11:17:08 PST
I can confirm that the tryserver build in comment 7 fixes the rendering issue I mentioned at the end of bug 821304.
Comment 9 Steven Michaud [:smichaud] (Retired) 2013-01-17 11:21:01 PST
(In reply to comment #8)

Interesting.

Here's another tryserver build to try:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-b16928116598/try-macosx64/firefox-21.0a1.en-US.mac.dmg

I expect it *won't* fix your rendering issue.  But please let us know whether it does or doesn't.
Comment 10 Noah Richards 2013-01-17 11:28:48 PST
Correct, that second tryserver build does *not* fix the rendering issue.
Comment 11 Steven Michaud [:smichaud] (Retired) 2013-01-17 11:37:30 PST
(In reply to comment #10)

Thanks very much for the info.

That shows that the rev1 version of my patch (from comment #7) is better than the first version (to which the tryserver build from comment #9 corresponds).  This is good to know.  I'll explain more fully later.
Comment 12 Steven Michaud [:smichaud] (Retired) 2013-01-17 15:26:06 PST
For the record (and my own future reference), here's a log of the test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18882745&tree=Try&full=1

They're real enough.  But I'm still scratching my head trying to figure out why they happen.
Comment 13 Steven Michaud [:smichaud] (Retired) 2013-01-17 15:48:32 PST
Other work is piling up, which I really need to get to soon.  So I'm going to put this bug aside for a few days, until sometime next week.
Comment 14 Steven Michaud [:smichaud] (Retired) 2013-01-23 13:20:01 PST
Created attachment 705523 [details] [diff] [review]
Fix rev2

Here's a revised fix that fixes the test failures.

And here's a tryserver build made with it:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-642fbd3603cf/try-macosx64/firefox-21.0a1.en-US.mac.dmg

Please test with it!

Not all the tryserver tests are finished.  But so far the previous failures haven't been repeated, and there've been no new (non-spurious) test failures.

Turns out the CGBridgeLayer objects we use to implement the CoreGraphics drawing model (while running OOP) need special treatment if they're to work properly as sublayers of "wrapper CALayers" that can change size (in nsCARenderer::SetBounds()).  I got the idea for this by looking at the source code for the WebKit's test plugin (in PluginObjectMac.mm).
Comment 15 Steven Michaud [:smichaud] (Retired) 2013-01-23 13:38:59 PST
(Following up comment #11 and comment #14)

My last two patch revisions stop trying to make any changes to a plugin's CALayer's contentsScale property at nsCARenderer::SetBounds().

While creating a patch for this bug, I discovered that changing a superlayer's contentsScale doesn't change that property in any of its sublayers.  I also tested with several different kinds of plugin (including Flash, Silverlight, Shockwave and Unity), and found that setting contentsScale on some or all of their CALayer object hierarchies never seemed to help -- the plugins never displayed at a higher resolution.

The only place I've found where setting contentsScale *does* help (when compiling with the 10.7 SDK or up) is with our CGBridgeLayer objects.  So now I set those objects' contentsScale (when appropriate) when initializing them.

And Noah Richards seems to have found a case where setting a plugin's CALayer's contentsScale actually causes trouble (aside from the OS-level weirdness I'd already found and worked around at bug 821304).
Comment 16 Steven Michaud [:smichaud] (Retired) 2013-01-23 14:05:06 PST
Created attachment 705561 [details] [diff] [review]
Fix rev3

This revision fixes a small problem:  Rev2 would have had problems on OS X 10.6 when built with the 10.7 SDK.
Comment 17 Steven Michaud [:smichaud] (Retired) 2013-01-23 14:21:27 PST
Comment on attachment 705561 [details] [diff] [review]
Fix rev3

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9624c7fca53f
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-01-24 09:49:24 PST
https://hg.mozilla.org/mozilla-central/rev/9624c7fca53f
Comment 19 Steven Michaud [:smichaud] (Retired) 2013-01-25 12:58:23 PST
This bug is now fixed in current mozilla-central nightlies (starting with today's).  Please test them out!
Comment 20 Martin Best (:mbest) 2013-02-01 10:50:03 PST
I have Unity Verify and it looks good, can we get this into Aurora?
Comment 21 Steven Michaud [:smichaud] (Retired) 2013-02-01 11:17:02 PST
Comment on attachment 705561 [details] [diff] [review]
Fix rev3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 785667
User impact if declined: Unity plugin broken in HiDPI mode
Testing completed (on m-c, etc.): About a week of testing on m-c
Risk to taking this patch (and alternatives if risky): low to moderate risk
String or UUID changes made by this patch: none

Inherently this patch has moderate risk -- it makes a fundamental (though small) change to how all plugins are handled in HiDPI mode.  But because the patch has already been on mozilla-central for about a week, the risk is low that it will cause regressions in any of the major plugins (like Flash, Silverlight, Quicktime and Java).

Users of other plugins do little if any testing of mozilla-central nightlies.  But they're more likely to test on the aurora and beta branches.  So the earlier this patch gets on to the aurora and beta branches, the less likely it'll cause regressions in a release.
Comment 22 Alex Keybl [:akeybl] 2013-02-01 15:38:41 PST
(In reply to Steven Michaud from comment #21)
> Users of other plugins do little if any testing of mozilla-central
> nightlies.  But they're more likely to test on the aurora and beta branches.
> So the earlier this patch gets on to the aurora and beta branches, the less
> likely it'll cause regressions in a release.

Agreed - let's land sooner rather than later.
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-04 16:05:14 PST
Comment on attachment 705561 [details] [diff] [review]
Fix rev3

Approving for uplift, will look for qa verification after this has landed.
Comment 24 Steven Michaud [:smichaud] (Retired) 2013-02-04 17:04:18 PST
Comment on attachment 705561 [details] [diff] [review]
Fix rev3

Landed on mozilla-aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/83ecaa8b2534
Comment 25 Tracy Walker [:tracy] 2014-01-10 10:40:14 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.