wayland: Surface Transactions #1009

Drakulix · 2023-05-03T17:15:38Z

Best reviewed commit-by-commit:

compositor: Use the TransactionQueue

Changes the internal handling of the compositor module to use the TransactionQueue. Should result in identical behavior, doesn't expose anything new to downstream.

compositor: Make it possible to add Blockers

Adds new public api to the compositor module to allow downstream to add Blockers to a surface, which will be considered on the next commit.

Couple of open questions for this commit:

The CompositorClientState::blocker_cleared-method takes a lot of really generic arguments. It works for the upcoming dmabuf-commit, but might be problematic for other use-cases. Could we require less? Should we possibly move the responsibility to re-run the commit-handler to downstream and just take a DisplayHandle? Necessary to call commit and less of an issue with the changes below.
If we keep the commit-callback here, do we maybe want to skip the original commit-callback in the first place? We can easily bubble up the return value of apply_ready to conditionally skip the call. commit call has been moved into Transaction::apply
If there are pending blockers, post-commit-hooks will not observe any changed state. Luckily smithay doesn't use them for anything, does any compositor? Do we consider this expected behaviour? Do we want to run them once the transaction is done instead? Can we maybe just drop them? post_commit-hooks as well

buffer: Add buffer_attached callback

Adds a new buffer_attached-callback to the BufferHandler. Prerequisite for the next commit

WIP: anvil: Wait for dmabufs to become ready

Very rudimentary test of the above functionality.

Possible todos:

The code should be moved into a different module
Maybe we can add some implementations/abstractions in smithay to make this easier, specifically for the dmabuf case?
We should poll new buffers before adding a blocker for them. We can skip a complete event loop cycle, if the buffers are already ready, which is not unlikely.

src/wayland/compositor/mod.rs

elinorbgr · 2023-05-04T07:02:50Z

src/wayland/compositor/mod.rs

@@ -425,6 +426,10 @@ pub fn add_destruction_hook(surface: &WlSurface, hook: fn(&SurfaceData)) {
    PrivateSurfaceData::add_destruction_hook(surface, hook)
 }

+pub fn add_blocker(surface: &WlSurface, blocker: impl Blocker + Send + 'static) {


We'll need documentation here before this PR is merged.

src/wayland/compositor/mod.rs

src/wayland/buffer/mod.rs

elinorbgr · 2023-05-04T07:41:40Z

Ok so, thinking more about it, regarding the commit callback and your questions on the second commit, a general answer:

Given how the code is right now, it's entirely possible that the invocation of apply_ready in blocker_cleared might apply transactions that are unrelated to the surface passed as argument, and in a future-proof view, applying a transaction can involve multiple surfaces, and ideally all of them should have their commit() callback invoked.

So, I think maybe we should check if it'd be possible to have those callbacks (and the post_commit hooks) invoked directly in Transaction::apply ? I believe it would solve the whole problem, if it is possible.

Drakulix · 2023-05-04T14:26:56Z

So, I think maybe we should check if it'd be possible to have those callbacks (and the post_commit hooks) invoked directly in Transaction::apply ? I believe it would solve the whole problem, if it is possible.

Seems like a good solution.
This is ready for another review round. :)

codecov-commenter · 2023-05-04T20:43:39Z

Codecov Report

Patch coverage: 18.61% and project coverage change: +0.08 🎉

Comparison is base (3bec067) 23.56% compared to head (7239191) 23.65%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1009      +/-   ##
==========================================
+ Coverage   23.56%   23.65%   +0.08%     
==========================================
  Files         132      134       +2     
  Lines       21089    21284     +195     
==========================================
+ Hits         4969     5034      +65     
- Misses      16120    16250     +130

Flag	Coverage Δ
wlcs-core	`20.79% <18.61%> (+0.08%)`	⬆️
wlcs-output	`8.69% <0.00%> (-0.07%)`	⬇️
wlcs-pointer-input	`22.91% <18.61%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
anvil/src/state.rs	`34.97% <0.00%> (ø)`
src/backend/allocator/dmabuf.rs	`0.00% <0.00%> (ø)`
src/wayland/buffer/mod.rs	`0.00% <0.00%> (ø)`
src/wayland/compositor/mod.rs	`66.07% <0.00%> (-6.48%)`	⬇️
anvil/src/shell/buffer.rs	`20.00% <20.00%> (ø)`
anvil/src/shell/mod.rs	`40.96% <66.66%> (+0.79%)`	⬆️
src/wayland/compositor/tree.rs	`56.21% <68.75%> (-0.23%)`	⬇️
src/wayland/compositor/transaction.rs	`53.67% <80.00%> (+28.27%)`	⬆️
src/wayland/compositor/handlers.rs	`38.74% <100.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

cmeissl

Overall I really like the blocker API, I just have a few questions that I left as comments.

src/backend/allocator/dmabuf.rs

