Groundwork for wp_linux_explicit_synchronization

[ascent12]

Another entry in my series of massive PRs that I take too long to finish.

Groundwork needed for #894
Closes #1546

Due to the very large amount of API breakage this introduces, I'd like to give people a heads up and get any feedback on the new design.

To see a full explanation for why I'm doing this, see the issues linked above. But to sum it up quickly:
Our current state-keeping method won't work for explicit-synchronization, since a surface being committed now doesn't really mean it's ready to be shown. Instead of just "pending" and "current", we have to keep a queue of commits, so that we always have a valid buffer handy.

The core part of this is wlr_commit. This represents a "coherent presentation" of the client's contents, and should contain all of the information to render a client correctly. Every bit of state synchronized by wl_surface.commit should be changed to use this.

Todo:

• wlr_compositor
• wlr_subcompositor
• wlr_xdg_shell and related (WIP)
• wlr_xdg_shell_v6 and related (There have been talks of removing this anyway)
• wlr_fullscreen_shell_v1
• wlr_layer_shell_v1
• xwm
• wlr_presentation_time
• wlr_linux_explicit_synchronization (Only acquire fences; maybe a different PR)
• wlr_buffer
• wlr_renderer
• wlr_seat and related (They set wlr_surface roles)
• wlr_pointer_constraints
• Other trivial uses of wlr_surface (e.g. wlr_output)
• tinywl
• rootston
• More comments and documentation

Extra notes:

