Add bit order and direction support to `ImageRaw` and `Framebuffer` #711

rfuest · 2022-11-16T18:39:53Z

This draft PR contains the latest WIP version of my experiment with adding bit order and direction settings to ImageRaw and Framebuffer.

Closes #697

jamwaffles · 2022-11-24T11:22:49Z

Hey Ralf, I just pushed some changes to this branch. Hopefully that's ok with you. If not, please let me know and I'll fork this PR into a new one.

I'll see if I can integrate FrameBuffer into ssd1306 and check the output on a real display 🤞

src/image/image_raw.rs

rfuest · 2022-11-24T11:29:44Z

Hopefully that's ok with you. If not, please let me know and I'll fork this PR into a new one.

Sure that is OK. Great to see some progress on this.

I'll see if I can integrate FrameBuffer into ssd1306 and check the output on a real display crossed_fingers

Good luck! While you are at it you could also try to use the rotation or orientation types from https://github.com/embedded-graphics/display-driver-hal, if you like.

I think we should release a first alpha after this PR is merged to make it easier to depend on the new features.

jamwaffles · 2022-11-24T15:50:28Z

I've just added horizontal/vertical support for < 8BPP, using this PR in ssd1306 as a test bench.

TODO

Vertical direction for 16BPP and above, mostly for completeness' sake. I don't know of a display that uses this but maybe there's one out there...
Unit tests

src/framebuffer.rs

rfuest · 2022-11-24T17:48:26Z

Should we revert the change I made in e-g 0.7 that ImageRaw::new only takes the width as an argument? IMO this doesn't work very well with horizontal and vertical images, where you need to specify the width in one case and the height in the other.

I would suggest to change the ImageRaw::new constructor to fn new(data: &[u8], size: Size) -> Result<Self, ImageRawError> and return an error, if the image data isn't large enough. This does work for vertical and horizontal images and provides better feedback to the user, if the data is incorrectly sized.

But we would also need an additional constructor, that could be used in const context, because you cannot unwrap the result of ImageRaw::new. This constructor would shrink the image if the data is too short, instead of returning an error.

src/framebuffer.rs

rfuest · 2022-11-24T18:08:36Z

One more difference between ImageRaw and Framebuffer: ImageRaw does support color types without Msb0/Lsb0 or BigEndian/LittleEndian annotations, which Framebuffer doesn't. In this case ImageRaw defaults to Msb0 and LittleEndian. Do you prefer that the bit order/byte order must be explicitly set or should we have one default order?

jamwaffles

This PR is enormous. Did it suffer from a little scope creep? I'm getting a bit concerned by the number of traits e-g has now. I appreciate we might need them all but I still think it's worth raising.

Looks like the CI has a doc check failure, nothing too difficult.

src/mono_font/mod.rs

rfuest · 2022-12-25T03:55:22Z

Did it suffer from a little scope creep?

A bit, but most changes are directly related to finishing and optimizing the framebuffer implementation. I needed a working framebuffer implementation for other projects and didn't want to wait any longer.

I'm getting a bit concerned by the number of traits e-g has now. I appreciate we might need them all but I still think it's worth raising.

If you have any concrete examples of traits which aren't necessary we could discuss to remove them.

rfuest · 2023-05-15T01:05:01Z

I've just updated the branch to the latest master version. While I'm not sure what my involvement with the e-g project will be in the future I would like to get this PR, or at least part of it, merged.
@bugadani I've noticed that you did recently start to contribute to e-g again, would you perhaps be interested in looking at the PR? A fresh set of eyes might help to get this PR over the line.

bugadani · 2023-05-15T06:33:48Z

@rfuest I think my contributions will be limited to bugfixes going forward, but I'm happy to take a look at this. I'll need to familiarize myself with the situation, though, which will take some time.

src/common.rs

bugadani

This was an experience for sure.

I'm not convinced ColorType is a good addition. It feels like extreme deduplication that just causes trait-bloat and overcomplicates things. I have similar feelings for StorablePixelColor.

Running clippy on the PR gives me 16 warnings. I don't know how many are from this PR, but you might want to clean up the ones introduced here.

core/src/draw_target.rs

core/src/pixelcolor/mod.rs

bugadani · 2023-05-16T08:17:25Z

core/src/pixelcolor/raw/order.rs

+#[cfg_attr(feature = "defmt", derive(::defmt::Format))]
+pub enum DataOrderEnum {
+    /// Default data order.
+    Default,


Instead of an indirect terminology, can we use MostSignificantFirst / LeastSignificantFirst here?

Also, I think deny(missing_docs) is a terrible idea if that results in multiplying trivial comments like the one above.

Instead of an indirect terminology, can we use MostSignificantFirst / LeastSignificantFirst here?

I don't like this either, but for bits the default order is Msb0 and for bytes it is LittleEndian.

src/array.rs

src/framebuffer/array.rs

bugadani · 2023-05-16T08:43:32Z

src/framebuffer/test_common.rs

+        assert_eq!(framebuffer.data(), &expected_data);
+    }
+}
+macro_rules! impl_framebuffer_tests {


I have to say, this really feels like a test suite that needs some tests to test itself, but I also understand the motivation of not wanting to write the same thing 15 times over.

I have to say, this really feels like a test suite that needs some tests to test itself, ...

It's not great, but I couldn't come up with something better.

..., but I also understand the motivation of not wanting to write the same thing 15 times over.

Make that 56 times 😉.

bugadani · 2023-05-16T08:50:53Z

src/image/image_raw.rs

    {
+        let size = self.size();
+


Shouldn't this part shrink the area to the image and try to draw that? Just doing nothing might be surprising for users.

Users aren't supposed to call this method directly. area should never be outside the image.

If that's the case, taking the condition should be unreachable!()

You are probably right, but I haven't looked at this code in detail for a long time and don't want to change it in this PR, without taking a closer look first.

src/image/mod.rs

bugadani · 2023-05-16T08:57:59Z

src/primitives/styled.rs

@@ -110,8 +111,11 @@ impl<T: StyledDimensions<S>, S> Dimensions for Styled<T, S> {
    }
 }

-impl<T: StyledDrawable<S>, S> Drawable for Styled<T, S> {
-    type Color = T::Color;
+impl<T: StyledDrawable<S>, S: ColorType> ColorType for Styled<T, S> {


And this was the point where I started wondering if ColorType should have been blanket-implemented for every drawable instead, and if you should have kept Drawable::Color. There are breaking changes, and there are soul-breaking changes, and this feels the latter.

The ColorType trait was the change I wasn't sure about. It was inspired by the ErrorType trait in embedded-hal. I like the idea of having one common place to specify the color type, instead of associated color types in 6 separate traits, but it causes a lot of changes.

There are breaking changes, and there are soul-breaking changes, and this feels the latter.

That might be a little dramatic. Would this affect the average user? And most library authors should only need to change a few lines of code.

That might be a little dramatic.

Of course, that came after an hour and a half of reading the PR :)

Of course, that came after an hour and a half of reading the PR :)

OK, I can understand that and thanks for taking the time to review this PR, which should never have gotten so huge.

src/image/mod.rs

rfuest · 2023-05-16T18:15:51Z

I've updated the changelog, because the entries were still included in the 0.5 section. From my point of view the PR should be ready to be merged, if there is no more opposition against the ColorType change.

Tested on Win11, AMD 5950X

This reverts commit 9ef61f1.

rfuest mentioned this pull request Nov 16, 2022

Add a way to specify packing direction for framebuffer #697

Open

rfuest commented Nov 24, 2022

View reviewed changes

src/image/image_raw.rs Outdated Show resolved Hide resolved

rfuest commented Nov 24, 2022

View reviewed changes

src/framebuffer.rs Outdated Show resolved Hide resolved

src/framebuffer.rs Outdated Show resolved Hide resolved

src/framebuffer.rs Outdated Show resolved Hide resolved

rfuest commented Nov 24, 2022

View reviewed changes

src/framebuffer.rs Outdated Show resolved Hide resolved

rfuest force-pushed the bit-order branch from 44d1fc6 to 6f104b6 Compare December 23, 2022 19:23

rfuest marked this pull request as ready for review December 23, 2022 19:24

rfuest requested a review from jamwaffles December 23, 2022 19:26

jamwaffles reviewed Dec 24, 2022

View reviewed changes

src/mono_font/mod.rs Show resolved Hide resolved

rfuest force-pushed the bit-order branch from 440f056 to e56a45f Compare May 15, 2023 00:54

bugadani reviewed May 15, 2023

View reviewed changes

src/common.rs Show resolved Hide resolved

bugadani reviewed May 16, 2023

View reviewed changes

src/image/mod.rs Show resolved Hide resolved

rfuest and others added 9 commits May 17, 2023 21:49

Add bit order and direction support

45873da

Fix benchmarks

e4b6a3f

Support MSB0 and LSB0 for <8BPP

f1e83e8

Fix some minor grammar

febc9af

Add ImageRaw benchmarks

339aa03

Improve ImageRaw pixel() perf by 40%

1acb56f

Tested on Win11, AMD 5950X

Revert "Improve ImageRaw pixel() perf by 40%"

17bea4c

This reverts commit 9ef61f1.

Add byte direction for < 8BPP

c5825ea

Remove macro arg, use generics instead

1bd4ef8

jamwaffles and others added 14 commits May 17, 2023 21:50

Require byte direction when calling buffer_size

5f7514d

Add test to mimick SSD1306 layout

e94ce5a

Clean up byte packing code a bit

6e4faae

Move byte direction to end, default to Horizontal

5f1d266

Fix doc test

474a274

Regenerate readme

6724889

Fix framebuffer benchmarks

8e10370

Finish the framebuffer and image implementations

a53e13a

Add ColorType trait

6e2eb75

Apply suggestions from review

2a0bb87

Remove unused references

af3ca29

Add bytes_to_pixels function

0993cce

Add main/cross conversions for pixel arrangements

b491d67

Update changelog

bced7dc

rfuest force-pushed the bit-order branch from da4a5ca to bced7dc Compare May 17, 2023 19:51

Update the png-target crate to the new API

be9e01d

rfuest mentioned this pull request Nov 7, 2023

adds Rgb444 and RawU12 #732

Open

rfuest marked this pull request as draft June 11, 2024 16:14

rfuest mentioned this pull request Jun 11, 2024

Move SDL objects to TLS, allowing multiple windows to co-exist. embedded-graphics/simulator#48

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bit order and direction support to `ImageRaw` and `Framebuffer` #711

Add bit order and direction support to `ImageRaw` and `Framebuffer` #711

rfuest commented Nov 16, 2022 •

edited by jamwaffles

jamwaffles commented Nov 24, 2022

rfuest commented Nov 24, 2022

jamwaffles commented Nov 24, 2022

rfuest commented Nov 24, 2022

rfuest commented Nov 24, 2022

jamwaffles left a comment

rfuest commented Dec 25, 2022

rfuest commented May 15, 2023

bugadani commented May 15, 2023

bugadani left a comment

bugadani May 16, 2023

rfuest May 16, 2023

bugadani May 16, 2023

rfuest May 16, 2023

bugadani May 16, 2023

rfuest May 16, 2023

bugadani May 16, 2023

rfuest May 16, 2023

bugadani May 16, 2023

rfuest May 16, 2023

bugadani May 16, 2023

rfuest May 16, 2023

rfuest commented May 16, 2023

Add bit order and direction support to ImageRaw and Framebuffer #711

Are you sure you want to change the base?

Add bit order and direction support to ImageRaw and Framebuffer #711

Conversation

rfuest commented Nov 16, 2022 • edited by jamwaffles

jamwaffles commented Nov 24, 2022

rfuest commented Nov 24, 2022

jamwaffles commented Nov 24, 2022

rfuest commented Nov 24, 2022

rfuest commented Nov 24, 2022

jamwaffles left a comment

Choose a reason for hiding this comment

rfuest commented Dec 25, 2022

rfuest commented May 15, 2023

bugadani commented May 15, 2023

bugadani 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfuest commented May 16, 2023

Add bit order and direction support to `ImageRaw` and `Framebuffer` #711

Add bit order and direction support to `ImageRaw` and `Framebuffer` #711

rfuest commented Nov 16, 2022 •

edited by jamwaffles