Closed Bug 1069573 Opened 5 years ago Closed 5 years ago

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

Categories

(Core Graveyard :: Widget: Gonk, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file, 5 obsolete files)

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 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.
Attachment #8491764 - Flags: review?(mwu)
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.
Attachment #8491764 - Attachment is obsolete: true
Attachment #8494226 - Flags: feedback?(mwu)
Attached image bootanimation.zip image (obsolete) —
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)
Attachment #8494228 - Attachment is obsolete: true
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
Attachment #8494226 - Flags: feedback?(mwu) → feedback+
Phew, thanks for the tip.  Worked after a little byte-order wrangling.
Attachment #8494226 - Attachment is obsolete: true
Attachment #8494230 - Attachment is obsolete: true
Attachment #8494280 - Flags: review?(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.)
Attachment #8494280 - Flags: review?(mwu) → review+
(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|.
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.
Attachment #8494280 - Attachment is obsolete: true
Attachment #8495652 - Flags: review+
Thanks!  Trying to get push access set up again, but not done yet.
https://hg.mozilla.org/mozilla-central/rev/44c3bd47de85
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
This seems to have randomly stopped working, though I don't seen any changes to the BootAnimation code.
Do our bootanimation pngs come with the proper bKGD chunks? identify -verbose suggests that they don't.
(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 ...
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.