Possible to implement for image::Rgb? about kmeans-colors

[okaneco]

Thanks for the issue, this is a bit of a gap in the library currently and I understand not wanting to use another dependency.

The kmeans calculation is done on a floating point component type. When I started this crate, image didn't support f32 images so I never thought about supporting their color struct before. The palette crate is convenient for me because of being to easily convert to Lab and Rgb<f32>. I also helped to optimize the component conversion between u8 and f32.

I was originally a little hesitant to support image::Rgb because

1. I don't personally use it, and
2. it increases maintenance burden as another public-facing feature that forces this crate to bump versions whenever they update their version

but I see the value in adding it for users. The current implementation is written in a less clear way than it used to be so it's harder to implement for your own types. I'll work on adding documentation for implementation and annotating the current implementation with comments. This was the previous version of the Srgb implementation which was a bit easier to copy/paste and replace for your own type
https://docs.rs/kmeans_colors/0.5.0/src/kmeans_colors/colors/kmeans.rs.html#88-162

After this is implemented, you'll need to use image::Rgb<f32> for kmeans. If you have an RgbImage you'll need to wrap it in a DynamicImage variant and then use into_rgb32f because RgbImage defaults to Rgb<u8>. Otherwise, image::open into an image::Rgb32FImage.

Another thought I had was implementing it for &[[T; 3]] so you wouldn't need the image::Rgb feature but I'll probably get around to that later.

Summary

I'll try to add an implementation for image::Rgb<f32> or image::Rgb<T: num_traits::Float> (in case f64 is supported some day in image).

[KyleMaas]

Awesome! That would save a bunch of the conversion I'm having to do right now. Thanks!

[KyleMaas]

By the way, this is what I'm using it for:

https://github.com/KyleMaas/realworldarchive

Specifically, the palette detection:

https://github.com/KyleMaas/realworldarchive/blob/89655e1a91eaea5600829a888ff95100c42c8947/src/color_multiplexer.rs#L111

The code quality is horrible on this project - it's very much experimental at this stage - but it does work pretty well.

[KyleMaas]

I mean, heck, if you could cluster directly on HSL pixels, that'd be pretty awesome, too. But I'd think that would be of far less utility to most people.

[okaneco]

I mean, heck, if you could cluster directly on HSL pixels, that'd be pretty awesome, too. But I'd think that would be of far less utility to most people.

That's why I'll probably implement the traits for something like [T; 3]. It'd have the benefit of being able to use the same impl for any arbitrary 3 component float color type (RGB, HSL, HSV, etc.) in a [f32; 3] array format. You could use bytemuck to cast from a &[f32] (that you'd get from calling ImageBuffer::as_raw on an Rgb32FImage) to a slice of arrays, or a buffer you made of HSL colors in the image. playground link

use bytemuck; // 1.13.1
use image::{ImageBuffer, Rgb32FImage}; // 0.24.6

fn main() {
    let data = vec![0_f32, 1., 2., 3., 4., 5.];
    let x: Rgb32FImage = ImageBuffer::from_raw(2, 1, data).unwrap();
    let y: &[[f32; 3]] = bytemuck::try_cast_slice(x.as_raw()).unwrap();
    dbg!(y); // [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0],]
}

---

Thanks for sharing your code, that gives me a better idea of what you're using it for.

For the convergence factors, I wish I had a better resource to point to but I ended up at those numbers from experimentation.

I know you're interested in removing dependency on palette but I had a few quick things to say on that. The errors can be slightly hard to understand at first but the maintainer is really helpful about answering questions if you have any in discussions. palette supports converting to/from Hsl and to/from u8/f32 easily so I find it convenient, but if what you're using already covers what you need then there's no need to use it too. I typically do a lot of things with Lab, so my workflow with image is to cast the decoded image file from Rgb to the palette types right away and save images by passing a Vec<u8> to the encoders so I never have to touch image::Rgb.

Two little things that probably won't matter anyway if you swap out palette but figured I'd share.

into_format should convert from u8 to f32 for you
https://github.com/KyleMaas/realworldarchive/blob/89655e1a91eaea5600829a888ff95100c42c8947/src/color_multiplexer.rs#L131

.map(|x| Srgb::new(x[0], x[1], x[2]).into_format()) // might need ::<_, f32>() annotation

This part could probably be rewritten to something like the following. In general with image::Rgb, I find it easier to construct my own [u8; 3] elsewhere and then pass it to the tuple as a single variable.
https://github.com/KyleMaas/realworldarchive/blob/89655e1a91eaea5600829a888ff95100c42c8947/src/color_multiplexer.rs#L164-L168

let color: [u8; 3] = c.into_format().into();
colors_rgb.push(Rgb(color));
colors_hsl.push(HSL::from_rgb(&color));

[okaneco]

I've done a first pass at #56. With this, you'll be able to use it for Hsl but you'll have to normalize the hue from [0.0, 1.0] if it's [0, 360]. (This is a shortcoming I noticed in the current design of the library while adding this, but it's a bigger change I want to address in a breaking version in the future)

I have a separate enhancement I want to investigate, but I should be able to push a new update so you don't need palette within the next few days.

[KyleMaas]

Cool. Thank you for the pointers. It'll be a little bit before I can test this, but I'll let you know. That'd be great if I could cluster on HSL without normalization - is it possible you could file an issue on that refactor so I can subscribe to it and know when you have it working?