Add no_std support #226

vstroebel · 2022-02-18T11:36:30Z

This is my attempt to make this crate usable on no_std.

I've splitted this changes into several commits to make it easier to review:

Introduce trait JpegRead as an replacement for std::io::Read. On std JpegRead is implemented for every std::io:::Read which should allow users to use every type implementing Read, too.
Some use relate changes that Move most uses of the std crate to core and alloc #196 didn't cover
Integer only replacement for f32::ceil based code that isn't available in ´core´
An ugly replacement for f32::fract. Maybe someone has a better idea to implement this for no_std.
An integration test that works on no_std. The integration test suite heavily depends on file system ops and the png crate. Having at least one test that works on no_std is better than nothing....
Add a std feature and use it as a marker for std/no_std.
Change github action to test std and no_std code

This should fix #199

f32::ceil isn't available in no_std

HeroicKatora

Opinion: This affects too much code, too visibly. Infecting and changing all public bounds is just one uncomfortable change that most users won't be positively affected by. The structure of this implementation forces us to do all-or-nothing, however I don't think this ready for merging. I'd like at least a significantly better test coverage of individual (and somewhat awkward) floating point workarounds, and that they not be present in the usual configuratio. Some more detailed concers with specific implementation choices are listed below.

Overall, I would much rather we have crate that addresses those concerns separately. One that can be shared across multiple of image-rs crates for no_std compat (png, tiff would be good starts) and provides Seek maybe as well. With focussed test coverage, orthogonality of design. There are plenty but none I'm aware of have been too convincing regarding the SemVer / compatibility problems briefly mentioned below.

HeroicKatora · 2022-02-18T12:50:15Z

src/reader.rs

+#[cfg(feature = "std")]
+impl<T: Read> JpegRead for T {


This doesn't work perfectly with features. If you include the crate with no-default-features, then this impl does not exist and consequently, anyone can legally define a type and impls like so:

struct SomeReader; impl std::io::Read for SomeReader { … } impl JpegRead for SomeReader { … }

However, then another unrelated dependency may include jpeg-decoder with feature = std and suddenly those two impls collide as the first implies this impl block, which conflicts with the second. This isn't necessarily a deal breaker, it's not unsound, but it's rather odd and unpredictable.

HeroicKatora · 2022-02-18T20:05:54Z

src/reader.rs

+            *self = remaining;
+            Ok(())
+        } else {
+            panic!();


This isn't very complete yet, right? And that's probably why I'm a bit wary introducing a new trait just for jpeg-decoder and then have it replicate mostly internals from what's usually in std.

And now that it existed once, my desire for much more extensive test coverage of all the impls has grown quite a lot.

HeroicKatora · 2022-02-18T20:12:31Z

src/error.rs

+    #[cfg(feature = "std")]
    /// An I/O error occurred while decoding the image.
    Io(IoError),
+
+    #[cfg(feature = "std")]
    /// An internal error occurred while decoding the image.
    Internal(Box<dyn StdError + Send + Sync + 'static>), //TODO: not used, can be removed with the next version bump


Having extensively worked with no_std-but-std-compatible crates in networking, variants guarded behind cfg flags are a major pita. This seems like the biggest risk to reject the PR.

If the enum itself is not non_exhaustive then consumers intending to match on these variants will having to work quite awkwardly around the presence and absence of certain variants/branches. Especially since it depends on the feature of a dependency, which they may not want to publicly expose as a dependency/toggle themselves. This works just good enough if the program is small and jpeg-decoder is a direct dependency but doesn't scale well. (Again, consider the feature may be activated by an unrelated crate).

This forces downstream crates, if they intend to be compatible with both, to run check coverage of both with and without the feature combinations. And in the end you get this where it's a jungle of things that may or may not work with each other.

HeroicKatora · 2022-02-18T20:20:52Z

src/parser.rs

+    let size = size as u32 * idct_size as u32;
+    let dim = size / 8 + u32::from(size % 8 != 0);
+    dim as u16
+}


For this change, you can probably PR this on its own. I like it better than the floating point code in any case as there aren't any subtle accuracy concerns.

HeroicKatora · 2022-02-18T20:21:23Z

src/parser.rs

@@ -113,6 +112,12 @@ pub struct IccChunk {
    pub data: Vec<u8>,
 }

+fn calc_output_size(size: u16, idct_size: usize) -> u16 {
+    let size = size as u32 * idct_size as u32;


Can we make sure the types are such that u32::from can be used in both cases, to statically ensure this can't overflow?

HeroicKatora · 2022-02-18T20:25:37Z

src/upsampler.rs

@@ -164,6 +164,13 @@ impl Upsample for UpsamplerH2V1 {
    }
 }

+// no_std version to calculate fractional part of positive numbers
+fn fract(v: f32) -> f32 {
+    debug_assert!(v >= 0.0 && v <= (1 << core::f32::MANTISSA_DIGITS - 1) as f32);


If we are doing this for positive numbers only, then why isn't it using f32::floor()? How does the conversion to usize work but not the fract() method? I'm just super confused. This needs better justification/eval of alternatives before affecting std code as well.

HeroicKatora · 2022-02-18T20:27:00Z

src/upsampler.rs

@@ -176,7 +183,7 @@ impl Upsample for UpsamplerH1V2 {
        let row_near = row as f32 / 2.0;
        // If row_near's fractional is 0.0 we want row_far to be the previous row and if it's 0.5 we
        // want it to be the next row.
-        let row_far = (row_near + row_near.fract() * 3.0 - 0.25).min((input_height - 1) as f32);
+        let row_far = (row_near + fract(row_near) * 3.0 - 0.25).min((input_height - 1) as f32);


Can we not just use integer fixed-point math here?

HeroicKatora · 2022-02-18T20:28:29Z

tests/lib.rs

+#[cfg(feature = "std")]
+mod std_tests {


I wouldn't mind if the whole module were moved to a separate file here.

vstroebel added 7 commits February 18, 2022 09:41

Introduce JpegRead

ccc2dcb

Add/Change some use statements to use core/alloc

5c514ee

Use integer arithmetics to calculate output size

be53dbe

f32::ceil isn't available in no_std

Replace f32::fract with no_std compliant version

861da02

Add no_std compliant integration test

ef5f1de

Add std feature for no_std support

a1e586b

Add no_std tests to github actions

529b616

HeroicKatora reviewed Feb 18, 2022

View reviewed changes

HeroicKatora closed this Apr 12, 2022

HeroicKatora reopened this Apr 12, 2022

linkmauve mentioned this pull request Oct 4, 2023

Use core and alloc a bit more #274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no_std support #226

Add no_std support #226

vstroebel commented Feb 18, 2022 •

edited

Loading

HeroicKatora left a comment

HeroicKatora Feb 18, 2022

HeroicKatora Feb 18, 2022

HeroicKatora Feb 18, 2022

HeroicKatora Feb 18, 2022

HeroicKatora Feb 18, 2022

HeroicKatora Feb 18, 2022

HeroicKatora Feb 18, 2022

HeroicKatora Feb 18, 2022

HeroicKatora Feb 18, 2022

Add no_std support #226

Are you sure you want to change the base?

Add no_std support #226

Conversation

vstroebel commented Feb 18, 2022 • edited Loading

HeroicKatora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstroebel commented Feb 18, 2022 •

edited

Loading