Closed Bug 1227544 Opened 4 years ago Closed 4 years ago

Scaling on 720p devices is broken

Categories

(Core Graveyard :: Widget: Gonk, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: afarden, Assigned: afarden)

Details

Attachments

(2 files, 2 obsolete files)

Attached image 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.
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.
Attached patch Gecko PRSplinter Review
:timdream you changed this to 300dpi, was there a specific reason? Are you happy that we change it back to 280dpi?
Flags: needinfo?(timdream)
Attachment #8691604 - Flags: review?(mwu)
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.
Flags: needinfo?(timdream)
> 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.
Attached patch Gecko PR (obsolete) — Splinter Review
Attachment #8691604 - Attachment is obsolete: true
Attachment #8691604 - Flags: review?(mwu)
Attachment #8692887 - Flags: review?(mwu)
Attachment #8692887 - Flags: review?(mwu) → review+
The patch reverted bug 961505? I guess we should keep it too?
Flags: needinfo?(mwu)
Flags: needinfo?(afarden)
It make sense to amend the comment with an explanation on bug 961505 too (i.e. what actually happen beyond xhdpi).
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.
Flags: needinfo?(mwu)
Attachment #8692887 - Flags: review+
Attached patch Gonk PR (obsolete) — Splinter Review
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!)
Attachment #8692887 - Attachment is obsolete: true
Flags: needinfo?(afarden)
Attachment #8693196 - Flags: review?(timdream)
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.
Attachment #8693196 - Flags: review?(timdream) → review+
Component: Gaia::System → Widget: Gonk
Product: Firefox OS → Core
Assignee: nobody → afarden
Status: NEW → ASSIGNED
(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.
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!
Attachment #8691604 - Attachment is obsolete: false
Attachment #8693196 - Attachment is obsolete: true
Attachment #8691604 - Flags: review?(timdream)
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!
Attachment #8691604 - Flags: review?(timdream) → review+
(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;|
(s/40dpi increase/80dpi increase/ on the two comment above)
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/10e00e356576
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.