search

Roughsketch / imagesize

Quickly probe the size of various image formats without reading the entire file.
MIT License
57 stars 12 forks source link

Add EXR/HDR/TGA/DDS/KTX2/QOI support #25

Closed AllenDang closed 1 year ago

AllenDang commented 1 year ago

Add EXR/HDR/TGA/DDS/KTX2/QOI support

Roughsketch commented 1 year ago

That's a lot, thanks for the PR! It'll probably take me a bit to review since I'm always busy so please be patient. I'm also not familiar with ktx2 or qoi files so I'll need to look them up.

One thing I'll note right now is I'm not 100% confident in the TGA header check which is the main reason why it wasn't supported so far. I know there's an optional footer with a more concrete value but I also don't like having to read the end of the file to verify the file type. Before I accept this I might need to verify that the chance of a false positive isn't super common. Don't want a bunch of random files being treated as TGAs after all.

AllenDang commented 1 year ago

@Roughsketch Yes, tga is tricky, currently implementation of type checking is not solid. And may be we should consider to use file extension for type checking first?

Roughsketch commented 1 year ago

File extension checking doesn't work for this crate because I leave the option of parsing from byte arrays or readers which don't have extension information. I'm debating whether or not it makes sense to feature lock it so that people can opt-in if they need it and avoid false positives.

Also while looking up TGA header stuff I found this reference which includes the values 0, 32, and 33 as possible data type codes. Might be something you want to add to the header check.

For now I need to sleep, but I hope to have time to fully look at the rest of this by Sunday.

AllenDang commented 1 year ago

File extension checking doesn't work for this crate because I leave the option of parsing from byte arrays or readers which don't have extension information. I'm debating whether or not it makes sense to feature lock it so that people can opt-in if they need it and avoid false positives.

Also while looking up TGA header stuff I found this reference which includes the values 0, 32, and 33 as possible data type codes. Might be something you want to add to the header check.

For now I need to sleep, but I hope to have time to fully look at the rest of this by Sunday.

tga type checking for 0, 32, 33 is added

Roughsketch commented 1 year ago

Thanks for the updates! I think most everything looks fine now except for the TGA matching stuff.

I'm wondering if it may be better to split TGA into its own PR since I believe the other stuff is fine. Alternatively I can merge the TGA stuff as-is and disable it internally until either you or I take a pass at it. I'm still not fully sure what to do for it but I don't want to hold up the other formats.

If you're done with everything besides TGA then the next step I want to do is run a fuzzer over the new formats to ensure nothing breaks and then after that it should be good to merge.

AllenDang commented 1 year ago

Yes, it's done on my side

Roughsketch commented 1 year ago

@AllenDang I made a PR into your branch with the fuzzer fixes. If it looks good to you go ahead and merge it and then I can do final tests and merge this PR. I'll probably try to figure out what to do with TGA and HDR before releasing it as a version. Hopefully can get it out this weekend.

AllenDang commented 1 year ago

@AllenDang I made a PR into your branch with the fuzzer fixes. If it looks good to you go ahead and merge it and then I can do final tests and merge this PR. I'll probably try to figure out what to do with TGA and HDR before releasing it as a version. Hopefully can get it out this weekend.

It's done! Thanks for the PR.

Roughsketch commented 1 year ago

Thanks again for the PR! Will try to get this up this weekend if I have time to revisit the TGA stuff at the very least.