image icon indicating copy to clipboard operation
image copied to clipboard

basic no_std support

Open n-prat opened this issue 2 years ago • 2 comments

Hello,

Following up on https://github.com/image-rs/image/issues/1184#issuecomment-1443804914 I am opening a PR to discuss about potential no_std support.

Copy-pasting and rewriting my comment:

TODO?:

  • it still needs more work: I had to butcher error.rs, but I can probably do better.
    • I could optionally move the whole "error.rs refactor" into a separate PR [e.g. replace custom errors by Snafu which supports no_std]
  • I had to be liberal with e.g. #[cfg(feature = "std")] but I can probably replace most of them by a new e.g. io feature.
    • instead of deactivating all io related code, there are other options: https://docs.rs/binrw/latest/binrw/ and https://github.com/dataphract/acid_io But I do not know these libs so not sure if going that way is a good idea?
  • Right now I just aimed to make the core lib compile b/c that is my use case [I need this crate b/c it is a dependency of imageproc which is the one I use directly] → making each and every feature work under no_std is a different beast I am guessing

I'm open to feedback!

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option.

n-prat avatar Mar 01 '23 14:03 n-prat

One comment I want to add outside the workflow of the PR review: If we need to get rid of a core buffer data structure, rewrite the DecoderTrait, etc. completely. Then I'm pretty open to that in a major version as long as there is a clear plan on how to push such rework forward, demonstration of persistent to complete the rework, and keep the test suite in tact while doing it (in a separate branch). That is, somehow keeping track of features that need to be ported. But a new major version is still better than needing an ecosystem split to a different crate, no?

197g avatar Mar 01 '23 22:03 197g

Except: Getting rid of/bounding DynamicImage to std is quite a deal breaker for adoption. It would seem that an alloc-but-no_std should be capable of preserving the type, or not?

I will try to make it work under no_std but my initial test shows it is not easy. Indeed it requires eg decoder_to_vec which in turn requires ImageDecoder::read_image_with_progress which in turn requires type Reader: std::io::Read + 'a I do agree that it should ideally be refactored b/c it makes no sense for "fn with slice input" to require std::io. edit: I have tried a first pass re-activating most parts of DynamicImage; the parts related to free_functions are still excluded

But why would it be a dealbreaker?

  • people using default feature would not see a difference
  • people requiring no_std don't really expect 100% of the features to be available Eg for my use case: I need image as a transitive dep of imageproc; and the parts of imageproc I use work apparently fine without DynamicImage cf https://github.com/Interstellar-Network/imageproc/tree/sgx-nostd-compat for the fork, and https://github.com/Interstellar-Network/lib-garble-rs/blob/main/lib-garble-rs/src/watermark.rs for how I use it

edit: don't really look at the imageproc fork's code; I need to completely rewrite it

n-prat avatar Mar 02 '23 12:03 n-prat