Allow compositors to actually access the surface->buffer on unmap

[ammen99]

Once #1062 is done, compositors will again be able to keep the buffer/texture of a wlr_surface longer than the wlr_surface itself, of course if this is possible at all. One of the use cases for that is window closing animation. However, currently compositors can't access the buffer on unmap, if the given app unmaps by committing a NULL buffer, because the pending state is applied before emitting the map/unmap signals (these come from the role_committed callback of the various shells). An example of such clients are many Xwayland apps.

Now, to let users access the previous surface->buffer on unmap when a NULL buffer is committed, there must be a way to signal the different shells before committing the new buffer, so that they can emit the unmap event on time. A possible solution could be to add a new role_precommit callback or somehow pass the old state to the existing role_commit.

[emersion]

The more general issue is that commit handlers need to access both the old state and the new one. For instance, if the surface size changes, you'll need to read both the old size and the new size to properly damage outputs (this is currently done by adding the biggest size to the surface damage in wlr_surface itself, which is kind of a hack I guess?).

[emersion]

We could also add a wlr_surface.previous state that is copied from wlr_surface.current right before commit.

[ammen99]

The more general issue is that commit handlers need to access both the old state and the new one. For instance, if the surface size changes, you'll need to read both the old size and the new size to properly damage outputs (this is currently done by adding the biggest size to the surface damage in wlr_surface itself, which is kind of a hack I guess?).

I didn't know that was done, but it seems like a good thing. I expect that all compositors will do this anyway, so adding it here makes life easier for the compositor.

[emersion]

I expect that all compositors will do this anyway, so adding it here makes life easier for the compositor.

Yeah, but it's a lie, and makes some things more complicated. For instance, the damage region can have extents bigger than the buffer size - so we need to crop it back when updating wl_shm textures. Also, it's unclear how the buffer position is handled - in fact I don't think it's supported at all right now. Luckily nobody seems to be using it.

All in all, we should probably keep states as the client set them - and maybe add wlr_surface.damage with the resize behaviour for convenience.

[ammen99]

All in all, we should probably keep states as the client set them - and maybe add wlr_surface.damage with the resize behaviour for convenience.

Your arguments make sense, so my initial claim isn't true. There might also be compositors doing even more complicated transforms than me, so for them this damage tracking won't actually work (for example if you do 3D transform of subsurfaces individually) and we'd have polluted their damage. However I still think that this is common enough, so it'd be nice to have a wlr_surface.damage which has the current behavior of wlr_surface.surface_damage.

[emersion]

So to fix this I originally wanted to add a previous_buffer field, but this is dumb, because we don't want to prevent the buffer from being mutated during a non-NULL commit.

So we just want to be able to access the old buffer during a NULL commit. I don't see a solution better than adding a special case. This could be done either:

• As you proposed by triggering unmap before the actual commit happens. I'm not keen of this idea.
• On a NULL commit, don't unref the buffer yet. Do it after the commit signal has been emitted.

[emersion]

Okay, as I make progress in #1076 I think I can reconsider how we should fix this issue. I think we have two solutions:

• Add a special field in wlr_surface for a "buffer that has just been unmapped" (previous_buffer is probably too confusing, and "unmap" is a shell concept - maybe null_commit_buffer?)
• Add a callback that would allow compositors to choose to customize the buffer management logic. Could be something among the lines of:

void (*wlr_surface_apply_damage_func_t)(struct wlr_surface *surface);

It would have a sensible default, say wlr_surface_apply_damage. Compositors will be able to do things like:

void custom_surface_apply_damage(struct wlr_surface *surface) {
    if (surface->current.buffer == NULL) {
        // NULL commit
        struct wlr_buffer *buf = wlr_buffer_ref(surface->buffer);
        // … And store the buffer somewhere
    }
    wlr_surface_apply_damage(surface);
}

[ddevault]

Add a special field in wlr_surface for a "buffer that has just been unmapped" (previous_buffer is probably too confusing, and "unmap" is a shell concept - maybe null_commit_buffer?)

This sounds better, or a second precommit signal like @ammen99 mentioned.

[emersion]

I'm still trying to figure out what's the best solution.

From purely an API point of view, triggering unmap before the surface has actually been unmapped is indeed better. That would work just like destroy which is triggered before things are actually destroyed.

So I'm considering again the role precommit thing. Or maybe we just need a null_buffer_commit event?

[ddevault]

You're overthinking this. Just add a precommit signal.