-
Notifications
You must be signed in to change notification settings - Fork 68
Add pixel formats and transparency #317
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
base: master
Are you sure you want to change the base?
Conversation
| /// Convert and write pixel data from one row to another. | ||
| /// | ||
| /// This is primarily meant as a fallback, so while it may be fairly efficient, that is not the | ||
| /// primary purpose. Users should instead. | ||
| /// | ||
| /// # Strategy | ||
| /// | ||
| /// Doing a generic conversion from data in one pixel format to another is difficult, so we allow | ||
| /// ourselves to assume that the output format is [`FALLBACK_FORMAT`]. | ||
| /// | ||
| /// If we didn't do this, we'd have to introduce a round-trip to a format like `Rgba32f` that is | ||
| /// (mostly) capable of storing all the other formats. | ||
| /// | ||
| /// # Prior art | ||
| /// | ||
| /// `SDL_ConvertPixels` + `SDL_PremultiplyAlpha` maybe? | ||
| pub(crate) fn convert_fallback(i: Input<'_>, o: Output<'_>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I'm fairly certain that we want this in some form, but the question is how?
Asked another way, what should happen when a requested pixel format is not supported by the backend?
- We allocate an additional buffer, give that to the user as
Buffer::pixels, and convert inBuffer::present. - The above, but as an optimization, when the format only differs in order (
PixelFormat::RgbavsPixelFormat::Bgra), we convert it in place inSurface::buffer_mutandBuffer::present(andBuffer::drop). - We expose a conversion function publicly, and error out on unsupported pixel formats. The user is expected to check the supported formats, and then use the conversion function to convert the buffer themselves.
See also the discussion in #98.
I haven't yet explored option 3, but it's likely it would be the most desirable, it will be surprising for the user when extra allocations and copying happens, and it seems more in line with Rust's philosophy to allow them to control this.
Especially relevant is that cargo bloat --example winit --crates --release reveals:
- Std: 279.1KiB
- Winit: 107.8KiB
- Softbuffer before this PR: 4.3KiB
- Softbuffer with
convert_fallbacksymbol (current, alpha monomorphized): 90.7KiB - Softbuffer with
convert_fallbacksymbol (alpha dynamic): 37.7KiB
Which would impact option 1 and 2 unless we change convert_fallback to be much more dynamic (and thus much less performant). Option 3 would be able to strip it out when unused, and probably also allow only emitting the relevant conversion code needed for a given application if they specify the pixel formats to be converted in line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The match seems problematic here. It forces every format to be included in the final binary even if they are never used.
If each individual conversion function (one per format) were exposed as a separate function rather than being wrapped into one function that dynamically dispatches (and it was left to the user to implement (static-or-dynamic) dispatch for the format(s) that they actually support) then the unused functions could likely be optimised out by the compiling at higher opt modes / LTO.
And if that isn't sufficient then they could be feature flagged.
Add:
When the pixel format or alpha mode is not supported by the backend, we allocate an intermediate buffer which is what the user is given to render into, and then we copy from that buffer to the actual buffer in
present. The actual buffer in this case is chosen to beDEFAULT_PIXEL_FORMATto reduce the different permutations of pixel format conversions.Fixes #17 by adding
AlphaMode.Fixes #98 by adding
PixelFormatand conversions between those.Replaces #186.
Replaces #241.
Draft because this relies on a bunch of other PRs to land first, and because I haven't actually implemented it fully yet. I'll probably split this PR out further too.