Closed Bug 846566 Opened 10 years ago Closed 6 years ago

[adbe 3490340] HiDPI on Retina display macbook does not work on Fullscreen Flash

Categories

(Firefox :: General, defect)

x86_64
macOS
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox29 --- disabled
firefox30 --- disabled
firefox31 --- disabled
firefox52 --- disabled
firefox53 --- verified

People

(Reporter: smadayag, Assigned: xidorn)

References

Details

Attachments

(1 file, 1 obsolete file)

Method:
1.) Goto any youtube video (make sure you are not using html 5 by going to youtube.com\html5 and opt out at the bottom of the page)
2.) Enter fullscreen and see that the text is very pixelated for the controls.

Result:
Notice that the text is very pixelated, and ContentsScaleFactor changes to 1, compare this to Safari and ConstentsScaleFactor stays at 2, and the text is not pixelated

Expected:
Should use HiDPI in fullscreen
Is this a Firefox bug or a Flash bug?
Attached patch v1 (obsolete) — Splinter Review
All those times I used Safari to watch YouTube videos in HiDPI fullscreen... and the fix was so simple all along.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #8365570 - Flags: review?(smichaud)
Comment on attachment 8365570 [details] [diff] [review]
v1

Odd that this works.  But if it does I'm all for it.  It's highly unlikely that this will do any harm.