• All wlr_surface functions are being merged into wlr_compositor. I haven't removed the old code yet, so I'm using a _2 suffix to just prevent some conflicts. That'll be removed eventually.
• wlr_region functions were merged into wlr_compositor.
• wlr_subcompositor was split out from wlr_compositor, technically making this an optional interface (but you'd want to use it)
• wlr_subcompositor is definitely the hardest part of this, because it's simply a complicated specification. Changing the other protocols will be much easier by comparison.
• I don't want wlr_surface managing any real buffer operations
  • wl_shm should be added to the renderer (using wlr_shm from another of my PRs)
  • Some callback functions to get information for a wlr_buffer would be needed, as they're needed for protocol compliance checks.
• I haven't done any real testing on a lot of the current code; more protocols need to be done before I can look at doing tinywl and rootston and giving it a real go. I've done some basic hack code to test the bare essentials, though.

[emersion]

Instead of just "pending" and "current", we have to keep a queue of commits, so that we always have a valid buffer handy.

Why is that? If we need a valid buffer, can't we keep just one? (And why keep the whole surface state instead of just buffers?)

Our current state-keeping method won't work for explicit-synchronization, since a surface being committed now doesn't really mean it's ready to be shown.

explicit-synchronization release events don't need to be sent when the buffer is really released. They need to be sent with wl_buffer.release.

wlr_subcompositor was split out from wlr_compositor, technically making this an optional interface (but you'd want to use it)

+1

I don't want wlr_surface managing any real buffer operations

Also +1

[ascent12]

Why is that? If we need a valid buffer, can't we keep just one? (And why keep the whole surface state instead of just buffers?)

Some extensions affect rendering, such as wp_viewport (which I eventually want to add support for) and the colour management protocol that's being worked on. We also store any wlr_commits of synchronized subsurfaces. We can't correctly render saved buffers unless we save this information too.

Others bits of information perhaps aren't as important to stash with the commit (e.g. wp_presentation_feedback), but it doesn't really hurt to have them there, and may make them easier to correctly manage for users.

The queue needs to exist for the situation that a client has multiple commits with slow to signal fences.

explicit-synchronization release events don't need to be sent when the buffer is really released. They need to be sent with wl_buffer.release.

Yeah, the release_fence can be sent earlier, and I was thinking of adding something like

void wlr_commit_send_release_fence(struct wlr_commit *commit, int fence_fd);

which sends the fence immediately to the zwp_linux_buffer_release_v1 object associated with this commit. Calling it twice would be an error or maybe just a no-op.
And if this function has not been called by the time the commit is destroyed, it will send "immediate_release" automatically.

A similar design could be done with things like wp_presentation_feedback.{presented,discarded}.

[emersion]

Some extensions affect rendering, such as wp_viewport (which I eventually want to add support for) and the colour management protocol that's being worked on. We also store any wlr_commits of synchronized subsurfaces. We can't correctly render saved buffers unless we save this information too.

Others bits of information perhaps aren't as important to stash with the commit (e.g. wp_presentation_feedback), but it doesn't really hurt to have them there, and may make them easier to correctly manage for users.

The queue needs to exist for the situation that a client has multiple commits with slow to signal fences.

Okay, this last bit is important. If I understand correctly:

• With explicit-synchronization, we may want for a buffer to be ready before attempting to render/display it (ie. wait on its in-fence FD).
• Because of this, we need a queue of states. All states in the queue (except maybe the front one) are being waited on. Once a state becomes ready, we discard older states.
• We need to be able to use and render any committed-but-not-released state. We need the whole state to be able to render (buffer + metadata).

A similar design could be done with things like wp_presentation_feedback.{presented,discarded}.

Note to self: we need to keep the state around for longer because of this.

Some other notes:

• I think this PR does too many things at once. Things like moving stuff in different files, removing the resource list can be done separately and are not really related to the redesign.
• I don't like the name wlr_commit, especially now that we have wlr_output_commit. I'd prefer to keep wlr_surface_state.

[ascent12]

Just some notes from when I was updating wlr_seat-related things. I won't be fixing these as part of this PR to try and keep some of the API breakages down, and will probably make a new issue for it.

• We aren't handling the x/y arguments gives to wl_surface.attach properly in a lot of cases. For example, with wl_pointer.set_cursor, those arguments are supposed to update the hotspot.
• wl_pointer.set_cursor and zwp_tablet_tool_v1.set_cursor just punts the hotspot handling off to the user. We could create extensions to wlr_commit to handle these properly.
• wlr_drag_icon seems largely unnecessary. It's just a container for unmap/map events on a surface, but we can just listen to events on the surface directly. This could be removed, unless it's kept around for managing wl_surface.attach's x/y.

[emersion]

We aren't handling the x/y arguments gives to wl_surface.attach properly in a lot of cases. For example, with wl_pointer.set_cursor, those arguments are supposed to update the hotspot.

We do update the hotspot, this is handled in wlr_output. It wouldn't make sense to do this in wlr_surface since this is a cursor-specific behaviour.

wl_pointer.set_cursor and zwp_tablet_tool_v1.set_cursor just punts the hotspot handling off to the user. We could create extensions to wlr_commit to handle these properly.

I'm not sure this is a good idea. Do you have an API proposal?

wlr_drag_icon seems largely unnecessary

We'd need to add map/unmap events to wlr_surface. Not sure it's a good idea either, but it could be.

[ascent12]

It wouldn't make sense to do this in wlr_surface since this is a cursor-specific behaviour.

It'd all be "extensions" to wlr_commit done by whatever the surface role is. There is no reason we couldn't add anything role-specific to it, and it was my intention to do that for things like xdg-shell anyway.

I'm not sure this is a good idea. Do you have an API proposal?

Something along the lines of

// false if not pointer role
bool wlr_commit_get_pointer_hotspot(struct wlr_commit *commit, int32_t *x, int32_t *y);

The hotspot is something tied to the "correct" presentation of a surface, so it makes sense for the wlr_commit to handle it.

We'd need to add map/unmap events to wlr_surface

No, you'd just listen for a commit, and check whether there is a buffer or not.

[emersion]

wlr_commit_get_pointer_hotspot

Not a fan of this TBH.

No, you'd just listen for a commit, and check whether there is a buffer or not.

We need to do a bunch of stuff on map/unmap. For instance damaging the whole surface.

[ascent12]

Not a fan of this TBH.

I'm not going to get too much into this now, since it's not something I'm going to do in this PR, but I don't see any advantage to NOT putting in with the surface/commit.

We do update the hotspot, this is handled in wlr_output.

Our cursor-related APIs need some serious thought put into them (#1363), and I consider wlr_output_cursor_set_surface to be broken, as it's completely incapable of handling subsurfaces or anything else that is going to have an effect on rendering. That work needs to be handled by the user, and they're going to know the role of the surface.

We need to do a bunch of stuff on map/unmap. For instance damaging the whole surface.

There is nothing stopping you from doing that. Just looking at the rootston code specifically: https://github.com/swaywm/wlroots/blob/master/rootston/seat.c#L570-L589
You've got 3 handlers for that, but you can do with just the commit handler and move the roots_drag_icon_damage_whole(icon); into there, but since roots_drag_icon_update_position(icon); does that, even that wouldn't be needed.

[emersion]

Our cursor-related APIs need some serious thought put into them (#1363), and I consider wlr_output_cursor_set_surface to be broken,

Agreed.

as it's completely incapable of handling subsurfaces or anything else that is going to have an effect on rendering.

I don't worry a lot about subsurfaces. But yeah, it does have design issues.

That work needs to be handled by the user, and they're going to know the role of the surface.

I think there should be a thin wlr_surface wrapper for cursors, with all the cursor-specific API.

There is nothing stopping you from doing that.

But then you need to check whether the last commit was NULL and this one wasn't, and the other way around, and make sure to damage on destroy, etc.

[emersion]

wlroots has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/1685