820831 - (flash-hidpi) Flash applet px units are not scaled to DPI on Windows

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Greg Edwards]

Attachment: Observe how the controls on the bottom are half their expected size.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/536.26.17 (KHTML, like Gecko) Version/6.0.2 Safari/536.26.17

Steps to reproduce:

If you have DPI scaling enabled in Windows, set the pref, layout.css.devPixelsPerPx to -1.0.
Otherwise, set the pref to a value of 1.5 or more. (You must have a high enough resolution screen or this will cause difficulties.)
Navigate to a website that uses Flash.


Actual results:

Absolutely sized elements are too small.


Expected results:

Absolutely sized elements should scale proportionately to the system DPI setting.

Comment 1

[Benjamin Smedberg]

These are Flash elements not HTML/DOM elements, right? Can we get a minimal testcase?

Comment 2

[Greg Edwards]

Correct. Replication is spotty on Youtube since it's mixed HTML5/Flash depending on the video or presence of ads. This is why I reference Twitch.

I can't prepare a testcase since I don't have Adobe Flash. A simple testcase should contain two objects: one with absolute sizing, the other sized relative to the applet container.

Chances are a fix would have to come from Adobe. IE9 has the same issue.

Comment 3

[Benjamin Smedberg]

I don't think we'll track this in our bugtracker then; feel free to file a bug directly with Adobe.

Comment 4

[Greg Edwards]

https://bugbase.adobe.com/index.cfm?event=bug&id=3536168

Comment 5

[Greg Edwards]

If Adobe doesn't have anything to say by the time Firefox 22 is ready for release, I strongly think we should consider doing something "drastic" like raster scaling Flash applets. This is exactly what Safari did before Adobe added hiDPI support to Mac after all.

Comment 6

[Ryan Rubley]

There are lots of test-cases out there. This is clearly a Firefox bug because all of these examples work just fine in the Chrome 39 and IE 11. On my 2880x1620 Sony Vaio Flip 15 with Windows 8.1, which has 200% scaling by default, here is a list of sites and problems that anybody can easily verify:

* All videos on CNN take up the top left quarter of the area they are supposed to. See http://www.cnn.com/video/ or any video from their home page.

* The playback controls on Youtube videos are tiny and very hard to use, but the videos do take up the entire area properly.

* The text and buttons on charts on Google Finance such as https://www.google.com/finance?q=USDEUR are tiny and hard to use

Again, all of these examples work in Chrome and IE and the Flash elements are scaled properly, on the same computer, with the same version of Flash, but Firefox 34 displays everything wrong for some reason.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Just because things are correct in Chrome or IE doesnt mean that its a Firefox bug.
Both of these browsers use completely different versions of the Flash plugin (ActiveX and Pepper based).

Comment 8

[Ryan Rubley]

That still sounds like a Firefox bug to me. You're just redirecting where the bug is inside of Firefox from one spot to another. The result is the same - terrible user experience in Firefox and good user experience in all other browsers. It sounds like you want to rename the bug to "Firefox doesn't support newer plugin APIs and as a result Flash on HiDPI devices is terribly unusable"

Comment 9

[Georg Fritzsche [:gfritzsche]]

FWIW, i just cross-checked with Opera and it's broken there too, which would make this a Flash or site issue.
Newer plugin APIs are not up for discussion here.

Comment 10

[Ryan Rubley]

It's still a Firefox bug if you can do something to work-around it, such as telling Flash to render at 50% of the size the area will really take up, and then just scaling it 200% in Firefox. At least then things would be clickable or semi-readble, even if somewhat blurry. Your answer of "Sorry, don't use Firefox, we don't really care if the only other two major browsers don't have any issues here" doesn't really seem like the best choice, but that is exactly what you are saying.
Perhaps if your computer or laptop was had a newer hidpi screen and you couldn't reasonably use 96dpi like I can't, you'd understand how important and horrible this issue really is. As much as we'd all really like Flash to go away, the fact is lots of big sites like CNN and Google Finance use it for important things that are just useless in Firefox right now with no work-around other than Chrome or IE.

Comment 11

[Greg Edwards]

That would debatably be a downgrade so is hard to justify. (But, fwiw, Safari does exactly this for non-DPI-aware plugins, like Mac Silverlight.)

Comment 12

[Ryan Rubley]

I'm not sure how it's a downgrade. I'd much rather prefer resampled text that is at least big enough to be readable over what amounts to an effective 4-point font size. For video like on CNN, you'd probably never even notice.

Comment 13

[Greg Edwards]

Only some applets are affected. (Stage scaling must be turned off to trigger the bug.) For the others, it would be a downgrade.

Comment 14

[Ryan Rubley]

