829284 - Unity plugin doesn't display in HiDPI mode

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

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)]

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)]

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)]

Will this be fixed in 18.0.1?

Comment 4

[Benjamin Smedberg]

No, this will not be fixed in 18.0.1

Comment 5

[Steven Michaud [:smichaud] (Retired)]

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)]

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)]

Attachment: 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/[e-mail]-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]

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)]

(In reply to comment #8)

Interesting.

Here's another tryserver build to try:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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]

Correct, that second tryserver build does *not* fix the rendering issue.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

(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)]

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)]

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)]

Attachment: 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/[e-mail]-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)]

(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)]

Attachment: 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)]

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]]

https://hg.mozilla.org/mozilla-central/rev/9624c7fca53f

Comment 19

[Steven Michaud [:smichaud] (Retired)]

This bug is now fixed in current mozilla-central nightlies (starting with today's).  Please test them out!

Comment 20

[Martin Best (:mbest)]

I have Unity Verify and it looks good, can we get this into Aurora?

Comment 21

[Steven Michaud [:smichaud] (Retired)]

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]]

(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]

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)]

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]]

mass remove verifyme requests greater than 4 months old