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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving imageops and similar functionality to the imageprocs crate #2238

Open
ripytide opened this issue May 18, 2024 · 8 comments
Open

Moving imageops and similar functionality to the imageprocs crate #2238

ripytide opened this issue May 18, 2024 · 8 comments

Comments

@ripytide
Copy link
Contributor

ripytide commented May 18, 2024

Tracking Table

Key

  • 馃煡: Not yet in imageproc
  • 馃煣: In imageproc but not yet released.
  • 馃煪: Released in imageproc but still in image
  • 馃煩: Deprecated in image
function status equivalent in imageproc
blur 馃煪 filter::gaussian_blur_f32
crop 馃煣 compose::crop (slightly different in that it returns an entirely new image vs returning a SubImage that does no processing)
crop_imm 馃煡 cannot be moved as it returns a SubImage that does no processing, compose::crop can be used instead but returns a new image
filter3x3 馃煣 filter::filter or filter::filter_clamped
flip_horizontal 馃煣 compose::flip_horizontal
flip_horizontal_in 馃煡 None
flip_horizontal_in_place 馃煣 compose::flip_horizontal_mut
flip_vertical 馃煣 compose::flip_vertical
flip_vertical_in 馃煡 None
flip_vertical_in_place 馃煡 compose::flip_vertical_mut
horizontal_gradient 馃煡 None
interpolate_bilinear 馃煣 Implemented but private and needs refactoring relevant issue
interpolate_nearest 馃煣 Implemented but private and needs refactoring relevant issue
overlay 馃煣 None
overlay_bounds 馃煡 Only makes sense as a helper function for overlay and replace so I don't see the point migrating this a public function
replace 馃煣 None
resize 馃煡 None
rotate90 馃煣 geometric_transformations::rotate but it takes a theta so not as precise
rotate90_in 馃煡 Same as above
rotate180 馃煣 Same as above
rotate180_in 馃煡 Same as above
rotate180_in_place 馃煣 Same as above
rotate270 馃煣 Same as above
rotate270_in 馃煡 Same as above
sample_bilinear 馃煡 None
sample_nearest 馃煡 None
thumbnail 馃煡 None
tile 馃煡 None
unsharpen 馃煡 Nearly, filter::sharpen_gaussian exists but lacks the threshold parameter and doesn't work on color images relevant issue
vertical_gradient 馃煡 None
colorops::brighten 馃煡 None
colorops::brighten_in_place 馃煡 None
colorops::contrast 馃煡 Nearly, contrast::stretch_contrast exists but doesn't work on color images yet
colorops::contrast_in_place 馃煡 Same as above
colorops::dither 馃煡 None
colorops::grayscale 馃煡 None
colorops::grayscale_alpha 馃煡 None
colorops::grayscale_with_type 馃煡 None
colorops::grayscale_with_type_alpha 馃煡 None
colorops::huerotate 馃煡 None
colorops::huerotate_in_place 馃煡 None
colorops::index_colors 馃煡 None
colorops::invert 馃煡 None

Original Comment

At the moment the image crate has several image processing functions in
the imageops module which seems a bit strange to me as they look like
they would better belong in the imageproc crate. Is there a reason why
such functionality is in this crate, or is perhaps a holdover from long
ago?

I would be happy to do the work migrating/integrating the functionality
into imageproc if you'd like.

@fintelia
Copy link
Contributor

The imageops module was added long before my time and has gotten very little attention in the last few years. It contains some methods like crop or resize that totally make belong in the crate, but also has others like huerotate that probably aren't necessary. I'd be open to moving functionality, but we should discuss on a case-by-case basis first

@ripytide
Copy link
Contributor Author

Great, image-rs/imageproc#7 is the corresponding issue on the imageprocs crate.

My current action plan is to write up a list of all the functions in imageops and then individually integrate each one into imageprocs. After each function is integrated and released that would give you the ability to deprecate those individual functions and point users at the corresponding functions in the imageprocs crate. Then at some point in the future you would have an easier time deleting the deprecated functions hoping most users will have migrated.

How does that sound?

@theotherphil
Copy link
Contributor

theotherphil commented May 19, 2024

As the image crate has a huge number of users and very little activity happens on the imageops code, the cost of maintaining duplicated functionality is low but the impact of breaking changes is potentially high.

I'm in favour of:

  1. Adding all current imageops functionality to imageproc, either vs re-exports or re-implementations depending on whether we want to tweak anything.
  2. Adding a note in the imageops module (via code comments/readme/PR template/whatever) that new image processing functionality belongs in imageproc.

The question of deprecation or retiring existing imageops functionality is trickier and thankfully not up to me!

(Edit to add context for other readers: I鈥檓 the maintainer of the imageproc crate.)

@fintelia
Copy link
Contributor

Yeah, this would involve deprecating the methods now and only removing in the next major release (which won't be any time soon given we haven't even started planning it yet)

During this process it is also worth looking at whether these methods are using internal functionality / are tightly tied to the image crate. Like the invert method only works because one of the required methods on the Pixel trait is Pixel::invert. The APIs around writing image processing code that's generic across image type is something I've held off working on because it seemed like there were no elegant options and any changes would just cause a lot of breakage

@ripytide
Copy link
Contributor Author

I've written up the list in a tracking table in the top comment which can be updated as we make progress toward this larger goal. Let me know if you'd like to change anything or if I've missed anything.

@fintelia
Copy link
Contributor

To clarify, the last step should probably be deprecated from image crate. We'll remove all the depreciated items in one batch when we do the 0.26 release in 6-12 months (or whenever it happens... haven't started planning that release yet)

@ripytide
Copy link
Contributor Author

ripytide commented May 20, 2024

I've finished my case-by-case analysis of the current state of things and it doesn't look great, 1/43 functions has an equivalent version in imageproc. This might be more work than I anticipated 馃槃

@johannesvollmer
Copy link
Contributor

As the image crate has a huge number of users and very little activity happens on the imageops code, the cost of maintaining duplicated functionality is low but the impact of breaking changes is potentially high.

This is true, but removing that functionality still has great value for simplifications across the library. For example, the Pixel::invert functionality is only required for image processing, but it is not helpful when trying to integrate non-standard decoders for niche pixel formats.

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

No branches or pull requests

4 participants