Skip to content

fix: make set_icc_profile function available#2322

Merged
fintelia merged 1 commit intoimage-rs:mainfrom
misl-smlz:icc_profile
Oct 7, 2024
Merged

fix: make set_icc_profile function available#2322
fintelia merged 1 commit intoimage-rs:mainfrom
misl-smlz:icc_profile

Conversation

@misl-smlz
Copy link
Copy Markdown
Contributor

@misl-smlz misl-smlz commented Sep 13, 2024

We need to set the icc profile for webp. This PR makes the set_icc_profile function available from the outside.

@misl-smlz misl-smlz mentioned this pull request Sep 13, 2024
Closed
@fintelia
Copy link
Copy Markdown
Contributor

fintelia commented Sep 14, 2024

I think this should probably be added as a method on ImageEncoder, with a default implementation that returns an unsupported error

@misl-smlz
Copy link
Copy Markdown
Contributor Author

misl-smlz commented Sep 25, 2024

@fintelia I've changed it the way you suggested

197g
197g reviewed Sep 26, 2024
View reviewed changes
src/image.rs Outdated
Comment on lines +785 to +808
fn set_icc_profile(&mut self, icc_profile: Vec<u8>) {
let _ = icc_profile;
panic!("ICC profiles are not supported for this format");
}
Copy link
Copy Markdown
Member

@197g 197g Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @fintelia meant an error, not a panic. We're already seeing bug reports for returning honest and correct error returns when calling functions on formats that do not support them (e.g. encoding floats into a png). I can't imagine error handling by panic to fare any better, it's probably significantly worse.

kornelski
kornelski previously requested changes Sep 26, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kornelski kornelski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should return Result<(), …>.

I'm not sure if a generic error or a dedicated UnsupportedError.

it would be good to also document that it doesn't check ICC for validity at this point, so doesn't guarantee to fail if the profile is invalid.

@misl-smlz
Copy link
Copy Markdown
Contributor Author

misl-smlz commented Oct 4, 2024

@kornelski Now it returns an Result

@fintelia
Copy link
Copy Markdown
Contributor

fintelia commented Oct 5, 2024

Looks like there's a few compile errors:

error[E0433]: failed to resolve: use of undeclared type `UnsupportedError`
   --> src/image.rs:798:13
    |
798 |         Err(UnsupportedError::from_format_and_kind(
    |             ^^^^^^^^^^^^^^^^ use of undeclared type `UnsupportedError`
    |
help: consider importing this struct
    |
2   + use crate::error::UnsupportedError;
    |

fintelia
fintelia reviewed Oct 6, 2024
View reviewed changes
src/image.rs Outdated
Comment on lines +793 to +795
/// # Panics
///
/// Panics if the ICC profile is not implemented for the format.
Copy link
Copy Markdown
Contributor

@fintelia fintelia Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update

@fintelia fintelia merged commit a373218 into image-rs:main Oct 7, 2024
@fintelia
Copy link
Copy Markdown
Contributor

fintelia commented Oct 7, 2024

Thanks!

@fintelia fintelia mentioned this pull request Oct 14, 2024
Closed
@fintelia fintelia mentioned this pull request Dec 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@197g 197g 197g left review comments

@fintelia fintelia fintelia approved these changes

@kornelski kornelski kornelski left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@misl-smlz @fintelia @kornelski @197g