Skip to content

Conversation

bushrat011899
Copy link

@bushrat011899 bushrat011899 commented May 8, 2025

Objective

Solution

  • Increased MSRV to 1.81 for core::error::Error
  • Added libm and std features.
    • std provides access to the std crate and is required for most existing features.
    • libm activates num-traits/libm, allowing for existing f32/f64 operations to work without std.
  • Addressed several Clippy lints using expect rather than allow
    • This ensures stale exceptions don't remain
  • Added a no_std CI task which checks against wasm32v1-none, a typical no_std target. This could be replaced with any other official target that does not have std.
  • Added #[cfg(feature = "std")] to items which require std
  • Added #[cfg(any(feature = "std", feature = "libm"))]to items which require fullf32/f64` support.
  • Made ImageError non_exhaustive to allow feature gating ImageError::IoError behind std. This isn't strictly required, but it ensures features are truly additive.
  • Imported num_traits::Float or num_traits::float::FloatCore where required for f32/f64 operations.
  • Increased version to 0.26 for semantic versioning reasons.

Notes

  • I have not attempted to add no_std support to any of the format features, as that will require careful consideration around how to decouple from std::io. For example, ciborium-io (or an equivalent trait local to image) could be used as a no_std-specific feature.
  • Increasing MSRV is only required for core::error::Error. Instead of the increase, a feature could be added to enable usage of core::error::Error. I opted to just increase MSRV for simplicity. 1.70 to 1.81 isn't too large of a jump IMO.
  • The bmp format looks to be the easiest to make work on no_std (almost easy enough to consider including in this PR if it wasn't for the open ended nature of how to handle a replacement for std::io::Write/Read). That should be the first format to investigate after this PR is potentially merged.
  • This is my first time contributing to this project, please let me know if there's anything I can do to help!

Brings support for `core::error::Error`
Included in all relevant existing features
Simplifies access to floating point operations in `no_std` contexts.
Highlights when `std` is being used when it doesn't have to be.
Testing against `wasm32v1-none` as a target which is known to not have an implementation of `std`, making compilation a sufficient test for general `no_std` compatibility.
`no-default-features` is separately checked using Clippy
@bushrat011899
Copy link
Author

bushrat011899 commented May 8, 2025

Unsure how to proceed regarding the cargo-semver-checks CI failure. I believe increasing the version number from 0.25 to 0.26 should be enough, but that didn't appear to work when I tested it locally.

Confirmed with cargo-semver-checks that increasing the version number to 0.26.0 is a sufficient major patch to permit the changes to ImageError. The CI task runs with an explicit --release-type minor flag, so it will continue to fail since it overrides the actual version number check.

Verified with `cargo semver-checks check-release --default-features`
@fintelia fintelia mentioned this pull request May 9, 2025
12 tasks
@fintelia
Copy link
Contributor

fintelia commented May 9, 2025

Thanks for putting this together! Seeing how much of the crate works with no_std (including ImageBuffer, DynamicImage, imageops, etc.) I do think this is worth doing.

Figuring out how to replace std::io::Write/Read is really the limiting factor for having encoding/decoding with no_std. I'm not sure there even is a way to do so without either making the feature non-additive or using non-standard traits for both std and no_std. At some point, the few remaining blockers to getting alloc::io::Error into the standard library might end up getting sorted out, which would be the ideal outcome.

As for breaking changes, the plan is for the next major release to be 1.0. We're tracking all the possible/planned changes for that release in #2245.

@bushrat011899
Copy link
Author

Thanks for putting this together! Seeing how much of the crate works with no_std (including ImageBuffer, DynamicImage, imageops, etc.) I do think this is worth doing.

Exactly my opinion. I've helped add no_std support to a few crates now, and it can be surprising just how much functionality exists just within core and alloc.

Figuring out how to replace std::io::Write/Read is really the limiting factor for having encoding/decoding with no_std. I'm not sure there even is a way to do so without either making the feature non-additive or using non-standard traits for both std and no_std.

I have been experimenting with this issue, and I think there are two paths forward:

  1. Update byteorder-lite upstream (I understand it's controlled by image-rs so this shouldn't be too painful) to abstract the std::io traits using the same technique as ciborium-io. The trick it uses is an additive feature allowing a single trait to be used by the API (ciborium_io::Read/Write) which can always be implemented by consumers (and has provided implementations for the relevant core and alloc types), but will also be blanket implemented for any type implementing the corresponding std::io trait.
  2. Use a sans-io approach as the internal implementation, and then expose it for external consumers. This would be a larger change to the API, but might have less code overall.

I personally think path 1 is the more viable option as it'd keep both internal and external usage of image largely the same.

At some point, the few remaining blockers to getting alloc::io::Error into the standard library might end up getting sorted out, which would be the ideal outcome.

This would be the ideal, completely agree. But I'm not sure how far away that future is at this point.

As for breaking changes, the plan is for the next major release to be 1.0. We're tracking all the possible/planned changes for that release in #2245.

Oh well that's good timing! Adding an std feature to control this functionality would've been a breaking change, hard to add after 1.0! I would say that if we can't get more functionality working in no_std before 1.0, it could be added in 1.1 as long as we're able to keep the public interfaces compatible.

@bushrat011899
Copy link
Author

color_quant/#24 adds no_std support which is relevant to this PR. I would recommend trying to get this merged.

@fintelia
Copy link
Contributor

Figuring out how to replace std::io::Write/Read is really the limiting factor for having encoding/decoding with no_std. I'm not sure there even is a way to do so without either making the feature non-additive or using non-standard traits for both std and no_std.

I have been experimenting with this issue, and I think there are two paths forward:

  1. Update byteorder-lite upstream (I understand it's controlled by image-rs so this shouldn't be too painful) to abstract the std::io traits using the same technique as ciborium-io. The trick it uses is an additive feature allowing a single trait to be used by the API (ciborium_io::Read/Write) which can always be implemented by consumers (and has provided implementations for the relevant core and alloc types), but will also be blanket implemented for any type implementing the corresponding std::io trait.

  2. Use a sans-io approach as the internal implementation, and then expose it for external consumers. This would be a larger change to the API, but might have less code overall.

I personally think path 1 is the more viable option as it'd keep both internal and external usage of image largely the same.

This is a very big topic so it probably warrants its own thread. Depending on the details, it could require simultaneous breaking changes across a dozen or more crates, many of which aren't controlled by image-rs. Additionally, a big benefit of the current API is that the traits from std::io are already familiar for most Rust users. That means lower friction and an easier time for beginners to get help if they run into difficulty. We also have to be mindful of performance, and make sure that any changes don't regress that aspect.

@197g
Copy link
Member

197g commented May 12, 2025

Since you've done the std transform in many crates, one central question is how are we to maintain this? As in how does this affect the ability to accept patches and new features.

  • It seems (with regards to libm and feature flags) that we have to tread somewhat careful not to populate the dependency tree with anything inconvenient as to say. How exactly is inconvenient defined and importantly, will we catch this in CI or manually?
  • The fallout from std::io was already mentioned. But also importantly, I disagree with the statement that the referenced implementation actually delivers what is claimed. Enabling the std feature in that crate adds a blanket implementation. Adding blanket implementations is a major breaking change according to the Rust semver specification. In particular, such a blanket implementation will conflict with any manual implementations on the same type. Thus all such no-std impls must be cfg-restricted to apply only when the ciborium_io/std feature is disabled. This works in the crate itself, but for reader wrappers outside, hardly. Tooling support for catching this just doesn't exist and it may result in compilation failures from a sibling dependency on ciborium_io/std if any such type is defined.
  • All kinds of locks must be changed spin iirc? Any downsides to this, in particular for the std users.
  • Will there come a point where we need to tradeoff performance vs. no-std support? If so, is there a good guidance on how one can achieve this correctly with a seamless experience when std is defined? It's already somewhat hard to test performance comprehensively. (What springs to mind is multi-threading, i.e. the rayon complications we had but maybe worse. I'd like to be wrong here, some write-up of this in practice would be nice to read).

