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

Improve panic messages when providing invalid buffer sizes to ImageEncoders #2116

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

MrGunflame
Copy link
Contributor

Replaces the cryptic assertion failed into a more useful panic message when writing a PNG image with invalid buffer/dimensions.

@fintelia
Copy link
Contributor

Thanks for this suggestion, I think a clearer panic message would be an improvement. One note is that you don't actually have to switch to if ... { panic!(...) } since there's a version of the assert_eq! macro that takes a format string as its third argument.

As far as the message itself, I think we might be able to be a bit more concise. Perhaps "Invalid buffer length: expected {} got {}"? The PNG encoding part should be evident from the backtrace into the PngEncoder::write_image` method.

Finally, this same assertion is actually present in all the other ImageEncoder implementations as well, so I think it would make sense to update them all at the same time.

@MrGunflame
Copy link
Contributor Author

As far as the message itself, I think we might be able to be a bit more concise. Perhaps "Invalid buffer length: expected {} got {}"? The PNG encoding part should be evident from the backtrace into the PngEncoder::write_image` method.

I do think it would be helpful to include how the buffer size was derived, i.e. include width and height and maybe even ColorType as that info makes tracking down the panic easier IMO.

@MrGunflame
Copy link
Contributor Author

I think that includes all encoders now. I've also attempted to make the panic message more concise, but if including the width/height is too verbose I'm also fine removing them again.

@kornelski
Copy link
Contributor

Maybe add #[track_caller] on these functions, since the assert is not an internal bug, but the fault of the caller.

@MrGunflame MrGunflame changed the title Provide a "real" panic message when writing an PNG image with bad size Improve panic messages when providing invalid buffer sizes to ImageEncoders Feb 1, 2024
src/codecs/avif/encoder.rs Outdated Show resolved Hide resolved
@fintelia fintelia merged commit d828548 into image-rs:master Feb 2, 2024
35 checks passed
@fintelia
Copy link
Contributor

fintelia commented Feb 2, 2024

Thanks!

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