Inverted 1bpp #19

jamwaffles · 2022-03-15T23:16:46Z

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
Run just build (Linux/macOS only) and make sure it passes. If you use Windows, check that CI passes once you've opened the PR.

PR description

Closes #18.

I wrote this pretty late, so my interpretation of the BMP format might be incorrect. I'd also gladly accept suggestions for improvements in the commented-on areas (see files tab) for performance and cleanliness reasons.

src/header.rs

src/raw_pixels.rs

rfuest

It would be great if we could also support 8bpp images with color tables.

src/header.rs

src/raw_pixels.rs

src/header.rs

jamwaffles · 2022-03-18T11:36:18Z

Thanks for the review! I agree with your comments; I'll find some time to refactor the header parsing to do it properly. Should we change the information in the publicly exposed Header, or just discard the extra fields from the various DIB variants?

rfuest · 2022-03-18T18:07:45Z

Should we change the information in the publicly exposed Header, or just discard the extra fields from the various DIB variants?

I think this depends on whether we want tinybmp to be a generic BMP parsing library in addition to the e-g centric use cases. If you intend tinybmp to be as generic as possible we should expose as much header information as possible, otherwise we don't need to store any more information than we use internally.

Some fields in the Header struct aren't necessary IMO. image_data_len, for example, isn't used at all. And image_data_start shouldn't be necessary either, because you can usually use RawData::image_data instead.

One refactoring I had previously considered was to remove all fields from Header and instead use getters which directly access the BMP data:

struct Header<'a> {
    data: &'a [u8],
}

impl Header {
    fn width(&self) -> i32 {
        i32::from_le_bytes(self.data[0x12..0x12+4].try_into().unwrap())
    }
}

Writing the getters would be some effort and not as nice as using nom, but we could add getters without any RAM overhead. The constructor for the Header struct would check for the size of the header and invalid values. In addition to the reduced memory overhead this method would also allow us to later add additional getters as non breaking changes.

jamwaffles · 2022-03-18T20:22:19Z

I think this depends on whether we want tinybmp to be a generic BMP parsing library in addition to the e-g centric use cases.

A good question, and one I don't really have a solid answer to. My best response is I want this crate to be "useful", so in that vein I think exposing more header data is probably the way to go (but not for this PR).

Some fields in the Header struct aren't necessary IMO. image_data_len, for example, isn't used at all. And image_data_start shouldn't be necessary either, because you can usually use RawData::image_data instead.

Good points, but for this PR, I think we should keep the public API the same and refactor the header parsing as you suggest, even if we discard most of what we parse. This lays the groundwork for changing the API in the future as well as keeping the scope of this PR to its original purpose.

That's an interesting idea for the getter-based API. I do like the cleanliness of getting a big struct of everything though, and the fields are very well defined so we know what to return without having to add breaking changes in the future. We could consider a hybrid approach and have getters for individual header structs instead of fields.

rfuest · 2022-03-19T15:08:03Z

Good points, but for this PR, I think we should keep the public API the same and refactor the header parsing as you suggest, even if we discard most of what we parse. This lays the groundwork for changing the API in the future as well as keeping the scope of this PR to its original purpose.

Agreed, let's keep this PR focused on the original problem.

tests/colors_8bpp_indexed.rs

jamwaffles · 2022-03-19T21:11:11Z

src/raw_pixels.rs

+                } else {
+                    // No color mapping - use value directly
+                    pixel_value[0] = *byte
+                }


I'm sure there's a better way to do these indexing operations, but I'm pretty tired at the moment.

As mentioned before, I would not mix the color table lookup and the RawPixels iter. Instead RawPixels would always return the raw image data, which would be the index into the color table for images with a pallete. The iterator could then be mapped to the actual color values by looking up the index in the color table. This step would be optional and shouldn't be used for true color images to improve performance.

That makes sense. Are you expecting the colour table logic to go into the Pixels iterator impl? I'm unsure how we'd be able to make the iterator zero cost for images without a colour table. Did you have a particular impl in mind?

The color table logic would need to go into the Pixels iterator impl and it couldn't be zero cost in this instance.

But we don't need to use the Pixels iterator in the ImageDrawable implementation. RawBmp::draw could use RawPixels instead of Pixels and have different code paths for image with or without a color table, like this pseudo code:

