Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lilith opened this issue Apr 5, 2024 · 28 comments

Comments

@lilith
Copy link

lilith commented Apr 5, 2024

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
Copy link
Author

lilith commented Apr 5, 2024

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

@etemesi254
Copy link
Owner

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
Copy link
Author

lilith commented Apr 5, 2024 via email

@etemesi254
Copy link
Owner

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
Copy link
Author

lilith commented Apr 5, 2024 via email

@etemesi254
Copy link
Owner

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
Copy link
Author

lilith commented Apr 5, 2024 via email

@etemesi254
Copy link
Owner

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
Copy link
Author

lilith commented Apr 5, 2024 via email

@etemesi254
Copy link
Owner

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
Copy link
Author

lilith commented Apr 6, 2024 via email

@etemesi254
Copy link
Owner

etemesi254 commented Apr 6, 2024 via email

@etemesi254
Copy link
Owner

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
Copy link
Author

lilith commented Apr 8, 2024

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
Copy link
Owner

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
Copy link
Owner

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

@lilith
Copy link
Author

lilith commented Apr 12, 2024

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
Copy link
Owner

etemesi254 commented Apr 12, 2024 via email

@lilith
Copy link
Author

lilith commented Apr 12, 2024

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
Copy link
Author

lilith commented Apr 12, 2024

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
Copy link
Owner

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
Copy link
Author

lilith commented Apr 12, 2024

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
Copy link
Owner

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

@etemesi254
Copy link
Owner

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
Copy link
Author

lilith commented Apr 19, 2024 via email

@etemesi254
Copy link
Owner

etemesi254 commented Apr 22, 2024

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
Copy link
Owner

Hi, any more concerns?

@lilith
Copy link
Author

lilith commented Sep 9, 2024

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

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

No branches or pull requests

2 participants