image icon indicating copy to clipboard operation
image copied to clipboard

Added Inefficiencies due to BGR/BGRA Removal

Open JonBoyleCoding opened this issue 3 years ago • 8 comments
trafficstars

I have just tried upgrading my version of image from 0.23.14 to 0.24.0 and found my code having issues due to the removal of BGR from DynamicImage as per issue #1482. I am creating a new issue as this is now a closed request from a few months ago now.

I work with both Academic and Commercial organizations that make heavy use of OpenCV and machine learning algorithms. My role over many years has been to interface with their software to write efficient programs integrating with both other software and hardware. All of the organizations I work with still either use OpenCV as a base (and thus BGR), or their custom hardware produces imagery in BGR format.

As OpenCV uses the BGR format by default, the removal of this format from this library is rather... disappointing. I work with making real-time software and it is rather costly to be byte swapping entire 4K images twice (as I also would have to convert back) for high FPS video streams. On my personal machine with a Ryzen 3700X at 3.6GHz, running a benchmark on this conversion takes an average of 22.3ms which is much slower than just receiving frames from a 60 FPS video stream (16.6ms) and 2/3 of a 30 FPS video stream (33.3ms). The mobile devices I also have to work with are slower and nowhere near as efficient, and also wanting to conserve battery power depending on the device.

While I understand that keeping BGR as some additional code to maintain, the sacrifice of efficiency for this is unfortunate. In my area of industry BGR is still very much a normal format to be working with.

I am honestly in quite a bind - currently I have the choice to stick with older libraries as this update is quite a new version. I can't stay on old versions forever, and the organizations I am working with won't be changing format. I have found using this crate absolutely amazing over the years.

JonBoyleCoding avatar Feb 10 '22 01:02 JonBoyleCoding

Is your code public anywhere? I'd like to understand what operations you're doing using the image crate for, and whether we might be able to accommodate your use case better without needing full BGR handling everywhere

fintelia avatar Feb 10 '22 01:02 fintelia

It's critical to understand, bgra it's not gone forever and it was removed from the enum specifically—not the IO layer, so decoders and encoders may return/conume that channel arrangement. You can consume the IO decoders directly for reading into another type of buffer, for example. In general, the current DynamicImage buffer approach is only slightly bearable from an evolvability pov (diverse texel format, planar images, color spaces). BGRA layout is likely going to be added back in some other form, with a different buffer approach.

The most promising avenue, at least as far as I can see, changing buffer to a a runtime-typed byte vector, which allows us to map more directly and easily to _GL, or specifically wgpu, in general. Passing bytes instead of strong types also maps more cleanly to software-defined graphics operations—such as conversions. And with better transparent conversion we could provide more flexibility with the underlying pixel formats, without the high overhead in maintenance of the enum-variant matching, likely with better usability as well. I've prototyped this appraoch for a couple of months and I'm a lot more content with that byte buffer + descriptor than the enum variant one will ever be able to deliver.

BGRA is not gone from 0.24 for good.

197g avatar Feb 10 '22 11:02 197g

Another thing we can do is to provide an optimized conversion function between RGBA <-> BGRA. The time to convert a 4K image should be more on the order of 2ms rather than 20ms, so that alone could go a long way towards addressing performance.

fintelia avatar Feb 10 '22 18:02 fintelia

Different user from the issue-raiser, but just chiming in to point out that by default PDF uses BGR8 internally, so for those poor souls like me who have to work with raw image data extracted from PDFs and were using the image crate to do so, upgrading to 0.24 breaks everything :/

I don't mind doing a bit of internal conversion myself in my own code, but what's the recommended way to convert BGR8 image data into RGB8 so it can be used in a DynamicImage? I already have an ImageBuffer<Bgra<u8, Vec<u8>>, so I'm sure it's easy enough to do, but it's not obvious to me and the documentation isn't giving me any clues.

