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

TRUECOLOR vs RGB8 #231

Open
GhostCore07 opened this issue Oct 30, 2022 · 4 comments
Open

TRUECOLOR vs RGB8 #231

GhostCore07 opened this issue Oct 30, 2022 · 4 comments
Labels
question Further information is requested

Comments

@GhostCore07
Copy link

In the following struct,

struct spng_ihdr
{
    uint32_t width;
    uint32_t height;
    uint8_t bit_depth;
    uint8_t color_type;
    uint8_t compression_method;
    uint8_t filter_method;
    uint8_t interlace_method;
};

Shouldn't color_type be of type enum spng_color_type rather than uint8_t ?

Also maybe this isn't a bug but it's worth noting that it's pretty confusing to have two different types enum spng_color_type and enum spng_format that have names with identical meaning, but different enum values such as SPNG_COLOR_TYPE_TRUECOLOR_ALPHA and SPNG_FMT_RGBA8. The reason I mention this is because it is often tempting to do something like:
int fmt = ihdr.color_type
But alas, you cannot and it takes awhile getting familiar with the API before you realize why it doesn't work.

@randy408
Copy link
Owner

Shouldn't color_type be of type enum spng_color_type rather than uint8_t ?

The fields have the same size as the described by the file format, but I'll probably change that for the next breaking version.

it's pretty confusing to have two different types enum spng_color_type and enum spng_format that have names with identical meaning

When decoding you're converting from ihdr to an spng_format, or from an spng_format to an ihdr when encoding. Formats are different, it's color type, channel order, bit depth and endianness in one enum except for SPNG_FMT_PNG, SPNG_FMT_RAW which only specify endianness and the rest is derived from ihdr.

@randy408 randy408 added the question Further information is requested label Oct 30, 2022
@GhostCore07
Copy link
Author

The fields have the same size as the described by the file format

Correct. The issue is that you can assign a spng_format to uint8_t color_type due to enum implicitly cast to int. It's much better if the static checker to throw an error in this case.

When decoding you're converting from ihdr to an spng_format, or from an spng_format to an ihdr when encoding.

Thanks for the clarification. I understand the rational for having different types between encoding and decoding steps.

I will try to clarify the point I was trying to make with truecolor.

https://en.wikipedia.org/wiki/Color_depth#True_color_(24-bit)

True color (24-bit), the use of 24 bits to store color information
24 bits almost always use 8 bits each of R, G, and B

Therefore,
"TRUECOLOR" and "RGB8" have the exact same semantic meaning and it's confusing to try and understand why the API uses different words for the same thing.

In my opinion something like:
SPNG_COLOR_TYPE_RGBA8
SPNG_FMT_RGBA8

Makes it expressly clear that we are talking about a color type and a fmt type. Perhaps I will just make this a typedef for myself if you are not interested in changing the API.

@randy408
Copy link
Owner

The standard uses the term "truecolor", at the time it made sense to use the exact same words.

I wouldn't add something like SPNG_COLOR_TYPE_RGBA8 because the standard expresses color type and bit depth in separate fields. Some PNG libraries also have *_COLOR_TYPE_RGBA but bit depth is still a separate value.

Makes it expressly clear that we are talking about a color type and a fmt type.

But they already start with SPNG_COLOR_TYPE and SPNG_FMT 🤔

@GhostCore07
Copy link
Author

Doesn't seem too far of a reach to have a COLOR_TYPE that combines the "truecolour"-ness and the 8 or 16 bit-ness in a single flag. That's just an observation. I see now that would be innovating on the standard a little bit. From a practical standpoint I was surprised to not see it is all. Take it for what it's worth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants