Closed Bug 449283 Opened 11 years ago Closed 10 years ago

DPI on n810 is incorrect

Categories

(Core :: Graphics, defect)

ARM
Maemo
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
fennec 1.0b4+ ---

People

(Reporter: dougt, Assigned: mfinkle)

References

Details

Attachments

(2 files, 7 obsolete files)

In InitDPS, we try to get the monitor's DPS using pango_cairo_context_get_resolution.  This method, on the device, returns -1 and we set the default DPI to 96.  

The n810 has a DPI of 225.

I am not sure if this is a terrible thing.  it looks like DPI is only used for system fonts?
Product: Core → Core Graveyard
Should not be in core graveyard
Component: GFX: Gtk → General
OS: Mac OS X → Linux (embedded)
Product: Core Graveyard → Fennec
QA Contact: gtk → general
Hardware: x86 → ARM
Version: unspecified → Trunk
tracking-fennec: --- → ?
Component: General → Linux/Maemo
QA Contact: general → maemo-linux
We need to find out what sDPI is getting set to in http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxPlatformGtk.cpp#528 on the n810.

We'd like for that to get set correctly automatically.

Either way, we basically need to ensure that layout.css.devPixelsPerPx is 1 and layout.css.dpi is the correct screen DPI
tracking-fennec: ? → 1.0b3+
Assignee: nobody → mark.finkle
this hardcodes the DPI on the n810 using a preference.  In a follow up patch, we can test the hardware version and do something different.
Assignee: mark.finkle → doug.turner
Attachment #390933 - Flags: review?(mark.finkle)
Attached patch set DPI based on pref for fonts (obsolete) — Splinter Review
Attachment #390934 - Flags: review?(pavlov)
Comment on attachment 390934 [details] [diff] [review]
set DPI based on pref for fonts

