921369 - GonkDisplay takes care of Framebuffer supporting only BRGA format

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Vincent Lin[:vilin]]

We met some platform only supporting BGRA format framebuffer. Here we extend GonkDisplay to take care of this issue.

Comment 1

[Vincent Lin[:vilin]]

Attachment: bug-921369-fix.patch

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 811004 [details] [diff] [review]
bug-921369-fix.patch

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

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +125,2 @@
>      status_t error;
> +    for(uint32_t i = 0; i < sizeof(FB_FORMAT) / sizeof(FB_FORMAT[0]); ++i) {

There's only one fallback case. A loop makes things less clear.

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 811004 [details] [diff] [review]
bug-921369-fix.patch

(in case I was being too cryptic - please unroll the loop)

Comment 4

[Vincent Lin[:vilin]]

Attachment: bug-921369-fix.patch

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 811803 [details] [diff] [review]
bug-921369-fix.patch

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

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +105,5 @@
>  
>      mAlloc = new GraphicBufferAlloc();
> +    // Not worried about surfaceformat may change later. 
> +    // Here is just to set a default format of BufferQueue. 
> +    // The practical format is actually determined by ANativeWindow.

This is true, but I think we can set the right one anyway by moving the createGraphicBuffer code here.

@@ +119,5 @@
>      mList = (hwc_display_contents_1_t *)malloc(sizeof(*mList) + (sizeof(hwc_layer_1_t)*2));
>      if (mHwc)
>          mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, 0);
>  
> +    // Create a valid GraphicBuffer for boot animation.

This does not seem to be a useful comment.

@@ +124,4 @@
>      status_t error;
> +    uint32_t usage = GRALLOC_USAGE_HW_FB | GRALLOC_USAGE_HW_RENDER | GRALLOC_USAGE_HW_COMPOSER;
> +    mBootAnimBuffer = mAlloc->createGraphicBuffer(mWidth, mHeight, surfaceformat, usage, &error);
> +    if(error != NO_ERROR || !mBootAnimBuffer.get()) {

Add a space after if

@@ +124,5 @@
>      status_t error;
> +    uint32_t usage = GRALLOC_USAGE_HW_FB | GRALLOC_USAGE_HW_RENDER | GRALLOC_USAGE_HW_COMPOSER;
> +    mBootAnimBuffer = mAlloc->createGraphicBuffer(mWidth, mHeight, surfaceformat, usage, &error);
> +    if(error != NO_ERROR || !mBootAnimBuffer.get()) {
> +	 ALOGI( "[Bootanimation] Try to create BRGA format framebuffer");

This line is misindented. Also, please don't prefix these messages with [Bootanimation]. Set LOG_TAG if you want messages from this file to be easier to find.

@@ +127,5 @@
> +    if(error != NO_ERROR || !mBootAnimBuffer.get()) {
> +	 ALOGI( "[Bootanimation] Try to create BRGA format framebuffer");
> +        surfaceformat = HAL_PIXEL_FORMAT_BGRA_8888;
> +        mBootAnimBuffer = mAlloc->createGraphicBuffer(mWidth, mHeight, surfaceformat, usage, &error);
> +    }		

Trailing whitespace.

@@ +129,5 @@
> +        surfaceformat = HAL_PIXEL_FORMAT_BGRA_8888;
> +        mBootAnimBuffer = mAlloc->createGraphicBuffer(mWidth, mHeight, surfaceformat, usage, &error);
> +    }		
> +    if (error == NO_ERROR && mBootAnimBuffer.get()) {
> +	 ALOGI( "[Bootanimation] Start bootanimation ... with (%d) format framebuffer", surfaceformat);

This is also misindented.

Other lines were also misindented similarly (see line 55-56) in bug 915487. Please fix them while you're at it.

@@ +136,1 @@
>      else

else needs to be on the same line as }

Comment 6

[Vincent Lin[:vilin]]

Attachment: bug-921369-fix.patch

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 812989 [details] [diff] [review]
bug-921369-fix.patch

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

Looks good - just have some nits.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +108,5 @@
> +    status_t error;
> +    uint32_t usage = GRALLOC_USAGE_HW_FB | GRALLOC_USAGE_HW_RENDER | GRALLOC_USAGE_HW_COMPOSER;
> +    mBootAnimBuffer = mAlloc->createGraphicBuffer(mWidth, mHeight, surfaceformat, usage, &error);
> +    if (error != NO_ERROR || !mBootAnimBuffer.get()) {
> +        ALOGI( "Try to create BRGA format framebuffer");

Remove the space after '('
s/Try/Trying/

@@ +127,5 @@
>      if (mHwc)
>          mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, 0);
>  
> +    if (error == NO_ERROR && mBootAnimBuffer.get()) {
> +        ALOGI("Start bootanimation ... with (%d) format framebuffer", surfaceformat);

s/Start/Starting/
s/... //

Comment 8

[Vincent Lin[:vilin]]

Attachment: bug-921369-fix.patch

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b62de18b545f

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2140044eba7d