I assume the only way it would make sense would be to whitelist known-non-DPI-aware plugins and someday cap the version number of the plugin when/if the plugin becomes DPI-aware. I don't think I've come across a flash applet that looks correct at 192dpi. Yes, some will scale everything to the full area, but the text is still always tiny because it assumes 96dpi.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Reopening since it is a firefox issue.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Picture of issue before, comparing firefox 41 nightly and chrome 43

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Picture of issue after, comparing patched firefox 41 nightly and chrome 43

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Turn on ContentScaleFactor calculation on Windows

We were only calculating ContentScaleFactor on OS X. We can easily calculate it for windows too, all the GDI work was already done, we just needed to fix the platform ifdefs.

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8611615 [details] [diff] [review]
Patch 1 (v1) - Turn on ContentScaleFactor calculation on Windows

Asking original implementer of bug 785667 for review since this is pretty much just adding a platform def around that patch.

Comment 20

[Ryan Rubley]

Can you compare the before/after at http://ark42.com/flashscale/ ?

Greg mentioned that it may be a downgrade in cases where stage scaling is on (the default) because the results may be blurry with this type of patch.

Personally, it's worth it to have the flash with stage scaling off be the correct size though.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8611615 [details] [diff] [review]
Patch 1 (v1) - Turn on ContentScaleFactor calculation on Windows

This looks fine to me, with the following quibble:

   // On Mac, device pixels need to be translated to (and from) "display pixels"
   // for plugins. On other platforms, plugin coordinates are always in device
   // pixels.

You'll also need to change this comment.

I can't speak to the issue of how to implement this on Windows, or whether it will work properly there.

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, fix ended up being way too good to be true, went to check the ark42 flashscale tests and am seeing all sorts of weirdness around this now. Will continue debugging.

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

BTW, for anyone that wants to play along, here's a try build of the current patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=592cd98c834d

The visual scaling worked on a couple of sites, but on the ark42 site, it makes the stage scaling and regular flash the same small size? Not sure what's up there. Also, it completely breaks the cursor position mapping, so while things look vaguely more correct, if blurry, they don't have any ability to detect where input is happening.

Comment 24

[Ryan Rubley]

Weird. I made that page as a very minimal test case for this exact bug. If you look at the HTML source, it's nothing but two <embed>'s for the Flash and the CSS only sets display: block;. The only difference is he second one adds scale="noscale" to the embed tag. The embed is 100x100 pixels and the SWF http://ark42.com/flashscale/ScaleTest.swf also is 100x100 pixels. On my 192 DPI Windows 8.1 laptop with Firefox 38, the default SWF is scaled up to the right size because the default for Flash is to scale up the stage (which will actually be 200x200 screen pixels at 192dpi).

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Ryan Rubley from comment #24)
> Weird. I made that page as a very minimal test case for this exact bug. If
> you look at the HTML source, it's nothing but two <embed>'s for the Flash
> and the CSS only sets display: block;. The only difference is he second one
> adds scale="noscale" to the embed tag. The embed is 100x100 pixels and the
> SWF http://ark42.com/flashscale/ScaleTest.swf also is 100x100 pixels. On my
> 192 DPI Windows 8.1 laptop with Firefox 38, the default SWF is scaled up to
> the right size because the default for Flash is to scale up the stage (which
> will actually be 200x200 screen pixels at 192dpi).

Yeah, since I started using the ContentsScaleFactor, there's some place where it's getting set to an incorrect value, looks to be the default 1.0 instead of whatever multiplier of 96dpi it should be at. It's not just your test that fails, it looks like I've broken most flash /except/ the google finance page, heh. I've at least fixed the position issue, just gotta figure out where to stick that multiplier and hopefully this will be ready for another revision of the patch.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

FYI, the Airmo video player shows this issue.

Comment 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - WIP: Turn on ContentScaneFactor for NPAPI on Windows

Turns out this was not nearly as simple as the first patch made it out to be. This is a WIP update while I continue work on it.

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - WIP: Mochitests for NPAPI ContentScaleFactor on Windows

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - WIP: Turn on ContentScaleFactor for NPAPI on Windows

This patch basically just replaces all of the ifdefs for ContentScaleFactor in OS X to be for both windows and OS X. This allows us to at least relay what we currently have as the ContentScaleFactor value to the plugin.

However, there's still things missing. This patch currently causes Bug 1231407 when applied, which I'm guessing means there's probably widget work that needs to be done to make all of this work correctly. Also, redrawing after events like changing the Zoom value (control-+/-) are pretty broken.

Jim, could you possibly take a look at this and let me know what else I might need to change/fix? I'm happy to do it myself if it won't take too much time to explain, just not super familiar with the Windows widget portion of the code.

Comment 30

[Greg Edwards]

