Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep fcTL after PLTE #626

Merged
merged 2 commits into from
Jun 22, 2024
Merged

Conversation

andrews05
Copy link
Collaborator

Fixes #625

@AlexTMjugador
Copy link
Collaborator

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! 👍

@AlexTMjugador AlexTMjugador merged commit 3759ca8 into shssoichiro:master Jun 22, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

after compress the apng file, YYimage decode can not get the frames count of the apng file.
2 participants