cmeissl · 2023-05-05T07:18:17Z

anvil/src/shell/mod.rs

+        if let Some(state) = client.get_data::<XWaylandClientData>() {
+            state
+                .user_data()
+                .insert_if_missing(CompositorClientState::default);
+            return state.user_data().get::<CompositorClientState>().unwrap();


Could that be directly added to XWaylandClientData? launch initializes XWaylandClientData where it would be possible to directly add CompositorClientState as a field instead of using user_data.

anvil/src/shell/buffer.rs

Drakulix · 2023-05-05T17:50:15Z

Now based on #1014 to make use of a pre-commit hook instead of buffer_attached for adding the blocker.

cmeissl

The whole anvil/src/shell/buffer.rs module looks like a left over from moving the block to the pre-commit handler.

Drakulix · 2023-05-08T10:54:44Z

The whole anvil/src/shell/buffer.rs module looks like a left over from moving the block to the pre-commit handler.

Removed.

Drakulix · 2023-05-09T10:02:19Z

Rebased after #1014 got merged without any other changes.

cmeissl

LGTM

Drakulix requested review from elinorbgr, i509VCB, PolyMeilex and cmeissl May 3, 2023 17:15

cmeissl reviewed May 3, 2023

View reviewed changes

src/wayland/compositor/mod.rs Outdated Show resolved Hide resolved

elinorbgr reviewed May 4, 2023

View reviewed changes

Drakulix force-pushed the feature/surface_transactions branch from b50e3a3 to f953500 Compare May 4, 2023 11:08

Drakulix marked this pull request as ready for review May 4, 2023 11:10

Drakulix force-pushed the feature/surface_transactions branch from f953500 to 04ab6d1 Compare May 4, 2023 14:24

Drakulix requested review from cmeissl and elinorbgr May 4, 2023 14:27

Drakulix force-pushed the feature/surface_transactions branch 3 times, most recently from 6d05ab7 to 7239191 Compare May 4, 2023 20:22

cmeissl reviewed May 5, 2023

View reviewed changes

Drakulix force-pushed the feature/surface_transactions branch from 4b356dc to 232b6a8 Compare May 5, 2023 17:49

Drakulix requested a review from cmeissl May 5, 2023 17:50

Drakulix force-pushed the feature/surface_transactions branch 2 times, most recently from 3cb7692 to 203c589 Compare May 5, 2023 18:52

cmeissl reviewed May 6, 2023

View reviewed changes

Drakulix force-pushed the feature/surface_transactions branch from 203c589 to f96abce Compare May 8, 2023 10:54

Drakulix force-pushed the feature/surface_transactions branch from f96abce to 92724d5 Compare May 8, 2023 10:55

Drakulix added 3 commits May 9, 2023 12:01

compositor: Use the TransactionQueue

d5bacc6

compositor: Make it possible to add Blockers

3129960

compositor: Move post-/commit callbacks into Transaction

7896f02

Drakulix added 6 commits May 9, 2023 12:01

compositor: Don't hold queue lock applying transactions

db9581b

compositor: Extend commit documentation

ab8e38c

xwayland: Add client compositor state

4ece76a

compositor: Add new_surface callback

b243b69

dmabuf: Add DmabufSource and DmabufBlocker

728ef17

anvil: Wait for dmabufs to become ready

bcae5c8

Drakulix force-pushed the feature/surface_transactions branch from 92724d5 to bcae5c8 Compare May 9, 2023 10:01

Drakulix requested a review from cmeissl May 9, 2023 10:39

cmeissl approved these changes May 10, 2023

View reviewed changes

Drakulix merged commit 80c094c into master May 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wayland: Surface Transactions #1009

wayland: Surface Transactions #1009

Drakulix commented May 3, 2023 •

edited

Loading

elinorbgr May 4, 2023

elinorbgr commented May 4, 2023

Drakulix commented May 4, 2023

codecov-commenter commented May 4, 2023

cmeissl left a comment

cmeissl May 5, 2023

Drakulix commented May 5, 2023

cmeissl left a comment

Drakulix commented May 8, 2023

Drakulix commented May 9, 2023

cmeissl left a comment

wayland: Surface Transactions #1009

wayland: Surface Transactions #1009

Conversation

Drakulix commented May 3, 2023 • edited Loading

elinorbgr May 4, 2023

Choose a reason for hiding this comment

elinorbgr commented May 4, 2023

Drakulix commented May 4, 2023

codecov-commenter commented May 4, 2023

Codecov Report

cmeissl left a comment

Choose a reason for hiding this comment

cmeissl May 5, 2023

Choose a reason for hiding this comment

Drakulix commented May 5, 2023

cmeissl left a comment

Choose a reason for hiding this comment

Drakulix commented May 8, 2023

Drakulix commented May 9, 2023

cmeissl left a comment

Choose a reason for hiding this comment

Drakulix commented May 3, 2023 •

edited

Loading