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

Desktop abstractions #423

Merged
merged 52 commits into from
Jan 7, 2022
Merged

Desktop abstractions #423

merged 52 commits into from
Jan 7, 2022

Conversation

Drakulix
Copy link
Member

@Drakulix Drakulix commented Nov 28, 2021

First draft of #363.

Notable changes:

  • Makes existing Output more versitile
  • Adds Window abstraction
  • Adds Space abstraction
  • Adds drawing helpers (Space::render_output and draw_window)
  • Very rough anvil implementation

Things missing:

space

  • xdg-popup handling
  • layer-shell support
  • track/call output enter/leave for windows
  • damage tracking is currently not used for damage of surfaces themselves (instead always the whole window is damaged). This obviously leaves a lot of performance/efficiency gains on the table.
    • this needs some kind of boolean for each window (was_moved/dirty) to track, which windows should be rendered in full and then this needs to be passed to draw_window to render only with surface damage.
    • we actually need to add the accumulated damage of all window surface, because surfaces can be transcluent and we therefor need to render everything that overlaps with the surface damage. Was done in f80b6ba.
    • solution is incomplete, we need to properly track this for multiple outputs/spaces. done
  • has no max size and it is awkward to add because it enforces a strict order of:
    • updating the size of windows
    • and remapping outputs if necessary
    • and updating the size of the space
    • e.g. If we change the space size and want to remap the outputs and then re-arrange the windows. Should the first two steps error, because we do now have non-accessible windows? Does the operation itself still succeed? Or do we need an api to do all of this atomically?
  • we can properly add MoveGrab and ResizeGrab to smithay directly and add some helpers to use them with spaces tbd in a later pr

window

  • has no max-size (which would also be nice for rendering) will be part of a follow-up PR
  • draw_window does not play well with multi-gpu setups (will be implemented in a later PR)
  • cache bbox
  • xwayland support will be part of a larger effort to better support xwayland in smithay

anvil

  • udev backend is not updated at all
  • winit makes now use of damage tracking winit backend does not make use of damage-tracking. I think we should not bother. Lets deprecate winit for 0.4 with less features and also add a wayland-backend before 0.4 as an alternative.
  • render layers again
  • render cursors again
  • render fps again
  • handle fullscreen surfaces
  • implement little workspace effect or something similar to show how the desktop abstraction can be temporarily avoided for fancy stuff before resuming efficient rendering. (implemented later)

