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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

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.

Comment 1

[Brian Nicholson (:bnicholson)]

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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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

Comment 3

[Brian Nicholson (:bnicholson)]

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.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: 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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[Chris Lord [:cwiiis]]

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.

Comment 7

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc75387637a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ecc75387637a