1227544 - Scaling on 720p devices is broken

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Adam Farden [:adfad666]]

Attachment: DSC_0041.JPG

Here we have five devices of different screen size and resolutions.

1. 4.5 inch (56x99 mm) 854x480 px
2. 5.2 inch (65x116 mm) 1280x720 px
3. 6.0 inch (75x134 mm) 1280x720 px
4. 5.2 inch (64x114 mm) 1920x1080 px
5. 5.5 inch (68x121 mm) 2560x1440 px

The device scaling ultimately depends on setting the right physical dimensions in the kernel, or you end up with a comically small UI. All of these devices have the correct configuration.

However if you look at device 2 and 3 the scaling is vastly smaller compared to the others. If you're not sure, just look at the location of NFC. on device 4 and 5 it is the last visible entry, but on 2 and 3 it is somewhere in the middle! (On device 1 it is off the screen.)

This is only affecting my 1280x720 px devices, all other devices are scaling correctly.

Comment 1

[Adam Farden [:adfad666]]

The logic for device scaling is here: https://github.com/mozilla/gecko-dev/blob/master/widget/gonk/nsWindow.cpp#L744

We have this nice comment:

>    // The mean pixel density for mdpi devices is 160dpi, 240dpi for hdpi,
>    // and 320dpi for xhdpi, respectively.
>    // We'll take the mid-value between these three numbers as the boundary.

Which gives us boundaries of 200dpi and 280dpi but unfortunately in Bug 884706 this was changed to 300dpi, breaking devices in the 280-300dpi range.

The device I photographed above has an actual density of 287dpi, so we need to change the logic back to 280dpi.

Comment 2

[Adam Farden [:adfad666]]

Attachment: Gecko PR

:timdream you changed this to 300dpi, was there a specific reason? Are you happy that we change it back to 280dpi?

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I re-read bug 884706 and the change was made to ensure 280dpi-300dpi devices are not floor()'d to 1. The comment on boundary was outdated by bug 884706 and I think your patch here can fix the boundary back to what has been said.

I suspect your patch does not really fix the problem described since a 281dpi-299dpi device will be floor()'d to dppx=1. The real patch should be 

if (dpi < 280.0) {
  return 1.5; // hdpi devices.
}
// xhdpi devices and beyond.
if (dpi < 300.0) {
  return 2.0;
}
return floor(dpi / 150.0);

(What's funny is that I made the exact comment in bug 884706 comment 4 mentioning the non-existence of such device.)

I hope this helps.

Comment 4

[Adam Farden [:adfad666]]

> I suspect your patch does not really fix the problem described since a
> 281dpi-299dpi device will be floor()'d to dppx=1. The real patch should be 
> 
> if (dpi < 280.0) {
>   return 1.5; // hdpi devices.
> }
> // xhdpi devices and beyond.
> if (dpi < 300.0) {
>   return 2.0;
> }
> return floor(dpi / 150.0);
> 

Wow, that is very interesting. I hadn't really noticed floor() at the end. However I built with only my simple change and scaling was performed correctly.

Your change should be the correct one, I'll update it.

Comment 5

[Adam Farden [:adfad666]]

Attachment: Gecko PR

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The patch reverted bug 961505? I guess we should keep it too?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It make sense to amend the comment with an explanation on bug 961505 too (i.e. what actually happen beyond xhdpi).

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 8692887 [details] [diff] [review]
Gecko PR

Oh I didn't notice the rounding was adjusted.

Tim, you can review these patches. I think you know/remember the details of this better than I do, and the math should be simple enough.

Comment 9

[Adam Farden [:adfad666]]

Attachment: Gonk PR

Actually this will break scaling on my 560dpi device too. Adding back the + 0.5 in this new patch.

There's still the 'issue' that we can't target 3.0 or 4.0 scaling, but in the photo above, both the 1080p (2.5x) and the 1440p (3.5x) are scaled absolutely correctly. If we come across a device with such an unusual resolution, we can deal with that later.

(Sony Xperia Z5 Premium, with its 4K 806dpi screen will fall in at 5.5x scaling. I'm curious how this will look!)

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8693196 [details] [diff] [review]
Gonk PR

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

Thanks for fixing this! Let's amend this comment [1] with (assuming my English is correct):

[1] https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#747-749


    // The mean pixel density for mdpi devices is 160dpi, 240dpi for hdpi,
    // and 320dpi for xhdpi, respectively.
    // We'll take the mid-value between these three numbers as the boundary
    // between pixel ratio of 1, 1.5, and 2.
    // For beyond 375dpi the pixel ratio values will be an integer calculated
    // by the math below.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Adam Farden [:adfad666] from comment #9)
> There's still the 'issue' that we can't target 3.0 or 4.0 scaling, but in
> the photo above, both the 1080p (2.5x) and the 1440p (3.5x) are scaled
> absolutely correctly. If we come across a device with such an unusual
> resolution, we can deal with that later.
> 
> (Sony Xperia Z5 Premium, with its 4K 806dpi screen will fall in at 5.5x
> scaling. I'm curious how this will look!)

Actually, floor( 806 / 150 + 0.5 ) = 5x. 0.5 was added inside floor() instead of outside.

I actually don't know why bug 961505 was did as such, maybe the reviewer would know.

Comment 12

[Adam Farden [:adfad666]]

OK I think we both got a bit confused along the way :)

Originally the code was anything below 280dpi was 1.5 scaling and anything above was 2.0 scaling.

Bug 884706 added the floor() math to get higher scaling values but it also raised the hdpi boundary to prevent 280-300dpi devices from falling down to 1.0 scaling.

I think you based your review on bug 884706 associated with history of that line because you said:

> I suspect your patch does not really fix the problem described since
> a 280dpi-300dpi device will be floor()'d to dppx=1.

but the floor behaviour was changed in Bug 961505 to add 0.5 before flooring.

After Bug 961505 any device with 280-300dpi will be correctly scaled to 2.0 

> floor(287 / 150.0 + 0.5) == 2.0

So my original one-line patch was actually correct!

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8691604 [details] [diff] [review]
Gecko PR

So, with this patch, the boundaries are

         160dpi (mdpi -- 1x)
                                delta=40
---------200dpi 
                                delta=40
         240dpi (hdpi -- 1.5x)
                                delta=40
---------280dpi
                                delta=40
         320dpi (xhdpi -- 2x)
                                delta=55
---------375dpi
                3x
                                delta=150
---------525dpi (=375+150)
                4x
---------675dpi
                5x

... and so on.

I am tempted to get these values strictly linear, i.e. every 40dpi increase cause a 0.5dppx bump all the way, but I don't think that's the scope of this bug (and I don't really know device porting well enough to tell the display densities of our porting targets are).

Let's land this first and maybe file another bug for improvement if necessary.

Thanks!

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #13)
> I am tempted to get these values strictly linear, i.e. every 40dpi increase
> cause a 0.5dppx bump all the way.

... and this is simply a one-liner: |return floor(dpi * 2 / 160) / 2;|

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(s/40dpi increase/80dpi increase/ on the two comment above)

Comment 16

[Adam Farden [:adfad666]]

That may be relevant to revisit this when we start porting more devices, eg via CyanogenMod, and if we consider a proper tablet variant in the future.

However for now this is working correctly on all 11 devices that I can test on, which includes a good range of sizes, resolutions, and densities.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10e00e356576

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10e00e356576