Open questions:

  • Figure out how to draw external stuff without breaking damage-tracking. Let compositors map arbitrary elements with textures? Maybe we can have a trait that can also work for windows and layers? (probably not layers though, since they map to outputs.)
  • Figure out a way to atomically update the size and the position of a window (wait for ack on size and next buffer before updating the position). I want to get this rid on first try, because otherwise this feels really bad for tiling. I have no good idea yet how to do that, so suggestions are very welcome tbd after wayland-rs 0.30
  • Figure out an API to support the size-constraints envisioned by @vberger s first proposal (see Window Management Abstraction #363)

My focus obviously was implementing damage-tracking and seeing if that works. I also took some inspiration from the code I wrote for fireplace. So please give feedback for ... well everything. I think the api was easy enough to implement for anvil, but it could likely be better.

@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 62ee10e to 6b500a0 Compare November 28, 2021 21:33
Comment on lines 411 to 418
damage.extend(attributes.damage.iter().map(|dmg| {
let mut rect = match dmg {
Damage::Buffer(rect) => rect.to_logical(attributes.buffer_scale),
Damage::Surface(rect) => *rect,
};
rect.loc += location;
rect
}));
Copy link
Member

Choose a reason for hiding this comment

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

On the longer-term, this could take into account the opaque_region specified by the client to discard damage that is completely behind an opaque surface.

To transpose that on the space level, the Window would need to have a method returning a Vec<Region> describing which parts of it are opaque I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah definitely. There is definitely room for improvement, but this is getting us probably already 80% of the way in terms of performance. (I still need to actually benchmark/profile 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.

Lets make another ticket for this one.

}

if let Some(surface) = kind.get_surface() {
with_surface_tree_downward(
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it makes sense to give enter/leave information at the subsurface granularity detail.

Wouldn't it be simpler to just always send the same enter/leave to the whole subsurface tree, based only on intersection with the bounding box? I seems to me that it's unnecessary detail computed and sent to the client.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Not sure actually. While that would probably be fine for most clients, I am not sure if more complex clients like firefox would agree?? But potentially worth a try? This is at least what anvil has been doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does the wayland core protocol say something about this?

Copy link
Member

Choose a reason for hiding this comment

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

Not much, tbh. The information of enter/leave is mostly used for deciding the scaling factor when drawing, the client cannot assume that its surface is actually visible on all the wl_outputs it is "entered" (as it might be hidden behind an other for example).

So I don't think it's actually possible for a client to rely on precise tracking of the wl_output its surface are entered on.

@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch 4 times, most recently from 3893274 to 1341535 Compare December 6, 2021 20:40
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch 5 times, most recently from f2e182a to e2b5560 Compare December 14, 2021 13:40
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from e2b5560 to 5166023 Compare December 16, 2021 15:43
@Drakulix Drakulix mentioned this pull request Dec 16, 2021
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 5166023 to 7a0dedd Compare December 17, 2021 11:59
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch 4 times, most recently from be4c99f to 02e582b Compare December 22, 2021 20:26
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch 4 times, most recently from c2d6236 to 37e7efa Compare December 30, 2021 15:00
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 30fb667 to 8059bdc Compare January 5, 2022 19:48
@Drakulix Drakulix marked this pull request as ready for review January 5, 2022 19:49
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from deba1cd to 7fc80e3 Compare January 5, 2022 20:24
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 7fc80e3 to 067e849 Compare January 5, 2022 20:26
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 067e849 to 36d9100 Compare January 5, 2022 20:45
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 5ef4b76 to 811421c Compare January 5, 2022 21:08
/// If `all` is set this will be send to `all` mapped surfaces.
/// Otherwise only windows and layers previously drawn during the
/// previous frame will be send frame events.
pub fn send_frames(&self, all: bool, time: u32) {
Copy link
Member

Choose a reason for hiding this comment

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

For future reference (as this was discussed not long ago on matrix), instead of iterating over all windows, we could maintain a list of windows that have requested a frame (by inspecting their state on commit) and only iterate that list.

That is a small optimization, I don't know if it's actually worth it at all, and in any case this is not blocking this PR imo.

damage.push(old_toplevel);
}

// lets iterate front to back and figure out, what new windows or unmoved windows we have
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that this damage tracking does not take into account Z-order, for example a changed window will cause damage to the output even if it is completely occulted by an other, undamaged window?

If so, maybe worth letting a TODO (and maybe a tracking issue?) to not forget that this is something that has room for improvement in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes correct, because windows can be transparent. We need to expose opaque_region similar to surface damage and then iterate twice through the elements.

Once front to back to figure out, what we need to render. Then back to front to render the filtered surfaces.

But that will be another PR 😅

@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 6030312 to 811421c Compare January 5, 2022 23:17
Co-authored-by: Victor Berger <vberger@users.noreply.github.com>
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 820e67f to 399bc6e Compare January 6, 2022 18:18
@@ -193,11 +193,17 @@ impl Space {
/// The scale is the what is rendered for the given output
/// and may be fractional. It is independent from the integer scale
/// reported to clients by the output.
///
/// *Note:* Remapping an output does reset it's damage memory.
Copy link
Member

Choose a reason for hiding this comment

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

So, does that mean the output will be considered as 100% damaged, or 0% damaged?

Copy link
Member Author

Choose a reason for hiding this comment

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

100% damaged.
without a memory the output assumes everything is new.

@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 753d386 to 7632d99 Compare January 7, 2022 18:31
@Drakulix Drakulix force-pushed the feature/desktop_abstractions branch from 7632d99 to 9f5bf25 Compare January 7, 2022 18:48
@Drakulix Drakulix merged commit 24b30e5 into master Jan 7, 2022
@Drakulix Drakulix deleted the feature/desktop_abstractions branch March 28, 2022 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants