Skip to content

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented May 10, 2025

Objective

Solution

  • Added #![no_std]
  • Added a polyfill for f64::abs based on the proven implementation from num_traits and comments on this PR.
  • Inlined the usage of f64::round into clamp and combined them into a new method clamp_round.
  • Increased MSRV to 1.36.0, which is required for the alloc crate.

Notes

Since this crate doesn't need std, there's no need to add an std feature, making this PR suitable for a minor patch.

No features required as crate only needs `core` and `alloc`
1.34 does not have access to `alloc`, 1.36 does
Copy link
Member

@197g 197g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One observation that this rework brought to attention: the way the data is shaped, I don't think any of the n.{b,g,r} in the network can actually be NaN and then neither can be their differences. I was initially concerned about performance (and still, might be nice to check with some examples) but really it may be possible to specialize some of the standard functions around the non-NaN property and win performance in that way if the optimizer can not proof the same already.

@bushrat011899
Copy link
Contributor Author

One observation that this rework brought to attention: the way the data is shaped, I don't think any of the n.{b,g,r} in the network can actually be NaN and then neither can be their differences. I was initially concerned about performance (and still, might be nice to check with some examples) but really it may be possible to specialize some of the standard functions around the non-NaN property and win performance in that way if the optimizer can not proof the same already.

I experimented in Godbolt and confirmed that a large number of NaN paths are already optimised out on release builds, so I don't believe we need many changes at this point to eek out maximum performance.

@bushrat011899
Copy link
Contributor Author

I've replaced the implementations in math.rs with a combination of feedback on this PR (thanks to everyone for their suggestions!) and my own experimentation in Godbolt playing assembly golf. A key insight that has informed my changes is that the clamp method is only ever called on a round'ed value, and round is only called inside clamp, so the two methods can be combined and should be optimized together rather than individually.

Doing so removes the performance boost that comes from std::f64::round, so no loss in performance from this PR. However, I think even a moderate loss in performance would've been acceptable, as the alternative is the addition of an std feature, and thus conditional compilation, adding complexity to testing for little gain.

Copy link
Member

@197g 197g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite polished to me now. @fintelia I'd say we go for a release under 1.* and at worst yank that if there are too many people still stuck under 1.34 as that would be highly unexpected to me.

@bushrat011899
Copy link
Contributor Author

Looks quite polished to me now. @fintelia I'd say we go for a release under 1.* and at worst yank that if there are too many people still stuck under 1.34 as that would be highly unexpected to me.

Checking crates.io, there are only 5 listed dependents on color_quant that aren't themselves obviously incompatible with Rust 1.34:

All other dependents either list an MSRV above 1.34, or are on edition 2021/2024 (and thus incompatible with Rust 1.34). gomander-engiffen depends on image 0.25 so that's incompatible with Rust 1.34. image-go-nord is also incompatible due to using newer Rust features in its manifest. The others currently don't compile on 1.34 for various other reasons (mostly due to their other dependencies updating).

As such, I think the bump from Rust 1.34 to 1.36 should have zero impact on the crates.io ecosystem. If someone is affected by this, it can be easily mitigated by adding the following to their own Cargo.toml (even if it's a transitive dependency that is broken):

[dependencies]
color_quant = "=1.1.0"

This will peg them to the exact version that is last compatible with Rust 1.34.

@fintelia
Copy link
Contributor

The MSRV bump is fine, and is in fact wildly more conservative than we've been using for any of the other image-rs crates.

@197g 197g merged commit 0c9c839 into image-rs:master May 14, 2025
9 checks passed
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.

4 participants