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

Distinguish between HEIF and HEIC #34

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Conversation

rsuu
Copy link
Contributor

@rsuu rsuu commented May 21, 2024

Now, return Avif for .avif and Heic for .heic/heif.

@virtualritz
Copy link

virtualritz commented Jun 6, 2024

This fails CI because of #35.
I.e. it's unrelated to the PR.

@Roughsketch
Copy link
Owner

Thanks for the PR, I'll try to review it later when I have time.

My main thought at the moment without reviewing is what is the use case for this change? I don't know if having 2 return values for HEIF and HEIC is helpful.

@rsuu
Copy link
Contributor Author

rsuu commented Jun 8, 2024

That because image_type() is now always return Heif on .heic and .avif.

match ... {
    // before 
    // failed: because HEIF is unsupported by image crate
    ImageType::Heif => image::open( .avif or .heic ),

    // after
    ImageType::Heic => heic::open(),
    ImageType::Avif => image::open(),
}

@Roughsketch
Copy link
Owner

Thanks, that makes sense to me.

Initial pass seems probably fine, but I want to take some time to do a code style pass and probably run a fuzzer on it to make sure there aren't any obvious edge cases.

rsuu added 2 commits June 8, 2024 11:32
Signed-off-by: RSUU <[email protected]>
Signed-off-by: RSUU <[email protected]>
@virtualritz
Copy link

We use imagesize for a file transfer tool with UI.
We want to display the right icon for each file type and we do have different icons for HEIF and HEIC.

I.e. for us this would be cool to have, even though we're not connected to this PR.

@Roughsketch Roughsketch merged commit ccd59d3 into Roughsketch:master Jun 17, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants