Closed vittorioromeo closed 3 months ago
Thanks for the PR! While the functionality and code style are good, I'm hesitant to merge this. It does not seem clear to me that a general "probe" functionality should be part of this en-/decoding library.
Other integrations that need this (e.g. ffmpeg in img2dec.c) have their own probe functions outside of the general decoding library. This also has the benefit of making certain decoders optional with the ability to show proper error messages (i.e. "QOI image detected, but QOI decoding was not compiled in"
).
Thanks for the PR! While the functionality and code style are good, I'm hesitant to merge this. It does not seem clear to me that a general "probe" functionality should be part of this en-/decoding library.
Thank you for the kind words on the quality of the PR and for sharing your thoughts!
Other integrations that need this (e.g. ffmpeg in img2dec.c) have their own probe functions outside of the general decoding library. This also has the benefit of making certain decoders optional with the ability to show proper error messages (i.e.
"QOI image detected, but QOI decoding was not compiled in"
).
I have considered writing my own probe function inside the SFML implementation, however after giving it some extra thought, I realized that I would be re-implementing a generally useful function that other QOI users would benefit from.
Case in point: the ffmpeg
example you linked. Had the functionality been available in QOI already, they could have just called qoi_detect_header_magic
to probe the magic bytes rather than reimplementing the wheel.
There are other advantages to implemeting the probe function directly in QOI:
Any future consumer of QOI will be able to benefit from qoi_detect_header_magic
without having to implement their own version.
Due to C's poor safety defaults, it can be easy to cause a buffer overrun when implementing such a probe from scratch. Providing a vetted probe function would reduce the likelihood of safety/stability issues in user applications using QOI.
Any future change to the QOI specification will result in qoi_detect_header_magic
being in sync. There was some discussion about possible future new revisions of the spec (https://github.com/phoboslab/qoi/issues/191#issuecomment-1165137133) that would have a new set of magic bytes -- having one centralized reference probing implementation would avoid desync if that ever happens.
So I kindly ask you to reconsider and give it some more thought. If you still decide to not merge the PR, no problem at all -- thank you for considering it in the first place and for the great work on QOI and QOA.
Sorry, I have to decline. I'm excited to see QOI support in SFML, but I want to keep this "reference implementation" as simple as possible. Exposing probing functionality would be valuable for a library that deals with different file types, but not for a single en-/decoder.
You bring up some points that would be valid if this probing was more difficult. Detecting the qoif
magic bytes is trivial and – if you desire – you can even use a memory safe language in your host software to do so :] The QOI format itself is final; there will be no version 2. If there ever will be a successor to QOI, it will have different magic bytes and a different library.
Hello, we're looking to add support for QOI and QOA in SFML 3.x (see https://github.com/SFML/SFML/issues/2969), and since we have a generic
sf::Image
abstraction that can be loaded from any pathit would be useful to be able to detect if the data we are reading is a valid QOI byte stream in order to internally decode it and store it in the right way.
For this purpose, I've added a
qoi_detect_header_magic
funcction to theqoi.h
file, which simply tells the caller if the given byte array contains the QOI magic header bytes.Such a function could then be used internally in SFML 3.x, to either end up calling
qoi_decode
, or falling back ontostbi_load
for non-QOI formats.Thank you for considering this contribution!
NOTE: I had considered making
qoi_detect_header_magic
share the same interface asqoi_decode
and to use it internally in the latter function to avoid repetition, but ultimately decided against it asqoi_detect_header_magic
does not needdesc
orchannels
, and it would have complicated the usage and interface of the simple header detection utility.