420416 - Artifacts in some APNG frames

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P2)

Description

[Andrew Smith]

Please see attachment. My trunk fireox build plays the animation but some frames have really ugly random-coloured areas in 3/4 of the surface.

I looked at each frame separately and confirmed they are ok.

Comment 1

[Andrew Smith]

It won't attach, please get the image here: http://littlesvr.ca/apng/images/Sun.png

I did not do a cvs update. I will if others can't reproduce.

Comment 2

[Matthias Versen [:Matti]]

I can confirm that on win32 with SM 2008022601 

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This may be similar to bug 419277.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hm, no, not related to that.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Stuart, can you take this?

Comment 6

[Stuart Parmenter]

alfred: can you take a look at this?

Comment 7

[Joe Drew (not getting mail)]

Alfred, unless you've got objections, I'll take this one.

Comment 8

[Alfred Kayser]

Go ahead. To me, it looks like a decoding problem in libpng itself.

Comment 9

[Andrew Smith]

Attachment: simpler example

From Max, who created the original Sun image that shows with the artifacts:

I found that as long as the buggy frame contain solid-black line, it helps the decoder, and everything below that line will be displayed correctly.

So I cleared the very first line in some of the "sun" frames.

I think there are some combinations of very dark pixels that could trigger the bug.

I created another example, it very simple, just two frames. It's hard to notice the dark vertical line at the second frame, so you'd have to increase the brightness. That vertical line is the problem, it triggers the bug.

Comment 10

[Joe Drew (not getting mail)]

Attachment: reset the previous row when we finish decoding IDAT

This problem was caused by PNG filtering not being reset between frame 0 (i.e., the initial IDAT) and subsequent fdAT frames. The PNG spec specifies that if you're using a filter like subtract (that uses the previous row) on the initial row, the "previous" row is defined as all-zeroes. We were relying on unsetting the row-initialized bit to do that, but in the PNG push-read codepath, it doesn't happen. Further, more code is necessary to properly handle that if we ever do want to reinitialize after the initial IDAT, because just unsetting PNG_FLAG_ROW_INIT won't cause the png pointer structure and info structures to be freed, just reallocated, leading to a leak.

So, instead, we perform the minimal work that is necessary, which is to zero out the previous row before proceeding to another fdAT. This fixes the two attached testcases.

I'm not 100% convinced this is a complete or 100% pretty fix, so some extra eyes on it would be nice. I think in the ideal world we'd just set the row-initialized bit to false and subsequent fdATs would automatically free and reinit, but that seems a bit intrusive.

Comment 11

[Glenn Randers-Pehrson]

Yes, the patch seems to fix a leak of png_ptr->prev_row.
In fact, regular libpng (without the APNG patch) also allows png_read_start_row() to be called multiple times, controlled by the PNG_FLAG_ROW_INIT flag, and if this happens the same leak will occur.

Although I understand how this patch fixes the leak, I'm not quite sure why it fixes the filtering problem, because png_read_start_row() contains the same "memset" line to clear the png_ptr->prev_row array, after it erroneously allocates another png_ptr->prev_row array.

I am planning to plug the leak in libpng-1.2.26.  I wonder if that would also fix this filtering problem.

Comment 12

[Andrew Smith]

Comment on attachment 310313 [details] [diff] [review]
reset the previous row when we finish decoding IDAT

The patch fixes the problem for me, though I'm also not sure why.

Comment 13

[Joe Drew (not getting mail)]

(From what I can tell,) The way Firefox uses libpng (the push_read codepath), png_read_start_row is only ever called from png_read_update_info. It's not called in any of the IDAT code pngpread.c code.