@bushrat011899
Copy link
Author

Since you've done the std transform in many crates, one central question is how are we to maintain this? As in how does this affect the ability to accept patches and new features.

Excellent question. The aspirational solution here is to make the no_std subset of the crate useful enough for enough people. For example, serde is no_std and a large number of its users are likely unaware that disabling default features and enabling derive means they're a no_std user.

Since the only way to use std in a crate is to either not be #![no_std] or include extern crate std; explicitly, compilation on a no_std target is largely sufficient when combined with std-enabled testing. This PR includes that in CI against wasm32v1-none, likely to be the most important single no_std platform going forward (as it's the only way to have high-compatibility WASM builds).

* It seems (with regards to `libm` and feature flags) that we have to tread somewhat careful not to populate the dependency tree with anything inconvenient as to say. How exactly is inconvenient defined and importantly, will we catch this in CI or manually?

Right now, any non-compliant dependencies (including transitively) will be caught at compile time in CI without exception. no_std is an explicit declaration that must be evaluated at compile-time, so no "runtime" testing is required to assure this.

* The fallout from `std::io` was already mentioned. But also importantly, I disagree with the statement that the referenced implementation actually delivers what is claimed. Enabling the `std` feature in that crate adds a blanket implementation. Adding blanket implementations is a major breaking change according to the Rust semver specification. In particular, such a blanket implementation will _conflict_ with any manual implementations on the same type. Thus all such no-std impls must be cfg-restricted to apply _only_ when the `ciborium_io/std` feature is disabled. This works in the crate itself, but for reader wrappers outside, hardly. Tooling support for catching this just doesn't exist and it may result in compilation failures from a sibling dependency on `ciborium_io/std` if any such type is defined.