I wonder if this makes a difference for other plugins?
Attachment #8365570 - Flags: review?(smichaud) → review+
(In reply to Steven Michaud from comment #3)
> Comment on attachment 8365570 [details] [diff] [review]
> v1
> 
> Odd that this works.

Yeah, I have no idea why Apple chose to use NSPrincipalClass as a HiDPI capabilitiy differentiator. Really unintuitive.

> I wonder if this makes a difference for other plugins?

I'd expect the same as for flash: working HiDPI. :)
https://hg.mozilla.org/mozilla-central/rev/fd0d0f6002dc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Keywords: verifyme
Juan, this bug seems to need verification, but we don't have the necessary hw here. Can someone on your side try to help here?
Verified this on latest Aurora and Nightly using MB Pro 13" with a Retina display and Flash v12 and v13(beta). Player controls' text looks sharp in fullscreen mode.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 988156
Backed out in bug 988156 for 29.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Attached patch patchSplinter Review
According to my test, this patch can fix this bug, but I don't know whether it could cause bug 988156 again because I cannot visit Netflix to test. I hope someone could check that.
Attachment #8365570 - Attachment is obsolete: true
Attachment #8492813 - Flags: review?(smichaud)
Comment on attachment 8492813 [details] [diff] [review]
patch

I'll need to do more than just review this -- I'll also need to test it.

Furthermore, a big pile of [something] has just been dumped onto my plate :-(

So I probably won't be able to get to this before next week, at the earliest.

Sorry for the delay.
(In reply to Steven Michaud from comment #11)
> Comment on attachment 8492813 [details] [diff] [review]
> patch
> 
> I'll need to do more than just review this -- I'll also need to test it.
> 
> Furthermore, a big pile of [something] has just been dumped onto my plate :-(
> 
> So I probably won't be able to get to this before next week, at the earliest.
> 
> Sorry for the delay.

No worry. I'll wait. But if you think there is someone else can review and test this patch, you can also redirect the review request to that person. Thanks.
Assignee: mstange → nobody
Xidorn, I'm still horribly busy, so I'll have to put this off again.  I hope next week will be better.
I don't know how to apply the patch/build Firefox, but I just manually added the key to the Info.plist in "Applications ▸ Firefox.app ▸ Contents ▸ MacOS ▸ plugin-container.app ▸ Contents" and restarted Firefox.

It fixes Flash/Youtube pixelation, but it *also* causes the black fullscreen Netflix bug 988156 on my system (macbook pro retina).

Netflix doesn't seem to run in HiDPI at all, in the browser it's clearly scaled (anti-aliased), and in fullscreen it's scaled (aliased). 

I'm running Firefox beta 33.0, Silverlight 5.1.20913.0, and Flash 15.0.0.152.

Hope this helps.
(In reply to Marcello Bastéa-Forte from comment #14)
Updated to Firefox beta 34.0, Silverlight 5.1.30514.0, and Flash 15.0.0.189 with the same results.
So it means that if we want to fix this problem, we have to use different config for different plugins. I wonder if there is any way to do so other than using two app bundle...
Comment on attachment 8492813 [details] [diff] [review]
patch

(In reply to comment #16)

I think it's better to leave this unfixed until we really know what the cause is -- practically speaking, until we have a good report of our bug (if it even is our bug), or until someone (besides me) has the time to reverse engineer what's going on.

Needless to say, this could very well be a Flash bug.
Attachment #8492813 - Flags: review?(smichaud) → review-
The bug here is in Silverlight, not in Flash. The fact that apps without an NSPrincipalClass entry in their Info.plist don't get HiDPI rendering is expected and documented.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #16)
> So it means that if we want to fix this problem, we have to use different
> config for different plugins. I wonder if there is any way to do so other
> than using two app bundle...

I couldn't find a different way. So either we need two app bundles, or we find a way to manipulate the plugin process for Silverlight in such a way that fullscreen Silverlight is no longer black when HiDPI is enabled.
If this really is a Silverlight bug, we might be able to fix it by disabling support for HiDPI mode in plugin-container when it hosts the Silverlight plugin.
(Following up comment #19)

Presumably after we've altered plugin-container's Info.plist as per this bug.
(In reply to Steven Michaud from comment #19)
> by disabling support for HiDPI mode in plugin-container

This is the missing piece. If you know how to do this without creating a new app bundle with its own Info.plist, then we're done here.
(In reply to comment #21)

I don't, off the top of my head.

But I'm reasonably familiar with the code that allows plugins to continue working when the browser is in HiDPI mode, since I wrote most of it.  I'll try to find time in the next couple of weeks to look at it.

Of course the browser itself will still be in HiDPI mode.  But there may be some tricks we can play to turn it off just in plugin-container, without messing up HiDPI mode in the main process.
So I've had this bug since I got my 15" Macbook Pro Retina a few months back. I originally used the <key>NSPrincipalClass</key> <string>NSApplication</string> fix, but after a recent Firefox beta update, I couldn't get it to work again. I close Firefox, add the string where it belongs, and re open Firefox, and still have the pixilated full screen Youtube. I tried the new fix here too, to no avail. What can I possibly be doing wrong. I tried to a clean reinstall of Firefox (beta and non beta) and flash, still nothing. Netflix works fullscreen now, so it seems that somehow Firefox is ignoring the new string. 

Any idea what I'm doing wrong, or how to get this fix to work.
Ahh, after hours I figured out what was wrong. OS X was caching the info.plist file. I had to duplicate Firefox, and delete the original, this made OS X re-cache the file. Netflix bug is back now however.
As long as the patch for bug 1118615 is in the tree, we're probably blocked from fixing this bug.
Depends on: 1118615
Is Netflix still using Silverlight? If no, I guess we should try to fix this bug now?

I'm not quite familiar with websites in western conturies, but AFAICS, there are still bunch of top video websites are still using Flash, especially in East Asia. That includes the largest ones in China (e.g. Youku, Tudou, iQIYI), and the second largest one in Japan, which is Niconico Video.

mstange, any thoughts?

FWIW, it seems to me none of the existing patches works now. Not sure why.
Flags: needinfo?(mstange)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #26)
> Is Netflix still using Silverlight? If no, I guess we should try to fix this
> bug now?

It's still using Silverlight. As soon as it stops doing so, I think we should land this.

> FWIW, it seems to me none of the existing patches works now. Not sure why.

You might have to "touch" the plugin-container .app bundle, and maybe also the Firefox .app bundle. Alternatively, renaming them in the Finder (and renaming them back, with a pause of 1 second) might work.
It's also possible that OS X has changed its heuristics, and that we need both patches (i.e. both NSPrincipalClass and NSHighResolutionCapable) now.
Flags: needinfo?(mstange)
Ah, the patch works with the local build, but doesn't work with my installed .app... Anyway, it works...
Is it possible to build an additional plugin-container just for flash? It seems that the plugin-container isn't very large, and thus we can create one with HiDPI support and run flash in it.
Sounds reasonable to me.
Depends on: npapi-eol
Comment on attachment 8492813 [details] [diff] [review]
patch

Now we've removed support for all other NPAPI plugins. Can we do this now?
Attachment #8492813 - Flags: review- → review?(mstange)
Comment on attachment 8492813 [details] [diff] [review]
patch

Review of attachment 8492813 [details] [diff] [review]:
-----------------------------------------------------------------

I think we can!
Attachment #8492813 - Flags: review?(mstange) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbad08fdc37c
Add HiDPI support for plugin-container. r=mstange
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dbad08fdc37c
Status: REOPENED → RESOLVED
Closed: 9 years ago6 years ago
Resolution: --- → FIXED
Assignee: nobody → xidorn+moz
Target Milestone: Firefox 29 → Firefox 52
This was backed out of 52 (as we need to keep non-flash plugins in 52ESR), moving to 53.
Target Milestone: Firefox 52 → Firefox 53
Xidorn, can you confirm that your HiDPI Flash fix was backed out of 52 and is still fixed 53? I don't see an Aurora backout comment in this bug.
Blocks: 1308761
Flags: needinfo?(xidorn+moz)
I can confirm that this is not fixed on the DevEdition 52.0a2 (2017-01-13), and it is still fixed on mozilla-central 845cc4dea57f (which is a merge commit dated Mon Jan 09 16:34:44 2017 -0800).
Flags: needinfo?(xidorn+moz)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.