-
Notifications
You must be signed in to change notification settings - Fork 30
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
Comments
Curiously, I'm seeing it decode into either RGBA32 or BGR24 |
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.
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. |
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
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2LH2U6U553QOGXP7VBHLY3Z2AHAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZZGQYTAOJQGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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 |
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
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2LH7ZUF2XWGMJXU3YQLTY33Y2LAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQGQ3TKMBZG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
all of the decoders do that, via the method 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) |
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
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2LH76EFSQJMDGDNR64XTY334RFAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQGUYTGMJXGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I may not get you here, when you say bgr24 do you mean you are packing them into a larger type, like u32? |
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
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2LH5P46YUTL5GMOZHAZDY3377XAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQGU2TCNJSGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Would it be possible if you could share the code you are using? Because I'm not sure the library can do that. Running
(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 |
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
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2LH5KMEZCJGCCDAFXACTY34R4NAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQG4ZDCNBYGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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
> <
#183 (comment)>,
> 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
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFZRVE7JFLU3RIB7UVXRF5LY3625TAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRGAZDGNJTGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Pushed |
I migrated to the new traits etc, but the issue remains. I've simplified it here:
|
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 |
5631f52 fixes for 24 bit and 32 bit. BMP has a lot of weird quirks I'm learning. |
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. |
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
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFZRVE6DL7BNESW7LHA7PILY47YVLAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJRHEYTQNRQGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
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 |
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). |
I am aware of BGRA, but that happens when you either reinterpret the buffer of pixels that is in When viewing them as individual bytes in a
|
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. |
I can add a flag to do that, will do it over the weekend. |
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 |
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
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2LH3XYWJQUXBFSL674BDY6EL5DAVCNFSM6AAAAABFYIIXK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRWGU4TCMBSHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Added initial support for this, using const generics so unnecessary paths should be optimized out (not yet confirmed), I put it in a feature flag to reduce code bloat. In zune-bmp={version="0.5.0-rc3", features=["rgb_inverse"]} In code, call let mut decoder = BmpDecoder::new();
decoder.preserve_bgra(true);
// other things |
Hi, any more concerns? |
The redesign in the IO interface requires some complex mapping to my own IO traits, and I haven't gotten that solved yet. |
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!
The text was updated successfully, but these errors were encountered: