831624 - Massive performance regression on some devices when bootanimation is enabled

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

D'oh, I thought this had been filed.  See bug 809665 comment 40.  This has been reported by downstreams.

It seems that we're not destroying some EGL things that we set up for the bootanimation, or we're otherwise failing to shut it down properly.

Needs to block if it repros on v1 HW.

m1, are you guys seeing this?  mwu can you look?  /pm me for an allegedly reproducible setup.

Comment 1

[Michael Vines [:m1] [:evilmachines]]

Yep, we see this on some devices.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Chris, if we can get a reproducible case, Benoit could shadow this.

Comment 3

[Andrew Overholt [:overholt]]

What's the status here?

Comment 4

[Michael Wu [:mwu]]

We sort of have an idea of what's going on and how to work around it, but I'm not sure if a workaround patch is going to be finished today. It also doesn't show up on either unagi or otoro.

Comment 5

[Michael Wu [:mwu]]

Attachment: Use fb/gralloc to render boot animation

This switches us to a much simpler gralloc/fb based drawing path. I think having this gl code run while gecko was running and initializing gl in the background did some weird things on certain configurations. This is simpler and more obviously correct. Unfortunately, we have to do our own format conversion now. I've tested the 565 case and RGBA 8888 case.

Joe, do you mind reviewing the libpng changes? I just turned on a few more functions for format conversion.

Comment 6

[Michael Wu [:mwu]]

Attachment: Use fb/gralloc to render boot animation, v2

I forgot to add some error checking.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 706196 [details] [diff] [review]
Use fb/gralloc to render boot animation, v2

Mid-aired v2, but the interdiff looks fine.

>diff --git a/b2g/app/BootAnimation.cpp b/b2g/app/BootAnimation.cpp

>+AnimationFrame::ReadPngFrame(int format)
> {

Deferring to joe for the libpng usage here.

>@@ -386,6 +366,18 @@ AnimationThread(void *)
>         return nullptr;
>     }
> 
>+    int format;
>+    ANativeWindow * window = gNativeWindow.get();

ANativeWindow*

>+                ANativeWindowBuffer *buf;

ANativeWindowBuffer*

>diff --git a/media/libpng/mozpngconf.h b/media/libpng/mozpngconf.h

More for joe, obviously.

Pretty scary change to take this late in the game but about as simple
as could be hoped.  Also very scary that this actually works around
whatever bug we're hitting :|.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 706196 [details] [diff] [review]
Use fb/gralloc to render boot animation, v2

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

::: b2g/app/BootAnimation.cpp
@@ +249,4 @@
>          return strcmp(path, other.path) < 0;
>      }
>  
> +    void ReadPngFrame(int format);

ugh, is int really the accepted type for storing HAL_PIXEL_FORMATs?

@@ +278,5 @@
>      state->offset += length;
>  }
>  
> +static void
> +TransformTo565(png_structp png_ptr, png_row_infop row_info, png_bytep data)

This is going to look pretty bad (banding, etc) if our source PNGs have values not representable in 16-bit.

