Expand BmpDecoder::decode_into to support writing BGRA32 with a provided stride

[lilith]

Hi!

I have a feature request here for the bmp decoder: Currently (from looking at the code), it looks like the decoder fills a u8 buffer with no padding, and some variant of RGB or RGBA depending on the format. It's not really clear the exact byte format used and it can vary based on the input.

In my framework I use BGRA32 and a stride with row padding (rounding the rows up so that SIMD operations don't slow down from misalignment). Would it be possible to add a decode_into_format(&mut [u8], PixelLayout::BGRA32, stride: u32) method or something similar? Or let me know if there is an API I should be using instead.

Thanks!

[lilith]

Curiously, I'm seeing it decode into either RGBA32 or BGR24

[etemesi254]

Hi, yes, we can only know the output format based on type, bmp doesn't tell us the format output, no one really knows the format output, we basically just try to agree to what is the current accepted (what web browsers do). There is https://blog.mozilla.org/nnethercote/2015/11/06/i-rewrote-firefoxs-bmp-decoder/ where someone talks about Firefox bmp decoder.

In my framework I use BGRA32 and a stride with row padding (rounding the rows up so that SIMD operations don't slow down from misalignment). Would it be possible to add a decode_into_format(&mut [u8], PixelLayout::BGRA32, stride: u32) method or something similar? Or let me know if there is an API I should be using instead.

This isn't implemented in any place, both in a decoder and in zune-image as a whole and I am not sure implementing it will bring any sort of performance improvements anywhere. I haven't seen slowdowns when loading and writing unaligned storage with SIMD on latest CPUs. And it's quite a headache to get it right but will complicate the api.

[lilith]

Well, it is confusing to see the decoder produce both BGR(A) order and RGB order, is that intentional? I don't want to hard code a translation behavior on something that might be a bug.

On Fri, Apr 5, 2024, 4:10 AM Caleb Etemesi @.***> wrote:

Hi, yes, we can only know the output format based on type, bmp doesn't tell us the format output, no one really knows the format output, we basically just try to agree to what is the current accepted (what web browsers do). There is https://blog.mozilla.org/nnethercote/2015/11/06/i-rewrote-firefoxs-bmp-decoder/ where someone talks about Firefox bmp decoder.

In my framework I use BGRA32 and a stride with row padding (rounding the rows up so that SIMD operations don't slow down from misalignment). Would it be possible to add a decode_into_format(&mut [u8], PixelLayout::BGRA32, stride: u32) method or something similar? Or let me know if there is an API I should be using instead.

This isn't implemented in any place, both in a decoder and in zune-image as a whole and I am not sure implementing it will bring any sort of performance improvements anywhere. I haven't seen slowdowns when loading and writing unaligned storage with SIMD on latest CPUs. And it's quite a headache to get it right but will complicate the api.

— Reply to this email directly, view it on GitHub https://github.com/etemesi254/zune-image/issues/183#issuecomment-2039410901, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH2U6U553QOGXP7VBHLY3Z2AHAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZZGQYTAOJQGE . You are receiving this because you authored the thread.Message ID: @.***>

[etemesi254]

hi, the behaviour of the decoders is to preserve as much information as possible, so that means if the image is rgb we return rgb, if rgba we return rgba

[lilith]

Ok, but it should also return which one is written to the byte array...

On Fri, Apr 5, 2024, 1:06 PM Caleb Etemesi @.***> wrote:

hi, the behaviour of the decoders is to preserve as much information as possible, so that means if the image is rgb we return rgb, if rgba we return rgba

— Reply to this email directly, view it on GitHub https://github.com/etemesi254/zune-image/issues/183#issuecomment-2040475097, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH7ZUF2XWGMJXU3YQLTY33Y2LAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQGQ3TKMBZG4 . You are receiving this because you authored the thread.Message ID: @.***>

[etemesi254]

all of the decoders do that, via the method (get)_colorspace

it's usually enough to decode the image headers to know which output format it will be

let mut decoder = BmpDecoder::new(Cursor::new([]));

decoder.decode_headers().unwrap()

let mut colorspace= decoder.colorspace();

if colorspace == Colorspace::RGB{
  // format is rgb24/bgr24
} 
if colorspace == Colorspace::RGBA{
  // format is rgba32
}
// do decoding now
decoder.decode();

The code above is pseudocode and probably doesn't run(I'm typing this on mobile) but it shows the general way to know what type

Initially in my naive ways I messed pixel format and Colorspace and I'm not sure it's worth changing the whole thing (colorspace here may be what libraries like ffmpeg call pixel format)

[lilith]

Right, I need to distinguish between rgb24 and bgr24 though. And I'm using rgb24.bmp and rgba32-1.bmp from your test images and seeing bgr24 and rgba32 byte layouts from decoding them.

On Fri, Apr 5, 2024, 1:37 PM Caleb Etemesi @.***> wrote:

all of the decoders do that, via the method (get)_colorspace

it's usually enough to decode the image headers to know which output format it will be

let mut decoder = BmpDecoder::new(Cursor::new([]));

decoder.decode_headers().unwrap() let mut colorspace= decoder.colorspace(); if colorspace == Colorspace::RGB{ // format is rgb24/bgr24} if colorspace == Colorspace::RGBA{ // format is rgba32}// do decoding now decoder.decode();

The code above is pseudocode and probably doesn't run(I'm typing this on mobile) but it shows the general way to know what type

Initially in my naive ways I messed pixel format and Colorspace and I'm not sure it's worth changing the whole thing (colorspace here may be what libraries like ffmpeg call pixel format)

— Reply to this email directly, view it on GitHub https://github.com/etemesi254/zune-image/issues/183#issuecomment-2040513172, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH76EFSQJMDGDNR64XTY334RFAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQGUYTGMJXGI . You are receiving this because you authored the thread.Message ID: @.***>

[etemesi254]

And I'm using rgb24.bmp and rgba32-1.bmp from your test images and seeing bgr24

I may not get you here, when you say bgr24 do you mean you are packing them into a larger type, like u32?

[lilith]

I mean the byte array produced by the decoder orders the bytes in blue, green, red order

On Fri, Apr 5, 2024, 2:07 PM Caleb Etemesi @.***> wrote:

And I'm using rgb24.bmp and rgba32-1.bmp from your test images and seeing bgr24

I may not get you here, when you say bgr24 do you mean you are packing them into a larger type, like u32?

— Reply to this email directly, view it on GitHub https://github.com/etemesi254/zune-image/issues/183#issuecomment-2040551522, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH5P46YUTL5GMOZHAZDY3377XAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQGU2TCNJSGI . You are receiving this because you authored the thread.Message ID: @.***>

[etemesi254]

Would it be possible if you could share the code you are using? Because I'm not sure the library can do that.

Running ./zune -i ./rgb24.bmp --view --trace using https://github.com/etemesi254/zune-image/releases/tag/v0.5.0-rc0 binary prints

INFO  [zune_bin::cmd_parsers::global_options] Initialized logger
INFO  [zune_bin::cmd_parsers::global_options] Log level :TRACE
INFO  [zune_bin::workflow] Creating workflows from input

TRACE [zune_image::pipelines] Current state: Decode

TRACE [zune_bmp::decoder] Width: 127
TRACE [zune_bmp::decoder] Height: 64
TRACE [zune_bmp::decoder] Pixel format : RGB
TRACE [zune_bmp::decoder] Compression  : RGB
TRACE [zune_bmp::decoder] Bit depth: 24
TRACE [zune_image::pipelines] Finished decoding in 0 ms
TRACE [zune_image::pipelines] Finished operations for this workflow
TRACE [zune_bin::show_gui] Wrote 24540 bytes

(On Linux). The pixel format is rgb, and depth 24, meaning it's rgb24, if it were bgr it would indicate pixel format being bgr as we have support for BGR images

[lilith]

I'm still on 0.4.0. I'll try updating it

On Fri, Apr 5, 2024, 4:40 PM Caleb Etemesi @.***> wrote:

Would it be possible if you could share the code you are using? Because I'm not sure the library can do that.

Running ./zune -i ./rgb24.bmp --view --trace using https://github.com/etemesi254/zune-image/releases/tag/v0.5.0-rc0 binary prints

INFO [zune_bin::cmd_parsers::global_options] Initialized logger INFO [zune_bin::cmd_parsers::global_options] Log level :TRACE INFO [zune_bin::workflow] Creating workflows from input

TRACE [zune_image::pipelines] Current state: Decode

TRACE [zune_bmp::decoder] Width: 127 TRACE [zune_bmp::decoder] Height: 64 TRACE [zune_bmp::decoder] Pixel format : RGB TRACE [zune_bmp::decoder] Compression : RGB TRACE [zune_bmp::decoder] Bit depth: 24 TRACE [zune_image::pipelines] Finished decoding in 0 ms TRACE [zune_image::pipelines] Finished operations for this workflow TRACE [zune_bin::show_gui] Wrote 24540 bytes

(On Linux). The pixel format is rgb, and depth 24, meaning it's rgb24, if it were bgr it would indicate pixel format being bgr as we have support for BGR images

— Reply to this email directly, view it on GitHub https://github.com/etemesi254/zune-image/issues/183#issuecomment-2040721482, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH5KMEZCJGCCDAFXACTY34R4NAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQG4ZDCNBYGI . You are receiving this because you authored the thread.Message ID: @.***>

[etemesi254]

Planning to make a release tommorow

On Sat, 6 Apr 2024, 12:03 Lilith River, @.***> wrote:

I'm still on 0.4.0. I'll try updating it

On Fri, Apr 5, 2024, 4:40 PM Caleb Etemesi @.***> wrote:

Would it be possible if you could share the code you are using? Because I'm not sure the library can do that.

Running ./zune -i ./rgb24.bmp --view --trace using https://github.com/etemesi254/zune-image/releases/tag/v0.5.0-rc0 binary prints

INFO [zune_bin::cmd_parsers::global_options] Initialized logger INFO [zune_bin::cmd_parsers::global_options] Log level :TRACE INFO [zune_bin::workflow] Creating workflows from input

TRACE [zune_image::pipelines] Current state: Decode

TRACE [zune_bmp::decoder] Width: 127 TRACE [zune_bmp::decoder] Height: 64 TRACE [zune_bmp::decoder] Pixel format : RGB TRACE [zune_bmp::decoder] Compression : RGB TRACE [zune_bmp::decoder] Bit depth: 24 TRACE [zune_image::pipelines] Finished decoding in 0 ms TRACE [zune_image::pipelines] Finished operations for this workflow TRACE [zune_bin::show_gui] Wrote 24540 bytes

(On Linux). The pixel format is rgb, and depth 24, meaning it's rgb24, if it were bgr it would indicate pixel format being bgr as we have support for BGR images

— Reply to this email directly, view it on GitHub < https://github.com/etemesi254/zune-image/issues/183#issuecomment-2040721482>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAA2LH5KMEZCJGCCDAFXACTY34R4NAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQG4ZDCNBYGI>

. You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/etemesi254/zune-image/issues/183#issuecomment-2041023536, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFZRVE7JFLU3RIB7UVXRF5LY3625TAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRGAZDGNJTGY . You are receiving this because you commented.Message ID: @.***>

[etemesi254]

Pushed 0.5.0-rc0. Could you test if this is the same behaviour? And if possible could you share the code you are using?

[lilith]

I migrated to the new traits etc, but the issue remains. I've simplified it here:

use zune_bmp::zune_core::bytestream::ZCursor;

#[test]
pub fn test_rgba32_bmp_channel_order(){
    let bytes32 = imageflow_http_helpers::fetch_bytes("https://raw.githubusercontent.com/etemesi254/zune-image/dev/test-images/bmp/rgba32-1.bmp").unwrap();
    let bytes24 = imageflow_http_helpers::fetch_bytes("https://raw.githubusercontent.com/etemesi254/zune-image/dev/test-images/bmp/rgb24.bmp").unwrap();

    let mut decoder32 = zune_bmp::BmpDecoder::new(ZCursor::new(&bytes32));
    let decoded_bytes = decoder32.decode().unwrap();
    // We know the first pixel is more red than blue, so RGB order means byte 0 is greater than byte 2
    assert!(decoded_bytes[0] > decoded_bytes[2]);
    // The above passes

    let mut decoder24 = zune_bmp::BmpDecoder::new(ZCursor::new(&bytes24));
    let decoded_bytes = decoder24.decode().unwrap();
    // We know the first pixel is more red than blue, so RGB order means byte 0 is greater than byte 2
    assert!(decoded_bytes[0] > decoded_bytes[2]);
    // But this one fails.
}

[etemesi254]

Aah, now I see, sorry for the long back and forth. It's an optimization gone wrong.

I will have a fix in ~24 hours. Thank you for your patience

[etemesi254]

5631f52 fixes for 24 bit and 32 bit. BMP has a lot of weird quirks I'm learning.

[lilith]

Thanks for making it consistent, I was concerned about future breakage. However, since I'm adding this format primarily as a high-performance buffer format for interoperability with other software that also generates images, I'm concerned that swapping bytes in every pixel only to immediately swap them back will add too much overhead. Like most windows-interacting image software, rendering is done in BGRA (just like bitmaps), so it's quite redundant to have to do per-pixel work.

[etemesi254]

Would it make sense if we add a flag to output to bgra?

But my concern is that bmp is weird, it is only uncompressed images that produce bgr/bgra images, others produce rgba we can add a flag but we then need to convert all other types like rle to bgra to ensure consistency.

The perf I'm yet to measure, will plug it in some bmp files I have and test perf concerns

On Fri, 12 Apr 2024, 17:55 Lilith River, @.***> wrote:

Thanks for making it consistent, I was concerned about future breakage. However, since I'm adding this format primarily as a high-performance buffer format for interoperability with other software that also generates images, I'm concerned that swapping bytes in every pixel only to immediately swap them back will add too much overhead. Like most windows-interacting image software, rendering is done in BGRA (just like bitmaps), so it's quite redundant to have to do per-pixel work.

— Reply to this email directly, view it on GitHub https://github.com/etemesi254/zune-image/issues/183#issuecomment-2051918606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFZRVE6DL7BNESW7LHA7PILY47YVLAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJRHEYTQNRQGY . You are receiving this because you commented.Message ID: @.***>

[lilith]

Actually, it's the opposite. Nearly all computing platforms use little-endian nowadays, which means that the byte order is swapped in memory compared to your hexadecimal on screen (since a pixel has historically been mapped to a 32-bit integer, even if nowadays we have alternatives).

Thus, BGRA is the native pixel layout in terms of memory, even if the corresponding hex representation for int32 would be 0xAARRGGBB.

https://chat.openai.com/share/1b58701c-4dd2-4896-911d-24e5f2cc9005

[lilith]

RGBA and ARGB are also commonly used on non-windows platforms. Thus, most image codecs will support decoding to all 3 (or more, for HDR).

[etemesi254]

I am aware of BGRA, but that happens when you either reinterpret the buffer of pixels that is in &[u8] to &[u32], you will get BGRA as your pixel type. At this point, it becomes implementation knowledge to know what you are doing.

When viewing them as individual bytes in a &[u8] , the red pixel comes first, followed by green and followed by blue, since the library tries to manipulate pixels as bytes, to it, red comes first, then green, then blue and finally alpha if we are doing rgba.

zune-image, for speed reasons de-interleaves the whole pixel array to allow for parallel processing of different channels, this means in it's memory, if the colorspace is rgb, the first channel contains R, second G, and third B.

[lilith]

Well, in windows memory the individual bytes are ordered B,G,R,A. Which is why they have the same layout in the windows bitmap format. If possible, it would be nice to have a flag that preserves that order.

[etemesi254]

I can add a flag to do that, will do it over the weekend.

[etemesi254]

First an update, all supported images decode to rgb now.

Second would a separate pass on rgba to bgra satisfy the need to ensure bgra output, It can be done via simd. I don't want to add flags because bmps have different configs, e.g palette images, 16 bit images (packed format), 32 bit images with different bitconfigurations, (see https://entropymine.com/jason/bmpsuite/bmpsuite/html/bmpsuite.html), so ensuring each can either do rgb(a) or bgr(a) is a lot of work and a maintenance burden, but having it as a separate entity means that it's just a different pass. And BMP isn't used for huge images so I don't think it would be a bottleneck

[lilith]

I'm personally using bmp for huge images as a bottleneck. I would say if you're doing 2 stages, leave the data in the original BGR form and swap bytes only if needed, rather than twice

On Fri, Apr 19, 2024, 7:29 AM Caleb Etemesi @.***> wrote:

First an update, all supported images decode to rgb now.

Second would a separate pass on rgba to bgra satisfy the need to ensure bgra output, It can be done via simd. I don't want to add flags because bmps have different configs, e.g palette images, 16 bit images (packed format), 32 bit images with different bitconfigurations, (see https://entropymine.com/jason/bmpsuite/bmpsuite/html/bmpsuite.html), so ensuring each can either do rgb(a) or bgr(a) is a lot of work and a maintenance burden, but having it as a separate entity means that it's just a different pass. And BMP isn't used for huge images so I don't think it would be a bottleneck

— Reply to this email directly, view it on GitHub https://github.com/etemesi254/zune-image/issues/183#issuecomment-2066591029, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2LH3XYWJQUXBFSL674BDY6EL5DAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRWGU4TCMBSHE . You are receiving this because you authored the thread.Message ID: @.***>

[etemesi254]

Added initial support for this, using const generics so unnecessary paths should be optimized out (not yet confirmed), 0.5.0-rc3 should have this.

I put it in a feature flag to reduce code bloat.

In Cargo.toml

zune-bmp={version="0.5.0-rc3", features=["rgb_inverse"]}

In code, call

let mut decoder = BmpDecoder::new();
decoder.preserve_bgra(true);
// other things

[etemesi254]

Hi, any more concerns?

[lilith]

The redesign in the IO interface requires some complex mapping to my own IO traits, and I haven't gotten that solved yet.