Before you get too far, I'd like to point out that Adobe claims to have fixed the underlying bug.

https://bugbase.adobe.com/index.cfm?event=bug&id=3536168

Comment 31

[Jim Mathies [:jimm]]

Kyle, sounds like you might want to take that beta build for a spin. I can dig into this if things are still broken.

http://labsdownload.adobe.com/pub/labs/flashruntimes/shared/air21_flashplayer21_releasenotes.pdf

Comment 32

[Ryan Rubley]

According to that PDF, it should be fixed in Firefox 45 + Flash 21. I'm using Firefox 45 with the Flash 21 beta and I see no changes on Google Finance or my test at http://ark42.com/flashscale/ either. Am I missing something?

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Um. I... have no idea why that PDF says that, as this patch obviously hasn't landed, and without this patch, that's not going to work. 

I'll check in with Adobe.

Comment 34

[Ryan Rubley]

The PDF says "It will be available from FP version 21 and is currently available on Firefox Nightly 45.0a1, the official Firefox version supporting the feature has yet to be announced."
I noticed that the final FP 21 was out, so I tried that, but no changes still. Are we waiting on a patch to Firefox that allows it to use this new fix in FP 21, or a patch to FP still? It's not clear which version number is wrong in that PDF (45 or 21).

Comment 35

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Well, right now, Firefox never tells Flash what it thinks the current DPI setting is, and that's where flash gets its information from. That's what this bug is here to fix. I need to test the 21 player with this patch applied and see what happens.

Comment 36

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, I finally got FP 21 and tested it with a nightly on the patch included in this bug. It doesn't seem to have the updates claimed. I have some test flash files from adobe that should show the different DPI multiplier, but it just shows 1.0 no matter what windows is set to, meaning FP doesn't include the code to deal with HiDPI on windows. I'll email adobe and see what's up.

Comment 37

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok actually I should've read the PDF first.

Browser Zoom is not HiDPI/Content Scaling. Browser Zoom was bug 1171182, which just landed today. Adobe did jump the gun on the version there, but that feature has nothing to do with this bug.

Not only that, the bug posted earlier is still listed as OPEN, not fixed. So there's been no changes there.

So, the problems still stand. NPAPI hands off the content scaling value to plugins fine, and the mochitest that checks this works. However, two problems remain when this patch is applied:

- In window mode, width and height are half of what is expected. Setting width/height to 100% in the embed tag with DPI multiplier set to 2.0 means the plugin on takes up a quarter of the screen.
- When zooming/unzooming (ctrl-+/-), plugin widget redrawing doesn't work correctly.

Any idea on either of these, Jim?

Comment 38

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 820831 - Turn on ContentScaleFactor calculation on Windows;

Changes ifdefs in NPAPI code for ContentScaleFactor to include windows
alongside OS X as a supporting platform.

Currently has the problem of causing windowed plugins to render at 50%
of requested width/height (so 100% w/h ends up taking up 50% w/h), and
scaling on windowless plugins to happen at 2x (so they render similarly
to having the page zoom at 2.0x)

Review commit: https://reviewboard.mozilla.org/r/58100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58100/

Comment 39

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8760519 [details]
Bug 820831 - Turn on ContentScaleFactor calculation on Windows;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58100/diff/1-2/

Comment 41

[Jim Mathies [:jimm]]

Sorry I have not had a chance to look at this at all but it is long over due. e10s is winding down so my free time is starting to open up. I will find some time in london next week to go through this.

Comment 42

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Jim Mathies [:jimm] from comment #41)
> Sorry I have not had a chance to look at this at all but it is long over
> due. e10s is winding down so my free time is starting to open up. I will
> find some time in london next week to go through this.

Great, thanks! Unfortunately I won't be around London but I can talk via here or email.

Comment 44

[u554753]

Removing test-case wanted flag due to previous discussion with Ryan on the issue. If you feel the tag is pertinent, please re-add as needed. Thanks.

Comment 45

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Any chance you'll be able to look at this soon Jim?

Comment 46

[Jim Mathies [:jimm]]

Hey Kyle,

Finally getting around to looking at this. I traced the window size values through the code on their way to the plugin and found that we shrink the window dims we pass here [1] based on the content scale factor. For windowed plugins this gets passed to the plugin via NPP_SetWindow calls. [2]

What do we expect flash to do in response to this? We're basically giving flash window dims that don't match the size of the viewport we define for the applet in content.

[1] http://searchfox.org/mozilla-central/source/layout/generic/nsPluginFrame.cpp#649
[2] https://developer.mozilla.org/en-US/Add-ons/Plugins/Reference/NPP_SetWindow

