Skip to content

Conversation

@Ddystopia
Copy link
Contributor

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

Added a next_solid_chunk for rle colors, it allows to make use of fill_solid that gave a huge performance boost on my mcu.

@Ddystopia
Copy link
Contributor Author

I did nothing new, just moved code to methods to reuse them from both Iterator::next and next_solid_chunk.

@rfuest
Copy link
Member

rfuest commented Nov 17, 2025

I like the performance improvement this brings, but the code is getting a bit repetitive. I think we should try to refactor it to make it easier to maintain in the future.

Here is my suggestion:
We replace the Rle4Colors and Rle8Colors iterators with Rle4Runs and Rle8Runs iterators. Instead of colors their Iterator impl returns the same as next_solid_chunk ((RawU4, usize) and (RawU8, usize)). This can be directly used in the drawing routine and a single generic RleColors iter is used to transform runs back into individual colors for RawBmp::colors.

@Ddystopia
Copy link
Contributor Author

What would you say about this api?

@Ddystopia Ddystopia force-pushed the fill_solid_rle branch 2 times, most recently from a021e6f to 169a74d Compare November 18, 2025 11:36
Copy link
Member

@rfuest rfuest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you say about this api?

Much better. I like the way the RLE decoding and conversion into pixels are now more decoupled and I think we can make the RLE state handling even simpler now (see comments below).

};
}
return Some(RawU8::from(value));
// total pixels represented by this run
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for the RLE8 case I don't think we need the RLE state anymore. If you take a look at, for example, the way RleState::Running is handled. The state is set to running in RleState::Starting, just to be immediately reset back to Starting in the next loop iteration. And the number of pixels is first decremented by one to then be incremented by one again.

Comment on lines +465 to +475
// alternating pattern -> only one pixel of this color now
if remaining == 0 {
self.rle_state = RleState::Starting;
} else {
self.rle_state = RleState::Running {
remaining: remaining.saturating_sub(1),
value,
is_odd,
};
}
return Some((RawU4::from(nibble_value), 1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the alternating case it might be worth looking into returning an enum instead of the tuple from the runs iterators. Something like:

enum RleRun<R> {
    Solid {
        color: R,
        count: usize,
    },
    Alternating {
        colors: [R; 2],
        count: usize,
    }
}

This would make it possible to draw the alternating pattern with DrawTarget::draw_contiguous, instead of drawing individual pixels.

Comment on lines +210 to +212
pub fn runs(&mut self) -> &mut I {
&mut self.runs
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be called as_runs and a to_runs method could also be useful:

Suggested change
pub fn runs(&mut self) -> &mut I {
&mut self.runs
}
pub fn as_runs(&mut self) -> &mut I {
&mut self.runs
}
pub fn to_runs(self) -> I {
self.runs
}

Comment on lines +219 to +233
loop {
let (raw, count) = match self.run.as_mut() {
Some(run) => run,
None => {
self.run = Some(self.runs.next()?);
self.run.as_mut().unwrap()
}
};
if *count > 0 {
*count -= 1;
return Some(*raw);
} else {
self.run = None;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to use an Option for the current run, because count == 0 can be used to describe the same state as None.

By changing the struct to:

pub struct RleColors<C, I: Iterator<Item = (C, usize)>> {
    runs: I,
    color: C,
    count: usize,
}

(color can be initialized to C::default() in the constructor)
the next method can be simplified to:

Suggested change
loop {
let (raw, count) = match self.run.as_mut() {
Some(run) => run,
None => {
self.run = Some(self.runs.next()?);
self.run.as_mut().unwrap()
}
};
if *count > 0 {
*count -= 1;
return Some(*raw);
} else {
self.run = None;
}
}
loop {
if self.count == 0 {
(self.color, self.count) = self.runs.next()?;
}
if self.count > 0 {
self.count -= 1;
return Some(self.color);
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants