bmp Decoder RLE bug for "encoded mode"

[silentbat]

When decoding bmp files that are encoded with RLE (in my case RLE8), the "Encoded mode" part of RLE compression is not done correctly. Description of Encoded mode: https://docs.microsoft.com/en-us/windows/win32/gdi/bitmap-compression

Expected

When reading RLEInsn::Delta(x_delta, y_delta) the imaginary cursor painting the image should jump from its current position to a new position, delta pixels away.

This should work for x_delta and y_delta both be zero or non-zero or any combination of that.

Actual behaviour

If (and only if) y_delta is non-zero, the behaviour is not a "jump of painter position" but something different:

• The current row gets filled with black for x_delta many pixels.
• The next y_delta - 1 rows get filled with black.

This leads to "skips" in the resulting image (see attachment).

As far as I would go the solution could roughly look like this:

1. iff there is any y_delta > 0, the current row has to be filled with black completely
2. after doing row skips, return to the x-position you were at before, then add x_delta to this position

Reproduction steps

Read and write the attached image with example code. See, that there are skips (look at the feet - the lowest row is snipped away).

Example Code:

use image::GenericImageView;

fn main() {
    // Use the open function to load an image from a Path.
    // `open` returns a `DynamicImage` on success.
    let img = image::open("image.bmp").unwrap();

    // Write the contents of this image to the Writer in PNG format.
    img.save("test.png").unwrap();
}

problematic code that I am talking about here

from codecs\bmp\decoder.rs:1351-1382

RLEInsn::Delta(x_delta, y_delta) => {
                                if y_delta > 0 {
                                    for n in 1..y_delta {
                                        if let Some(row) = row_iter.next() {
                                            // The msdn site on bitmap compression doesn't specify
                                            // what happens to the values skipped when encountering
                                            // a delta code, however IE and the windows image
                                            // preview seems to replace them with black pixels,
                                            // so we stick to that.
                                            for b in row {
                                                *b = 0;
                                            }
                                        } else {
                                            delta_pixels_left = x_delta;
                                            // We've reached the end of the buffer.
                                            delta_rows_left = y_delta - n;
                                            break 'row_loop;
                                        }
                                    }
                                }

                                for _ in 0..x_delta {
                                    if let Some(pixel) = pixel_iter.next() {
                                        for b in pixel {
                                            *b = 0;
                                        }
                                    } else {
                                        // We can't go any further in this row.
                                        break;
                                    }
                                }
                            }

image.zip

[silentbat]

as this is my first PR for this repo - please tell me when I am doing something totally wrong here.

For my test image, this PR fixes it, although someone else should verify this.

PS: how come #455 did not show this error btw?

[197g]

how come https://github.com/image-rs/image/issues/455 did not show this error btw?

I'll check again but it may have been that some RLE images were skipped due to being undefined, or they were accepted accidentally due to undefined pixels. In particular, q/pal4rlecut.bmp, says:

It’s okay if the viewer’s image doesn’t exactly match any of the reference images.

This comment may have caused some complacency with disregarding parts of the test suite.

[silentbat]

It’s okay if the viewer’s image doesn’t exactly match any of the reference images.

This comment may have caused some complacency with disregarding parts of the test suite.

I am pretty sure what was meant by the comment was about the color for pixels that got skipped by deltas.

AFAIK there is no specific color that should be set in those pixels, windows does black most of the time. Other viewers (vscode's image viewer for example) insert their version of a transparency pattern.

This way there are differences in the displayed image, but I'm pretty sure line skips and pixels in the wrong place should be counted as errors.

---

EDIT: I was wrong as there is no such comment for q/pal4rletrns.bmp (or the pal8rle variants for that matter). But this makes the bug even more apparent: the parts where no cuts are present should be a match. Either having transparency or black or the first color of the palette at undefined pixels but the pixels with normal colors should all be at their designated position.