Proposal: wlr_surface_state improvements

ascent12 commented 5 years ago

Right now, if an extension protocol wants to have any state tied to a particular wl_surface commit, it's basically forced to implement its own tracking of different commits, or forces the burden onto the user, and there is often no value in making them do it.

Instead, I think it'd be worth creating a more general system for this, where extension protocols (and even non-protocols) can add their own state directly to a wlr_surface and can be managed properly over multiple commits. One thing in particular I think we need in the future (for explicit sync) is tracking of more than just a current + pending commit, and I think that this would make this easier.

It will allow us to easily take "snapshots" of the entirety of a client's state, instead of just their buffer. This is somewhat important for things like wp_viewporter and other extensions that affect rendering state.

struct wlr_compositor {
    // ...
    struct {
        struct wl_signal new_surface;
        struct wl_signal new_state;
    } events;
};

struct wlr_compositor_new_state_args {
    // Will be NULL if this is the first state.
    // If the protocol requires it, you should copy your old state from here
    struct wlr_surface_state *old;
    struct wlr_surface_state *new;
};

struct wlr_compositor_create(struct wl_display); // As normal
// Returns a unique id which can be used for the following functions.
// Is essentially 'return ++comp->ids;'
uint32_t wlr_compositor_register(struct wlr_compositor *comp);

struct wlr_surface_state {
    size_t ref_cnt;
    // ...
    struct {
        struct wl_signal commit;
        // Called when reference count is 0, and state is superseded by a new one
        struct wl_signal destroy;
    } events;

    // Something implementing what is essentially a (uint32_t -> void *) hashmap
};

void wlr_surface_state_add(struct wlr_surface_state *st, uint32_t id, void *data);
void *wlr_surface_state_get(struct wlr_surface_state *st, uint32_t id);
void wlr_surface_state_remove(struct wlr_surface_state *st, uint32_t id);

// Instead of users probing wlr_surface directly, they should use this function when it
// comes time to render. +1 to reference count.
struct wlr_surface_state *wlr_surface_get_state(struct wlr_surface *s);
struct wlr_surface_state *wlr_surface_get_pending(struct wlr_surface *s);
void wlr_surface_state_unref(struct wlr_surface_state *st);

Examples

Here are some examples of what I think the usage would look like. I've skipped over some of the details, but it should gets the point across.

// Example of something late-binding and isn't preserved over commits

struct wlr_presentation_feedback {
    struct wl_resource *resource;
    // ...

    struct wl_listener state_destroy;
};

static void feedback_state_destroy(struct wl_listener *l, void *data)
{
    struct wlr_presentation_feedback *fb = wl_container_of(l, fb, state_destroy);

    // Feedback still exists, so the wlroots user didn't use this buffer
    if (fb->resource) {
        wp_presentation_feedback_send_discarded(fb->resource);
        wl_resource_destroy(fb->resource);
    }

    // Other cleanup
    free(fb);
}

static void presentation_handle_feedback(struct wl_client *client,
        struct wl_resource *presentation_res, struct wl_resource *surface_res,
        uint32_t id) {
    struct wlr_presentation *presentation = wl_resource_get_user_data(presentation_res);
    struct wlr_surface *surface = wl_resource_get_user_data(surface_res);
    struct wlr_surface_state *st = wlr_surface_get_pending(surface);

    if (wlr_surface_state_get_presentation_feedback(st)) {
        // error: already exists
    }

    struct wlr_presentation_feedback *fb = ...;
    fb->state_destroy.notify = feedback_state_destroy;
    wl_signal_add(&st->events.destroy, &fb->state_destroy);

    wlr_surface_state_add(st, presentation->id, fb);
}

struct wlr_presentation *wlr_presentation_create(struct wlr_compositor *comp) {
    // ...
    present->id = wlr_compositor_register(comp);
}

