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

Possible edge case in decoder when very first OP is QOI_OP_RUN #258

Open
phoboslab opened this issue Jan 23, 2023 · 8 comments
Open

Possible edge case in decoder when very first OP is QOI_OP_RUN #258

phoboslab opened this issue Jan 23, 2023 · 8 comments

Comments

@phoboslab
Copy link
Owner

phoboslab commented Jan 23, 2023

As recently pointed out by @andrews05, there is an edge in QOI files that could be handled differently by different decoders.

This edge case can produce a transparent background when fully opaque black was intended. This may happen, when decoding to RGBA (4 channels) and the very first OP of a QOI file is a QOI_OP_RUN, that repeats the initial opaque black pixel.

Some decoders omit putting the initial black pixel into the index in this case. Later in the image the decoder may refer to this to the index position where the opaque black should have been stored, but that instead is empty – i.e. just the initial transparent black that comes from the zero initialized index.

As the spec points out, admittedly not very clearly:

Each pixel that is seen by the encoder and decoder is put into this array at the position formed by a hash function of the color value.

... this must include the initial opaque black when the very first OP is a QOI_OP_RUN.

I have updated the qoi_test_images.zip to include an image where this edge-case is present, aptly named edgecase.qoi.

As further pointed out by @andrews05, the reference encoder in this repository does not produce QOI images that have this edge-case (it never starts a QOI file with a QOI_OP_RUN), but other encoders might.

After going through the decoders listed in this readme, I have identified a few implementations where this edge case is not handled according to the spec – again, mea culpa, it's not the clearest wording. I will notify those implementations now.

Thanks again to @andrews05 for pointing it out.

@pfusik
Copy link
Contributor

pfusik commented Feb 4, 2024

If the encoded data starts with QOI_OP_INDEX | 0x35, is it a black or transparent pixel?

@phoboslab
Copy link
Owner Author

Not sure I understand the question correctly. Per the spec, the 64 elements of the index are zero-initialized. I.e. all colors in the index are initially transparent black.

If the very first decode is QOI_OP_INDEX | 0x35 and you're emitting RGBA (as opposed to RGB), the pixel will be transparent.

@pfusik
Copy link
Contributor

pfusik commented Feb 5, 2024

QOI_OP_RUN normally writes pixels same as the pixel before, therefore there is no need to update the index.
The only exception is if QOI_OP_RUN is the first code, where it writes black pixels that are not in the index.
I considered initializing the index so that the black pixel is there to start with. Your answer clarifies that it would break QOI_OP_INDEX as the first code, which unfortunately means that QOI_OP_RUN needs to update index just for this edge case.

@pfusik
Copy link
Contributor

pfusik commented Feb 5, 2024

Another solution is to look-ahead QOI_OP_RUN and handle the index outside the main loop.

@phoboslab
Copy link
Owner Author

Yes, this is somewhat unfortunate and maybe could have been solved with some more thought about the initial values for previous pixel and the index...

For what it's worth, this implementation here only sets the index at the start of a run; it doesn't calculate the hash when emitting pixels within a run. I assume the performance impact is negligible.

@alexriegler12
Copy link

Would it work if you always inserted the initial opaque black into the array and not just if the first chunk is a run?

@pfusik
Copy link
Contributor

pfusik commented Feb 20, 2024

That would break QOI_OP_INDEX which, as discussed above, must produce a transparent pixel.

@wx257osn2
Copy link
Contributor

@phoboslab

I have updated the qoi_test_images.zip to include an image where this edge-case is present, aptly named edgecase.qoi.

There is edgecase.qoi and edgecase.png in qoi_test_images.zip , but edgecase.png is 3-channel png even though edgecase.qoi has an alpha channel. So, we can detect the issue by roundtrip verifying with edgecase.qoi, but we can't with edgecase.png for some implementations that doesn't handle alpha channel on 3-channel decoder.
Would you replace the edgecase.png with 4-channel one?

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

4 participants