More public items #51

Ddystopia · 2025-10-27T16:06:36Z

Thank you for helping out with tinybmp development! Please:

Check that you've added passing tests and documentation
Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, etc) if your changes affect the public API
Run rustfmt on the project

PR description

Raw API that was exposed by that crate is not really enough. You already have RLE implemented, so why not just expose it? I of course can rewrite it and use image_data that is provided by RawBmp, but why if you already have an implementation? raw_pixels method goes through the dynamic dispatch and introduces unnecessary work, alternative is just make a couple of methods public.

rfuest

Just making the constructors of the iterators public isn't the correct way to implement this. A user could, for example, create a Rle4Pixels iterator for a non RLE compressed image.

It's also more common to have a method, in this case inside RawBmp, to create the iterator than to use a constructor.

I would suggest to add methods like fn try_rle8_pixels(&self) -> Result<Rle8Pixels<'_'>, SomeErrorType> to RawBmp to only allow the creation of the iterators if the format of the BMP file matches.

Ddystopia · 2025-10-28T11:24:26Z

What if instead make DynamicRawColors public? Constructor is already pub. User will DynamicRawColors::new(&raw_bmp) and then match on it, getting either RawColors, Rle4Pixels of Rle8Pixels?

If they want to remain generic, they will call RawBmp::pixels, if they want to specialize, they will match on that enum and get the iterator they need.

rfuest · 2025-10-28T15:18:59Z

What if instead make DynamicRawColors public?

That's a good idea. But I would still prefer to have it constructed via a RawBmp::colors method.

The only issue with exposing DynamicRawColors is that with the addition of RLE compression it now contains two kinds of iterators, the regular ones only return the color and the RLE ones return pixels. I would prefer to clean this up before adding it to the public API. My idea would be to change the RLE iterators to also only return colors and to add a RowOrder enum to DynamicRawColors, which could be used by tinybmp internally, but would also help external users.

By having all variants of DynamicRawColors return only colors it would also be possible to implement Iterator for it, to make the result of RawBmp::colors also usable without matching on the enum variants, if the performance penalty doesn't matter for the application.

Ddystopia · 2025-11-05T11:21:31Z

commit "feat: RawBmp::colors" has a question in the description, please read it. In short, maybe remove that assertion that the line wrapped?

`header_type` is never read and `DibHeader` is private, so the compiler emits a warning. I added `#[expect(unused)]` so if it gets used at some point, compiler will ask you to remove it. `#[allow(dead_code)]` is worse because it will itself become a dead code if `header_type` gets used.

In preparation of remove that logic from them and moving it up.

Additional work that was required by `RawBmp::colors`: - Making `DynamicRawColors` public. - Making `DynamicRawColors` implement iterator. - Refactoring logic. - Rename `Rle{4,8}Pixels` to `Rle{4,8}Colors` - Due to removal of position handling in `Rle{4,8}Pixels`, they can no longer assert the the new line corresponds with the width. I left the assertion functionally the same, but with slightly different approach. Personally, I don't think it is necessary that that assert there at all - like, either way the image would be broken and developer will have to fix the `bmp` file, that assertion isn't helpful at all.

rfuest

Having all DynamicRawColors variants behave the same now is a nice improvement.

I haven't had time to look at all of the code changes in detail yet, but here is a review that primarily focuses on docs and coding style.

rfuest · 2025-11-05T18:53:17Z

src/raw_iter.rs

+            DynamicRawColors::Bpp1(_)
+            | DynamicRawColors::Bpp4(_)
+            | DynamicRawColors::Bpp8(_)
+            | DynamicRawColors::Bpp16(_)
+            | DynamicRawColors::Bpp24(_)
+            | DynamicRawColors::Bpp32(_) => RowOrder::TopDown,


Uncompressed BMP files can be top down or bottom up. It should be enough to return the value in Header::row_order for both, uncompressed and RLE files.

In this context I don't see access to the header. Uncompressed ones have row_order inside the struct, but not RLE. For now I will return the value from uncompressed and BottomUp for compressed.

src/raw_iter.rs

src/header/dib_header.rs

src/raw_bmp.rs

src/raw_iter.rs

rfuest · 2025-11-05T19:32:37Z

src/raw_iter.rs

+    /// Indicate that a new line is starting. Required for correct RLE decoding.
+    pub fn start_a_line(&mut self) {
+        self.start_of_line = true;
+    }


I haven't looked into this yet, but I guess that this is an internal method that isn't supposed be called by a user. If that is the case it shouldn't be public.

If this method should be present, it should be public - if user matches on colors and uses rle4colors, they must call that method at the start of a line too.

But I believe the better way would be to just not assert 😄

rfuest · 2025-11-05T19:33:03Z

src/raw_iter.rs

