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

RESOLVED FIXED in mozilla17

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Max Stepin, Assigned: Max Stepin)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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);
 }
Component: Untriaged → ImageLib
OS: Windows XP → All
Product: Firefox → Core
Hardware: x86 → All
(Assignee)

Comment 2

5 years ago
Created attachment 627657 [details] [diff] [review]
v01-759067.patch

proposed patch.
Assignee: nobody → newstop
Status: UNCONFIRMED → ASSIGNED
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

#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()
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
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.
(Assignee)

Comment 8

5 years ago
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...
(Assignee)

Comment 9

5 years ago
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.
Attachment #627657 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
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
Attachment #628245 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
Created attachment 629512 [details] [diff] [review]
v04-759067.patch

OK, this one works.
Attachment #628336 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #629512 - Flags: review?(joe)
Looks OK to me.
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.
Attachment #629512 - Flags: review?(joe)
Upstream libpng doesn't support APNG.
uh, duh.
Comment on attachment 629512 [details] [diff] [review]
v04-759067.patch

ok r=me. :) sorry!
Attachment #629512 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9b06abad5e

I updated apng.patch and MOZCHANGES as well.
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8a9b06abad5e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.