phoboslab / qoi

The “Quite OK Image Format” for fast, lossless image compression
MIT License
6.96k stars 330 forks source link

Reference implementation bug or something else? #163

Closed aldanor closed 2 years ago

aldanor commented 2 years ago

I was digging through some test examples and found a weird one. Apologies in advance if it's something obvious.

Here's a brief example:

#define QOI_IMPLEMENTATION
#include "qoi.h"

int main(int argc, char **argv) {
    unsigned char encoded[14 + 11 + 8] = {
        113, 111, 105, 102, 0, 0, 0, 5, 0, 0, 0, 1, 3, 0, // header (5x1x3)

        53,                 // QOI_OP_INDEX(53) -> [0, 0, 0]
        254, 244, 252, 6,   // QOI_OP_RGB(244, 252, 6)
        254, 22, 24, 17,    // QOI_OP_RGB(22, 24, 17)
        38,                 // QOI_OP_INDEX(38) -> [22, 24, 17] (???)
        127,                // QOI_OP_DIFF(3, 3, 3)

        0, 0, 0, 0, 0, 0, 0, 1 // padding
    };
    qoi_desc desc;
    int len = 14 + 11 + 8;
    unsigned char* px = qoi_decode(encoded, len, &desc, 3);
    for (int i = 0; i < desc.width * desc.height; ++i) {
        printf("[%d, %d, %d]\n", px[i * 3], px[i * 3 + 1], px[i * 3 + 2]);
    }
    free(px);
    return 0;
}

which outputs

[0, 0, 0]
[244, 252, 6]
[22, 24, 17]
[0, 0, 0]      # <--- why is this [0, 0, 0] and not [22, 24, 17]?
[1, 1, 1]

Note that

(22 * 3 + 24 * 5 + 17 * 7 + 255 * 11) % 64 = 38

@phoboslab any ideas perhaps?

aldanor commented 2 years ago

Hmm, is that because the initial index load "spoils" the .a field of the current pixel, and so it computes the hash of (22, 24, 17, 0) instead of (22, 24, 17, 0xff)? Btw, in my opinion it may not have been the best decision to zero-initialize the index array; filling it with (0, 0, 0, 0xff) would match the initial default pixel value and this would keep the alpha channel at 0xff for 3-channel images, always. And/or, for 3-channel images, alpha channel could have been ignored when decoding it from index - currently, QOI_OP_INDEX is the only operation that could mess up the alpha channel in 3-channel decodes, zeroing it out.

(going to try and debug)

aldanor commented 2 years ago

Yes, confirming it was an error on my side, loading a [0, 0, 0] from index in a 3-channel mode essentially zeroes out the 0xff alpha channel from the 'current' pixel.