Closed Bug 831624 Opened 9 years ago Closed 9 years ago

Massive performance regression on some devices when bootanimation is enabled

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: cjones, Assigned: mwu)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(1 file, 2 obsolete files)

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.
Yep, we see this on some devices.
Assignee: nobody → mwu
blocking-b2g: tef? → tef+
Chris, if we can get a reproducible case, Benoit could shadow this.
What's the status here?
Flags: needinfo?
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.
Flags: needinfo?
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.
Attachment #706183 - Flags: review?(jones.chris.g)
Attachment #706183 - Flags: review?(joe)
I forgot to add some error checking.
Attachment #706183 - Attachment is obsolete: true
Attachment #706183 - Flags: review?(jones.chris.g)
Attachment #706183 - Flags: review?(joe)
Attachment #706196 - Flags: review?(jones.chris.g)
Attachment #706196 - Flags: review?(joe)
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 :|.
Attachment #706196 - Flags: review?(jones.chris.g) → review+
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).
Attachment #706196 - Flags: review?(joe)
Attachment #706196 - Flags: review?(glennrp+bmo)
Attachment #706196 - Flags: review+
(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 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.
Attachment #706196 - Flags: review?(glennrp+bmo) → review-
(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.
Attachment #706196 - Attachment is obsolete: true
Attachment #706613 - Flags: review?(glennrp+bmo)
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 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+
Attachment #706613 - Flags: review?(glennrp+bmo) → review+
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.
(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.
Filed bug 835015
Blocks: 835015
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.
Also, the setjmp should be placed immediately after creating the info struct.
http://hg.mozilla.org/releases/mozilla-b2g18/rev/32ade9479a2c

I'll file a follow up to fix setjmp.
Glenn, thanks as always for your in-depth review.
https://hg.mozilla.org/mozilla-central/rev/358dd2a32990
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
Whiteboard: [needs-b2g-v1.0 uplift]
Blocks: 835325
Blocks: 836338
"Can you please provide steps to verify this fix - as we will blackbox test from the UI?"
The issue does not need a test case.
Flags: in-moztrap-
Chris Jones if you can Verify this bug out, it would be much appreciated.
Flags: needinfo?(cjones.bugs)
Keywords: verifyme
Whiteboard: QARegressExclude
The report originated downstream, will let them decide the best course of action ;).
Flags: needinfo?(cjones.bugs) → needinfo?(mvines)
Status: RESOLVED → VERIFIED
Flags: needinfo?(mvines)
Per comment 30,I clear "Verifyme".
You need to log in before you can comment on or make changes to this bug.