1069573 - Allow filling a solid color behind animation frames in the boot animation

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

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

Use case: on devices like the RPi that are connected to external displays of different resolutions, being forced to use a single bootanimation.zip means we can't fill the display for all resolutions.  So enabling an (optional) fill color behind the animation lets us "cheat" and use a small-ish bootanimation that matches up with a solid background color.  Patch forthcoming.

Memory-constrained devices can also use this feature to reduce the resolution of image frames in bootanimation.zip.

Comment 1

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

Attachment: Optionally clear the background behind boot animations to a solid color

Comment 2

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

See https://github.com/mozilla-b2g/device-rpi/blob/master/rpi.mk and https://github.com/mozilla-b2g/device-rpi/blob/master/bootanimation.zip for an example usage of this feature.

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 8491764 [details] [diff] [review]
Optionally clear the background behind boot animations to a solid color

I like this, though I don't think specifying this in a system property is the right way to do this.

I would prefer reading the bKGD chunk from the PNG and using that to fill the background.

Comment 4

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

Attachment: Try to use background color from png file (doesn't work)

This patch doesn't work, but I don't see why and it's almost literally not worth my time to play with this more (on a deadline).  If there's not an easy fix we'll either not have nice things or fork.

I'll post the image I'm using next.  I broke on |png_handle_bKGD()| but it's not being hit, so the png_info isn't ever updated with this.

Comment 5

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

Attachment: bootanimation.zip image

Comment 6

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

Attachment: bootanimation.zip image

Thus sayeth |identify -verbose|:

Image: frames/ffos.png
  Format: PNG (Portable Network Graphics)
[snip]
  Background color: srgba(0,83,159,1)
[snip]
    png:bKGD: chunk was found (see Background color, above)

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 8494226 [details] [diff] [review]
Try to use background color from png file (doesn't work)

Make sure you remove bKGD from the unknown chunks list -

http://mxr.mozilla.org/mozilla-central/source/widget/gonk/libdisplay/BootAnimation.cpp#291

Comment 8

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

Attachment: Use the bKGD PNG background color (if present) to fill the background of animation frames

Phew, thanks for the tip.  Worked after a little byte-order wrangling.

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 8494280 [details] [diff] [review]
Use the bKGD PNG background color (if present) to fill the background of animation frames

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

Looks good! r=me with comments addressed.

::: widget/gonk/libdisplay/BootAnimation.cpp
@@ +420,5 @@
> +    } color;
> +    color.b8 = color16.blue;
> +    color.g8 = color16.green;
> +    color.r8 = color16.red;
> +    color.x8 = 0;

I would prefer x8 to be 0xFF in case we get some bizarre device that cares about alpha.

@@ +582,5 @@
>  
> +                if (frame.has_bgcolor) {
> +                    wchar_t bgfill = AsBackgroundFill(frame.bgcolor, format);
> +                    wmemset((wchar_t*)vaddr, bgfill,
> +                            (buf->width * buf->height * frame.bytepp) / sizeof(wchar_t));

You'll want to use buf->stride rather than buf->width * frame.bytepp. (The Omate TrueSmart has a 240 x 240 display with a stride of 256.)

Comment 10

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

(In reply to Michael Wu [:mwu] from comment #9)
> Comment on attachment 8494280 [details] [diff] [review]
> Use the bKGD PNG background color (if present) to fill the background of
> animation frames
> 
> Review of attachment 8494280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! r=me with comments addressed.
> 
> ::: widget/gonk/libdisplay/BootAnimation.cpp
> @@ +420,5 @@
> > +    } color;
> > +    color.b8 = color16.blue;
> > +    color.g8 = color16.green;
> > +    color.r8 = color16.red;
> > +    color.x8 = 0;
> 
> I would prefer x8 to be 0xFF in case we get some bizarre device that cares
> about alpha.

Done.

> @@ +582,5 @@
> >  
> > +                if (frame.has_bgcolor) {
> > +                    wchar_t bgfill = AsBackgroundFill(frame.bgcolor, format);
> > +                    wmemset((wchar_t*)vaddr, bgfill,
> > +                            (buf->width * buf->height * frame.bytepp) / sizeof(wchar_t));
> 
> You'll want to use buf->stride rather than buf->width * frame.bytepp. (The
> Omate TrueSmart has a 240 x 240 display with a stride of 256.)

It's not documented, but buf->stride seems to be a pixel stride, not a byte stride.  So fixed, but I kept in the |* frame.bytepp|.

Comment 11

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

Attachment: Use the bKGD PNG background color (if present) to fill the background of animation frames, v2

I've changed my ssh key since I left Moz, so I don't have push access right now.  Does anyone have some time to push this to try for me?  Would appreciate it.  No rush though.

Comment 12

[Michael Wu [:mwu]]

https://tbpl.mozilla.org/?tree=Try&rev=49ed1c81460a

Comment 13

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

Thanks!  Trying to get push access set up again, but not done yet.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44c3bd47de85

Comment 15

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

Thanks!

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44c3bd47de85

Comment 17

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

This seems to have randomly stopped working, though I don't seen any changes to the BootAnimation code.

Comment 18

[Michael Wu [:mwu]]

Do our bootanimation pngs come with the proper bKGD chunks? identify -verbose suggests that they don't.

Comment 19

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

(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #17)
> This seems to have randomly stopped working, though I don't seen any changes
> to the BootAnimation code.

This is randomly back working at the sha1 we're stuck on for bug 1085599, so I'm not going to worry about it for now.

(In reply to Michael Wu [:mwu] from comment #18)
> Do our bootanimation pngs come with the proper bKGD chunks? identify
> -verbose suggests that they don't.

Do you mean the one in device-rpi?  Here's what my identify says

$ identify -verbose frames/ffos.png 
[...]
  Background color: srgba(0,83,159,1)
[...]
    png:bKGD: chunk was found (see Background color, above)

I don't see anything there to make me believe they're ill-formed ...