macOS Screenshot application generates invalid PNG files

[ronaldtse]

For example, this file is invalid.

Screen Shot 2022-09-12 at 8.13.24 PM.png  illegal (unless recently approved) unknown, public chunk iDOT
ERROR: Screen Shot 2022-09-12 at 8.13.24 PM.png

However I'm having a hard time believing that the Screenshot app generates an invalid PNG.

@maxirmx any thoughts here?

[ronaldtse]

Wow! https://developer.apple.com/forums/thread/26893

[maxirmx]

In the gem just wrapped pngcheck that rigorously checks the image against specification
I guess that significant amount of violations do not prevent the image from being displyed correctly by certain tools.
So it is a matter of requirements - what exactly do we want to check

[ronaldtse]

As a simple workaround, I propose that the Ruby library hides this iDOT error since it is acceptable for readers.

Ideally there is a PngCheck initialization context that I can do:

instance = PngCheck.new
instance.verify(IO.read("my.png"))
=> raise "illegal (unless recently approved) unknown, public chunk iDOT"

instance = PngCheck.new(:readonly)
instance.verify(IO.read("my.png"))
=> :ok

Ref: w3c/png#45 (comment)

[maxirmx]

@ronaldtse
Please re-consider it.

1. We use pngcheck to be sure that any (unknown) third party tool will be able to read the image.
   Nobody said that iDOT was acceptable for (all) readers. We were said that iDOT was risky for writers. However you request to extend this rule further: any image that is risky for writers is also acceptable for readers That does not sound safe. I understand that for practical reasons we have to accepts MacOS screenshots as valid which means that we shall support masking of iDOT errors.

2. I do not think it is worth making it a pngcheck instance attribute. I will rather do it a parameter like: PngCheck.analyze_file(path: "spec/examples/macos-screenshot.png", mask_iDOT_error: true)

[ronaldtse]

The iDOT packet is not a registered chunk, which means it is considered an "unknown" chunk.

We do not know if all PNG editors can read the image. But that is irrelevant, because the PNG specification allows unknown chunks (see below). The point is, if a PNG editor cannot read the image with an iDOT or cannot preserve the iDOT, it is in violation of the PNG specification.

According to the PNG specification:

15.2.3 Conformance of PNG decoders

A PNG decoder conforms to this International Standard if it satisfies the following conditions.

a. It is able to read any PNG datastream that conforms to this International Standard, including both public and private chunks whose types may not be recognized.
...
b. Unknown chunk types are handled as described in 5.4 Chunk naming conventions. An unknown chunk type is not treated as an error unless it is a critical chunk.
...

And

15.2.4 Conformance of PNG editors

A PNG editor conforms to this International Standard if it satisfies the following conditions.

...
b. It conforms to the requirements for PNG decoders.
...
d. It preserves the ordering of the chunks presented within the rules in 5.6: Chunk ordering.
e. It properly processes the safe-to-copy bit information and preserves unknown chunks when the safe-to-copy rules permit it.
...

Upon reading this it is clear an unknown chunk type is at most a "WARNING", not an "ERROR". So we should not raise any Error class when encountering it.

Since pngcheck produces a number of warning/error messages, but does not properly categorize them, it is up to us (the consumer library) to categorize them and present them to our users.

[maxirmx]

@Ronaldse

1. pngcheck now has the following status codes:

  STATUS_OK = 0
  STATUS_WARNING = 1        # an error in some circumstances but not in all
  STATUS_MINOR_ERROR = 3    # minor spec errors (e.g., out-of-range values)
  STATUS_MAJOR_ERROR = 4    # file corruption, invalid chunk length/layout, etc.
  STATUS_CRITICAL_ERROR = 5 # unexpected EOF or other file(system) error

Based on your analisys I will patch pngcheck so that it will return STATUS_WARNING if a chunk of unknown type is found
Is it correct ?

2. I have some doubts regarding return value in case of STATUS_WARNING
   Now in the interface you requested there are two options:

• return true
• raise exception

In case of WARNING we have two options:

• ignore it and return true
• raise exception but add status code field to it so that it is possible to recover warnings upstream

Please advise

[ronaldtse]

Based on your analisys I will patch pngcheck so that it will return STATUS_WARNING if a chunk of unknown type is found

Agree. I know this is not ideal but it's what is possible.

In case of WARNING we have two options:

• ignore it and return true
• raise exception but add status code field to it so that it is possible to recover warnings upstream

The latter please.