getScreenDepth() doesn't handle all 24-bit pixel formats

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

Trunk
Firefox 25
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
adb shell cat /proc/meminfo
MemTotal:         860816 kB

Running on Android 4.1.2, if that matters. Verified that 16-bit color is being used by logging sScreenDepth in GeckoAppShell.
(Assignee)

Comment 1

5 years ago
I found this after seeing banding on http://www.lagom.nl/lcd-test/gradient.php.

I opened the same page in Chrome and the gradient looks normal, so I assume the device does actually support 24-bit color.
Would probably be useful to know what the getPixelFormat call is returning on that device: http://hg.mozilla.org/mozilla-central/file/e5d74eebd0e2/mobile/android/base/GeckoAppShell.java#l1375
(Assignee)

Comment 3

5 years ago
It's not even hitting that code path. getGeckoInterface() == null when getScreenDepth() is first called, so looks like we have initialization race setting GeckoInterface, and the depth gets stuck at 16.
Summary: 24-bit color not enabled on Droid RAZR → getScreenDepth() can be called before GeckoInterface is set, preventing 24-bit color
(Assignee)

Comment 4

5 years ago
Created attachment 776824 [details] [diff] [review]
Fix 24-bit color detection

Sorry...I was a bit too hasty before, and I made a dumb mistake doing the logging. Turns out GeckoInterface is initialized just fine, and the problem is that getPixelFormat() is returning 5, a number that's not even documented on http://developer.android.com/reference/android/graphics/PixelFormat.html. I found http://stackoverflow.com/questions/11949709/confused-about-pixelformat, and it looks like 5 is mapped to BGRA_8888. Rather than checking for each format, though, I think we can simply look at PixelFormat.bitsPerPixel.

I also removed the getGeckoInterface() != null check to prevent potential problems like comment 3. I think it's better to crash than fail silently so we can any races if they exist.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #776824 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

5 years ago
Summary: getScreenDepth() can be called before GeckoInterface is set, preventing 24-bit color → getScreenDepth() doesn't handle all 24-bit pixel formats
Comment on attachment 776824 [details] [diff] [review]
Fix 24-bit color detection

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

Looks OK to me, bouncing to Chris to make sure there isn't some reason he didn't do it this way in the first place.
Attachment #776824 - Flags: review?(bugmail.mozilla) → review?(chrislord.net)

Comment 6

5 years ago
Comment on attachment 776824 [details] [diff] [review]
Fix 24-bit color detection

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

Ignorance is why I didn't do it like this in the first place. LGTM.
Attachment #776824 - Flags: review?(chrislord.net) → review+
https://hg.mozilla.org/mozilla-central/rev/ecc75387637a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.