image-rs / color_quant

Color quantization library
MIT License
35 stars 16 forks source link

no_std support #21

Open n-prat opened 1 year ago

n-prat commented 1 year ago

Hello,

I am currently in the process of porting image and imageproc to work in SGX env, and I hit the issue of this crate using std.

So I am opening the PR to see if this could interest you. And if so, I could clean up the commits, and/or rewrite them a bit more cleanly.

n-prat commented 1 year ago

I am definitely not a Rust expert, so there may be a better way to it.

In any case, without that use num_traits we get:

color_quant$ cargo +nightly test
   Compiling color_quant v1.1.0 (/XXX/color_quant)
error[E0599]: no method named `abs` found for type `f64` in the current scope
   --> src/lib.rs:302:30
    |
302 |             dist = (n.b - b).abs();
    |                              ^^^ method not found in `f64`

error[E0599]: no method named `abs` found for type `f64` in the current scope
   --> src/lib.rs:303:31
    |
303 |             dist += (n.r - r).abs();
    |                               ^^^ method not found in `f64`

error[E0599]: no method named `abs` found for type `f64` in the current scope
   --> src/lib.rs:305:35
    |
305 |                 dist += (n.g - g).abs();
    |                                   ^^^ method not found in `f64`

error[E0599]: no method named `abs` found for type `f64` in the current scope
   --> src/lib.rs:306:35
    |
306 |                 dist += (n.a - a).abs();
    |                                   ^^^ method not found in `f64`

error[E0599]: no method named `round` found for type `f64` in the current scope
   --> src/lib.rs:394:58
    |
394 |             self.colormap[i].b = clamp(self.network[i].b.round() as i32);
    |                                                          ^^^^^ method not found in `f64`

error[E0599]: no method named `round` found for type `f64` in the current scope
   --> src/lib.rs:395:58
    |
395 |             self.colormap[i].g = clamp(self.network[i].g.round() as i32);
    |                                                          ^^^^^ method not found in `f64`

error[E0599]: no method named `round` found for type `f64` in the current scope
   --> src/lib.rs:396:58
    |
396 |             self.colormap[i].r = clamp(self.network[i].r.round() as i32);
    |                                                          ^^^^^ method not found in `f64`

error[E0599]: no method named `round` found for type `f64` in the current scope
   --> src/lib.rs:397:58
    |
397 |             self.colormap[i].a = clamp(self.network[i].a.round() as i32);
    |                                                          ^^^^^ method not found in `f64`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `color_quant` due to 8 previous errors
warning: build failed, waiting for other jobs to finish...
n-prat commented 1 year ago

I have done a bit of clean up: it now works both for cargo +nightly test --no-default-features --features=alloc and cargo +nightly test. My bad, I should have done it this way in the first place.

NOTE: I have put num-traits behind a new alloc feature b/c it seems to be the convention, but I can change it if you want?

PS: the #[allow(unused_imports)] is still needed; I really don't get why. But I quick googling around shows a lot of people doing the same thing.

HeroicKatora commented 1 year ago

NOTE: I have put num-traits behind a new alloc feature b/c it seems to be the convention, but I can change it if you want?

Isn't it possible to use libm::* directly? The traits are just an indirection to floating point methods from that dependency it seems. The majority of the build costs / dependency overhead are in num-traits due to its auto-cfg build dependency.

n-prat commented 1 year ago

Probably not? I am not using the libm feature of num-traits. Indeed FloatCore does not seem to use libm.

HeroicKatora commented 1 year ago

Huh, I wasn't aware num-traits was actually implementing some IEEE math itself without libm. My perception was they'd just defer to one of the available methods.

n-prat commented 1 year ago

Hi,

I have downgraded the edition to 2018 to make it compatible with Rust 1.34(not 100% sure about this one) [even though I don't really see the point of compiling with a 3 years old compiler?]. And more importantly, I have added both default and alloc feature to the CI matrix.

Also, do you have a better idea for the feature's name because alloc is not really appropriate. Maybe num_traits? Something else?

HeroicKatora commented 1 year ago

After some consideration of the API, there's some good ideas for a renovation and 2.0 release anyways. For instance map_pixel and index_of should take arrays respectively. Thus, I'd personally be fine with bumping the minimum-rust-version to 1.56 (or higher if needed). Not to mention that technically the feature selection is breaking if downstream somewhere default-features = false was selected. Eh, little thing.

HeroicKatora commented 1 year ago

Also, do you have a better idea for the feature's name because alloc is not really appropriate. Maybe num_traits? Something else?

Generally, features should be (super-) additive, not subtractive. In this sense, num_traits is a fallback for a missing other feature (std). There's a possible selection between floating point operations from std and those from num_traits. So, num_traits itself would be appropriate to use as a feature. You can use optional dependencies in cfg(feature = …) to detect it.

HeroicKatora commented 1 year ago

So, there could be merely two features: std, and num_traits as an optional dependency. Then the crate would require either to work, and ignore num_traits if std is also selected.

n-prat commented 1 year ago

Allright.

So I have:

FIY this PR is blocking for https://github.com/image-rs/image/pull/1868 But not sure how to set things up b/c color_quant is both a direct dep of image[ok b/c we can bump this create version in the linked PR] and an indirect dep of gif...

n-prat commented 1 year ago

NOTE: if you would rather keep min Rust to 1.34 I could alternatively gate the use alloc behind #[cfg]?

HeroicKatora commented 1 year ago

I'd prefer 1.56 and adding [package.rust-version].

n-prat commented 1 year ago

Done.

I have also removed the if: ${{ matrix.rust in .github/workflows/rust.yml b/c it should not be needed anymore?

n-prat commented 1 year ago

I have added a no_std to the README. Is that OK? Do you require changes?

n-prat commented 1 year ago

I had forgotten to install "cargo-no-std-check". Should hopefully be good now.