714668 - use 24bit/32bit surfaces on devices with 24/32bit displays

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 1

[Doug Turner (:dougt)]

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 2

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

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?

Comment 3

[Doug Turner (:dougt)]

> 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 4

[Doug Turner (:dougt)]

Comment on attachment 585329 [details] [diff] [review]
patch v.1

snorp, can you also review this?  What am I missing?

Comment 5

[Patrick Walton (:pcwalton)]

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

Comment 6

[Doug Turner (:dougt)]

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

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: WIP patch based on dougt's original

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

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 9

[Patrick Walton (:pcwalton)]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 10

[Dmitrij Petters]

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.

Comment 11

[Chris Lord [:cwiiis]]

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.