note: 1) Bitmap.Config doesn't have a 24 bit value 2) PixelFormat is also incomplete. It doesn't have a value for the display type of the Google Nexus (and various other devices w/ 24 displays)

Comment on attachment 585329 [details] [diff] [review] patch v.1 Review of attachment 585329 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java @@ +111,5 @@ > + int format = display.getPixelFormat(); > + > + // format 5 = BGRA_8888. > + // PixelFormat isn't complete. See google bug: > + // http://code.google.com/p/android/issues/detail?id=371 This link doesn't say anything about format 5. Where did you get format 5 = BGRA_8888 from? @@ +119,5 @@ > + else if (format == 5 || > + format == PixelFormat.RGB_888) > + mFormat = CairoImage.FORMAT_RGB24; > + else > + mFormat = CairoImage.FORMAT_ARGB32; It would be cleaner to factor this code out into a CairoUtils.pixelFormatToCairoFormat function, so that all the conversion code is one place. @@ +121,5 @@ > + mFormat = CairoImage.FORMAT_RGB24; > + else > + mFormat = CairoImage.FORMAT_ARGB32; > + > + Log.d(LOGTAG, "screen denisty = " + display.getPixelFormat()); Typo: denisty -> density @@ +175,5 @@ > + int size; > + if (mFormat == CairoImage.FORMAT_RGB16_565) > + size = mBufferSize.getArea() * 2; > + else > + size = mBufferSize.getArea() * 4; Why doesn't this check for FORMAT_RGB24 and return mBufferSize.getArea() * 3? ::: widget/src/android/AndroidGraphicBuffer.cpp @@ +410,5 @@ > return HAL_PIXEL_FORMAT_RGBX_8888; > case gfxImageFormat::ImageFormatRGB16_565: > return HAL_PIXEL_FORMAT_RGB_565; > + case gfxImageFormat::ImageFormatARGB32: > + return HAL_PIXEL_FORMAT_RGB_888; I don't know what this is doing; snorp should review this?

> This link doesn't say anything about format 5. Where did you get format 5 = BGRA_8888 from? It matches the HAL_ define. Also see hardware/libhardware/include/hardware/hardware.h > It would be cleaner to factor this code out into a CairoUtils.pixelFormatToCairoFormat function, so that all the conversion code is one place. Sure. Maybe a follow up too. > Typo: denisty -> density Thanks. I'll drop the log. > Why doesn't this check for FORMAT_RGB24 and return mBufferSize.getArea() * 3? I don't think RGB24 is packed -- it allocates 4 bytes just like ARGB and just ignores the alpha. > I don't know what this is doing; snorp should review this? Yeah, i'll add him.

Comment on attachment 585329 [details] [diff] [review] patch v.1 snorp, can you also review this? What am I missing?

Is there any device with 24/32-bit displays in which gralloc isn't working, other than the Galaxy S2? The way that this interacts with gralloc is a bit confusing to me...

snorp, i heard you have an updated patch. If you can't get to this for ff11, please unassign your self or speak up.

This is my current WIP. I think it should be mostly ok, but I haven't had time to test it much.

FWIW, I didn't notice much improvement with this. In fact it's possible it could be slower (more checkerboarding, that is).

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #8) > FWIW, I didn't notice much improvement with this. In fact it's possible it > could be slower (more checkerboarding, that is). Yeah, it strikes me as quite possibly faster if the GPU is doing the color space conversion and the CPU is doing cairo rendering in 16-bit. We aren't fillrate-bound here.

Tested 16bit (CairoImage.FORMAT_RGB16_565) rendering vs 32bit (CairoImage.FORMAT_ARGB32) rendering on a Galaxy Nexus (BGRA_8888) using pcwalton's test-async-texture-upload (https://github.com/pcwalton/test-async-texture-upload). 16-bit rendering was 1FPS faster on average. Marking as won't fix.

This seems like a slightly dodgy reason to close this? Of course the texture upload is slower, it's uploading more data - that it's a negligible difference is interesting though. What needs to be tested is whether render performance is improved - this could be done by adding some timing in nsWindow:DrawTo. There's also the issue of image quality - gradients and such will look better in 32-bit than 16, so if the performance was identical and we weren't concerned about the increased memory use, 32-bit would be favourable. I have no strong opinion on this, but WONTFIX seems a bit extreme.