Parameterize BitMapBackend with Generic Pixel #76
Conversation
0b01
changed the title
| pub(super) use std::path::Path; | ||
| pub(super) type BorrowedImage<'a> = ImageBuffer<Rgb<u8>, &'a mut [u8]>; | ||
| pub(super) type BorrowedImage<'a, P> = ImageBuffer<P, &'a mut [u8]>; |
There was a problem hiding this comment.
Note: bounds on generic parameters are not enforced in type aliases.
There was a problem hiding this comment.
Hi there,
Thanks for the PR. I went through the code, and come up a few things.
- This is a fundamentally breaking change, which we should definitely avoid
- This change should break compilation if the
image_encodefeature isn't optin - The BitMapBackend isn't use image crate for pixel manipulation, because of the performance.
You may not realized, the BitMapBackend isn't fully relies on image. Only the image encoding part do.
Which means even if there's no image crate dependecies, this backend still works for FrameBuffer use cases.
Another think is, this backend is optimized for speed. And this is very important for some of the realtime rendering, especially some embed use cases. As you can see, there's a lot of trick has been done to accelerate the pixel manipulation. Compare to the one uses image crate's pixel manipulation API, this backend is 10X faster than that.
Even we work out an acceptable API change, the performance regression is still another concern of using image crate for pixel manipulations.
I would suggest you dig into the fast rect fill code and discuss with me.
I am very happy to talk and please let me know if you have any ideas.
| @@ -147,7 +147,7 @@ And the following code draws a quadratic function. `src/main.rs`, | |||
| ```rust | |||
| use plotters::prelude::*; | |||
| fn main() -> Result<(), Box<dyn std::error::Error>> { | |||
| let root = BitMapBackend::new("plotters-doc-data/0.png", (640, 480)).into_drawing_area(); | |||
| let root = BitMapBackend::<image::Rgb<u8>>::new("plotters-doc-data/0.png", (640, 480)).into_drawing_area(); | |||
There was a problem hiding this comment.
This is breaking change, isn't it ? If this is the case, we need to at least do something on that. AFAK, BitMapImage is one of the most widely used backend. So a lot of impact will going on
There was a problem hiding this comment.
Thank you for the quick review. There are too many breaking changes in this PR and I didn't notice that color is directly coupled with buffer manipulation.
| @@ -107,7 +107,7 @@ fn draw_func_2x1_inplace_parallel(c: &mut Criterion) { | |||
| let mut buffer = vec![0u8; (W * H * 3) as usize]; | |||
| c.bench_function("parallel::draw_func_2x1_inplace", |b| { | |||
| b.iter(|| { | |||
| let mut back = BitMapBackend::with_buffer(&mut buffer, (W, H)); | |||
| let mut back = BitMapBackend::<image::Rgb<u8>>::with_buffer(&mut buffer, (W, H)); | |||
There was a problem hiding this comment.
As I previously mentioned, this change breaks too much.
src/drawing/backend_impl/bitmap.rs
Outdated
| @@ -111,7 +111,8 @@ impl<'a> Buffer<'a> { | |||
| } | |||
|
|
|||
| /// The backend that drawing a bitmap | |||
| pub struct BitMapBackend<'a> { | |||
| pub struct BitMapBackend<'a, P: 'static + Pixel<Subpixel=u8>> { | |||
There was a problem hiding this comment.
This should breaks WASM build, or BitMap backend without image_encode feature opt in.
| @@ -151,6 +153,7 @@ impl<'a> BitMapBackend<'a> { | |||
| frame_delay: u32, | |||
| ) -> Result<Self, BitMapBackendError> { | |||
| Ok(Self { | |||
| _pix: PhantomData, | |||
There was a problem hiding this comment.
And I believe this change doesn't support pixel format other than RGB888, unless we change the fast pixel manipulation code as well. See function blend_rect_fast, fill_rect_fast
|
Too many breaking changes. Abandoning PR. |
Closes #70