399542 - libpng 1.2.21 update broke a unit test

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Justin Dolske [:Dolske]]

The update to 1.2.21 caused the modules/libpr0n/test/unit/test_encoder_png.js unit test to fail... The encoded APNG looked and worked properly, but the bits were different.

pngcheck on the expected output is:

File: png1-ok.png (0 bytes)
  chunk IHDR at offset 0x0000c, length 13
    3 x 3 image, 24-bit RGB, 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 16
    zlib:  deflated, 256-byte window, default compression
    zlib line filters (0 none, 1 sub, 2 up, 3 avg, 4 paeth):
      1 2 2 (3 out of 3)
  chunk fcTL at offset 0x0007b, length 26
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fdAT at offset 0x000a1, length 22
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fcTL at offset 0x000c3, length 26
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fdAT at offset 0x000e9, length 27
    unknown private, ancillary, unsafe-to-copy chunk
  chunk IEND at offset 0x00110, length 0
No errors detected in png1-ok.png (-937.0% compression).


pngcheck on the new output is:

File: png1-bad.png (0 bytes)
  chunk IHDR at offset 0x0000c, length 13
    3 x 3 image, 24-bit RGB, 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 16
    zlib:  deflated, 256-byte window, default compression
    zlib line filters (0 none, 1 sub, 2 up, 3 avg, 4 paeth):
      0 0 0 (3 out of 3)
  chunk fcTL at offset 0x0007b, length 26
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fdAT at offset 0x000a1, length 19
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fcTL at offset 0x000c0, length 26
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fdAT at offset 0x000e6, length 19
    unknown private, ancillary, unsafe-to-copy chunk
  chunk IEND at offset 0x00105, length 0
No errors detected in png1-bad.png (-896.3% compression).


Note that in the original image, the IDAT line filters are set to 1/2/2, but in the new output they're 0/0/0. The fDAT checks also changed in size. [They're basically IDATs, so I'd suspect the same thing happened, pngcheck just doesn't know how to handle them.]

The libpng update seems to have added |#define PNG_NO_WRITE_FILTER| to modules/libimg/png/mozpngconf.h, so it looks like this is probably expected if I'm understanding that right.

Our nsIImageEncoder interface doesn't support specifying row filters, so this seems fine... I do wonder how the 1/2/2 values were getting set before -- were those the values, or were we picking up initialized data?

Glenn, if this all seems normal and expected I'll just update the unit tests for the new expected values.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch to temporarily disable tests.

Comment 2

[Justin Dolske [:Dolske]]

Checking in modules/libpr0n/test/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/test/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1

I'll back this out when checking in a test fix (assuming that's what we do).

Comment 3

[Justin Dolske [:Dolske]]

(In reply to comment #0)

> Our nsIImageEncoder interface doesn't support specifying row filters, so this
> seems fine... I do wonder how the 1/2/2 values were getting set before -- were
> those the values, or were we picking up initialized data?

Err, let me try making sense this time:

I do wonder how the 1/2/2 values were getting set before -- were those the default values, or were we picking up uninitialized data?

Comment 4

[Glenn Randers-Pehrson]

That is correct.  The embedded libpng-1.2.21 writes all 0 filters while
the previous version would compute filters that could be anything 0-4.
The change was made to save footprint size, and because the usual PNG being
written is a screen dump for which the 0 filter is almost always the best
one anyhow.  Notice that the file has smaller fDATs now.

Please update the unit tests.  Note, however, that you will get a different
result when the system library is used instead of the embedded one.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

This reenables the tests, and uses new expected data per discussion above. [I verified that the expected data is rendered correctly.]

I noticed that the tests complained about trailing commands in strict mode, so I went ahead and removed the unneeded commas.

Comment 6

[Justin Dolske [:Dolske]]

Checking in modules/libpr0n/test/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/test/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in modules/libpr0n/test/unit/test_encoder_apng.js;
/cvsroot/mozilla/modules/libpr0n/test/unit/test_encoder_apng.js,v  <--  test_encoder_apng.js
new revision: 1.2; previous revision: 1.1
done
Checking in modules/libpr0n/test/unit/test_encoder_png.js;
/cvsroot/mozilla/modules/libpr0n/test/unit/test_encoder_png.js,v  <--  test_encoder_png.js
new revision: 1.2; previous revision: 1.1

Comment 7

[Justin Dolske [:Dolske]]

Hmm. Forgot to mark this as FIXED.