Skip to content
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

Add experimental plumbing for hardware accelerated rendering operations with the software renderer #7685

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

tronical
Copy link
Member

No description provided.

@tronical tronical requested a review from ogoffart February 20, 2025 16:44
@tronical
Copy link
Member Author

cc @scristall-bennu @0x6e

@@ -82,7 +80,7 @@ impl RenderingRotation {
matches!(self, Self::Rotate90 | Self::Rotate180)
}
/// Angle of the rotation in degrees
fn angle(self) -> f32 {
pub fn angle(self) -> f32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds public API. Is that on purpose?
This is a non-exhaustive enum, would angle work with any possible future value we can imagine? (for example rotation around another axis or transpose)
should the function signature somehow specify it is in degree and not radians or something else?
If in doubt, we can always use pub(crate)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes. It can't be pub(crate) because it's accessed from the cpp crate, but I can work around it and duplicate this function, alternatively. I think the signature and docs are fine as-is, but I can do the workaround if you prefer.

renderer.actual_renderer.processor.dirty_region = dirty_region.clone();
renderer.actual_renderer.processor.process_rectangle(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the original code should fill the background and not blend it.
That is, if the background is transparent, we need to end up with transparent pixels in the pixel buffer. (as opposed to whatever was in the pixel buffer before)
It is my impression that process_rectangle will blend into what's currently in the buffer.

(That makes a difference if the target pixel is ARGB, which is possible with winit software)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, indeed. I'll add a composition mode to handle this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to implement this now, but there's a caveat/question, see process_rectangle_impl.

) {
let mut line = geometry.min_y();
while let Some(mut next) =
region_line_ranges(&self.dirty_region, line, &mut self.dirty_range_cache)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

region_line_ranges tries to split the ranges so that they span on lines. for the line by line rendering.
But here, i think it would be wiser to simply iterate over the rectangle in the region. I think they shouldn't overlap.

Example:
Imagine the following dirty region

+----+    +----+
|  A |    |  B |
|    |    +----+
|    |    
+----+

This will then generate a region for the upper part of A, B, and the lower part of A.
But it'd be better to "simply" draw A, then B. Not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I understood this correctly and applied this to rectangle and texture fills.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you understood.
My point was that, in this funciton, instead of calling region_line_ranges, just call self.dirty_region.iter_box(), but the rectangles can overlap so that is not a good idea.

There's no need to split the areas to span lines.
Don't blend the background but fill it when going through ProcessScene.
match composition_mode {
CompositionMode::Source => buffer.line_slice(l as usize)
[begin as usize..end as usize]
.fill(<T as TargetPixel>::from_rgb(color.red, color.green, color.blue)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem entirely correct to me, as it doesn't copy over the alpha and it doesn't divide the pre-multipled alpha.

Should I add a fill_slice() to TargetPixel, similar to blend_slice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is not correct as this loses the alpha.

You can do what was done before:

let mut bg = T::background();
T::blend(&mut bg, color);

buffer.line_slice(/*...*/)[/*...*/].fill(bg)

But adding a fill_slice would also work, i guess.

dirty_range_cache: Vec<core::ops::Range<i16>>,
dirty_region: PhysicalRegion,
}

impl<T: TargetPixel> RenderToBuffer<'_, T> {
impl<T: TargetPixel, B: target_pixel_buffer::TargetPixelBuffer<Pixel = T>> RenderToBuffer<'_, B> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need T?
Can it be done with

Suggested change
impl<T: TargetPixel, B: target_pixel_buffer::TargetPixelBuffer<Pixel = T>> RenderToBuffer<'_, B> {
impl<B: target_pixel_buffer::TargetPixelBuffer> RenderToBuffer<'_, B> {

And then use P::Pixel instead of T

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea. Done :)

match composition_mode {
CompositionMode::Source => buffer.line_slice(l as usize)
[begin as usize..end as usize]
.fill(<T as TargetPixel>::from_rgb(color.red, color.green, color.blue)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is not correct as this loses the alpha.

You can do what was done before:

let mut bg = T::background();
T::blend(&mut bg, color);

buffer.line_slice(/*...*/)[/*...*/].fill(bg)

But adding a fill_slice would also work, i guess.

extra_right_clip,
);
});
if !self.buffer.draw_texture(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was better before. If you are not passing the dirty region to the function, how is it going to clip for the dirty region?
So I see two possibility:

  1. Put pack the call to draw_texture within foreach_region
  2. Or pass the dirty region to the draw_texture function and let the implementer do the clipping.

I'd think 1. is the most convenient, unless there is a way to do hardware clip, but i don't think that's the case on most hardware i know.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I reverted back to 1.

Comment on lines 1224 to 1227
self.foreach_region(&geometry, |buffer, rect, extra_left_clip, extra_right_clip| {
let begin = rect.min_x();
let end = rect.max_x();
for l in rect.min_y()..rect.max_y() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's just foreach_ranges

_color: PremultipliedRgbaColor,
_composition_mode: CompositionMode,
) -> bool {
false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of returning bool, we could make the default implementation call the fallback.
But then the implementer has no way to call the fallback only in some cases. so i guess it's good the way you did it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my thinking, too.

@tronical
Copy link
Member Author

I hope I addressed all feedback. There's the open question about the angle() function: I'm happy to go either way, make it public or not. Let me know what you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants