Closed Bug 759067 Opened 11 years ago Closed 10 years ago

(libpng) Progressive loader shouldn't always return APNG frames


(Core :: Graphics: ImageLib, defect)

Not set





(Reporter: newstop, Assigned: newstop)



(2 files, 3 obsolete files)

Attached image Mageia2-GIF-APNG.png
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120523114940

Steps to reproduce:

Lately, lots of Linux distros ship with apng-patched libpng (that comes from Firefox), but many apps and libraries (like gdk-pixbuf) are not APNG-aware and the normal thing for them is to load only the first frame from APNG. 

But that's not always happening. 

When such apps use progressive mode for loading PNGs, apng-patched libpng simply returns all rows for all frames, but since the app doesn't know when one frame ends and another begins, all rows pile up on top of each other, creating visual mess. See screenshot.
I propose to add "APNG-aware app" flag, and to set it here:

 void PNGAPI
 png_set_progressive_frame_fn(png_structp png_ptr,
    png_progressive_frame_ptr frame_info_fn,
    png_progressive_frame_ptr frame_end_fn)
    png_ptr->frame_info_fn = frame_info_fn;
    png_ptr->frame_end_fn = frame_end_fn;
+   png_ptr->apng_flags |= PNG_APNG_APP;

Using this function is the only way for application to know when a current frame ends. If it's not called, then PNG_APNG_APP flag is not set, and png_ptr->row_fn() callback function should only return rows from the first frame:

 void /* PRIVATE */
 png_push_have_row(png_structp png_ptr, png_bytep row)
+   if (png_ptr->apng_flags & PNG_APNG_APP || png_ptr->num_frames_read == 0)
    if (png_ptr->row_fn != NULL)
       (*(png_ptr->row_fn))(png_ptr, row, png_ptr->row_number,
Component: Untriaged → ImageLib
OS: Windows XP → All
Product: Firefox → Core
Hardware: x86 → All
Attached patch v01-759067.patch (obsolete) — Splinter Review
proposed patch.
Assignee: nobody → newstop
Ever confirmed: true
QA Contact: untriaged → imagelib
The patch doesn't make sequential readers ignore the APNG chunks.  Also it does not prevent the library from decoding the APNG chunks for progressive readers; the patch only makes the library ignore the data after it's been decoded.  The patch does solve the problem, but it would be more efficient to ignore the APNG chunks entirely.  This can be accomplished by revising the block of code beginning at line 341 in pngread.c to read

      else if ((png_ptr->apng_flags & PNG_APNG_APP) && chunk_name == png_acTL)
         png_handle_acTL(png_ptr, info_ptr, length);

      else if ((png_ptr->apng_flags & PNG_APNG_APP) && chunk_name == png_fcTL)
         png_handle_fcTL(png_ptr, info_ptr, length);

      else if ((png_ptr->apng_flags & PNG_APNG_APP) && chunk_name == png_fdAT)
         png_handle_fdAT(png_ptr, info_ptr, length);

With this change, the new test in png_push_have_row in the v01 patch wouldn't
be necessary.

I would rather see the PNG_APNG_APP flag set in a separate function (e.g.,
png_set_enable_apng(png_ptr) ) so it could be used by both progressive and sequential
decoders alike.  Note that such a function could coexist with this patch that enables it
within png_set_progressive_read_fn()
Sequential loader usually works by invoking png_read_image() for every frame.
Apng-unaware apps only invoke it once, so there shouldn't be a problem. 

If sequential loader works by invoking png_read_rows(), then the app stops after enough rows are read for one whole frame. Additional frames will be processed by png_read_end() and ignored.

I'm not sure we should touch the sequential code, since it's not broken...

Except maybe we could remove apng stuff from png_read_end()
There is still a performance issue.  With my suggested change in comment #3, the decoder simply skips the APNG chunks. With the current code and with the v01 patch it might decompress all of the APNG chunks just in case they will be needed later on, with either the progressive or sequential decoding.  I don't know whether that means the code is "broken" or not.
Your suggestion about line 341 in pngread.c - it's inside png_read_info()

png_read_info() only reads small stuff before the first IDAT. It's not decoding any frames.

Overall, it's not much of a performance issue:
png_handle_acTL() is processing 8 bytes of useful info inside acTL.
png_handle_fcTL() is processing 22 bytes of useful info inside fcTL.
That's very little work.

png_handle_fdAT() does even less, its job is to skip out-of-place fdAT:

void /* PRIVATE */
png_handle_fdAT(png_structp png_ptr, png_infop info_ptr, png_uint_32 length)
    png_ensure_sequence_number(png_ptr, length);

    /* This function is only called from png_read_end(), png_read_info(),
    * and png_push_read_chunk() which means that:
    * - the user doesn't want to read this frame
    * - or this is an out-of-place fdAT
    * in either case it is safe to ignore the chunk with a warning */
    png_warning(png_ptr, "ignoring fdAT chunk");
    png_crc_finish(png_ptr, length - 4);

The only way to hit costly inflate() in pngread.c is to specifically request rows. And APNG-unaware apps will not request extra rows, only enough to get the first image.
Right, I forgot that the actual work of decoding fdAT chunks happens in png_read_row(), not in png_handle_fdAT(), so my suggestion wouldn't have any discernable effect on performance.  The v01 patch looks OK.
The only thing we could do in sequential loader is remove apng stuff from png_read_end(). It looks useless.

But you're right about progressive loader, I think we might improve the performance by moving that flag check from row level to chunks level.

I'm thinking to move it to line 219 at pngpread.c
Let me test it...
Attached patch v02-759067.patch (obsolete) — Splinter Review
This version addresses performance issue, by checking "apng-aware app" flag on chunks level, it avoids apng frames decoding if application does not support apng. Also removes apng stuff from png_read_end() as it's serves no useful purpose there.
Attachment #627657 - Attachment is obsolete: true
Attached patch v03-759067.patch (obsolete) — Splinter Review
The same as v02, but also adds checks suggested by Glenn at lines 663 and 673 in pngpread.c
Attachment #628245 - Attachment is obsolete: true
After further testing, v03 appears to be incorrect. 

When we read acTL at line 663 in pngpread.c it's too early to check for the flag.
At that point we didn't have a chance to set the flag yet.
Attached patch v04-759067.patchSplinter Review
OK, this one works.
Attachment #628336 - Attachment is obsolete: true
Attachment #629512 - Flags: review?(joe)
Looks OK to me.
Comment on attachment 629512 [details] [diff] [review]

Review of attachment 629512 [details] [diff] [review]:

Max, can you submit this to Glenn upstream for inclusion? We can then pick it up when we upgrade libpng.
Attachment #629512 - Flags: review?(joe)
Upstream libpng doesn't support APNG.
uh, duh.
Comment on attachment 629512 [details] [diff] [review]

ok r=me. :) sorry!
Attachment #629512 - Flags: review+

I updated apng.patch and MOZCHANGES as well.
Flags: in-testsuite-
Keywords: checkin-needed
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.