image-rs / imageproc

Image processing operations
MIT License
762 stars 151 forks source link

Ergonomics #225

Closed theotherphil closed 7 months ago

theotherphil commented 7 years ago

What can we do to make the implementation and use of this library more ergonomic?

Traits, method chaining, cleaner wrapping/unwrapping/transforming of pixel contents, reducing noise from trait bounds, etc.

HeroicKatora commented 6 years ago

Method types and bounds on various methods of the filter mod are not consistent, e.g.


median_filter<P> restricts to the u8 subpixel type exactly:

P: Pixel<Subpixel = u8> + 'static

It also only support square regions, contrary to many other types.


box_filter feels generally superfluous, its input and output image types being extremely specific. Instead a more general mean_filter might be clearer and more useful.


horizontal_filter<P, K>, separable_filter<P, K>, separable_filter_equal<P, K>, vertical_filter<P, K> are polymorphic over the input subpixel type:

P: Pixel + 'static, <P as Pixel>::Subpixel: ValueInto<K> + Clamp<K>


filter3x3<P, K, S> is instead polymorphic over an input and an output subpixel type, as well as additionally requiring the output pixel type to explicitely implement Primitive:

P::Subpixel: ValueInto<K>, S: Clamp<K> + Primitive + 'static, P: WithChannel<S> + 'static

While some of these issues are likely linked to implementation complexity and/or some performance concerns, it is currently annoying to swap the different algorithms for one another in some cases.

theotherphil commented 6 years ago

Thanks, these are good points. As you suggest, some of these constraints are indeed due to performance concerns, but other others are just because a more general implementation hasn't been written yet.

median_filter: the restriction to u8 subpixels here is required to make the algorithm O(radius) instead of O(radius * radius) in the more general case. However, there's no reason that we can't support non-square kernels (I've created https://github.com/PistonDevelopers/imageproc/issues/250 for this feature).

box_filter: the reason for this function to exist in its current form is to allow filtering with time constant in the size of the filter. I'm not clear what you mean by implementing a mean_filter - is this just a suggestion for a better name, or are you after a function with different behaviour. I've created https://github.com/PistonDevelopers/imageproc/issues/251 to cover adding support for colour images.

Filter trait bounds: yes, the trait bounds are pretty inconsistent. I've created https://github.com/PistonDevelopers/imageproc/issues/252 to look at making them more consistent.

HeroicKatora commented 6 years ago

I see how median_filter doesn't generalize with the same runtime. Nevertheless, shouldn't O(radius*ln(radius)) per pixel be possible through maintaining a sorted list/other structure of the values?

The proposition of mean_filter was just a name change during the interface change and simply the name I would have expected to find the function under (also in symetry to median_filter).

theotherphil commented 6 years ago

Yes, it should be, but I suspect the constants would be a lot worse. I don't have any intention of addressing this particular issue myself, but would be happy to be see someone else have a go.

I agree that mean_filter is a more descriptive name. I think box_filter is more widely used (this is what the filter is called on Wikipedia, for example), but I wouldn't object to a PR changing the name.