809665 - Boot animation support

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Michael Wu [:mwu]]

This is required to support bug 799714 .

Comment 1

[Michael Wu [:mwu]]

Attachment: Support android bootanimation.zip

Comment 2

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

Dpes the BootAnimation code originate elsewhere or is it ours?  Would like to know so I know how closely to look over it.

Comment 3

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> Dpes the BootAnimation code originate elsewhere or is it ours?  Would like
> to know so I know how closely to look over it.

It's largely original but I used vlad's old 2D rendering code from the android widget impl as a basis of the gl code: https://hg.mozilla.org/users/vladimir_mozilla.com/mozilla-droid/file/2bce1dacf4d9/widget/src/android/nsWindow.cpp#l782 I switched to a single fragment shader, switched to vec2 for the vector shader input (z was always 0), and switched to array buffers to store the vectors. The createconfig code was originally derived from GLContextProviderEGL.cpp but switched to Android's simple configuration selection logic.

The jar reader code is all original (though with structs derived from a zip manipulation tool I wrote) and so is the png reader code/glue.

Comment 4

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

Comment on attachment 679443 [details] [diff] [review]
Support android bootanimation.zip

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

>+static android::sp<android::FramebufferNativeWindow> gNativeWindow;

|using namespace android|

>+static pthread_t sSplashThread;
>+static bool sRunSplash;
>+

Nit: this is "BootAnimation.cpp", but we're calling this "splash"
which is a little confusing.  I'd s/splash/animation/ here.

>+/* XXX: fix for big endian */
>+#define le16toh(x) (x)
>+#define le32toh(x) (x)
>+#define htole16(x) (x)
>+#define htole32(x) (x)
>+

We don't have something like endian.h in bionic?