-        }
-        old_position
+    /// Indicate that a new line is starting. Required for correct RLE decoding.
+    pub fn start_a_line(&mut self) {


See comment about the other start_a_line method.

src/raw_iter.rs

Ddystopia · 2025-11-10T11:28:43Z

Uhm, this is strange.
Test fails with

            DynamicRawColors::Bpp16(colors) => colors.row_order,

But passes with

            DynamicRawColors::Bpp16(colors) => RowOrder::TopDown,

Test expects TopDown order, but colors.row_order contains BottomUp, which was assigned from the header (raw_iter.rs:38).

It seems to me that either image or header parsing has an error, but it was not visible before.

Should I do something with it? It seems like a separate PR needed.

Ddystopia · 2025-11-10T15:02:41Z

By the way, I implemented pub fn next_solid_chunk(&mut self) -> Option<(RawU4, usize)> locally and it gave huge performance boost (15ms down) because it can be used with fill_solid. Would it be able to merge this?

src/raw_iter.rs

rfuest · 2025-11-10T15:21:43Z

Uhm, this is strange. Test fails with
            DynamicRawColors::Bpp16(colors) => colors.row_order,
But passes with
            DynamicRawColors::Bpp16(colors) => RowOrder::TopDown,
Test expects TopDown order, but colors.row_order contains BottomUp, which was assigned from the header (raw_iter.rs:38).

It seems to me that either image or header parsing has an error, but it was not visible before.

Should I do something with it? It seems like a separate PR needed.

Seems like the test is invalid. The header information, which is tested in chessboard-8px-color-16bit.rs, shows that it is a bottom up image. If you don't want to fix it in this PR you can just add an ignore attribute for now.

rfuest · 2025-11-10T15:29:08Z

By the way, I implemented pub fn next_solid_chunk(&mut self) -> Option<(RawU4, usize)> locally and it gave huge performance boost (15ms down) because it can be used with fill_solid. Would it be able to merge this?

Drawing repeated pixels by using fill_solid sounds like a good idea. If you want to open another PR for that I'll merge it.

CHANGELOG.md

…-down order

rfuest

LGTM

rfuest requested changes Oct 27, 2025

View reviewed changes

Ddystopia force-pushed the more-public-items branch from 9f76701 to d809be2 Compare November 5, 2025 11:24

Ddystopia requested a review from rfuest November 5, 2025 11:26

Ddystopia marked this pull request as draft November 5, 2025 11:49

Ddystopia force-pushed the more-public-items branch from d809be2 to 9fbaff9 Compare November 5, 2025 12:02

Ddystopia marked this pull request as ready for review November 5, 2025 12:03

Ddystopia added 5 commits November 5, 2025 13:10

feat: expose more structs to the user

28481b4

chore: changelog entry

22b7dc4

refactor: group logic for rle pixel coordinates

e4530f8

In preparation of remove that logic from them and moving it up.

Ddystopia force-pushed the more-public-items branch from fc09f4b to 04991b7 Compare November 5, 2025 12:12

rfuest requested changes Nov 5, 2025

View reviewed changes

Ddystopia force-pushed the more-public-items branch from fa951aa to 016e3ad Compare November 10, 2025 11:16

Ddystopia force-pushed the more-public-items branch from 016e3ad to e3c6a72 Compare November 10, 2025 11:38

rfuest requested changes Nov 10, 2025

View reviewed changes

src/raw_iter.rs Outdated Show resolved Hide resolved

rfuest requested changes Nov 10, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Ddystopia force-pushed the more-public-items branch from 854d6b1 to 0ca058f Compare November 10, 2025 15:35

Ddystopia added 4 commits November 10, 2025 16:36

chore: changelog

a038e54

fix: remove unused header_type from private DibHeader

44c5e4c

fix: review comments

732f35b

fix: incorrect test, image has bottom-up order but expectation is top…

efe1fc2

…-down order

Ddystopia force-pushed the more-public-items branch from 0ca058f to efe1fc2 Compare November 10, 2025 15:37

rfuest approved these changes Nov 10, 2025

View reviewed changes

rfuest merged commit d61a220 into embedded-graphics:main Nov 10, 2025
6 checks passed

Uh oh!

More public items #51

More public items #51

Uh oh!

Conversation

Ddystopia commented Oct 27, 2025

PR description

Uh oh!

rfuest left a comment

Choose a reason for hiding this comment

Uh oh!

Ddystopia commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfuest commented Oct 28, 2025

Uh oh!

Ddystopia commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfuest left a comment

Choose a reason for hiding this comment

Uh oh!

rfuest Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Ddystopia Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rfuest Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Ddystopia Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Ddystopia Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

rfuest Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ddystopia commented Nov 10, 2025

Uh oh!

Ddystopia commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rfuest commented Nov 10, 2025

Uh oh!

rfuest commented Nov 10, 2025

Uh oh!

Uh oh!

rfuest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Ddystopia commented Oct 28, 2025 •

edited

Loading

Ddystopia commented Nov 5, 2025 •

edited

Loading

Ddystopia commented Nov 10, 2025 •

edited

Loading