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

Join chunk ordering rules #467

Open
ProgramMax opened this issue Sep 28, 2024 · 9 comments
Open

Join chunk ordering rules #467

ProgramMax opened this issue Sep 28, 2024 · 9 comments

Comments

@ProgramMax
Copy link
Collaborator

§ 5.6 Chunk ordering lists out specific chunks and their ordering requirements.

§ 14.3 Ordering of chunks lists out the more general rules.

These two sections should perhaps be merged.
I don't see a benefit to having them separate.
But I do see drawbacks (such as a reader thinking they have absorbed the ordering rules after only seeing half with no indication to them).

I'm adding the "Future edition considerations" Milestone because I don't want to introduce new changes to Third Edition. This ordering split has existed since PNG 1.0 spec.

@randy408
Copy link

Can't help but notice the acTL chunk listed as "Before PLTE and IDAT" under § 5.6 Chunk ordering, I can't find anything that supports the "before PLTE" part in the original specification or the libpng patch. Am I missing something?

@ProgramMax
Copy link
Collaborator Author

Can you help me understand your question a little more? I don't fully grasp it.

When you say the original specification, if you meant PNG 1.0, § 4.3. Summary of standard chunks has several "Before PLTE" (cHRM, gAMA, sBIT). So perhaps you meant the original Mozilla spec for APNG?

I haven't carefully combed through the libpng patch but I would believe it could be missing something.

I wouldn't be surprised if the original Mozilla APNG patch simply left out some of the ordering requirements and the libpng patch was based on it. Then we filled the hole but the libpng patch didn't get updated.

@randy408
Copy link

randy408 commented Nov 18, 2024

So perhaps you meant the original Mozilla spec for APNG?

Yes.

Looking at the generated test images from https://philip.html5.org/tests/apng/tests.html linked in the Mozilla specification page out of 60 images there's 3 indexed-color images: 036, 037, 038, they all have the acTL chunk after the PLTE chunk. I get that they're generated by a perl script instead of the patched libpng, and that libpng probably writes acTL before PLTE but I couldn't find a "before PLTE" check for acTL in the read part of the patch, it's easy to find the same checks for example cHRM or gAMA in pngrutil.c, but nothing for acTL with the apng patch.

If that rule has been missing from the code and the specification all along there could be a lot of APNG's in the wild created without libpng where acTL comes after PLTE, adding the rule now could break existing APNG's.

@ProgramMax
Copy link
Collaborator Author

I see.

Man, I keep finding myself wanting to write a crawler to fetch and analyze pngs in the wild. I might be able to have an old Google coworker tell me the results of a test if I write it into Chrome. (We were able to write anonymous test cases and deploy it to say 1% of the population for things like this.)

For now, that "could" (break existing APNGs) is difficult to make a decision on. I don't have data on the likelihood of "could".

If we add a requirement in this edition and find it to be a problem, we can remove it in a later edition and maintain backwards compatibility with this edition. If we don't add the requirement to this edition, a future edition would be unable to add it without breaking backwards compatibility. So I'm tempted to leave it in this edition. But only because I don't know how powerful "could" is.

I'll look into the Chromium and Firefox codebases and see if they enforce a restriction. I assume the patch you are referring to is the libpng APNG patch, right?
If either Chromium or Firefox enforce a restriction, I'm tempted to also enforce. If neither of them do, we have no real enforcement anywhere we know of. Then I'm tempted not to, except for the backward compatibility thing.

@ProgramMax
Copy link
Collaborator Author

It looks like FireFox does not check that there is a PLTE chunk before the acTL chunk:
https://searchfox.org/mozilla-central/source/media/libpng/pngrutil.c#2880

Nor does Chromium:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc;l=656

I reached out to someone at Google about possibly running a test to see how many APNGs in the wild users encounter where the acTL chunk comes before the PLTE chunk.

@svgeesus
Copy link
Contributor

svgeesus commented Dec 5, 2024

Man, I keep finding myself wanting to write a crawler to fetch and analyze pngs in the wild. I

That would be very useful, for this and for other issues.

@svgeesus
Copy link
Contributor

svgeesus commented Dec 6, 2024

Yup, the Mozilla APNG spec says:

To be recognized as an APNG, an acTL chunk must appear in the stream before any IDAT chunks. The acTL structure is described below.

and PNG 3rd Edition says the same

To be recognized as an APNG, an acTL chunk must appear in the stream before any IDAT chunks. The acTL structure is described below.

and (in the table of chunk ordering rules):

Chunk name | Multiple allowed | Ordering constraints
acTL | No | Before PLTE and IDAT
fcTL | Yes | One may occur before IDAT; all others shall be after IDAT

The intent was to exactly follow the Mozilla spec so the "Before PLTE" is inconsistent with the chunk definition, inadvertent (likely my fault, sorry) and should be corrected.

@svgeesus
Copy link
Contributor

svgeesus commented Dec 6, 2024

The lattice diagrams also don't require acTL to be before PLTE. So it is just the table that says that.

@svgeesus
Copy link
Contributor

svgeesus commented Dec 6, 2024

PR for review

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

No branches or pull requests

3 participants