>+struct local_file_header {

This name seems a bit too general ... this is a header for a file
contained in a zip?

Per discussion, if this code to deal with .zip's is already somewhere
we can borrow it with minimal dependencies, I'd rather do that.

>+class ZipReader {
>+    const char *buf;

Since these aren't "pure data", please stick with mBuf style.  Maybe
not needed if we grab an existing impl.

>+struct AnimationFrame {

>+    void readPngFrame();

ReadPngFrame().

>+struct AnimationPart {
>+    int32_t count;
>+    int32_t pause;
>+    char path[256];
>+    std::vector<AnimationFrame> frames;

|using namespace std;|

>+static void
>+rawReader(png_structp png_ptr, png_bytep data, png_size_t length)

RawReader()

>+void
>+AnimationFrame::readPngFrame()
>+{

>+    buf = (char *)malloc(width * height * 3);

Are we guaranteed to always get packed r8g8b8 out of here?

>+    std::vector<char *> rows(height + 1);

|using namespace std|

>+static void *
>+splashAnimation(void *)

How about |AnimationThread()|.

>+    int format;
>+    ANativeWindow const * const window = gNativeWindow.get();

Why do we know that gNativeWindow has been initialized by now?  Plz
document.

>+    std::vector<AnimationPart> parts;

|using namespace std|

>+    } while (end && (line = end + 1));

Do you mean |*(line = end + 1)| ?


>+    for (uint32_t i = 0; i < parts.size(); i++) {

This loop mostly seems OK, but a high-level description of the format
somewhere would be helpful.  Here is fine.

So a full-generality GL loop isn't quite the right tool for the job
here.  Using hwc would be simpler, cleaner, and more efficient.  We
can discuss that, but I'll go ahead and review the GL code here.

>+                glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,
>+                             width, height, 0,
>+                             GL_RGB, GL_UNSIGNED_BYTE, NULL);

This isn't necessary.  (The indentation is weird too.)

>+    uint32_t framedelay = 1000000 / fps;

frameDelayUs

>+    for (uint32_t i = 0; i < parts.size(); i++) {

Is the animation supposed to loop indefinitely?  I don't see how that
would happen here.

>+        while (sRunSplash && (!part.count || j++ < part.count)) {
>+            for (uint32_t k = 0; k < part.frames.size(); k++) {

>+                glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,
>+                             frame.width, frame.height, 0,
>+                             GL_RGB, GL_UNSIGNED_BYTE, frame.buf);

This is pretty hurty but the only way to improve is memcpy to gralloc
buffer(s).

>+                glDrawArrays(GL_TRIANGLE_STRIP, 0, 4);

I don't see where a vertex array is enabled here, so I'm not sure how
this is working.

>+                if (tv2.tv_usec < framedelay)
>+                    usleep(framedelay - tv2.tv_usec);

Hm, this sleep doesn't make sense to me.  I don't see us decoding in
this inner loop.  If we try to SwapBuffers() too many times within the
swap interval, then the SwapBuffers() call will block.

>+            usleep(framedelay * part.pause);

Are the |pause|s supposed to be precise?  This may cause us to
oversleep a frame boundary and drop frames.

>+extern "C" __attribute__ ((visibility ("default")))
>+android::FramebufferNativeWindow*
>+NativeWindow()

I don't see any reason for these to be extern "C".  C code couldn't
use these anyway without serious hackery.

>+{
>+    if (gNativeWindow.get())
>+        return gNativeWindow.get();

{ }

>diff --git a/b2g/app/Makefile.in b/b2g/app/Makefile.in

>+LIBS += -lpng -lGLESv2 -lEGL -lui -lhardware_legacy -lhardware -lcutils

Is it possible for us to statically link our own libpng clone into the
b2g binary?  It would be nice to stay off one more gonk treadmill and
have a better set of known bugs/optimizations etc.

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

>+#ifdef MOZ_WIDGET_GONK
>+namespace android {
>+class FramebufferNativeWindow;
>+}
>+
>+extern "C" android::FramebufferNativeWindow* NativeWindow();
>+extern "C" void StopBootAnimation();
>+#endif

We should also pull these in through a common header.

>+#ifdef MOZ_WIDGET_GONK
>+  NativeWindow();

Let's do |unused << NativeWindow()| so it's clearer that this getter
is being invoked for its side effects.  ("NativeWindow" isn't a verb
phrase so this call is a little confusing at first.)

>diff --git a/widget/gonk/nsAppShell.cpp b/widget/gonk/nsAppShell.cpp

>+    nsCOMPtr<nsIObserverService> obsServ =
>+        mozilla::services::GetObserverService();

|GetObserverService()|.

> NS_IMETHODIMP
>+nsAppShell::Observe(nsISupports* aSubject,
>+                    const char* aTopic,
>+                    const PRUnichar* aData)
>+{
>+    if (!strcmp(aTopic, "browser-ui-startup-complete")) {
>+        mEnableDraw = true;
>+        NotifyEvent();
>+    } else {
>+        return nsBaseAppShell::Observe(aSubject, aTopic, aData);
>+    }

Unless you have a specific reason not to, let's forward all
notifications to base.  So move the else block out of the else.

>+NS_IMETHODIMP
> nsAppShell::Exit()
> {
>+    nsCOMPtr<nsIObserverService> obsServ =
>+        mozilla::services::GetObserverService();

|GetObserverService()|

>diff --git a/widget/gonk/nsAppShell.h b/widget/gonk/nsAppShell.h

>+    bool mEnableDraw;

Document this.

>diff --git a/widget/gonk/nsWindow.cpp b/widget/gonk/nsWindow.cpp

>+/* Implemented in b2g/app/BootAnimation.cpp */
>+extern "C" __attribute__ ((weak))
>+android::FramebufferNativeWindow* NativeWindow();

>+extern "C" __attribute__ ((weak))
>+void StopBootAnimation();

Move imported symbols to a BootAnimation.h.

Comment 5

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> >+/* XXX: fix for big endian */
> >+#define le16toh(x) (x)
> >+#define le32toh(x) (x)
> >+#define htole16(x) (x)
> >+#define htole32(x) (x)
> >+
> 
> We don't have something like endian.h in bionic?
> 

Fixed. Was just too lazy to look it up.

> >+struct local_file_header {
> 
> This name seems a bit too general ... this is a header for a file
> contained in a zip?
> 
> Per discussion, if this code to deal with .zip's is already somewhere
> we can borrow it with minimal dependencies, I'd rather do that.
> 

There's one that's part of frameworks/base, which is part of the gonk treadmill you want to avoid. My version is better anyway.

There's also libjar but it requires nsILocalFile.

> >+void
> >+AnimationFrame::readPngFrame()
> >+{
> 
> >+    buf = (char *)malloc(width * height * 3);
> 
> Are we guaranteed to always get packed r8g8b8 out of here?
> 

r5g6b5 gets converted to that, but I'll make sure other cases get handled right.

> >+    int format;
> >+    ANativeWindow const * const window = gNativeWindow.get();
> 
> Why do we know that gNativeWindow has been initialized by now?  Plz
> document.
> 

The code that starts this code is the code that initializes gNativeWindow. It seems obvious to me but I can document if that's not enough.

> >+    for (uint32_t i = 0; i < parts.size(); i++) {
> 
> This loop mostly seems OK, but a high-level description of the format
> somewhere would be helpful.  Here is fine.
> 

Documented.

> >+                glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,
> >+                             width, height, 0,
> >+                             GL_RGB, GL_UNSIGNED_BYTE, NULL);
> 
> This isn't necessary.  (The indentation is weird too.)
> 

Oh, that was some debugging cruft that I missed while cleaning this up. Removed.

> >+    for (uint32_t i = 0; i < parts.size(); i++) {
> 
> Is the animation supposed to loop indefinitely?  I don't see how that
> would happen here.
> 

A single part can loop indefinitely, but the animation will stop when we're out of animation parts.

> >+                glDrawArrays(GL_TRIANGLE_STRIP, 0, 4);
> 
> I don't see where a vertex array is enabled here, so I'm not sure how
> this is working.
> 

We enable both vertex arrays once before the loop and leave it.

> >+                if (tv2.tv_usec < framedelay)
> >+                    usleep(framedelay - tv2.tv_usec);
> 
> Hm, this sleep doesn't make sense to me.  I don't see us decoding in
> this inner loop.  If we try to SwapBuffers() too many times within the
> swap interval, then the SwapBuffers() call will block.
> 

ReadPngFrame does png decoding.

> >+            usleep(framedelay * part.pause);
> 
> Are the |pause|s supposed to be precise?  This may cause us to
> oversleep a frame boundary and drop frames.
> 

Nothing about this animation is very precise. Not sure how we would drop frames though.

> >diff --git a/b2g/app/Makefile.in b/b2g/app/Makefile.in
> 
> >+LIBS += -lpng -lGLESv2 -lEGL -lui -lhardware_legacy -lhardware -lcutils
> 
> Is it possible for us to statically link our own libpng clone into the
> b2g binary?  It would be nice to stay off one more gonk treadmill and
> have a better set of known bugs/optimizations etc.
> 

That would be a nice option, especially if we want to add APNG support. Our use of libpng is pretty simple though so I would want to look into this in a followup.

Comment 6

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> > NS_IMETHODIMP
> >+nsAppShell::Observe(nsISupports* aSubject,
> >+                    const char* aTopic,
> >+                    const PRUnichar* aData)
> >+{
> >+    if (!strcmp(aTopic, "browser-ui-startup-complete")) {
> >+        mEnableDraw = true;
> >+        NotifyEvent();
> >+    } else {
> >+        return nsBaseAppShell::Observe(aSubject, aTopic, aData);
> >+    }
> 
> Unless you have a specific reason not to, let's forward all
> notifications to base.  So move the else block out of the else.
> 

Turns out it crashes if you try this.

Comment 7

[Michael Wu [:mwu]]

Attachment: Incremental patch addressing review comments

For reference. Going to try switching to hwc for rendering.

Comment 8

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

(In reply to Michael Wu [:mwu] from comment #5)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> > >+struct local_file_header {
> > 
> > This name seems a bit too general ... this is a header for a file
> > contained in a zip?
> > 
> > Per discussion, if this code to deal with .zip's is already somewhere
> > we can borrow it with minimal dependencies, I'd rather do that.
> > 
> 
> There's one that's part of frameworks/base, which is part of the gonk
> treadmill you want to avoid. My version is better anyway.
> 

I was suggesting importing it not dynamically linking it, which doesn't get us on a treadmill.  But that's fine.  Moar descriptive names plz.

> There's also libjar but it requires nsILocalFile.
> 

Yeah, that thought never crossed my mind ;).

> > >+    for (uint32_t i = 0; i < parts.size(); i++) {
> > 
> > Is the animation supposed to loop indefinitely?  I don't see how that
> > would happen here.
> > 
> 
> A single part can loop indefinitely, but the animation will stop when we're
> out of animation parts.
> 

Gotcha.  (I hope that went into the format description ;).)

> > >+                if (tv2.tv_usec < framedelay)
> > >+                    usleep(framedelay - tv2.tv_usec);
> > 
> > Hm, this sleep doesn't make sense to me.  I don't see us decoding in
> > this inner loop.  If we try to SwapBuffers() too many times within the
> > swap interval, then the SwapBuffers() call will block.
> > 
> 
> ReadPngFrame does png decoding.
> 

Gotcha, I totally missed that call on first read.  What's the usleep() for then?  I still don't understand that.

Comment 9

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> > > >+                if (tv2.tv_usec < framedelay)
> > > >+                    usleep(framedelay - tv2.tv_usec);
> > > 
> > > Hm, this sleep doesn't make sense to me.  I don't see us decoding in
> > > this inner loop.  If we try to SwapBuffers() too many times within the
> > > swap interval, then the SwapBuffers() call will block.
> > > 
> > 
> > ReadPngFrame does png decoding.
> > 
> 
> Gotcha, I totally missed that call on first read.  What's the usleep() for
> then?  I still don't understand that.

This one is to ensure a minimum duration between frames so we play at or below the requested FPS. If the decoding took too long, we just don't usleep().

Comment 10

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

OK.  That's not the way I would write that, but I don't think it's particularly important for an initial landing.

Comment 11

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> >+struct local_file_header {
> 
> This name seems a bit too general ... this is a header for a file
> contained in a zip?
> 

The zip spec at http://www.pkware.com/documents/casestudies/APPNOTE.TXT calls these local file header records. The other structs are also based on the terms used in the spec. It's a bit generic - I can prefix these with zip_ if that helps.

Comment 12

[Michael Wu [:mwu]]

Attachment: Support android bootanimation.zip, v2

Comment 13

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

Comment on attachment 680273 [details] [diff] [review]
Support android bootanimation.zip, v2

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

>+class ZipReader {

Please add a brief comment explaining why we're reimplementing another
zip reader.

>+static void *
>+AnimationThread(void *)
>+{
>+    ZipReader reader;
>+    if (!reader.OpenArchive("/system/media/bootanimation.zip"))
>+        return nullptr;

{ } and add a LOGW()

>+    char *name = nullptr;
>+    const cdir_entry *entry = nullptr;
>+    char *descCopy = nullptr;
>+    while ((entry = reader.GetNextEntry(entry))) {
>+        name = reader.GetEntryName(entry, name);
>+        if (!strcmp(name, "desc.txt")) {
>+            descCopy = new char[entry->GetDataSize() + 1];
>+            break;
>+        }
>+    }
>+
>+    if (!descCopy) {
>+        free(name);

This doesn't make sense ... why don't we free |name| in the loop
above?  The memory ownership model isn't obvious here on first glance.
Try to avoid raw C char* here and use |string| instead, string manip
isn't going to be hot.

Please add a comment above this chunk of code briefly describing what
it's doing at a high level.  I think it's searching for the animation
spec file.

>+    /*
>+     * bootanimation.zip
>+     *
>+     * This is the boot animation file format that Android uses.
>+     * It's a zip file with a directories containing png frames
>+     * and a desc.zip that describes how they should be played.

desc.txt

>+     *
>+     * desc.txt contains two types of lines
>+     * 1. [width] [height] [fps]
>+     *    There is one of these lines per bootanimation.

What if width/height is smaller than the screen?  Is that legal?

>+    delete[] descCopy;

Let's try to kill this with |string| if we can.

>+        while (sRunAnimation && (!part.count || j++ < part.count)) {

>+                if (!frame.buf)
>+                    frame.ReadPngFrame();

{ }

>+                if (tv2.tv_usec < frameDelayUs)
>+                    usleep(frameDelayUs - tv2.tv_usec);

Document what this is trying to do.

>+                else
>+                    LOGW("Frame delay is %d us but decoding took %d us", frameDelayUs, tv2.tv_usec);

{ }

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

>+__attribute__ ((weak))
>+android::FramebufferNativeWindow* NativeWindow();
>+
>+__attribute__ ((weak))
>+void StopBootAnimation();
>+

Doc plz.

Does weak linking imply default visibility?  Not sure why we can get
away without MOZ_EXPORT here.  If it's by "accident" let's fix.

>diff --git a/b2g/app/Makefile.in b/b2g/app/Makefile.in

>+ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
>+CPPSRCS += BootAnimation.cpp
>+LIBS += -lpng -lGLESv2 -lEGL -lui -lhardware_legacy -lhardware -lcutils

Let's file a followup on trying to use our own libpng here.

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

>+#ifdef MOZ_WIDGET_GONK
>+  /* Called to start the boot animation */
>+  NativeWindow();

  (void) NativeWindow();

(|unused| would be better here but it's a libxul symbol.)

>diff --git a/widget/gonk/nsAppShell.cpp b/widget/gonk/nsAppShell.cpp

> NS_IMETHODIMP
>+nsAppShell::Observe(nsISupports* aSubject,
>+                    const char* aTopic,
>+                    const PRUnichar* aData)
>+{
>+    if (!strcmp(aTopic, "browser-ui-startup-complete")) {
>+        mEnableDraw = true;
>+        NotifyEvent();
>+    } else {
>+        return nsBaseAppShell::Observe(aSubject, aTopic, aData);
>+    }
>+    return NS_OK;

Let's do

  if (strcmp(aTopic, "browser-ui-startup-complete")) {
    return nsBaseAppShell::Observe(aSubject, aTopic, aData);
  }
  mEnableDraw = true;
  NotifyEvent();
  return NS_OK;

Your current impl is more "forwards compatible" for more notification
types, but we really ought not to be hacking around with those in
widget.

This looks good now.  Need to sort out the memory ownership issue
above, fix the nits, and we're ready to go.

Comment 14

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> Comment on attachment 680273 [details] [diff] [review]
> Support android bootanimation.zip, v2
> 
> >diff --git a/b2g/app/BootAnimation.cpp b/b2g/app/BootAnimation.cpp
> 
> >+class ZipReader {
> 
> Please add a brief comment explaining why we're reimplementing another
> zip reader.
> 

Will do, though I just noticed that glandium also implemented a custom zip reader for the android linker which might be a better choice. Its API looks pretty close to what I want. However, it says "No boundary checks are performed, which means hand-crafted malicious Zip archives can make the code fail in bad ways." whereas this zip reader is fairly paranoid.

> >diff --git a/b2g/app/BootAnimation.h b/b2g/app/BootAnimation.h
> 
> >+__attribute__ ((weak))
> >+android::FramebufferNativeWindow* NativeWindow();
> >+
> >+__attribute__ ((weak))
> >+void StopBootAnimation();
> >+
> 
> Doc plz.
> 
> Does weak linking imply default visibility?  Not sure why we can get
> away without MOZ_EXPORT here.  If it's by "accident" let's fix.
> 

Huh, I'm not sure why this works either. I'll dig deeper.

Comment 15

[Michael Wu [:mwu]]

(In reply to Michael Wu [:mwu] from comment #14)
> > >diff --git a/b2g/app/BootAnimation.h b/b2g/app/BootAnimation.h
> > 
> > >+__attribute__ ((weak))
> > >+android::FramebufferNativeWindow* NativeWindow();
> > >+
> > >+__attribute__ ((weak))
> > >+void StopBootAnimation();
> > >+
> > 
> > Doc plz.
> > 
> > Does weak linking imply default visibility?  Not sure why we can get
> > away without MOZ_EXPORT here.  If it's by "accident" let's fix.
> > 
> 
> Huh, I'm not sure why this works either. I'll dig deeper.

Nevermind, this is exactly how it's suppose to work. This header is only for users of these two symbols. The actual implementation in BootAnimation.cpp has the right visibility specified.

Comment 16

[Michael Wu [:mwu]]

Attachment: Support android bootanimation.zip, v3

Comment 17

[Michael Wu [:mwu]]

Attachment: Interdiff between v2 and v3 for reference

Comment 18

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

Comment on attachment 681305 [details] [diff] [review]
Support android bootanimation.zip, v3

Sorry for the review lag, I've been traveling and mostly without internet access.

Comment 19

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> Comment on attachment 681305 [details] [diff] [review]
> Support android bootanimation.zip, v3
> 
> Sorry for the review lag, I've been traveling and mostly without internet
> access.

No problem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d12d63253125

Comment 20

[Michael Wu [:mwu]]

Backed out due to red. Might need a toolchain update.

https://hg.mozilla.org/integration/mozilla-inbound/rev/14f42821a132

Comment 21

[Michael Wu [:mwu]]

Looks like we need external/libpng here.

Comment 22

[Michael Wu [:mwu]]

Turns out the shared library version of libpng is a caf only thing, so we'll have to mess with linking with our own libpng now instead of later.

Comment 23

[Michael Wu [:mwu]]

Attachment: Support android bootanimation.zip, v4

This version links against our static copy of libpng. The only important changes here are in media/libpng/mozpngconf.h and b2g/app/Makefile.in, so no interdiff was made. BootAnimation.cpp has one minor modification to use png_err instead of png_error since png_error isn't supported in our copy of libpng. mozpngconf.h was modified to add some features that the boot animation code relies on. Makefile.in was just modified to replace -lpng with our static copy of libpng and libz.

Comment 24

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

Comment on attachment 686213 [details] [diff] [review]
Support android bootanimation.zip, v4

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

>+/* necessary for boot animation code */
>+#ifdef MOZ_WIDGET_GONK
>+#define PNG_SEQUENTIAL_READ_SUPPORTED

Nit: |# define| and below.

Interdiff looks fine to me, but I'm not capable of reviewing the mozpngconf.h changes.  Phone-a-friend to an imagelib peer.

jlebar, your input is requested on just the mozpngconf.h change.

Comment 25

[Mozilla RelEng Bot]

Try run for 15fedd0dce06 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=15fedd0dce06
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-15fedd0dce06

Comment 26

[Justin Lebar (not reading bugmail)]

> Nit: |# define| and below.

The patch as it is matches the style in mozpngconf.h.

I have no idea what these defines do, but it looks like they don't change any behavior and just add additional PNG features?  If so, that's fine with me.

joe should probably sign off on this and not me, though.

Comment 27

[Justin Lebar (not reading bugmail)]

> Attachment #686213 [details] [diff] - Flags: review?([e-mail]) → review?([e-mail])

joe, this is r? specifically on the mozpngconf.h changes

Comment 28

[Michael Wu [:mwu]]

Yup this just turns on libpng functions that we use.

Comment 29

[Joe Drew (not getting mail)]

Comment on attachment 686213 [details] [diff] [review]
Support android bootanimation.zip, v4

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

I'm fine with this. In order to find out if we're going to be shooting ourselves in the foot, though, I'm asking Glenn for feedback; don't wait on him to check it in though.

Glenn, can you look at the changes in mozpngconf.h and tell us whether we should be avoiding any of that?

Comment 30

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab440f162ab8

Comment 31

[Glenn Randers-Pehrson]

I don't see a problem with the mozpngconf.h
changes.  Enabling sequential read will make the
library a lot larger but I guess we don't care
about footprint any more.  Enabling easy access
also increaes the footprint but not as much.
That could be avoided by using png_get_IHDR()
instead of retrieving the width and height
separately.

Elsewhere in the patch, this looks unsafe:   

    buf = (char *)malloc(width * height * 3);

because there seems to be no guarantee that the
input file is opaque and 8-bit; if there is
an alpha channel or if the samples are 16 bits,
the buffer will be overfilled.

    Use png_set_strip_alpha()

and

    png_set_scale_16()

unless you have somehow previously guaranteed
that the input is 8-bit and opaque.

Both of those would need to be enabled in
mozpngconf.h.  SCALE_16 already is, but you'd
have to add

#define PNG_READ_STRIP_ALPHA_SUPPORTED

Comment 32

[Justin Lebar (not reading bugmail)]

> Enabling sequential read will make the library a lot larger

Can you quantify this (even roughly)?

Comment 33

[Michael Wu [:mwu]]

Glenn, thanks for the review. I'll address these concerns in a follow up bug. These pngs are suppose to come in the right format so we shouldn't expect anything too weird but it's worth checking and rejecting pngs we don't support.

Comment 34

[Glenn Randers-Pehrson]

I tested turning SEQUENTIAL_READ on and off in the
"contrib/pngminim/preader" that comes with libpng.
The size increase was about 3K with a minimal build and
about 4K with more complete build.

Comment 35

[Michael Wu [:mwu]]

Requesting bb since this blocks bug 799714 which is already bb+.

Comment 36

[Glenn Randers-Pehrson]

The lines added to mozpngconf.h should probably be moved up a couple of lines so they fall inside the MOZ_PNG_READ block.  This is just a nitpick, though.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89d29a551f80
https://hg.mozilla.org/releases/mozilla-beta/rev/24ebf9eeb854

Comment 39

[Glenn Randers-Pehrson]

Comment on attachment 686213 [details] [diff] [review]
Support android bootanimation.zip, v4

See my feedback in comment #31, comment #32, comment #34, and comment #36.  Sorry didn't realize I had to "+feedback" until bugzilla pinged me today.

Comment 40

[leo.bugzilla.gecko]

I checked there is a FPS performance degradation on real device after this commit.

It looks like the NativeWindow is called very often, not only at booting.

More specifically, the pthread_create in NativeWindow function seems to cause the performance degradation.

Before these changes, the FPS reached over 50fps, but now it is under 30fps (enabling "Show frame per second" at developer menu).