pub(crate) fn draw<D>(&self, target: &mut D) -> Result<(), D::Error> where D: DrawTarget, D::Color: From<<D::Color as PixelColor>::Raw>, { if has_color_table { target.fill_contiguous( &Rectangle::new(Point::zero(), self.size()), self.pixels().map(|Pixel(_, color)| do_color_mapping(color)), ) } else { target.fill_contiguous( &Rectangle::new(Point::zero(), self.size()), self.pixels().map(|Pixel(_, color)| D::Color::Raw::from_u32(color).into()), ) } }

jamwaffles · 2022-03-19T21:12:13Z

Header parsing has been refactored to handle different DIB types better, although it ignores most fields from them.

Thank you also for showing me length_data - I didn't know it existed and it's a handy lil function!

src/header/mod.rs

src/header/dib_header.rs

tests/chessboard-8px-1bit.rs

tests/colors_8bpp_indexed.rs

rfuest · 2022-03-20T18:38:52Z

src/raw_pixels.rs

+                } else {
+                    // No color mapping - use value directly
+                    pixel_value[0] = *byte
+                }


As mentioned before, I would not mix the color table lookup and the RawPixels iter. Instead RawPixels would always return the raw image data, which would be the index into the color table for images with a pallete. The iterator could then be mapped to the actual color values by looking up the index in the color table. This step would be optional and shouldn't be used for true color images to improve performance.

src/header/dib_header.rs

rfuest · 2022-03-20T18:50:11Z

Thank you also for showing me length_data - I didn't know it existed and it's a handy lil function!

It is, I only recently discovered it myself. Really unfortunate that the BMP format makes it harder to use by including the length bytes in the data size.

Co-authored-by: Ralf Fuest <mail@rfuest.de>

rfuest · 2022-03-29T16:21:31Z

Rust 1.40.0 doesn't seem to like one of the trait bounds. Do we want to keep the old Rust version or update to the new Rust version and edition 2021?

Edit: see #21 (comment)

jamwaffles · 2022-03-29T18:05:12Z

I think I've fixed the 1.40 build issue, although I'm on macOS so can't test locally.

I'd like to at least release this PR before we bump to 2021.

One issue I noticed is that we didn't check for invalid color indices if colors_used < (1 << bpp). How should we handle that case?

Let's just abort for now.

As for this PR, let's limit the scope creep and call it done if you're happy with the public API. Internals can be refactored later :)

rfuest · 2022-03-29T18:07:05Z

The problem is that is also kind of broken, e.g. that DynamicBmp doesn't use the color table.

rfuest · 2022-03-29T18:09:48Z

I would be OK with merging this now, but IMO we should fix some issues before the next release.

jamwaffles · 2022-03-29T22:31:35Z

but IMO we should fix some issues before the next release.

Agreed. I didn't realise DynamicBmp was broken when I wrote my comment. I think it makes sense to fix that in another PR though, and I'll hold off on a release until it's fixed of course.

rfuest · 2022-03-30T13:59:37Z

Agreed. I didn't realise DynamicBmp was broken when I wrote my comment. I think it makes sense to fix that in another PR though, and I'll hold off on a release until it's fixed of course.

If we want to do one non breaking release before the change to a newer Rust version I would suggest to leave DynamicBmp as it is for that release. The original intent of fixing 1 BPP images doesn't apply to DynamicBmp, because it never supported 1 BPP images. I would like to, instead of fixing DynamicBmp now, clean up the color type mess and probably get rid of DynamicBmp in the next breaking release.

jamwaffles · 2022-03-30T14:02:01Z

Ok, that sounds like a good approach :)

rfuest · 2022-03-30T16:06:13Z

Hmm, this is even more broken than I noticed before. The color table is always stored as Rgb888 and at the moment we just convert the raw value into C, which works for our examples but not all BMP files. I don't think this is fixable in a non breaking way, because we would at least need to require that C in Bmp implements From<Rgb888>.

New plan: We remove all user visible references to the new color table code and perhaps disable some of the additional checks. Then we ship one last non breaking version that only fixes the original issue. After the release we make the new code accessible to the user and clean up the rest.

jamwaffles · 2022-04-14T22:24:06Z

I'm sorry this is 15 days late. It's been 15 days of "I don't feel like programming today, I'll do it tomorrow"...

I've pushed a change that un-pubs ColorTable and its getters. I've split the image tests that use the colour table into the color_table module which I'm not too happy about so suggestions welcome.

What checks should we walk back? The ones in the header parse, or the ones that check the bit depth in Bmp::from_slice?

rfuest · 2022-04-15T15:51:07Z

I'm sorry this is 15 days late. It's been 15 days of "I don't feel like programming today, I'll do it tomorrow"...

