-
Notifications
You must be signed in to change notification settings - Fork 590
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
Remove Bgr and Bgra color types #1482
Conversation
If there are no objections, I'll go ahead and merge this into |
Hi, I think Bgr and Bgra are still relevant if you have to make intensive use of system APIs. Here's some snippet from our code // Windows: Get color type from BITMAPINFOHEADER
let color = if bmih.biBitCount == 24 {
ColorFormat::Bgr
} else if bmih.biBitCount == 32 {
ColorFormat::Bgra
} else {
// ...
};
// macOS: Get color type from CGImage
let info = unsafe { CGImageGetAlphaInfo(image.as_ptr()) };
let color = match info {
| CGImageAlphaInfo::CGImageAlphaFirst
| CGImageAlphaInfo::CGImageAlphaPremultipliedFirst => ColorFormat::Bgra,
// ...
}; Although this can be done by swapping raw bytes, keeping BGR(A) type would make it more convenient to use, as stated in #451. |
@gyk Unfortunately having more color types adds complexity throughout the crate and to our public API. If you have a specific use case and want to talk through possible API changes to accommodate, please feel free to open a new issue 😄 |
@gyk Also note: we've removed them from our internal buffer representation as it vastly simplified the algorithmic portions. The expectation was that one could perform a conversion at the interface, where this has always been necessary in many cases regardless. For example, expanding bit-maps to bytes due to Rust's aliasing unit being a full byte. Or to merge all channels to a single plane. Similar issues had been internally where the duplication of channel order meant some inconsistencies in supported operations. I hope that this motivates better converters rather than undoing the buffer simplifications? |
@fintelia @HeroicKatora Ok, that sounds reasonable. Thanks! |
Why would you remove BGR/BGRA? It is a very common format for system APIs and buffers. What's worse is that this PR didn't even come with a major version bump. This is a breaking change. |
@WilliamVenner some of the motivation is described earlier in this thread. And we did do a major version bump when making this change: |
@fintelia macos CVPixelBufferCreate uses kCVPixelFormatType_32BGRA and it doesn't support kCVPixelFormatType_32RGBA |
Example of converting from BGRA8888 to RGBA: fn convert_bgra(width: u32, height: u32, mut bgra: Vec<u8>) -> Option<RgbaImage> {
for src in bgra.chunks_exact_mut(4) {
let (blue, green, red, alpha) = (src[0], src[1], src[2], src[3]);
src[0] = red;
src[1] = green;
src[2] = blue;
src[3] = alpha;
}
RgbaImage::from_raw(width, height, bgra)
} You can find more alternatives in this thread: https://users.rust-lang.org/t/converting-a-bgra-u8-to-rgb-u8-n-for-images/67938 |
Remove all handling of BGR and BGRA images, except for in ExtendedColorType. Users can trivially convert images just by swapping the red and blue components, but it is unclear whether they're even being used at all now.
Started as an experiment to see what it would take to remove ColorType::{Bgr8, Bgra8}, but it turns out that that the pixel structs are sufficiently entangled that you can't really remove one without the other.