411852 - nsPNGDecoder uses GetWidth twice instead of GetHeight

Status: RESOLVED FIXED

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

Description

[worms]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.9b2pre) Gecko/2007120703 SeaMonkey/2.0a1pre
Build Identifier: 

I made the image with aPNG Edit.

In Firefox 2007-Oct-16
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.9a9pre) Gecko/2007101605 Minefield/3.0a9pre
it shows alright, there is no glitch

In SeaMonkey 2007-Oct-25
Build identifier: Mozilla/5.0 (Windows; U; Windows NT
5.0; rv:1.9a9pre) Gecko/2007102503 SeaMonkey/2.0a1pre
it shows normally without glitch

In SeaMonkey 2007-Dec-07
Build identifier: Mozilla/5.0 (Windows; U; Windows NT
5.0; rv:1.9b2pre) Gecko/2007120703 SeaMonkey/2.0a1pre
there is a strange glitch at the tail of the
rocketship. Like a vertical bar made of the colours of
the images in the frame, it's *very* noticeable.

The glitch also shows in Firefox 
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.9b3pre) Gecko/2007121905 Minefield/3.0b3pre 

and in  Firefox 
rv:1.9b3pre) Gecko/2008010805 Minefield/3.0b3pre

as well.

The glitch also shows, but differently, as noticed Justin Dolske, in the thumbnail of the image in the tab.

Both glithces show when the image is loaded directly from the hard disk.

Reproducible: Always

Steps to Reproduce:
1. Load image http://corpsmoderne.net/~eve97/gothdns/stuff/rocket_fast.png in a recent trunk build of Firefox or SeaMonkey
2.
3.
Actual Results:  
Right "compressed" image on the right of the animation, which doesn't exist in the actual animation.

Expected Results:  
A nice rocketship :)

Comment 1

[Justin Dolske [:Dolske]]

Oh boy. This could be a mix of one or more things:

1) Bug in APNGedit? Was the animation created using frame offsets or frames smaller than the animation? The spec requires offset frames to be clipped to the animation size (aka the initial frame), so APNGedit does some math to do so. It's possible I messed up some case here, although I thought I tested this well.

2) Bug in APNG encoder? pngcheck shows an odd sequence of chunks in the file:

  chunk IHDR at offset 0x0000c, length 13
    822 x 240 image, 32-bit RGB+alpha, non-interlaced
  chunk acTL at offset 0x00025, length 8
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fcTL at offset 0x00039, length 26
    unknown private, ancillary, unsafe-to-copy chunk
  chunk IDAT at offset 0x0005f, length 3179
    zlib:  deflated, 32K window, default compression
  chunk fcTL at offset 0x00cd6, length 26
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fdAT at offset 0x00cfc, length 8196
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fdAT at offset 0x02d0c, length 8196
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fdAT at offset 0x04d1c, length 1307
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fcTL at offset 0x05243, length 26
    unknown private, ancillary, unsafe-to-copy chunk

Why are there multiple fdAT chunks in between fcTL? Shouldn't they alternate, 1 fcTL per fdAT?

3) Bug 403364? That seemed to be triggered by discarding/redecoding image data, but I see the glitches here immediately.

4) Cario bug? I had seen some vaguely similar problems in bug 379414. If you look at the tab favicon while the animation plays, I can see that it's not scaled properly, and there sometimes appears to be random garbage pixels.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Screengrab of on glitch (garbage pixels)

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Screengrab of on glitch (wrong tiling)

Note that in the content the rocket is just entering from the right, but the tab icon also shows a tail section exiting on the left.

Comment 4

[Glenn Randers-Pehrson]

(In reply to comment #1)

> Why are there multiple fdAT chunks in between fcTL? Shouldn't they
> alternate, 1 fcTL per fdAT?

That's OK.  There can be multiple fdAT chunks per frame, containing a
single zlib datastream when their data segments are concatenated.

Comment 5

[worms]

re Comment #2: I never see the garbage pixels you're seeing, when the aPNG is in the page, I see a thin bar of colours, see screenshot

http://corpsmoderne.net/~eve97/gothdns/stuff/rocket_screenshot.png

Sometimes the bar is a bit larger, but it's always a thin bar. It seems located at 148 pixels from the right edge (where the rocketship "enters").

I've made a page with the 11 frames, displayed as I used them in aPNG edit, specifying each frame size and offset.

frame 1: offset 686 - image 136 - total 822
frame 2: offset 556 - image 266 - total 822
frame 3: offset 424 - image 398 - total 822
frame 4: offset 291 - image 531 - total 822
frame 5: offset 160 - image 662 - total 822
frame 6: offset   0 - image 822 - total 822
frame 7: offset   0 - image 662 - total     662
frame 8: offset   0 - image 511 - total     511 
frame 9: offset   0 - image 377 - total     377
frame 10: offset  0 - image 260 - total     260
frame 11: offset  0 - image 136 - total     136

I have noticed the same thing as in Comment #3 for the tab icon, the glitch is different and shows a much larger tail section.

Hope this can help - I've checked again, nothing of this happens in earlier builds of Firefox/SeaMonkey.

 

Comment 6

[worms]

Attachment: Screenshot of the glitch

It's about 148 pixels from the right.

Comment 7

[worms]

(In reply to comment #5)
> re Comment #2: I never see the garbage pixels you're seeing, when the aPNG is
> in the page, I see a thin bar of colours, see screenshot
> 
> http://corpsmoderne.net/~eve97/gothdns/stuff/rocket_screenshot.png
> 
> Sometimes the bar is a bit larger, but it's always a thin bar. It seems located
> at 148 pixels from the right edge (where the rocketship "enters").
> 
> I've made a page with the 11 frames, displayed as I used them in aPNG edit,
> specifying each frame size and offset.
> 
> frame 1: offset 686 - image 136 - total 822
> frame 2: offset 556 - image 266 - total 822
> frame 3: offset 424 - image 398 - total 822
> frame 4: offset 291 - image 531 - total 822
> frame 5: offset 160 - image 662 - total 822
> frame 6: offset   0 - image 822 - total 822
> frame 7: offset   0 - image 662 - total     662
> frame 8: offset   0 - image 511 - total     511 
> frame 9: offset   0 - image 377 - total     377
> frame 10: offset  0 - image 260 - total     260
> frame 11: offset  0 - image 136 - total     136
> 
> I have noticed the same thing as in Comment #3 for the tab icon, the glitch is
> different and shows a much larger tail section.
> 
> Hope this can help - I've checked again, nothing of this happens in earlier
> builds of Firefox/SeaMonkey.
> 
> 
> 

I forgot the URL of the pages with the individual frames:

http://corpsmoderne.net/~eve97/gothdns/stuff/rocket_frames.htm

Comment 8

[Justin Dolske [:Dolske]]

Fixes for bug 403364, bug 411224, and one I can't remember landed recently. With a currently nightly, I no longer see the garbage pixels, but I do see the main glitch (attachment 296750 [details]) and the wrong tiling in the favicon (attachment 296658 [details]).

Comment 9

[worms]

(In reply to comment #8)
> Fixes for bug 403364, bug 411224, and one I can't remember landed recently.
> With a currently nightly, I no longer see the garbage pixels, but I do see the
> main glitch (attachment 296750 [details]) and the wrong tiling in the favicon (attachment
> 296658 [details]).
> 

I've downloaded latest trunk builds of 
Firefox Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008012004 Minefield/3.0b3pre
and 
SeaMonkey  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008012001 SeaMonkey/2.0a1pre

and indeed, the glitches still show.
No garbage added :)

Comment 10

[Mike Schroepfer]

(In reply to comment #9)
> (In reply to comment #8)
> > Fixes for bug 403364, bug 411224, and one I can't remember landed recently.
> > With a currently nightly, I no longer see the garbage pixels, but I do see the
> > main glitch (attachment 296750 [details] [details]) and the wrong tiling in the favicon (attachment
> > 296658 [details]).
> > 
> 
> I've downloaded latest trunk builds of 
> Firefox Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre)
> Gecko/2008012004 Minefield/3.0b3pre
> and 
> SeaMonkey  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre)
> Gecko/2008012001 SeaMonkey/2.0a1pre
> 
> and indeed, the glitches still show.
> No garbage added :)
> 

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008012104 Minefield/3.0b3pre ID:2008012104

See the glitch as well

Comment 11

[Justin Dolske [:Dolske]]

Attachment: [fixed] Testcase 1

Hmm. So, at least part of the problem seems to be that frame compositing is sometimes broken when the frames are smaller than the total image since.

Testcase 1 (attached) is a simple 2 frame animation. 10ms delay between frames, no looping, disposal method = clear.

Frame 1: 200x100, 50% red.
Frame 2: 50x100, 50% green.

Frame 2 should be drawn as a green 50x100 vertical bar on the left. Instead, it's a 50x50 square (the bottom 50x50 region is missing). I confirmed that my APNG encoder is sending the right input to the encoder, based on the size it specifies, the length of the pixel array sent, and the color values of the first and last pixels.

Almost looks like something is clipping the second frame to (w,w) instead of (w,h).

Comment 12

[Justin Dolske [:Dolske]]

Attachment: [fixed] Testcase 1, reference

(non animated, expected output)

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Testcase 2 (slow loop)

This is another test case 201x100, demonstrating incorrectly rendered frames. It think it may be a different problem than testcase 1 illustrates.

Frame 1 is 201x100, 50% (opacity) red.
Frame 2 is 100x100, 50% green.
Frame 3 is 150x100, 50% blue
Frame 4 is 200x100, 50% purple.

The first loop through, each frame is drawn correctly. On the second and future loops, the 3rd and 4th frame are drawn incorrectly. Frame 3 has part of frame 4 (!) mixed in with it, and frame 4 has part of itself drawn twice (color is right, but part looks 100% opaque instead of 50%).

Note that frame 4 is one pixel smaller than the animation's width (201 vs 200). If I use a 200x100 image for frame 1 (so the animation size is 200x100), the animation plays correctly.

I

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Testcase 2 (fast, loops twice)

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Testcase 2 reference

(non animated, expected output)

Comment 16

[Stuart Parmenter]

Dolske: are you looking at how to fix this?

Comment 17

[Justin Dolske [:Dolske]]

Hmm, never saw last comment in email.

From what I looked at before, I think it's pretty clearly a bug in gfx-land, and not a problem with the APNG encoder/extension. I did some cursory investigation through the frame compositing code, but I'm not really familiar with it and didn't see anything terribly obvious.

Comment 18

[Glenn Randers-Pehrson]

(In reply to comment #11)

> Almost looks like something is clipping the second frame to (w,w) instead of
> (w,h).

I think the bug is here in libpr0n/decoders/png/nsPNGDecoder.cpp:

    // Tell the image renderer that the frame is complete
    PRInt32 width, height;
    mFrame->GetWidth(&width);
    mFrame->GetWidth(&height);

Shouldn't that last line be using GetHeight() ?

Comment 19

[Justin Dolske [:Dolske]]

(In reply to comment #18)

> Shouldn't that last line be using GetHeight() ?

I think so. :) This fixes the first testcase in this bug, but the second one is still broken as before.

Comment 20

[Justin Dolske [:Dolske]]

Attachment: [checked in] GetHeight fix, patch v.1

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311191 [details] [diff] [review]
[checked in] GetHeight fix, patch v.1

a1.9=beltzner

Comment 22

[Justin Dolske [:Dolske]]

Checked in the GetHeight() fix, with a testcase. Leaving bug open for the other issue still. :(

Checking in modules/libpr0n/decoders/png/nsPNGDecoder.cpp;
  new revision: 1.81; previous revision: 1.80
Checking in modules/libpr0n/test/reftest/reftest.list;
  new revision: 1.12; previous revision: 1.11
Checking in modules/libpr0n/test/reftest/apng/bug411852-1-ref.png;
  initial revision: 1.1
Checking in modules/libpr0n/test/reftest/apng/bug411852-1.png;
  initial revision: 1.1
Checking in modules/libpr0n/test/reftest/apng/delaytest.html;
  initial revision: 1.1
Checking in modules/libpr0n/test/reftest/apng/reftest.list;
  initial revision: 1.1

Comment 23

[Glenn Randers-Pehrson]

Does the "v6" patch from bug #420416 have any effect on this bug?

Comment 24

[Justin Dolske [:Dolske]]

(In reply to comment #23)
> Does the "v6" patch from bug #420416 have any effect on this bug?

Doesn't seem to. I applied it locally, and the first attachment in 420416 works fine but Testcase 2 attachment and the rocketship animation in comment 0 still have the same glitches.

Comment 25

[Helder "Lthere" Magalhães]

(In reply to comment #24)
> (In reply to comment #23)
> Doesn't seem to. I applied it locally, and the first attachment in 420416 works
> fine but Testcase 2 attachment and the rocketship animation in comment 0 still
> have the same glitches.

On the other hand, these changes seem to have fixed bug 418848. :-)

Comment 26

[Joe Drew (not getting mail)]

Comment on attachment 311191 [details] [diff] [review]
[checked in] GetHeight fix, patch v.1

Morphing this bug to be about the GetWidth patch, and filing a follow-on bug.

Comment 27

[Joe Drew (not getting mail)]

Please direct all further comments regarding the bug to bug 433047. This one we're calling fixed.