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

Expose in place operations on DynamicImage #2292

Closed
wants to merge 3 commits into from

Conversation

Shnatsel
Copy link
Contributor

This is a straightforward pass-through of the existing imageops functions.

@ripytide
Copy link
Member

I thought we were removing the image ops functionality to the imageproc crate, propagating the imageops methods seems to be in conflict with this goal #2238 as they would eventually be deprecated and removed

@Shnatsel
Copy link
Contributor Author

Rotations and flips are required to apply JPEG orientation metadata. We will want reading JPEGs with the correct orientation to "just work" in the future without extra dependencies, so rotations and flips specifically have to stay.

I agree with removing blur, huerotate, etc.

@ripytide
Copy link
Member

But is that not the responsibility of the decoding jpeg library and not something we should expose as a public method on the image types in this crate? Especially since none of the rotation or flipping functions are used anywhere within the crate as far as I can see.

@Shnatsel
Copy link
Contributor Author

Exif metadata can also appear on PNG and WebP images. And none of the format decoders actually parse Exif themselves. So it is preferable to implement it once on top of the different decoders, instead of trying to plumb it into each low-level format decoder.

@ripytide
Copy link
Member

Would you be able to use the flip/rotate functions from the imageproc library when they eventually get released?

@kornelski
Copy link
Contributor

These days EXIF rotation is de-facto required for decoding of JPEG images the way users expect.

For historical reasons it's a bit overcomplicated, but it can be considered a part of the codec now.

Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

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

I commented here but I'd actually rather only expose a single rotate-in-place method. The DynamicImage type already has so many methods that each additional one we add makes it that much harder for users to find the functionality they're looking for

@Shnatsel
Copy link
Contributor Author

Understood. I'll change the newly added methods to private and add a single function for in-place rotation in #2299. Closing in favor of #2299.

@Shnatsel Shnatsel closed this Aug 27, 2024
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.

4 participants