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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Max Stepin]

Attachment: 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]

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]

Attachment: v01-759067.patch

proposed patch.

Comment 3

[Glenn Randers-Pehrson]

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]

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]

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]

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]

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]

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]

Attachment: 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]

Attachment: 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]

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]

Attachment: v04-759067.patch

OK, this one works.

Comment 13

[Glenn Randers-Pehrson]

Looks OK to me.

Comment 14

[Joe Drew (not getting mail)]

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]]

Upstream libpng doesn't support APNG.

Comment 16

[Joe Drew (not getting mail)]

uh, duh.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 629512 [details] [diff] [review]
v04-759067.patch

ok r=me. :) sorry!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9b06abad5e

I updated apng.patch and MOZCHANGES as well.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a9b06abad5e