I hope this wasn't caused by my constant nagging about issues with this PR. But unfortunately this PR uncovered some existing issues that weren't directly related to your contribution in this PR.

I've pushed a change that un-pubs ColorTable and its getters. I've split the image tests that use the colour table into the color_table module which I'm not too happy about so suggestions welcome.

I would suggest to leave the tests in the color_table for now and move it back with the next breaking release, which makes the color table public again. IMO just add a TODO in the code or open a tracking issue for the next breaking release.

What checks should we walk back? The ones in the header parse, or the ones that check the bit depth in Bmp::from_slice?

After taking another look at the checks I believe we can keep them. But the changelog should mention that 1 BPP and 8 BPP images are now required to have a color table.

jamwaffles · 2022-04-15T17:31:55Z

I hope this wasn't caused by my constant nagging about issues with this PR.

Ah, no, sorry if it came across that way. I was grumbling about my lack of motivation, not being driven away :). The points you've raised so far are all valid so are all reasonable.

I've added a TODO and tweaked the changelog entry a bit. Let me know if there's anything else.

rfuest · 2022-04-16T14:14:16Z

Did you notice this comment https://github.com/embedded-graphics/tinybmp/pull/19/files#r835934583? I would prefer to ignore colors_important, because its value shouldn't matter.

I also noticed that you reordered some of the use statements. Is this the order you prefer or caused by rustfmt? I would like too keep the order consistent across the e-g crates (at least to some extent). I try to maintain the same order as rust-analyzer uses: https://rust-analyzer.github.io/manual.html#auto-import

jamwaffles · 2022-04-16T16:16:26Z

Did you notice this comment

Thanks for reminding me. I've pushed some changes to completely ignore colors_important. cargo test still passes so I'm guessing it's correct... or our tests are junk :D

I also noticed that you reordered some of the use statements. Is this the order you prefer or caused by rustfmt?

I prefer to clump all imports together with no separating lines, but I've pushed a commit to make this PR's diff smaller in regard to imports. I don't want to bikeshed this too much but I'm happy to accept a PR in the future that unifies the style across tinybmp.

rfuest · 2022-04-16T16:30:00Z

cargo test still passes so I'm guessing it's correct... or our tests are junk :D

Let's just assume the tests are perfect ;)

I prefer to clump all imports together with no separating lines, but I've pushed a commit to make this PR's diff smaller in regard to imports. I don't want to bikeshed this too much but I'm happy to accept a PR in the future that unifies the style across tinybmp.

I don't like to mix crate imports with other imports, but have no preference if std/core imports should be mixed with imports from other crates.

jamwaffles · 2022-04-16T17:16:03Z

Thanks for the reviews!

jamwaffles · 2022-04-16T17:16:59Z

Shall I release 0.3.2 as a new patch release?

rfuest · 2022-04-16T17:19:07Z

Shall I release 0.3.2 as a new patch release?

SGTM.

How do we want to continue after that release? Do you want to work on fixing the color issues for the next breaking release?

jamwaffles · 2022-04-16T17:21:35Z

That, and adding better error handling as per #20 would be good. I'm focussing on other things at the moment so don't have much time to work on tinybmp but I'm happy to review stuff and help out on smaller stuff.

rfuest · 2022-04-16T17:24:01Z

OK, I'll try to implement these changes if I have some time. It would be good if you could take a look at embedded-graphics/tinytga#16, because the changes to tinybmp would be similar.

jamwaffles · 2022-04-16T17:38:25Z

The tinytga PR is on my todo list :). I had a quick skim and it looks good so far. I'll take a closer look when I've got a moment.

jamwaffles added 3 commits March 15, 2022 23:01

Add support for 1bpp color table mapping

6470940

Add an inverted test

e908bcf

Cleanup

a3dd79f

jamwaffles commented Mar 15, 2022

View reviewed changes

src/header.rs Outdated Show resolved Hide resolved

jamwaffles commented Mar 15, 2022

View reviewed changes

src/raw_pixels.rs Outdated Show resolved Hide resolved

jamwaffles added 2 commits March 15, 2022 23:18

Changelog entry

1c00f77

Regenerate readme

3ae13e6

jamwaffles marked this pull request as ready for review March 15, 2022 23:20

jamwaffles requested a review from rfuest March 15, 2022 23:20

jamwaffles mentioned this pull request Mar 15, 2022

Color palette not parsed correctly for 1BPP images #18

Closed

rfuest requested changes Mar 16, 2022

View reviewed changes

src/header.rs Outdated Show resolved Hide resolved

src/header.rs Outdated Show resolved Hide resolved