After experimenting I do agree that the trait would probably need to be sealed (or potentially marked unsafe? Worth considering). The semver-break comes from implementing both ciborium_io::Write and std::io::Write when ciborium_io/std is enabled. I would say that we might not need to poly-fill std::io. For example, some methods in image don't accept a generic std::io type (e.g., dyn Read), they instead accept a closure for the particular method they need, similar to a C-style V-Table object. Instead of using this technique ad-hoc, maybe it could be formalised and exposed to end-users as a sort-of "raw" version of the recommended std::io-based API.

* All kinds of locks must be changed `spin` iirc? Any downsides to this, in particular for the `std` users.

It can be a zero-cost option pretty easily. I created bevy_platform for Bevy as a way to handle std::sync (and others). It provides a consistent API for the sync primitives and features control the back-end implementation (std, spin, and we've even discussed parking_lot). Since it's internal to the API there's no semver concern here. This is the same technique used by wgpu to provide either std, parking_lot, or their own custom dead-lock detecting backend, so lots of precedent here.

* Will there come a point where we need to tradeoff performance vs. no-std support?

In my experience, no. no_std lends itself to simplicity (since you don't have the std crate to provide that baseline complexity boost), and away from syscalls/allocation entirely. Since embedded devices (the prime audience for no_std) are typically so resource constrained, their libraries are designed for maximum performance. So far, this has been my experience in wgpu, tracing, and Bevy as some examples.

  If so, is there a good guidance on how one can achieve this correctly with a seamless experience when `std` is defined? It's already somewhat hard to test performance comprehensively.

I would say that if this is a concern, the normal feature guidelines apply; as long as features are truly additive testing shouldn't be too onerous.

@bushrat011899
Copy link
Author

[...] a big benefit of the current API is that the traits from std::io are already familiar for most Rust users. That means lower friction and an easier time for beginners to get help if they run into difficulty. We also have to be mindful of performance, and make sure that any changes don't regress that aspect.

Absolutely on both fronts. The image APIs reliant on std::io should absolutely remain and be considered the recommended way to work with the crate. My ideal would be that those public APIs wrap a lower-level (but also public) API that is independent of std::io. If that can be done, and provided that abstraction is visible to the compiler (e.g., no opaque function pointers) then we should be able to have our cake and eat it too.

@bushrat011899
Copy link
Author

As a proof-of-concept for wider no_std support, I've updated the qoi feature to be as-complete-as-possible in no_std, short of much larger changes to the rest of the public API. I want to bring particular attention to the QoiEncoder type, as that highlights the way a no_std API can be clean without a high maintenance burden or challenging feature flags.

I think the clearest path forward at this point would be to use the same technique employed by qoi, with privately implemented Write, Read, etc. traits which will either blanket-impl when std is enabled, or only be implemented for some subset of types (e.g., Vec<u8>) which would already be covered by the blanket-impl.

This is based on ImageReader::make_decoder, which I think highlights a critical minimum API for reading images (BufRead + Seek).

@fintelia
Copy link
Contributor

I think looking at the QOI codec might give a false impression of the work involved in moving encoding/decoding over to no_std. Others are far more complicated. The TIFF and WebP decoders do a bunch of seeking around the file. The png and tiff crates rely on flate2 which doesn't support no_std. etc.

QOI, by constrast, is a comparatively simple format and already exposes a no_std interface. And if you look into the details, they actually have separate implementations for the std::Read decoder and byte-slice decoder.

@197g
Copy link
Member

197g commented May 12, 2025

If you want to read over Vec<u8> and &'_ [u8] only, I might suggest zune-image. The implementation quality is solid and the author knows what they're doing. As coincidence would have it however, its support table is missing those two formats that @fintelia mentioned directly. I'm doubtful of the purpose of a flag turning image into zune-image. What we've done with zune-jpeg seemed quite promising, we let competitive implementations play out and then integrated when we have a clear picture. This avoids the complexity problems of all the intermediate states where the architecture doesn't serve either structure.

That is, I really like the approach of no_sd support bottom up. I'd welcome your eyeballs on reviewing the fitness of image-texel/image-canvas as a no-std image data buffer that may eventually end up here to enable all the non-rgb data representations. (This PR already brought up that it could use atomic-polyfill instead of the core atomics. Good points). And tiff will get some more of my attention this year, I'll keep the no-std focus in mind. no-std and sans-IO synergize well, and the latter will also help with async support that's already been highly requested. But image is a bit too high-level still, it's not ready for it.

@bushrat011899
Copy link
Author

@HeroicKatora and @fintelia, I've opened image-gif/#200 to not just add no_std support to one of image's dependencies, but to also test out a general approach to replacing std::io in a zero-compromises way. Would appreciate some eyes there, as the discussions here and there are closely related.

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.

no_std support
3 participants