Closed Bug 829284 Opened 11 years ago Closed 11 years ago

Unity plugin doesn't display in HiDPI mode

Categories

(Core Graveyard :: Plug-ins, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox20+ fixed, relnote-firefox 20+)

RESOLVED FIXED
mozilla21
Tracking Status
firefox20 + fixed
relnote-firefox --- 20+

People

(Reporter: smichaud, Assigned: smichaud)

References

()

Details

(Keywords: qawanted)

Attachments

(1 file, 2 obsolete files)

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).
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.
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).
Assignee: nobody → smichaud
Will this be fixed in 18.0.1?
No, this will not be fixed in 18.0.1
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.
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.
Attached patch Provisional fix (obsolete) — Splinter Review
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".
I can confirm that the tryserver build in comment 7 fixes the rendering issue I mentioned at the end of bug 821304.
(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.
Correct, that second tryserver build does *not* fix the rendering issue.
(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.
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.
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.
Attached patch Fix rev2 (obsolete) — Splinter Review
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).
Attachment #703396 - Attachment is obsolete: true
Attachment #705523 - Flags: review?(bgirard)
(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).
Attached patch Fix rev3Splinter Review
This revision fixes a small problem:  Rev2 would have had problems on OS X 10.6 when built with the 10.7 SDK.
Attachment #705523 - Attachment is obsolete: true
Attachment #705523 - Flags: review?(bgirard)
Attachment #705561 - Flags: review?(bgirard)
Attachment #705561 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/9624c7fca53f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
This bug is now fixed in current mozilla-central nightlies (starting with today's).  Please test them out!
I have Unity Verify and it looks good, can we get this into Aurora?
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.
Attachment #705561 - Flags: approval-mozilla-aurora?
(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.
Keywords: qawanted, verifyme
QA Contact: jbecerra
Comment on attachment 705561 [details] [diff] [review]
Fix rev3

Approving for uplift, will look for qa verification after this has landed.
Attachment #705561 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.