fix(rle): return all pixels instead of only first row#57
Merged
rfuest merged 4 commits intoMay 13, 2026
Merged
Conversation
`start_of_row` was initialized to `false` in both `Rle8Colors` and `Rle4Colors`. Additionally, the end-of-line escape handler (`00 00`) returned `None` if `start_of_row` was not set. This combination caused the iterator to terminate prematurely after processing the first row when using `Bmp::pixels()`. Fix by: - Initializing `start_of_row` to `true`. - Setting `start_of_row = false` after yielding each pixel. - Changing the end-of-line handler to set `start_of_row = true` instead of returning `None`.
1 task
rfuest
requested changes
May 12, 2026
Member
rfuest
left a comment
There was a problem hiding this comment.
Looks OK at first glance. But this needs to have a test to make sure it doesn't silently break again in the future.
This adds regression tests verifying that pixels().count() returns the correct total for indexed (1bpp, 4bpp, 8bpp), RLE-compressed (RLE4, RLE8), and RGB formats (555, 565, 24bpp, 32bpp) ensuring the earlier start_of_row fix holds across all supported formats.
Contributor
Author
|
Thank you for the feedback. I've added pixel iteration tests in commit ea128da using the existing logo test images that were already in the test suite. |
rfuest
approved these changes
May 13, 2026
Member
rfuest
left a comment
There was a problem hiding this comment.
LGTM and thanks for also adding the tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHANGELOG.mdentry in the Unreleased section under the appropriate heading (Added, Fixed, etc) if your changes affect the public APIrustfmton the projectjust build(Linux/macOS only) and make sure it passes. If you use Windows, check that CI passes once you've opened the PR.Problem
When iterating over pixels of an RLE4 or RLE8 compressed BMP image using
Bmp::pixels(), the iterator seems to only return pixels from the first row instead of all pixels in the image. (fixes: #56)For example, with a 640×480 RLE8 image:
PixelCount: 307200(640 × 480)PixelCount: 640(only one row)Root Cause
It looks like this was introduced in commit d61a220, which refactored the RLE decoder to use a
start_of_rowflag instead of tracking absolute pixel coordinates. There appear to be two issues:start_of_rowinitialized tofalse: BothRle8Colors::new()andRle4Colors::new()initializestart_of_rowtofalse, but the flag is only set totruewhenstart_row()is explicitly called. When usingBmp::pixels()(which doesn't callstart_row()), the iterator starts withstart_of_row = false.End-of-line escape handler returns
None: When the decoder encounters an end-of-line marker (00 00), the code checksif !self.start_of_row { return None; }. Sincestart_of_rowisfalseat the start, the iterator terminates early after processing the first row's end-of-line marker.Proposed Fix
This PR attempts to fix the issue by:
start_of_rowtotruein bothRle8Colors::new()andRle4Colors::new()start_of_row = falseafter returning each pixel (in both Absolute and Running modes)return Nonetoself.start_of_row = trueto properly transition to the next rowChanges
src/raw_iter.rs:Rle8Colors::new():start_of_row: false→start_of_row: trueRle4Colors::new():start_of_row: false→start_of_row: trueself.start_of_row = falsebefore returning pixels in Absolute mode (Rle8Colors, Rle4Colors)self.start_of_row = falsebefore returning pixels in Running mode (Rle8Colors, Rle4Colors)if !self.start_of_row { return None; }→self.start_of_row = true(Rle8Colors, Rle4Colors)