Last Comment Bug 759067 - (libpng) Progressive loader shouldn't always return APNG frames
: (libpng) Progressive loader shouldn't always return APNG frames
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Max Stepin
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-28 03:24 PDT by Max Stepin
Modified: 2012-07-17 02:11 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mageia2-GIF-APNG.png (303.32 KB, image/png)
2012-05-28 03:24 PDT, Max Stepin
no flags Details
v01-759067.patch (2.05 KB, patch)
2012-05-28 03:41 PDT, Max Stepin
no flags Details | Diff | Review
v02-759067.patch (3.09 KB, patch)
2012-05-29 23:14 PDT, Max Stepin
no flags Details | Diff | Review
v03-759067.patch (3.91 KB, patch)
2012-05-30 07:44 PDT, Max Stepin
no flags Details | Diff | Review
v04-759067.patch (3.29 KB, patch)
2012-06-02 12:42 PDT, Max Stepin
joe: review+
Details | Diff | Review

Description Max Stepin 2012-05-28 03:24:56 PDT
Created attachment 627651 [details]
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.
Comment 1 Max Stepin 2012-05-28 03:34:43 PDT
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)
 {
+#ifdef PNG_READ_APNG_SUPPORTED
+   if (png_ptr->apng_flags & PNG_APNG_APP || png_ptr->num_frames_read == 0)
+#endif
    if (png_ptr->row_fn != NULL)
       (*(png_ptr->row_fn))(png_ptr, row, png_ptr->row_number,
          (int)png_ptr->pass);
 }
Comment 2 Max Stepin 2012-05-28 03:41:02 PDT
Created attachment 627657 [details] [diff] [review]
v01-759067.patch

proposed patch.
Comment 3 Glenn Randers-Pehrson 2012-05-28 08:25:26 PDT
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

#ifdef PNG_READ_APNG_SUPPORTED
      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);
#endif

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()
Comment 4 Max Stepin 2012-05-28 13:29:36 PDT
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()
Comment 5 Glenn Randers-Pehrson 2012-05-28 14:50:53 PDT
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.
Comment 6 Max Stepin 2012-05-28 22:09:43 PDT
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);
    PNG_UNUSED(info_ptr)
}


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.
Comment 7 Glenn Randers-Pehrson 2012-05-29 03:17:45 PDT
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.
Comment 8 Max Stepin 2012-05-29 04:17:33 PDT
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...
Comment 9 Max Stepin 2012-05-29 23:14:36 PDT
Created attachment 628245 [details] [diff] [review]
v02-759067.patch

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.
Comment 10 Max Stepin 2012-05-30 07:44:35 PDT
Created attachment 628336 [details] [diff] [review]
v03-759067.patch

The same as v02, but also adds checks suggested by Glenn at lines 663 and 673 in pngpread.c
Comment 11 Max Stepin 2012-05-30 22:32:52 PDT
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.
Comment 12 Max Stepin 2012-06-02 12:42:20 PDT
Created attachment 629512 [details] [diff] [review]
v04-759067.patch

OK, this one works.
Comment 13 Glenn Randers-Pehrson 2012-07-06 08:58:51 PDT
Looks OK to me.
Comment 14 Joe Drew (not getting mail) 2012-07-16 12:31:02 PDT
Comment on attachment 629512 [details] [diff] [review]
v04-759067.patch

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.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-07-16 12:36:28 PDT
Upstream libpng doesn't support APNG.
Comment 16 Joe Drew (not getting mail) 2012-07-16 12:39:45 PDT
uh, duh.
Comment 17 Joe Drew (not getting mail) 2012-07-16 12:40:08 PDT
Comment on attachment 629512 [details] [diff] [review]
v04-759067.patch

ok r=me. :) sorry!
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-07-16 17:42:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9b06abad5e

I updated apng.patch and MOZCHANGES as well.
Comment 19 Ed Morley [:emorley] 2012-07-17 02:11:03 PDT
https://hg.mozilla.org/mozilla-central/rev/8a9b06abad5e

Note You need to log in before you can comment on or make changes to this bug.