[PATCH v2] Post buffer release events instead of queue when no frame callback

[Neil Roberts]

Pekka Paalanen wrote:

> Instead of all this, could you not have made Weston simply use
> wl_resource_post_event() instead of wl_resource_queue_event()?

Yes, posting the event rather than queuing it when there are no frame
callbacks (is that what you meant?) is a lot simpler than having the
queue disabling mechanism. I'm not sure why I didn't think of that.

Otherwise if you meant using post all of the time, then that was
discussed here and it doesn't seem like such a good idea:

http://lists.freedesktop.org/archives/wayland-devel/2013-September/010987.html

> I can't say if that is a good heuristic or not, to assume that
> surfaces where a client is not interested in frame callbacks need
> buffer releases ASAP. Apart from an explicit request, I have no
> better suggestions.

As far as I understand, the queuing mechanism is specifically an
optimisation aimed at the frame callback mechanism to avoid waking up
the client twice. Therefore disabling it when there are no frame
callbacks seems like quite a natural thing to do in my mind. It's not
that the clients want the event ‘ASAP’ but rather that without the
patch the compositor will never send the events no matter how long the
client waits.

Here is a second version of the patch to do the posting. It makes the
patch for doing the queue disabling redundant.

Regards,
- Neil

-- >8 --

Weston now keeps track of the number of frame callbacks that are
installed for a particular client using some data attached in the
destroy listener of the wl_client. Whenever a buffer release is sent
this number is checked and if it is zero then the event will be posted
instead of queued. That way if the client isn't using frame callbacks
it can get buffer release events immediately. Without this the client
may never get the buffer release events because nothing else will post
an event to the client.
---
 src/compositor.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 105 insertions(+), 8 deletions(-)

diff --git a/src/compositor.c b/src/compositor.c
index 8c9e0fe..766c2cc 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -998,6 +998,7 @@ weston_surface_unmap(struct weston_surface *surface)
 struct weston_frame_callback {
 	struct wl_resource *resource;
 	struct wl_list link;
+	int committed;
 };
 
 
@@ -1106,6 +1107,71 @@ weston_buffer_reference_handle_destroy(struct wl_listener *listener,
 	ref->buffer = NULL;
 }
 
+struct weston_client_data {
+	struct wl_listener listener;
+	int n_pending_frame_callbacks;
+};
+
+static void
+destroy_client_data(struct wl_listener *listener, void *data)
+{
+	struct weston_client_data *client_data =
+		container_of(listener,
+			     struct weston_client_data,
+			     listener);
+
+	free(client_data);
+}
+
+static struct weston_client_data *
+weston_get_client_data(struct wl_client *client,
+		       int create)
+{
+	struct weston_client_data *data;
+	struct wl_listener *listener;
+
+	listener = wl_client_get_destroy_listener(client, destroy_client_data);
+	if (listener) {
+		return container_of(listener,
+				    struct weston_client_data,
+				    listener);
+	}
+
+	if (!create)
+		return NULL;
+
+	data = malloc(sizeof(struct weston_client_data));
+	if (!data)
+		return NULL;
+
+	data->n_pending_frame_callbacks = 0;
+	data->listener.notify = destroy_client_data;
+	wl_client_add_destroy_listener(client, &data->listener);
+
+	return data;
+}
+
+static void
+send_buffer_release(struct wl_resource *resource)
+{
+	struct wl_client *client = wl_resource_get_client(resource);
+	struct weston_client_data *client_data;
+
+	assert(client);
+
+	client_data = weston_get_client_data(client, 0 /* no create */);
+
+	/* If the client has frame callbacks installed then we'll
+	 * queue the event rather than posting it immediately so that
+	 * it will be sent along with the frame callback to avoid
+	 * waking up the client an extra time */
+	if (client_data &&
+	    client_data->n_pending_frame_callbacks > 0)
+		wl_resource_queue_event(resource, WL_BUFFER_RELEASE);
+	else
+		wl_resource_post_event(resource, WL_BUFFER_RELEASE);
+}
+
 WL_EXPORT void
 weston_buffer_reference(struct weston_buffer_reference *ref,
 			struct weston_buffer *buffer)
@@ -1113,9 +1179,7 @@ weston_buffer_reference(struct weston_buffer_reference *ref,
 	if (ref->buffer && buffer != ref->buffer) {
 		ref->buffer->busy_count--;
 		if (ref->buffer->busy_count == 0) {
-			assert(wl_resource_get_client(ref->buffer->resource));
-			wl_resource_queue_event(ref->buffer->resource,
-						WL_BUFFER_RELEASE);
+			send_buffer_release(ref->buffer->resource);
 		}
 		wl_list_remove(&ref->destroy_listener.link);
 	}
@@ -1483,6 +1547,15 @@ destroy_frame_callback(struct wl_resource *resource)
 {
 	struct weston_frame_callback *cb = wl_resource_get_user_data(resource);
 
+	if (cb->committed) {
+		struct wl_client *client = wl_resource_get_client(resource);
+		struct weston_client_data *client_data =
+			weston_get_client_data(client, 0 /* no create */);
+
+		if (client_data)
+			--client_data->n_pending_frame_callbacks;
+	}
+
 	wl_list_remove(&cb->link);
 	free(cb);
 }
@@ -1562,12 +1635,41 @@ weston_surface_commit_subsurface_order(struct weston_surface *surface)
 }
 
 static void
+add_frame_callbacks(struct weston_surface *surface,
+		    struct wl_list *callbacks)
+{
+	struct weston_frame_callback *cb;
+	int count = 0;
+
+	wl_list_for_each(cb, callbacks, link) {
+		cb->committed = 1;
+		count++;
+	}
+
+	if (count > 0) {
+		struct wl_client *client =
+			wl_resource_get_client(surface->resource);
+		struct weston_client_data *client_data =
+			weston_get_client_data(client, 1 /* create */);
+
+		if (client_data)
+			client_data->n_pending_frame_callbacks += count;
+
+		wl_list_insert_list(&surface->frame_callback_list, callbacks);
+	}
+}
+
+static void
 weston_surface_commit(struct weston_surface *surface)
 {
 	pixman_region32_t opaque;
 	int surface_width = 0;
 	int surface_height = 0;
 
+	/* wl_surface.frame */
+	add_frame_callbacks(surface, &surface->pending.frame_callback_list);
+	wl_list_init(&surface->pending.frame_callback_list);
+
 	/* wl_surface.set_buffer_transform */
 	surface->buffer_transform = surface->pending.buffer_transform;
 
@@ -1626,11 +1728,6 @@ weston_surface_commit(struct weston_surface *surface)
 	pixman_region32_intersect(&surface->input,
 				  &surface->input, &surface->pending.input);
 
-	/* wl_surface.frame */
-	wl_list_insert_list(&surface->frame_callback_list,
-			    &surface->pending.frame_callback_list);
-	wl_list_init(&surface->pending.frame_callback_list);
-
 	weston_surface_commit_subsurface_order(surface);
 
 	weston_surface_schedule_repaint(surface);
-- 
1.8.3.1