* Do you want to use a #ifdef MOZ_PLATFORM_HILDON here?
* You have some branding "dsstore" changes in here too
mfinkle, no, i think that change should be free of ifdefs, and I don't even know what a dsstore is.  that part will not land.
vote for not blocking1.0
(In reply to comment #7)
> vote for not blocking1.0

Ok, this should block
Comment on attachment 390934 [details] [diff] [review]
set DPI based on pref for fonts

This won't work for n810 / n900, which have different DPIs. We can't set a pref at build time.
Attachment #390934 - Flags: review?(pavlov) → review-
Attached patch WIP 1 (obsolete) — Splinter Review
This patch uses a component_version file on the n810/n900 to determine at runtime the device. It then sets a hard coded DPI or falls back to the baseline.

The code _does_ determine the device, but returning the DPI breaks Fennec. The app doesn't even load. Still investigating.
Assignee: doug.turner → mark.finkle
Attachment #390934 - Attachment is obsolete: true
i would rather you have this code somewhere else and have it just set the DPI pref correctly.  Otherwise, you are also going to have to do a similar thing here:

http://mxr.mozilla.org/mozilla-central/source/gfx/src/thebes/nsThebesDeviceContext.cpp#673

Any every new place that tests for this preference.
Attached patch patch (platform) (obsolete) — Splinter Review
This patch uses a file shipped on the Nokia devices to determine the DPI. It caches the value so the file is only read and parsed once.
Attachment #400959 - Attachment is obsolete: true
Attachment #401312 - Flags: review?(roc)
This code badly needs to be refactored so that we call just down into gfxPlatform to get the DPI and the platform-specific code can be hosted there. Are you up for that? It should be pretty quick and easy. If you post a patch for Windows and GTK I can finish off the Mac part.
Attached patch patch 2 (platform) (obsolete) — Splinter Review
This patch reorganizes the DPI code to live in gfxPlatform. It also adds a "ViewingRatio" which is created based on the DiagonalSize of the screen (in millimeters).

The patch doesn't do anything with ViewingRatio yet - I need some direction in that area.

I the patch builds and runs fine on my N900 (linux ARM). I pushed it to the try servers too and will report the results.
Attachment #401312 - Attachment is obsolete: true
Attachment #401384 - Flags: review?(roc)
Attachment #401312 - Flags: review?(roc)
Attached patch patch (fennec) (obsolete) — Splinter Review
WIP patch of Fennec CSS changes needed for the corrected DPI on the N900 (N810)

I have a little more work to do and will post some screenshots too. Obviously depends on the platform patch
Attachment #390933 - Attachment is obsolete: true
Attachment #390933 - Flags: review?(mark.finkle)
Attached patch patch 3 (platform) (obsolete) — Splinter Review
Fixed some Windows breakage and tweaked the ViewingRatio estimates a bit
Attachment #401384 - Attachment is obsolete: true
Attachment #401452 - Flags: review?(roc)
Attachment #401384 - Flags: review?(roc)
+PRInt32 gCachedDPI = -1;
+PRInt32 gCachedDiagonal = -1;

Why do we need these since they're cached in sDPI/sDiagonalSize anyway?

+    static PRInt32 sDPI;
+    static PRInt32 sDiagonalSize;

Can you document what these mean? sDiagonalSize is the size of the (virtual) desktop in mm I guess?

GetViewingRatio needs to be documented to say what it means.

+        // Small handhelds and phones (14")
+        if (sDiagonalSize < 127)

That's 5" not 14". What do those parenthetical numbers mean? They're all off.

+        if (sDiagonalSize < 600)
+            return 1.0;
+
+        // Large screens (In a room ~10ft)
+        if (sDiagonalSize < 1000)
+            return 4.0;

I think lots of people (certainly at Mozilla) have screens more than 23" along the diagonal that they use from less than 10 feet away.

It seems to me that GetViewingRatio as implemented requires a lot of guesswork. Maybe the best thing to do for now is to hardcode the value in a pref for Fennec and not use sDiagonal at all?
(In reply to comment #17)
> +PRInt32 gCachedDPI = -1;
> +PRInt32 gCachedDiagonal = -1;
> 
> Why do we need these since they're cached in sDPI/sDiagonalSize anyway?

We don't - removed

> +    static PRInt32 sDPI;
> +    static PRInt32 sDiagonalSize;
> 
> Can you document what these mean? sDiagonalSize is the size of the (virtual)
> desktop in mm I guess?
> 
> GetViewingRatio needs to be documented to say what it means.

I removed sDiagonalSize and GetViewingRatio from this patch

> +        // Small handhelds and phones (14")
> +        if (sDiagonalSize < 127)
> 
> That's 5" not 14". What do those parenthetical numbers mean? They're all off.

The parenthetical numbers were the viewing distance, not the display size, but this method has been removed.

> It seems to me that GetViewingRatio as implemented requires a lot of guesswork.
> Maybe the best thing to do for now is to hardcode the value in a pref for
> Fennec and not use sDiagonal at all?

In fact, I'd like to take viewing ratio to a new bug and leave this bug focused on DPI: Getting the right DPI on Maemo and moving the code for DPI into gfx platform.

Yes, I chickened out, but I need to land DPI for Maemo quickly :(

Patch coming up
This patch removes the viewing ratio code and fixes other review comments
Attachment #401452 - Attachment is obsolete: true
Attachment #401927 - Flags: review?(roc)
Attachment #401452 - Flags: review?(roc)
tracking-fennec: 1.0b3+ → 1.0b4+
Component: Linux/Maemo → Graphics
Product: Fennec → Core
QA Contact: maemo-linux → thebes
Comment on attachment 401927 [details] [diff] [review]
patch 4 (platform)

Moved to Core:Graphics and requesting approval for 1.9.2
Attachment #401927 - Flags: approval1.9.2?
Attachment #401927 - Flags: approval1.9.2? → approval1.9.2+
Attached patch patch 2 (fennec)Splinter Review
This is an updated Fennec CSS patch, needed to handle the new true DPI on the n900/n810.
Attachment #401386 - Attachment is obsolete: true
Attachment #402184 - Flags: review?(gavin.sharp)
Attachment #402184 - Flags: review?(gavin.sharp) → review+
pushed the fennec CSS patch:
https://hg.mozilla.org/mobile-browser/rev/6b89b88cca17
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 518550
Depends on: 519108
Depends on: 518378
Depends on: 534918
Comment on attachment 401927 [details] [diff] [review]
patch 4 (platform)

I'm not sure why but my Windows debug shared build didn't like this change - layers.dll was trying to link to sDPI (I fixed the problem locally by moving GetDPI to gfxPlatform.cpp).
Depends on: 562311
You need to log in before you can comment on or make changes to this bug.