Comment 47

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Huh, I have no idea why we do that. It looks like we do this for all platforms, so I'm guessing OS X handles this differently than windows, since that's the only platform we had this capability on before. I guess we could wrap the calculation in platform defines so it only happens on OS X?

Comment 48

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, yeah, following up, it looks like this is an OS X case we wrapped in nsPluginInstanceOwner [1], but left open in nsPluginFrame because we figured we'd always get back 1 if we're not on OS X. So we should be able to fix this in nsPluginFrame and I think things should work.

 [1] http://searchfox.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp#3709

Comment 49

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Changing the scaling in nsPluginFrame to only happen on OS X seems to have fixed the scaling issues on windows. Running a try build now, then will post patches here and ship a test build to adobe.

Comment 50

[Jim Mathies [:jimm]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #49)
> Changing the scaling in nsPluginFrame to only happen on OS X seems to have
> fixed the scaling issues on windows. Running a try build now, then will post
> patches here and ship a test build to adobe.

awesome, lets hope this is the only issue. Sorry again for taking so long to get back to this.

Comment 51

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 820831 - Mochitest changes for contentsScaleFactor on windows;

Review commit: https://reviewboard.mozilla.org/r/77326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77326/

Comment 52

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8760519 [details]
Bug 820831 - Turn on ContentScaleFactor calculation on Windows;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58100/diff/2-3/

Comment 53

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Try is looking good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=87a7f0380d41&selectedJob=27107288

Posted new patches for review, and notified adobe of new build.

Comment 54

[Jim Mathies [:jimm]]

Attachment: patch

mozreview is giving me http 500 server errors this morning.

Comment 55

[Jim Mathies [:jimm]]

Comment on attachment 8789352 [details] [diff] [review]
patch

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

You have a few lines in the PluginInstance's that have tab chars in them, please clean those up.

Not much left to do here, but I think we want those handle event updates.

I'm also building up a local copy to do some manual testing with various flash test cases I have.

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +1567,5 @@
>  #endif
>      NPRemoteEvent npremoteevent;
>      npremoteevent.event = *npevent;
>  #if defined(XP_MACOSX)
> +    // Not needed on windows?

I think we want this and we want to update mContentScaleFactor in PluginInstanceChild [1] in response.  Windows 8.1 and up support per dpi monitors and I'm pretty sure we have support baked into core for this as well.

[1] http://searchfox.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceChild.cpp#870

Comment 56

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Jim Mathies [:jimm] from comment #55)
> You have a few lines in the PluginInstance's that have tab chars in them,
> please clean those up.
> 
> Not much left to do here, but I think we want those handle event updates.
> 
> I'm also building up a local copy to do some manual testing with various
> flash test cases I have.

Ok, I'll take care of the event stuff today. Got positive confirmation from Adobe on the other outstanding issues being fixed, so once these patches are r+'d, they're probably ok to land and we can fix anything else in followup.

Comment 57

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8760519 [details]
Bug 820831 - Turn on ContentScaleFactor calculation on Windows;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58100/diff/3-4/

Comment 58

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8789088 [details]
Bug 820831 - Mochitest changes for contentsScaleFactor on windows;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77326/diff/1-2/

Comment 59

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Oops, forgot to update review status on the mochitest patch. The first one is all that needs review, there were no changes to tests.

Comment 60

[Jim Mathies [:jimm]]

Comment on attachment 8760519 [details]
Bug 820831 - Turn on ContentScaleFactor calculation on Windows;

https://reviewboard.mozilla.org/r/58100/#review76252

Comment 61

[Jim Mathies [:jimm]]

Comment on attachment 8789088 [details]
Bug 820831 - Mochitest changes for contentsScaleFactor on windows;

https://reviewboard.mozilla.org/r/77326/#review76254

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a569f8c9cf10
Turn on ContentScaleFactor calculation on Windows; r=jimm
https://hg.mozilla.org/integration/autoland/rev/59b665a08aab
Mochitest changes for contentsScaleFactor on windows; r=jimm

Comment 63

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a569f8c9cf10
https://hg.mozilla.org/mozilla-central/rev/59b665a08aab

Comment 64

[Stefan [:StefanG_QA]]

Attachment: faulty flash player

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

I have tested the issue on latest FF release (50.1.0), latest Nightly build (20161214030231) and managed to reproduce it. I have forced YouTube to use Flash to play the videos. 

The issues that I experienced are only on YouTube:

Tiny button on Flash video player (Release only)
Right click menu causes the video to stop, but the audio is still playing in the background. 
Right click menu is not scaled properly with the rest of the webpage, text is too small. 

Other than that everything looks fine, like Google Finance, CNN, Twitch, etc.

Comment 65

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, filing a followup on flash youtube. Not super high priority since we're already rewriting most flash to html5, but still worrisome that any flash still shows up with wrong scaling settings.