src/raw_pixels.rs Outdated Show resolved Hide resolved

src/header.rs Outdated Show resolved Hide resolved

jamwaffles added 2 commits March 19, 2022 21:06

Refactor header parsing to better support color tables

471bd12

Cleanups and changelog update

d4f6f79

jamwaffles commented Mar 19, 2022

View reviewed changes

tests/colors_8bpp_indexed.rs Outdated Show resolved Hide resolved

jamwaffles commented Mar 19, 2022

View reviewed changes

jamwaffles requested a review from rfuest March 19, 2022 21:12

rfuest requested changes Mar 20, 2022

View reviewed changes

jamwaffles and others added 7 commits March 22, 2022 19:49

Improve indexed test with image <-> image compare

b510dc4

Use variable to make test clearer

c1ee8d2

Apply suggestions from code review

3c7b82a

Co-authored-by: Ralf Fuest <mail@rfuest.de>

Use checked subtraction

8a09599

Make color table size non-optional

a4697e4

Move color table into RawBmp

1e44346

use variable instead of 0 in other test

1090814

Attempt to fix 1.40.0 compile

838b5e9

jamwaffles mentioned this pull request Mar 29, 2022

Add BMP viewer example #21

Merged

Un-pub ColorTable struct

94f14c9

jamwaffles force-pushed the inverted-1bpp branch from 33b1d8f to 94f14c9 Compare April 14, 2022 22:19

Tweak changelog, add TODO

365a558

jamwaffles added 2 commits April 16, 2022 17:11

Refactor color table entries from comments

7010a58

Undo some import reordering changes

304c2b4

rfuest approved these changes Apr 16, 2022

View reviewed changes

jamwaffles merged commit 5d3f3ce into master Apr 16, 2022

jamwaffles deleted the inverted-1bpp branch April 16, 2022 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inverted 1bpp #19

Inverted 1bpp #19

jamwaffles commented Mar 15, 2022 •

edited

Loading

rfuest left a comment

jamwaffles commented Mar 18, 2022

rfuest commented Mar 18, 2022

jamwaffles commented Mar 18, 2022

rfuest commented Mar 19, 2022

jamwaffles Mar 19, 2022

rfuest Mar 20, 2022

jamwaffles Mar 22, 2022

rfuest Mar 23, 2022

jamwaffles commented Mar 19, 2022

rfuest Mar 20, 2022

rfuest commented Mar 20, 2022

rfuest commented Mar 29, 2022 •

edited

Loading

jamwaffles commented Mar 29, 2022

rfuest commented Mar 29, 2022

rfuest commented Mar 29, 2022

jamwaffles commented Mar 29, 2022

rfuest commented Mar 30, 2022

jamwaffles commented Mar 30, 2022

rfuest commented Mar 30, 2022

jamwaffles commented Apr 14, 2022

rfuest commented Apr 15, 2022

jamwaffles commented Apr 15, 2022

rfuest commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

rfuest commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

rfuest commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

rfuest commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

Inverted 1bpp #19

Inverted 1bpp #19

Conversation

jamwaffles commented Mar 15, 2022 • edited Loading

PR description

rfuest left a comment

Choose a reason for hiding this comment

jamwaffles commented Mar 18, 2022

rfuest commented Mar 18, 2022

jamwaffles commented Mar 18, 2022

rfuest commented Mar 19, 2022

jamwaffles Mar 19, 2022

Choose a reason for hiding this comment

rfuest Mar 20, 2022

Choose a reason for hiding this comment

jamwaffles Mar 22, 2022

Choose a reason for hiding this comment

rfuest Mar 23, 2022

Choose a reason for hiding this comment

jamwaffles commented Mar 19, 2022

rfuest Mar 20, 2022

Choose a reason for hiding this comment

rfuest commented Mar 20, 2022

rfuest commented Mar 29, 2022 • edited Loading

jamwaffles commented Mar 29, 2022

rfuest commented Mar 29, 2022

rfuest commented Mar 29, 2022

jamwaffles commented Mar 29, 2022

rfuest commented Mar 30, 2022

jamwaffles commented Mar 30, 2022

rfuest commented Mar 30, 2022

jamwaffles commented Apr 14, 2022

rfuest commented Apr 15, 2022

jamwaffles commented Apr 15, 2022

rfuest commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

rfuest commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

rfuest commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

rfuest commented Apr 16, 2022

jamwaffles commented Apr 16, 2022

jamwaffles commented Mar 15, 2022 •

edited

Loading

rfuest commented Mar 29, 2022 •

edited

Loading