Glenn, if libpng-1.2.26 also calls png_read_update_info e.g. in png_push_read_IDAT, then that'll also fix the bug. Combined with the leak fix, we'll be fine. (That's the "bit intrusive" I alluded to above.)

Comment 14

[Glenn Randers-Pehrson]

Attachment: plug leak in libpng/pngrutil.c

This plugs the leak of prev_row in pngrutil.c and changes the handling of big_row_buf and row_buf, so that the size of these arrays (and prev_row) is set to the size actually needed for each frame.

Comment 15

[Alfred Kayser]

Some comments:
1. Instead of if (ptr) png_free(...); png_malloc(...) a realloc(...) would be nicer (but there is no png_realloc). So this is something for a separate bug.
2. Move the checking of the ROW_INIT flag into png_read_start_row itself (so that it becomes 'idempotent' - so that it can be called multiple times without harm...).

Could you combine the two patches into one for easier review?

Comment 16

[Glenn Randers-Pehrson]

We could of course combine them, but as I said earlier I'm wondering if this fix alone will stop the artifacting, and if not, whether it helps us figure out what is going on by removing the double-malloc issue.  My patch is really only for testing since it will also be done in libpng-1.2.26 which is due out in about 2 weeks.

Comment 17

[Glenn Randers-Pehrson]

> 1. Instead of if (ptr) png_free(...); png_malloc(...) a realloc(...) would be
> nicer (but there is no png_realloc). So this is something for a separate bug.

It would be a libpng bug (enhancement) not a mozilla bug.  We can consider it on the png-mng-implement list.  I'll pass along your comments to that list.

> 2. Move the checking of the ROW_INIT flag into png_read_start_row itself (so
> that it becomes 'idempotent' - so that it can be called multiple times without
> harm...).

Again, a libpng enhancement.   We have to take care of the times (don't happen in mozilla) when the row_buf size increases due to the transformations being requested.

Comment 18

[Glenn Randers-Pehrson]

(In reply to comment #13)

> Glenn, if libpng-1.2.26 also calls png_read_update_info e.g. in
> png_push_read_IDAT, then that'll also fix the bug. Combined with the leak fix,
> we'll be fine. (That's the "bit intrusive" I alluded to above.)

Libpng does not call png_read_update_info().  It is the application's
responsibility to call it, after setting any transforms such as gray_to_rgb
that increase the size of the row buffer (libpr0n seems to be doing it
properly).

Comment 19

[Joe Drew (not getting mail)]

(In reply to comment #16)
> We could of course combine them, but as I said earlier I'm wondering if this
> fix alone will stop the artifacting

It won't -- if png_read_start_row was called before handling fdAT chunks, there would have been no artifacting in the first place, but there would have been a leak. This will fix that theoretical leak, but won't fix the artifacting.

(In reply to comment #18)
> Libpng does not call png_read_update_info().

I'm sorry, I meant png_read_start_row().

Comment 20

[Glenn Randers-Pehrson]

We clear the PNG_FLAG_ROW_INIT bit in png_read_reset() (if we don't apply your patch), which should cause png_start_read_row() to be called from png_read_row(),
so there should have been no artifacting.  I am guessing that it might be some side effect of the double-malloc of prev_row.  A way to find out is to apply my patch and see what happens.  Sorry I can't do that myself because of out-of-date pango libraries on the platform I'm using.

Comment 21

[Joe Drew (not getting mail)]

(In reply to comment #20)
> We clear the PNG_FLAG_ROW_INIT bit in png_read_reset() (if we don't apply your
> patch), which should cause png_start_read_row() to be called from
> png_read_row(),

Sure -- but png_read_row() isn't called in the path that Gecko uses. Instead, png_process_some_data() calls png_push_read_IDAT() which calls png_process_IDAT_data() which calls png_push_process_row(). There is no call to png_start_read_row() anywhere. The only time it's called (verified by setting a breakpoint) is when nsPNGDecoder calls png_read_update_info(). Except for that call, the row wouldn't be initialized at all.

Comment 22

[Glenn Randers-Pehrson]

Maybe nsPNGDecoder should call png_read_update_info() or png_read_start_row() whenever it starts a new frame.  Libpng isn't designed internally to handle multiple images.

Comment 23

[Joe Drew (not getting mail)]

But should it be necessary to call png_read_update_info() in order for the push code to work at all? There's no call to png_start_read_row() in it anywhere.

Comment 24

[Glenn Randers-Pehrson]

There is in my copy:

void PNGAPI
png_read_update_info(png_structp png_ptr, png_infop info_ptr)
{
   png_debug(1, "in png_read_update_info\n");
   if(png_ptr == NULL) return;
   if (!(png_ptr->flags & PNG_FLAG_ROW_INIT))
      png_read_start_row(png_ptr);

There should be a call to png_read_update_info() whenever the frame
dimensions change.  There *must* be one if the frame dimensions increase,
which I believe doesn't happen in the current version of APNG.

Comment 25

[Joe Drew (not getting mail)]

I meant there's no call to png_read_start_row() in the push code anywhere. If we didn't call png_read_update_info, nothing would work.

Comment 26

[Glenn Randers-Pehrson]

That's true.  The libpng documentation about the info_callback() for the
progressive reader says:

       For now, you _must_ call
       either png_start_read_image() or
       png_read_update_info() after all the
       transformations are set (even if you don't set
       any).

You could infer from that that nothing would work.

Comment 27

[Joe Drew (not getting mail)]

Yes, and that's what we do in our info_callback. However, and I guess this is specific to APNG decoding, the info_callback isn't called for subsequent frames.

Comment 28

[Glenn Randers-Pehrson]

Right.  we could put a call to png_read_start_image() in the frame reset code, where you changed the flag clearing instruction to the memset().  I can set up a combined patch that does that and fixes the memory leak.

Comment 29

[Glenn Randers-Pehrson]

Attachment: combined patch for leak and artifacts

Comment 30

[Joe Drew (not getting mail)]

Comment on attachment 310549 [details] [diff] [review]
combined patch for leak and artifacts

>-   if (png_ptr->big_row_buf == NULL)
>-      png_ptr->big_row_buf = (png_bytep)png_malloc(png_ptr, row_bytes+64);
>-   if (png_ptr->row_buf == NULL)
>-      png_ptr->row_buf = png_ptr->big_row_buf+32;
>+
>+   if (png_ptr->big_row_buf)
>+      png_free(png_ptr,png_ptr->big_row_buf);
>+   png_ptr->big_row_buf = (png_bytep)png_malloc(png_ptr, row_bytes+64);
>+   png_ptr->row_buf = png_ptr->big_row_buf+32;

Some brief checks haven't told me whether we can do a check with png_ptr->rowbytes to see if this reallocation actually needs to be done. It would be nice, since at least in the APNG case it's never going to be a different size, to avoid this possible source of fragmentation.

>     png_ptr->flags &= ~PNG_FLAG_ROW_INIT;

I don't think we need to unset this flag, since png_read_start_row() immediately sets it. I guess it makes sense from a semantic correctness POV, though.

>+    png_read_restart_row(png_ptr);

This needs to be png_read_start_row(png_ptr) - there is no png_read_restart_row().

Comment 31

[Glenn Randers-Pehrson]

(In reply to comment #30)
> (From update of attachment 310549 [details] [diff] [review])
> >-   if (png_ptr->big_row_buf == NULL)
> >-      png_ptr->big_row_buf = (png_bytep)png_malloc(png_ptr, row_bytes+64);
> >-   if (png_ptr->row_buf == NULL)
> >-      png_ptr->row_buf = png_ptr->big_row_buf+32;
> >+
> >+   if (png_ptr->big_row_buf)
> >+      png_free(png_ptr,png_ptr->big_row_buf);
> >+   png_ptr->big_row_buf = (png_bytep)png_malloc(png_ptr, row_bytes+64);
> >+   png_ptr->row_buf = png_ptr->big_row_buf+32;
> 
> Some brief checks haven't told me whether we can do a check with
> png_ptr->rowbytes to see if this reallocation actually needs to be done.

I'm pretty sure we can.

> It
> would be nice, since at least in the APNG case it's never going to be a
> different size, to avoid this possible source of fragmentation.

Never going to be a larger size.  It might be smaller.

> 
> >     png_ptr->flags &= ~PNG_FLAG_ROW_INIT;
> 
> I don't think we need to unset this flag, since png_read_start_row()
> immediately sets it. I guess it makes sense from a semantic correctness POV,
> though.

I left it in in case we decide later to use it in png_read_start_row().
I'll take it out with #if 0 and a comment.

> 
> >+    png_read_restart_row(png_ptr);
> 
> This needs to be png_read_start_row(png_ptr) - there is no
> png_read_restart_row().

phooey.  Thanks.

Comment 32

[Joe Drew (not getting mail)]

(In reply to comment #31)
> > It
> > would be nice, since at least in the APNG case it's never going to be a
> > different size, to avoid this possible source of fragmentation.
> 
> Never going to be a larger size.  It might be smaller.

Doh, duh.

Anyways, will you be able to integrate the rowbuf size checks too? Or shall I?

Comment 33

[Glenn Randers-Pehrson]

Attachment: combined patch (v1)

Revised per comment #30 and comment #31

Comment 34

[Glenn Randers-Pehrson]

Attachment: combined patch (v2)

Whoops, I should not have made the size-checking available only when APNG is supported.

Comment 35

[Alfred Kayser]

Looking good!
May be only free/malloc the row buffer when it needs to be larger?

Comment 36

[Joe Drew (not getting mail)]

Unfortunately, the current patch doesn't compile, and even after making it compile there are problems with old_row_bytes being uninitialized. Fixing those led me to realize that I don't think we can actually reliably detect when the number of bytes in the row has changed, since rowbytes is only used to allocate the png_ptr's big_row_buf, not prev_row, and png_ptr->rowbytes is updated prior to png_read_start_row() (so we can't use its value).

There are three options:
 - Look at irowbytes; doesn't seem to be a good idea, because the mapping from irowbytes to rowbytes doesn't seem to be simple and reliable.
 - Implement png_realloc(). This doesn't seem like a quick-and-easy solution, but it's probably the right one.
 - Forget it, and just reallocate on every frame. This is the way I'm leaning.

Thoughts?

Comment 37

[Glenn Randers-Pehrson]

Attachment: combined patch (v3)

Adds old_big_row_buf_size and old_prev_row_size elements to png->ptr and uses those instead of trying to use existing variables that we can't be sure of.  Only reallocates if the size increases.

Comment 38

[Glenn Randers-Pehrson]

(In reply to comment #36)
> Unfortunately, the current patch doesn't compile, and even after making it
> compile there are problems with old_row_bytes being uninitialized. Fixing those
> led me to realize that I don't think we can actually reliably detect when the
> number of bytes in the row has changed, since rowbytes is only used to allocate
> the png_ptr's big_row_buf, not prev_row, and png_ptr->rowbytes is updated prior
> to png_read_start_row() (so we can't use its value).
> 
> There are three options:
>  - Look at irowbytes; doesn't seem to be a good idea, because the mapping from
> irowbytes to rowbytes doesn't seem to be simple and reliable.
>  - Implement png_realloc(). This doesn't seem like a quick-and-easy solution,
> but it's probably the right one.
>  - Forget it, and just reallocate on every frame. This is the way I'm leaning.
> 
> Thoughts?

A fourth option, as in patch v3, is to explicitly store the old_big_row_buf_size and old_prev_row_size in the png_struct.  This should be simple and reliable, but does add two 4-byte members to the png_struct.

We are looking at png_realloc() for libpng, but it seems that we won't be
able to put that in until libpng-1.4.x because an ABI change seems unavoidable.

Comment 39

[Joe Drew (not getting mail)]

(In reply to comment #37)
> Created an attachment (id=310652) [details]
> combined patch (v3)
> 
> Adds old_big_row_buf_size and old_prev_row_size elements to png->ptr and uses
> those instead of trying to use existing variables that we can't be sure of. 
> Only reallocates if the size increases.

This patch looks great. I've compiled it and done some stepping through the code, and it all works as it should. Thanks, Glenn!

Comment 40

[Joe Drew (not getting mail)]

It seems it's not easy to automate the testing of animated graphics, so here's a Litmus-ish testcase:

Testgroup: No clue
Subgroup: No clue
Testcase: APNG artifacts
Product: Gecko/Firefox
Branch: 1.9/3
Steps to perform: Load https://bugzilla.mozilla.org/attachment.cgi?id=309081. 
Expected Results: This is an APNG with two frames. The text "Frame 1" and "Frame 2" should be clearly visible on a black background. There should be no stretching or smearing of any text on any frames.
Regression Bug ID#: 420416

Comment 41

[Glenn Randers-Pehrson]

Looking at it with IE6 SP2 and with FF-1.5.0.12 I just see one static frame, which I expected.  The text reads "TEST 1" not "Frame 1", though.

Comment 42

[Joe Drew (not getting mail)]

Ahem, yes. Apparently I can't read. :)

Testgroup: No clue
Subgroup: No clue
Testcase: APNG artifacts
Product: Gecko/Firefox
Branch: 1.9/3
Steps to perform: Load https://bugzilla.mozilla.org/attachment.cgi?id=309081. 
Expected Results: This is an APNG with two frames. The text "TEST 1" and
"TEST 2" should be clearly visible on a black background. There should be no
stretching or smearing of any text on any frames.
Regression Bug ID#: 420416

Comment 43

[Andrew Smith]

With the v3 patch the two following sampls are corrupted (they weren't before, though maybe that's a different issue - see bug 423798).

https://bugzilla.mozilla.org/attachment.cgi?id=310412

Comment 44

[Glenn Randers-Pehrson]

I can't see how the patch would cause that.  The only thing I can suggest is to change "#if 0  /* ... */" to "#if 1 /* ... */", to enable clearing the PNG_FLAG_ROW_INIT bit.  But just looking at the code, I don't think that is likely to make any difference.

Comment 45

[Glenn Randers-Pehrson]

(In reply to comment #43)
> With the v3 patch the two following sampls are corrupted (they weren't before,
> though maybe that's a different issue - see bug 423798).

The transparent holes in the rendered frames of those images is correct behavior of FF.  The user was expecting something else due to misinterpretation of the spec.  This doesn't seem to be related to any of these patches; I see the same behavior with unpatched FF-3.0b4.

Comment 46

[Stephen Donner [:stephend] Not actively reading bugmail]

in-litmus+

https://litmus.mozilla.org/show_test.cgi?id=5213

(Just for JOEDREW!) :-)

Comment 47

[Glenn Randers-Pehrson]

Attachment: combined patch (v4)

Simplified sequential vs progressive resetting to make actual code
agree with the comment "same as png_read_reset() but for the progressive
reader".

Removed two "#if 0" blocks.

Comment 48

[Andrew Smith]

Attachment: screenshot of problem with v3 patch

(In reply to comment #45)
> (In reply to comment #43)
> > With the v3 patch the two following sampls are corrupted (they weren't before,
> > though maybe that's a different issue - see bug 423798).
> 
> The transparent holes in the rendered frames of those images is correct
> behavior of FF.  The user was expecting something else due to misinterpretation
> of the spec.  This doesn't seem to be related to any of these patches; I see
> the same behavior with unpatched FF-3.0b4.
> 

They used to be transparent before the v3 patch. After the patch some of the holes are not transparent but have what looks like uninitialized junk in it.

This is with a build from yesterday from CVS, on linux.

Comment 49

[Andrew Smith]

Changing the #if 0 to #if 1 doesn't seem to make any difference.

I have no reason to believe the patch is causing the problem. It could just be exposing a bug that was there all along. Or it could even be that I'm doing something wrong, if noone else sees it.

Comment 50

[Glenn Randers-Pehrson]

The "GetHeight()" bug from bug #411852 may be interfering.  Please apply the bugfix from there, plus the v4 patch from here, and see what happens.

Comment 51

[Alfred Kayser]

One nit: instead of adding two new members:
  png_uint_32 old_big_row_buf_size, old_prev_row_size;
one is enough:
  png_uint_32 old_row_bytes;

So:
  if(row_bytes + 64 > png_ptr->old_big_row_buf_size)
becomes:
  if(row_bytes > png_ptr->old_row_bytes)

etc...

Comment 52

[Glenn Randers-Pehrson]

row_bytes and png_ptr->row_bytes are not necessarily the same, so it's probably safer to use two members.  png_ptr->row_bytes is the length from the IHDR chunk, while the other is the length after accounting for various transformations such as gray_to_rgb.  That being said, I can't actually think of a scenario using just one member that would cause an overflow.

Comment 53

[Andrew Smith]

The GetHeight() patch doesn't fix it for me. Am I the only one who sees the problem?

Comment 54

[Glenn Randers-Pehrson]

(In reply to comment #48)
> After the patch some of the
> holes are not transparent but have what looks like uninitialized junk in it.

Looking at your screenshot, it seems that the holes are transparent (where the "1" was in the second frame) but the "2" image (third frame) is corrupted.

Comment 55

[Glenn Randers-Pehrson]

(In reply to comment #53)

Did you try the v4 patch?

Comment 56

[Pete]

Here there is a render bug here somewhere and there different between FF3Beta4 and FF3 Beta 4 and there both ugly

These animations I made the first ones are just unoptimized and show correctly in FF3 Beta3 but the break up in FF3 Beta4

Just plunk these onto a dark page and blow them up really big Full screen each and look at the edges..

Layer mask type:

[img]http://img186.imageshack.us/img186/7554/vaccum40lmco5.png[/img][img]http://img186.imageshack.us/img186/5320/vaccum80lmuy7.png[/img][img]http://img186.imageshack.us/img186/8576/vaccum96lmlh6.png[/img]


These next there are optimized and they don't display correctly in ether Beta 3 or 4.. If you stick them on a black background web page and blow them up full screen you can see along the edges that the transparent edge part is being added to the previous frame instead of being cleared. It ends up displaying a ugly white line near the edges..


Chopped with Layer mask type:

[img]http://img186.imageshack.us/img186/5748/vaccum40chgg4.png[/img][img]http://img186.imageshack.us/img186/9742/vaccum80chqx7.png[/img][img]http://img186.imageshack.us/img186/7338/vaccum96chhd5.png[/img]

These are the ones we need to get working correctly cause they are the optimized versions.. The smaller file type.

Try them in XnView with a black background to and see what they are suppose to look like.

Both these problems do no show up on XnView on black background don't matter how bit you make it.. 

Comment 57

[Andrew Smith]

The v4 patch won't build for me at all. I get this when linking:

a ../../staticlib/libthebes.a ../../staticlib/libjsj.a  -Wl,--no-whole-archive -L../../dist/lib -lsqlite3 -L../../dist/bin -L../../dist/lib  -L../../dist/bin -L../../dist/lib -L../../jpeg -lmozjpeg -L../../modules/libimg/png -lmozpng -L../../dist/bin -lmozlcms -L../../dist/bin -lmozjs -L../../dist/bin -L../../dist/lib -lcrmf -lsmime3 -lssl3 -lnss3 -lnssutil3 -lsoftokn3  -L../../modules/zlib/src -lmozz -lpangocairo-1.0 -lcairo -lpangoft2-1.0 -lpango-1.0 -lgobject-2.0 -lgmodule-2.0 -ldl -lglib-2.0   ../../gfx/cairo/cairo/src/libmozcairo.a ../../gfx/cairo/libpixman/src/libmozlibpixman.a   -lXrender -lfreetype -lfontconfig -L../../dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl  -lX11  -lXft -lXrender -lfontconfig -lfreetype -lz -lX11   -lgtk-x11-2.0 -latk-1.0 -lgdk-x11-2.0 -lgdk_pixbuf-2.0 -lm -lpangocairo-1.0 -lpango-1.0 -lcairo -lgmodule-2.0 -ldl -lgobject-2.0 -lglib-2.0   -lXt -lgthread-2.0 -lfreetype -lz ../../memory/jemalloc/libjemalloc.a -ldl -lm     
../../modules/libimg/png/libmozpng.a(pngread.o): In function `png_read_frame_head':
pngread.c:(.text+0x349): undefined reference to `png_read_reset'
collect2: ld a retourné 1 code d'état d'exécution
gmake[4]: *** [libxul.so] Erreur 1
gmake[4]: quittant le répertoire « /home/andrew/prog/mozilla-trees/firefox-canvas/mozilla/ff-opt-static/toolkit/library »
gmake[3]: *** [libs_tier_toolkit] Erreur 2
gmake[3]: quittant le répertoire « /home/andrew/prog/mozilla-trees/firefox-canvas/mozilla/ff-opt-static »
gmake[2]: *** [tier_toolkit] Erreur 2
gmake[2]: quittant le répertoire « /home/andrew/prog/mozilla-trees/firefox-canvas/mozilla/ff-opt-static »
make[1]: *** [default] Erreur 2
make[1]: quittant le répertoire « /home/andrew/prog/mozilla-trees/firefox-canvas/mozilla/ff-opt-static »
make: *** [build] Erreur 2

Comment 58

[Glenn Randers-Pehrson]

Attachment: combined patch (v5)

restores png_read_reset() link

Comment 59

[Andrew Smith]

The v5 patch has a syntax error (in png_progressive_read_reset()) and even with that fixed no APNGs show at all.

Comment 60

[Glenn Randers-Pehrson]

Attachment: combined patch (v6) [checked in]

revert to v3 and restore material that was removed via #if 0

Comment 61

[Glenn Randers-Pehrson]

There will be a slight merge conflict to resolve if the libpng-1.2.26 patch from bug #418900 is checked in first.

Comment 62

[Justin Dolske [:Dolske]]

(In reply to comment #40)
> It seems it's not easy to automate the testing of animated graphics

True in general, but in many cases it should be possible to construct a test animation for visual glitches... Make it play quickly, (looping once or twice if needed) and end on a frame that demonstrates the problem. I did this for bug 411852. See:

http://mxr.mozilla.org/seamonkey/source/modules/libpr0n/test/reftest/apng/

Comment 63

[Andrew Smith]

With the v6 patch everything seems to be ok for me.

Comment 64

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is this ready to land? The freeze is tomorrow so the sooner the better.

Comment 65

[Reed Loden [:reed]]

Checking in modules/libimg/png/MOZCHANGES;
/cvsroot/mozilla/modules/libimg/png/MOZCHANGES,v  <--  MOZCHANGES
new revision: 3.25; previous revision: 3.24
done
Checking in modules/libimg/png/png.h;
/cvsroot/mozilla/modules/libimg/png/png.h,v  <--  png.h
new revision: 3.22; previous revision: 3.21
done
Checking in modules/libimg/png/pngrutil.c;
/cvsroot/mozilla/modules/libimg/png/pngrutil.c,v  <--  pngrutil.c
new revision: 3.23; previous revision: 3.22
done