@@ +285,5 @@
> +    uint8_t *inbuf = (uint8_t *)data;
> +    for (int i = 0; i < row_info->rowbytes; i += 3) {
> +        *outbuf++ = ((inbuf[i]     & 0xF8) << 8) |
> +                    ((inbuf[i + 1] & 0xFC) << 3) |
> +                    ((inbuf[i + 2] & 0xF8) >> 3);

You can omit the & 0xF8 on the blue component that you're shifting right, since we're already getting the most significant bits.

@@ +312,5 @@
>      width = png_get_image_width(pngread, pnginfo);
>      height = png_get_image_height(pngread, pnginfo);
> +    switch (format) {
> +    case HAL_PIXEL_FORMAT_BGRA_8888:
> +        png_set_bgr(pngread);

add
// FALL THROUGH

@@ +322,5 @@
> +    case HAL_PIXEL_FORMAT_RGB_888:
> +        bytepp = 3;
> +        break;
> +    default:
> +        LOGW("Unknown pixel format %d. Assuming RGB 565.", format);

and here.

@@ +328,5 @@
> +        bytepp = 2;
> +        png_set_read_user_transform_fn(pngread, TransformTo565);
> +        break;
> +    }
> +    buf = (char *)malloc(width * (height + 1) * bytepp);

Why height + 1?

::: media/libpng/mozpngconf.h
@@ +49,5 @@
>  #define PNG_EASY_ACCESS_SUPPORTED
>  #define PNG_READ_EXPAND_SUPPORTED
> +#define PNG_READ_USER_TRANSFORM_SUPPORTED
> +#define PNG_READ_BGR_SUPPORTED
> +#define PNG_READ_FILLER_SUPPORTED

Glenn, are there any pitfalls we might be running in to here? (Note that these aren't used from our PNG decoder, just in a libpng user that's used only on B2G).

Comment 9

[Michael Wu [:mwu]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #8)
> Comment on attachment 706196 [details] [diff] [review]
> Use fb/gralloc to render boot animation, v2
> 
> Review of attachment 706196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/app/BootAnimation.cpp
> @@ +249,4 @@
> >          return strcmp(path, other.path) < 0;
> >      }
> >  
> > +    void ReadPngFrame(int format);
> 
> ugh, is int really the accepted type for storing HAL_PIXEL_FORMATs?
> 

Yeah, as far as I can tell. The fb device stores it as an int and the enum for HAL_PIXEL_FORMATs isn't named.

> @@ +278,5 @@
> >      state->offset += length;
> >  }
> >  
> > +static void
> > +TransformTo565(png_structp png_ptr, png_row_infop row_info, png_bytep data)
> 
> This is going to look pretty bad (banding, etc) if our source PNGs have
> values not representable in 16-bit.
> 

Yeah I don't mind much. We aren't loading arbitrary web content - things should be optimized for the device.

> @@ +285,5 @@
> > +    uint8_t *inbuf = (uint8_t *)data;
> > +    for (int i = 0; i < row_info->rowbytes; i += 3) {
> > +        *outbuf++ = ((inbuf[i]     & 0xF8) << 8) |
> > +                    ((inbuf[i + 1] & 0xFC) << 3) |
> > +                    ((inbuf[i + 2] & 0xF8) >> 3);
> 
> You can omit the & 0xF8 on the blue component that you're shifting right,
> since we're already getting the most significant bits.
> 

Right, I'll do that. (Wonder what gcc actually does..)

> @@ +328,5 @@
> > +        bytepp = 2;
> > +        png_set_read_user_transform_fn(pngread, TransformTo565);
> > +        break;
> > +    }
> > +    buf = (char *)malloc(width * (height + 1) * bytepp);
> 
> Why height + 1?
> 

We allocate only enough space to fit the target format, so for cases where the source format is larger than the target format, we need to give libpng enough space to write the last row. Adding an extra row gives us enough space in the worst case that we're converting from RGBA 8888 to RGB 565.

Comment 10

[Glenn Randers-Pehrson]

Comment on attachment 706196 [details] [diff] [review]
Use fb/gralloc to render boot animation, v2

width and height are uint16 while png_get_width() and png_get_height() each returns a png_uint_32 that can be as large as 1,000,000 (or whatever other limit happens to be set in mozpngconf.h or in nsPNGDecoder.cpp).

I would rather see "format" called "output_format" or something similarly descriptive like "outputFormat".  I believe we can assume that the input format (from the PNG file) is RGB_888 or RGBA_8888 possibly promoted from grayscale or indexed and demoted to 8 bits from 16 bits per sample.

I guess providing an extra row for temporary storage of rows that have more than 2 bytes per pixel will work and is more efficient that actually allowing sufficient space in each row, but there should probably be a comment to that effect.

Inside the TransformTo565 callback it seems to be assumed that the input format is RGB_888, which is not going to be the case after the png_set_filler() operation, which changes the input format's bytepp to 4.

Comment 11

[Michael Wu [:mwu]]

(In reply to Glenn Randers-Pehrson from comment #10)
> Comment on attachment 706196 [details] [diff] [review]
> Use fb/gralloc to render boot animation, v2
> 
> width and height are uint16 while png_get_width() and png_get_height() each
> returns a png_uint_32 that can be as large as 1,000,000 (or whatever other
> limit happens to be set in mozpngconf.h or in nsPNGDecoder.cpp).
> 

Will switch to uint32 (though this issue isn't introduced by this patch, I might as well patch it while you're here)

> I would rather see "format" called "output_format" or something similarly
> descriptive like "outputFormat".  I believe we can assume that the input
> format (from the PNG file) is RGB_888 or RGBA_8888 possibly promoted from
> grayscale or indexed and demoted to 8 bits from 16 bits per sample.
> 

Ok.

> I guess providing an extra row for temporary storage of rows that have more
> than 2 bytes per pixel will work and is more efficient that actually
> allowing sufficient space in each row, but there should probably be a
> comment to that effect.
> 

Ok.

> Inside the TransformTo565 callback it seems to be assumed that the input
> format is RGB_888, which is not going to be the case after the
> png_set_filler() operation, which changes the input format's bytepp to 4.

png_set_filler is not called for 565 target surfaces.

Comment 12

[Michael Wu [:mwu]]

Attachment: Use fb/gralloc to render boot animation, v3

Comment 13

[Michael Wu [:mwu]]

I added strip_16 and strip_alpha to ensure this code can handle more types of pngs, as inappropriate for the task as they may be.

Comment 14

[Glenn Randers-Pehrson]

Comment on attachment 706613 [details] [diff] [review]
Use fb/gralloc to render boot animation, v3

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

> png_set_filler is not called for 565 target surfaces.

OK, understood.  So with the use of 32-bit width and height for safety, r+

Comment 15

[Glenn Randers-Pehrson]

Since these PNG datastreams are not being handled by image/decoders/nsPNGDecoder.cpp I recommend for safety duplicating the two "PNG_HANDLE_AS_UNKNOWN_SUPPORTED" blocks into ReadPngFrame(), right after creating the pngread struct.  If we're not doing color management of these, move iCCP and cHRM into the unused_chunks array, too.
This is important to do for performance and security if there's a chance that the input PNG datastream might have any of those unused chunks, less important if the input is under our control.

Comment 16

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/358dd2a32990

Comment 17

[Michael Wu [:mwu]]

(In reply to Glenn Randers-Pehrson from comment #15)
> Since these PNG datastreams are not being handled by
> image/decoders/nsPNGDecoder.cpp I recommend for safety duplicating the two
> "PNG_HANDLE_AS_UNKNOWN_SUPPORTED" blocks into ReadPngFrame(), right after
> creating the pngread struct.  If we're not doing color management of these,
> move iCCP and cHRM into the unused_chunks array, too.
> This is important to do for performance and security if there's a chance
> that the input PNG datastream might have any of those unused chunks, less
> important if the input is under our control.

The input is under our control, but I will file a bug about doing this anyway.

Comment 18

[Michael Wu [:mwu]]

Filed bug 835015

Comment 19

[Glenn Randers-Pehrson]

Sorry I missed this in my review.

   The return from "setjmp" call should be tested and error exit taken:

Change
     setjmp(png_jmpbuf(pngread));
to
     if(setjmp(png_jmpbuf(pngread)))
     {
        /* error: cleanup and return */
        png_destroy_read_struct(&pngread, &pnginfo, nullptr);
        return nullptr;
     };

and wherever ReadPngFrame() is called, check for null return
and take appropriate action.

Comment 20

[Glenn Randers-Pehrson]

Also, the setjmp should be placed immediately after creating the info struct.

Comment 21

[Michael Wu [:mwu]]

http://hg.mozilla.org/releases/mozilla-b2g18/rev/32ade9479a2c

I'll file a follow up to fix setjmp.

Comment 22

[Joe Drew (not getting mail)]

Glenn, thanks as always for your in-depth review.

Comment 23

[Michael Vines [:m1] [:evilmachines]]

FYI, this needs uplift to http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/358dd2a32990

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/1541e71551ab

Comment 26

[Deepa Subramanian]

"Can you please provide steps to verify this fix - as we will blackbox test from the UI?"

Comment 27

[Darren]

The issue does not need a test case.

Comment 28

[cbarker]

Chris Jones if you can Verify this bug out, it would be much appreciated.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The report originated downstream, will let them decide the best course of action ;).

Comment 30

[Shally Li（leave from Mozilla）]

Per comment 30,I clear "Verifyme".