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

Add a function to check if Orientation can be applied in-place #2329

Closed
wants to merge 1 commit into from

Conversation

Shnatsel
Copy link
Contributor

The doc comment should explain everything. If it doesn't, I'll adjust the doc comment.

@fintelia
Copy link
Contributor

Do you have specific code in wondermagick or elsewhere that needs this? I'm generally hesitant to add APIs to programmatically determine details of how the library is implemented. On the other hand, the need for extra memory will likely remain for a while because I don't expect we'll be adding an in-place transpose implementation any time soon

@Shnatsel
Copy link
Contributor Author

I am not enforcing memory limits in wondermagick from within because it is a losing battle. imagemagick has lots of CVEs about memory limits not being enforced, and with Rust not offering a systematic way to eliminate that on the memory allocator level, I don't think I can do much better. Instead I advise to enforce them on the OS level.

My thinking was that if anything needs to enforce the memory limits, it will need to do this exact thing anyway, so it's best to just implement it once in image instead of having every API user reimplement it.

While this undoubtedly exposes implementation details, these are the kind of implementation details that some users care about. And they are exposed in a way that requires the API user to opt in to knowing them. Rather than having apply_orientation() return Option<ImageBuffer> in case it made a copy and having everyone deal with that, the copying details are abstracted away by default, and you have to to deliberately query this function to find them out.

@fintelia
Copy link
Contributor

fintelia commented Oct 5, 2024

This crate has literally hundreds of public methods. To the point where I regularly see user code that is overly complicated because the author didn't know about some specific method that would do the same thing. Individually, those API additions made sense, but in aggregate things are somewhat of a mess.

There's a lot that could be written about the importance for OSS maintainers saying "no" , and the relative difficulty of deprecating then removing methods compared to adding them, but I think the main takeaway is that we should be very hesitant to add functionality without specific user intent to make use of it.

I think I'll close this PR for now. However, if anyone has specific needs for controlling memory use, I'd love to hear them. Especially given that many of our decoders either ignore memory limits or only enforce them haphazardly.

@fintelia fintelia closed this Oct 5, 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.

2 participants