Obviously hindsight is 20/20 but it would have been handy if these variants had been marked as deprecated in 0.23... :) (Perhaps they were, and I just didn't notice.)

ajrcarey avatar Feb 10 '22 20:02 ajrcarey

Thank you for your responses, and I appreciate how fast you were able to respond - I apologise for taking time myself, it's been very busy over here preparing for project demonstrations. It's good to hear that BGR is not gone for good, and that you're considering other ways forward! I've had time to properly think things through more since my original post as well, as well as considering what you say.

Unfortunately a lot of the code I use/interact with is proprietary - as much as I try to open source when possible. One thing to note though is that it's not just this crate I use, but others such as imageproc.

I've tried to boil down to the essential aspects so it's clear to understand where I'm coming from. There are some more complexities than mentioned here, but I think it's best to keep this as simplistic as possible.

  1. Some generic way to keep track of Pixel Format with the Image. This is where DynamicImage has been really helpful for me in previous versions with regards to BGR (made it a whole lot easier than in OpenCV for example!).
  • The underlying protocols in our project provide uncompressed images via a networking protocol... not my decision by any means, but that's the way it is. I currently manually construct the image buffer and store it in a Dynamic image as that's nice and simple way to keep track, and if for a particular task I need to convert it I do. This isn't just BGR, sometimes it's RGB or another format.
  1. A simple way to convert images into a specified format when necessary (this includes BGR).
  • This aligns with what @ajrcarey mentioned. If I have to track pixel formats on my own, then I can do that, although I'd prefer cleaner way as part of a standard API similar to DynamicImage. It is not clear to me at the moment with 0.24 how this is a doable. (Also, if you can make the conversions faster than they were in 0.23.14 (this is what I ran the benchmarks on in my original post on a release build) as you mentioned @fintelia, that would be fantastic!).

I quite liked the way DynamicImage and ImageBuffer interacted and is in the same vein as a C++ image library I had to write from scratch many years ago, with also taking advantage of keeping Rust's enum system. Just being able to do .to_bgr() or vice versa or to any pixel format is a very convenient way to handle it.

After thinking through my situation, I think it's clear that if I'm to use 0.24 as is, then I would have to migrate my code away from DynamicImage rather than converting into a format it supports as I was thinking in my first post - so perhaps not as bad as I was thinking, just a bit of inconvenience - other than I don't see how to convert to/from BGR at the moment (if it is possible).

Thanks for reading and your responses.

JonBoyleCoding avatar Feb 14 '22 09:02 JonBoyleCoding

BGRA also comes up sometimes with graphics libraries - specifically if I want to pass data into cairo without converting it twice (into a pixbuf then into cairo) I need 8bit bgra with premultiplied alpha. For my application I swizzle once from rgba to bgra then wrap the "RgbaImage" in a newtype so I don't confuse it with real rgba images.

awused avatar Feb 21 '22 04:02 awused

Are you using the decoding and encoding functionality of the image library, or are you using the image processing functionality, or both?

johannesvollmer avatar Mar 19 '22 22:03 johannesvollmer

I was initially quite baffled to see that a minor version change had broken my program, and even more so baffled to see it was because BGRA was removed.

it wasn't a minor version change, semver is different pre-1.0.0

ZeWaka avatar Jul 24 '22 01:07 ZeWaka

I think it best to close this issue down in favour of #1686 and #1723 as they focus constructively on how to deal with the changes and address (at least partially) my use cases. I believe they are also better placed for other people to

Also - for anyone that wishes to do a fast conversion from BGRA to RGBA, there's an interesting topic discussing various implementations here (I imagine the solution from this post can be modified for BGR/RGB): https://users.rust-lang.org/t/converting-a-bgra-u8-to-rgb-u8-n-for-images/67938/12

This can be used in combination with a custom type similar to what was suggested in #1686.

Thanks for engaging with me when I initially posted! I shall close down the issue now :) If I come up with any good solution that is easily shareable/reusable I shall share it somewhere that people can easily find from one of these issues or elsewhere.

JonBoyleCoding avatar Oct 30 '22 17:10 JonBoyleCoding