struct wlr_presentation_feedback *wlr_surface_state_get_presentation_feedback(
        struct wlr_presentation *presentation, struct wlr_surface_state *st) {
    assert(presentation && st);
    return wlr_surface_state_get(st, presentation->id);
}

// Example of something early-binding and preserved over commits

struct wlr_viewport {
    struct wl_resource *resource;
    // ...

    struct wl_listener state_commit;
    struct wl_listener state_destroy;
};

static void viewport_handle_set_source(...) {
    struct wlr_surface_state *st = ...;
    struct wlr_viewport *vp = wlr_surface_state_get_viewport(viewporter, st);

    vp->source = ...;
}

static void viewport_state_commit(struct wl_listener *l, void *data) {
    struct wlr_viewport *vp = wl_container_of(l, vp, state_commit);
    struct wlr_surface_state *st = data;

    if (/* viewport source larger than buffer */) {
        // protocol error
    }
}

static void viewport_state_destroy(struct wl_listener *l, void *data) {
    struct wlr_viewport *vp = wl_container_of(l, vp, state_destroy);
    // Other cleanup
    free(vp);
}

static void new_state(struct wl_listener *l, void *data) {
    struct wlr_viewporter *viewporter = wl_container_of(l, viewporter, new_state);
    struct wlr_compositor_new_state_args *args = data;

    struct wlr_viewport *vp = ...;
    if (args->old) {
        struct wlr_viewport *old = wlr_surface_state_get_viewport(args->old);
        // Copy values over

        // So new calls affect the new viewport
        wl_resource_set_user_data(vp->resource, vp);
    } else {
        // Set defaults
    }

    vp->state_destroy.notify = viewport_state_commit;
    wl_signal_add(&args->new->events.commit);

    vp->state_destroy.notify = viewport_state_destroy;
    wl_signal_add(&args->new->events.destroy);

    wl_surface_state_add(args->new, viewporter->id, vp);
}

struct wlr_viewporter *wlr_viewporter_create(struct wlr_compositor *comp) {
    // ...
    viewporter->id = wlr_compositor_register(comp);
    viewporter->new_state.notify = new_state;
    wl_signal_add(&comp->events.new_state, &viewporter->new_state);
}

struct wlr_viewport *wlr_surface_state_get_viewport(
        struct wlr_viewporter *viewporter, struct wlr_surface_state *st) {
    assert(viewporter && st);
    return wlr_surface_state_get(st, viewporter->id);
}

Other notes

This doesn't have to specifically relate to a protocol. wlr_renderer could use this, and add wlr_surface_state_get_texture.
Maybe we could rename it to something like wlr_commit. wlr_surface_state is kind of a long name.
The existing usage of surface roles could integrate into this, but I haven't really thought much about it.
There may be some extra functions I want to add so wp_linux_explicit_synchronization will work with this, but I haven't fully thought of the details. It would revolve around "locking" a state so that it won't be returned by wlr_surface_get_state.
It may be possible to allow reusing allocations more, but it may not be worth the effort.

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

https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/1546

emersion commented 5 years ago

This seems fairly complicated. I'm not sure it's worth it.

ascent12 commented 5 years ago

This seems fairly complicated. I'm not sure it's worth it.

That's certainly a possibility. I'm currently writing a more complete proof-of-concept to see if it'll work as nicely in practice as I hope it will. If it turns out to be overly complicated or unwieldy, I'll just throw it out.

emersion commented 4 years ago

Note, the current solution for surface state add-ons is the commit sequence number.

The viewporter case is a little annoying because the viewport changes the buffer size. This matters because helpers like wlr_surface_at rely on the surface size. I still don't know how to handle this, I can't think of anything else than adding a viewport field to the surface state directly. The viewport implementation would still separated from the surface implementation of course, but would populate the surface viewport field.

Another solution would be to add some way for external interfaces to mutate the surface state before commit, but it starts getting complicated (in which order should external interfaces mutate the state? etc).

swaywm / wlroots

Proposal: wlr_surface_state improvements #1546

Examples

Other notes