Closed Bug 888121 Opened 11 years ago Closed 11 years ago

[helix] Overwrite incorrect DPI returned from the device

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: vlin, Assigned: vlin)

Details

(Whiteboard: [POVB])

Attachments

(3 files)

Attached image UI layout in DPI = 160
In Helix device, all UI layout including icon size, position is quite weird. This is due to wrong DPI value query from ANativeWindow(We got 160 DPI, but Helix is actually suitable for 240 DPI). Is it a reliable way ? We have to investigate how it works in Android OS and find a suitable solution for convenient hacking in the future.
Assignee: nobody → vlin
blocking-b2g: --- → hd?
Here are two possible changes.

--- a/widget/gonk/nsWindow.cpp
+++ b/widget/gonk/nsWindow.cpp
@@ -529,7 +529,14 @@ nsWindow::MakeFullScreen(bool aFullScreen)
 float
 nsWindow::GetDPI()
 {
-    return NativeWindow()->xdpi;
+    // Note that xdpi value depends on graphic HAL and FB driver implementation
+    // sometimes unreliable while ro.sf.lcd_density is defined in PRODUCT_PROPE
+    // we have.
+    // Query xdpi from ANativeWindow except if ro.sf.lcd_density property is av
+    char density[PROPERTY_VALUE_MAX];
+    property_get("ro.sf.lcd_density", density, nullptr);
+
+    return density ? atoi(density) : NativeWindow()->xdpi;
 }

--- a/full_helix.mk
+++ b/full_helix.mk
@@ -17,7 +17,8 @@ PRODUCT_PROPERTY_OVERRIDES += \
   ro.use_data_netmgrd=true \
   ro.moz.ril.emergency_by_default=true \
   ro.moz.omx.hw.max_width=800 \
-  ro.moz.omx.hw.max_height=480
+  ro.moz.omx.hw.max_height=480 \
+  ro.sf.lcd_density=240
Device Helix needs 240 DPI
Attachment #770670 - Flags: review?(mwu)
Query xdpi from ANativeWindow except if ro.sf.lcd_density property is available.
Attachment #770671 - Flags: review?(mwu)
The summary should be more descriptive.
Summary: [HD] The UI layout is weird. → [helix] Overwrite incorrect DPI returned from the device
Comment on attachment 770671 [details] [diff] [review]
Diff in nsWindow.cpp

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

::: widget/gonk/nsWindow.cpp
@@ +555,5 @@
> +    // Query xdpi from ANativeWindow except if ro.sf.lcd_density property is available.
> +    char density[PROPERTY_VALUE_MAX];
> +    property_get("ro.sf.lcd_density", density, nullptr);
> +
> +    return density ? atoi(density) : GetGonkDisplay()->xdpi;

I doubt that density will never be nullptr so the "GetGonkDisplay()->xdpi" never be called too.
In property_get(), if you put nullptr to third argument then it just don't copy any default chars into density array.
Then density will be always a variable pointer value.
Comment on attachment 770671 [details] [diff] [review]
Diff in nsWindow.cpp

We have a kernel side solution for this. See https://github.com/gp-b2g/gp-peak-kernel/commit/3568b0d881b7c8afb3cfc833caca64b061afa2d3 for an example of how to report the dpi correctly in the kernel. We will not be supporting userspace dpi overrides.
Attachment #770671 - Flags: review?(mwu) → review-
(In reply to Michael Wu [:mwu] from comment #6)
> Comment on attachment 770671 [details] [diff] [review]
> Diff in nsWindow.cpp
> 
> We have a kernel side solution for this. See
> https://github.com/gp-b2g/gp-peak-kernel/commit/
> 3568b0d881b7c8afb3cfc833caca64b061afa2d3 for an example of how to report the
> dpi correctly in the kernel. We will not be supporting userspace dpi
> overrides.

I know that kernel side solution. But kernel side does not guarantee to assign width/height, there's just something wrong with Helix(Qcom platform). We always get default value(160) through ANativeWindow->xdpi. It really depends on partner's bootimg. This patch just provide a flexibility for B2G, or we have to ask partner for an updated/modified bootimg each time we met this issue.
Flags: needinfo?(mwu)
(In reply to vlin from comment #7)
> I know that kernel side solution. But kernel side does not guarantee to
> assign width/height, there's just something wrong with Helix(Qcom platform).
> We always get default value(160) through ANativeWindow->xdpi.

160 is a result of the kernel returning -1 for width and height in the fbinfo. Please work with the vendor to patch the kernel to return the correct value.

> It really
> depends on partner's bootimg. This patch just provide a flexibility for B2G,
> or we have to ask partner for an updated/modified bootimg each time we met
> this issue.

This is one of our platform requirements. The framebuffer device should report the width and height correctly if it expects correct behavior based on this information.
Flags: needinfo?(mwu)
POVB THIS
blocking-b2g: hd? → ---
Whiteboard: [POVB]
Attachment #770670 - Flags: review?(mwu)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: