shssoichiro / oxipng

Multithreaded PNG optimizer written in Rust
MIT License
2.94k stars 125 forks source link

Keep fcTL after PLTE #626

Closed andrews05 closed 4 months ago

andrews05 commented 5 months ago

Fixes #625

AlexTMjugador commented 4 months ago

I did some tests in relation to this PR with the PNG file at http://littlesvr.ca/apng/resources/apng11.png (which is linked from http://littlesvr.ca/apng/, a website that hosts a collection of development resources for APNG) and observed the following original layout of chunks:

File: /tmp/apng11.png (893 bytes)
  chunk IHDR at offset 0x0000c, length 13
    63 x 63 image, 1-bit palette, non-interlaced
  chunk acTL at offset 0x00025, length 8
    unknown private, ancillary, unsafe-to-copy chunk
  chunk gAMA at offset 0x00039, length 4: 1.0000
  chunk PLTE at offset 0x00049, length 6: 2 palette entries
  chunk fcTL at offset 0x0005b, length 26
    unknown private, ancillary, unsafe-to-copy chunk
  chunk IDAT at offset 0x00081, length 15
    zlib: deflated, 1K window, maximum compression
  chunk fcTL at offset 0x0009c, length 26
    unknown private, ancillary, unsafe-to-copy chunk
  chunk fdAT at offset 0x000c2, length 31
    unknown private, ancillary, unsafe-to-copy chunk

In this test image the fcTL chunk goes after PLTE, which is the "good" ordering. The ffmpeg -i /tmp/apng11.png /tmp/out.mp4 command worked well, as expected.

I then ran OxiPNG before this commit on apng11.png and repeated the above ffmpeg command. This time it failed, like on the linked issue, due to the "bad" reordering of the first fcTL before PLTE. When applying this commit to OxiPNG, it did indeed fix decoding the optimized original test image with ffmpeg, which uses libavformat's apngdec internally.

The PNG specification explicitly mentions that fcTL thunks "[one] may occur before IDAT; all others shall be after IDAT" (note how that wording is vague enough to allow fcTL to go either before or after PLTE). But an overly literal interpretation of its Figure 13, combined with decoder programmers overfitting to the exact behavior of reference encoders, may justify this state of affairs. In my opinion, this is a non-conformance in libavformat's APNG decoder and, allegedly, YYImage, but as conformant decoders are able to handle the fcTL chunk in either position with respect to PLTE, I agree it's a good thing for OxiPNG to restrict fcTL's ordering to not cause issues in the broken decoders.

Therefore, I'm pushing a commit to edit the code comment to mention the issue, making it clearer that we're doing this to make out of spec decoders happy, and merging this. Nice work! :+1: