Add new damage test and unify buffer release code in xdg_surface

* 12to11-test.xml (test_surface): <description>: Document when
frame callbacks are run.
* 12to11.c (HandleCmdline): Accept new arg `printsocket'.
(XLMain): Pass socket to HandleCmdline.
* 12to11.man: Document new `printsocket' option.
* mime0.awk:
* mime1.awk:
* mime2.awk:
* mime3.awk:
* mime4.awk: Fix typos in copyright blurb.
* test.c (NoteBounds): Sync with X server after XResizeWindow.
(Commit): Run frame callbacks after unmap as well.
(GetTestSurface): Make windows override redirect.

* tests/Imakefile (SRCS2, OBJS2): New program.
(PROGRAMS): Add damage_test.
(damage_test): New program.  Depend on SRCS2.
* tests/README: Improve documentation on how to run tests.
* tests/simple_test.c (test_names): New list.
(LAST_TEST): New define.
(verify_single_step): Complete test upon last test being run.
(test_single_step): Submit correct surface damage.
(test_next_step): New function.  Does nothing here.
(handle_wl_callback_done): Run the next test.
(submit_surface_damage): New function.
* tests/svnignore.txt: Add damage_test and Makefile.bak.
* tests/test_harness.c (test_complete): New function.
* tests/test_harness.h: New prototypes.
* xdg_surface.c (struct _XdgRole): Use a buffer release helper.
(struct _ReleaseLaterRecord, DeleteRecord, FreeRecords)
(AddRecordAfter): Delete unused structs and functions.
(RunFrameCallbacksConditionally, BufferIdleCallback)
(ReleaseBacking, ReleaseBuffer, XLGetXdgSurface): Use and free
buffer release helper; run frame callbacks from its callback.

12to11-test.xml

@@ -67,6 +67,9 @@
       This role provides a test surface.  Various buffers and
       subsurfaces can be attached, and the resulting display contents
       validated.
+
+      When a buffer is commited to a test surface, the frame callback
+      is run after any window configuration or resize has completed.
     </description>
 
     <request name="destroy" type="destructor">

12to11.c

@@ -74,7 +74,7 @@ DetermineServerTime (void)
 }
 
 static void
-HandleCmdline (Display *dpy, int argc, char **argv)
+HandleCmdline (Display *dpy, const char *socket, int argc, char **argv)
 {
   int i;
   XrmDatabase rdb, initial_rdb;
@@ -107,7 +107,8 @@ HandleCmdline (Display *dpy, int argc, char **argv)
 	{
 	print_usage:
 	  fprintf (stderr,
-		   "usage: %s [-name name] [-class class] [-xrm resourcestring...]\n",
+		   "usage: %s [-name name] [-class class] [-printsocket]"
+		   " [-xrm resourcestring...]\n",
 		   argv[0]);
 	  exit (!strcmp (argv[i], "-help") ? 0 : 1);
 	}
@@ -144,6 +145,8 @@ HandleCmdline (Display *dpy, int argc, char **argv)
 
 	  XrmPutLineResource (&rdb, argv[++i]);
 	}
+      else if (!strcmp (argv[i], "-printsocket"))
+	puts (socket);
       else
 	{
 	  fprintf (stderr, "%s: bad command line option \"%s\"\n",
@@ -194,7 +197,7 @@ XLMain (int argc, char **argv)
   XGetDefault (dpy, "dummmy", "value");
 
   /* Parse command-line arguments.  */
-  HandleCmdline (dpy, argc, argv);
+  HandleCmdline (dpy, socket, argc, argv);
 
   compositor.display = dpy;
   compositor.conn = XGetXCBConnection (dpy);

12to11.man

@@ -36,6 +36,11 @@ after printing the message.
 This option specifies a resource string to be used.  This is
 especially useful for setting resources that do not have separate
 command line options.
+.TP
+.B \-printsocket\fP
+This option makes the protocol translator print the name of the
+Wayland socket to standard output.
+.TP
 .SH RESOURCES
 \fI12to11\fP understands some resource names and classes that can be
 used to specify various settings that affect its behavior.  Those

mime0.awk

@@ -1,4 +1,4 @@
-# Wayland compositor running on top of an X serer.
+# Wayland compositor running on top of an X server.
 
 # Copyright (C) 2022 to various contributors.
 

mime1.awk

@@ -1,4 +1,4 @@
-# Wayland compositor running on top of an X serer.
+# Wayland compositor running on top of an X server.
 
 # Copyright (C) 2022 to various contributors.
 

mime2.awk

@@ -1,4 +1,4 @@
-# Wayland compositor running on top of an X serer.
+# Wayland compositor running on top of an X server.
 
 # Copyright (C) 2022 to various contributors.
 

mime3.awk

@@ -1,4 +1,4 @@
-# Wayland compositor running on top of an X serer.
+# Wayland compositor running on top of an X server.
 
 # Copyright (C) 2022 to various contributors.
 

mime4.awk

@@ -1,4 +1,4 @@
-# Wayland compositor running on top of an X serer.
+# Wayland compositor running on top of an X server.
 
 # Copyright (C) 2022 to various contributors.
 

test.c

@@ -151,6 +151,9 @@ NoteBounds (void *data, int min_x, int min_y, int max_x, int max_y)
       /* Resize the window to fit.  */
       XResizeWindow (compositor.display, test->window,
 		     bounds_width, bounds_height);
+      /* Sync with the X server.  */
+      XSync (compositor.display, False);
+
       test->bounds_width = bounds_width;
       test->bounds_height = bounds_height;
     }
@@ -206,8 +209,13 @@ Commit (Surface *surface, Role *role)
     /* Map the surface now.  */
     MapTestSurface (test);
   else if (!surface->current_state.buffer)
-    /* Unmap the surface now.  */
+    {
-    UnmapTestSurface (test);
+      /* Unmap the surface now.  */
+      UnmapTestSurface (test);
+
+      /* Run frame callbacks if necessary.  */
+      RunFrameCallbacksConditionally (test);
+    }
 
   /* Finally, do a subcompositor update if the surface is now
      mapped.  */
@@ -389,7 +397,9 @@ GetTestSurface (struct wl_client *client, struct wl_resource *resource,
   attrs.border_pixel = border_pixel;
   attrs.event_mask = DefaultEventMask;
   attrs.cursor = InitDefaultCursor ();
-  flags = CWColormap | CWBorderPixel | CWEventMask | CWCursor;
+  attrs.override_redirect = True;
+  flags = (CWColormap | CWBorderPixel | CWEventMask
+	   | CWCursor | CWOverrideRedirect);
 
   test->window = XCreateWindow (compositor.display,
 				DefaultRootWindow (compositor.display),

tests/Imakefile

@@ -23,7 +23,9 @@ ScannerTarget(12to11-test)
 
           SRCS1 = $(COMMONSRCS) simple_test.c
           OBJS1 = $(COMMONOBJS) simple_test.o
-       PROGRAMS = simple_test
+          SRCS2 = $(COMMONSRCS) damage_test.c
+          OBJS2 = $(COMMONOBJS) damage_test.o
+       PROGRAMS = simple_test damage_test
 
 /* Make all objects depend on HEADER.  */
 $(OBJS1): $(HEADER)
@@ -32,6 +34,7 @@ $(OBJS1): $(HEADER)
 depend:: $(HEADER) $(SRCS1)
 
 NormalProgramTarget(simple_test,$(OBJS1),NullParameter,$(LOCAL_LIBRARIES),NullParameter)
-DependTarget3($(SRCS1),NullParameter,NullParameter)
+NormalProgramTarget(damage_test,$(OBJS2),NullParameter,$(LOCAL_LIBRARIES),NullParameter)
+DependTarget3($(SRCS1),$(SRCS2),NullParameter)
 
 all:: $(PROGRAMS)

tests/README

@@ -1,6 +1,16 @@
 This directory holds some work-in-progress code for testing the
-protocol translator.  At present, there is only a simple smoke test of
+protocol translator.  The current test suite is nowhere near
-limited utility.
+comprehensive.
 
 Each test must be individually run on a system with an a8r8g8b8
-visual, GLOBAL_SCALE and OUTPUT_SCALE set to 1.
+visual, GLOBAL_SCALE and OUTPUT_SCALE set to 1.  They also rely on
+reference data; if some legitimate changes are made that affect test
+results, then the tests should be run with TEST_WRITE_REFERENCE=1,
+which will make the test binaries write out reference data to disk.
+
+When tests are being run, the tester must be very careful to not
+interfere with the test operation by moving or resizing the test
+window.  A compositing manager should be running along with the test.
+
+Most likely, you do not want to run these tests manually, as the
+`run_tests.sh' script does all the setup for you.

tests/simple_test.c

@@ -25,6 +25,14 @@ enum test_kind
     BASIC_TEST_CARD_IMAGE_KIND,
   };
 
+static const char *test_names[] =
+  {
+    "map_window",
+    "basic_test_card_image",
+  };
+
+#define LAST_TEST	BASIC_TEST_CARD_IMAGE_KIND
+
 /* The display.  */
 static struct test_display *display;
 
@@ -45,13 +53,26 @@ static struct wl_surface *wayland_surface;
 
 /* Forward declarations.  */
 static void submit_frame_callback (struct wl_surface *, enum test_kind);
+static void submit_surface_damage (struct wl_surface *, int, int, int, int);
 
 
 
 static void
 verify_single_step (enum test_kind kind)
 {
-  verify_image_data (display, test_surface_window, "simple_test.dump");
+  switch (kind)
+    {
+    case BASIC_TEST_CARD_IMAGE_KIND:
+      verify_image_data (display, test_surface_window,
+			 "simple_test.dump");
+      break;
+
+    default:
+      break;
+    }
+
+  if (kind == LAST_TEST)
+    test_complete ();
 }
 
 static void
@@ -59,6 +80,8 @@ test_single_step (enum test_kind kind)
 {
   struct wl_buffer *buffer;
 
+  test_log ("running test step: %s", test_names[kind]);
+
   switch (kind)
     {
     case MAP_WINDOW_KIND:
@@ -68,6 +91,8 @@ test_single_step (enum test_kind kind)
 	report_test_failure ("failed to load blue.png");
 
       wl_surface_attach (wayland_surface, buffer, 0, 0);
+      submit_surface_damage (wayland_surface, 0, 0,
+			     INT_MAX, INT_MAX);
       wl_surface_commit (wayland_surface);
       wl_buffer_destroy (buffer);
       break;
@@ -80,12 +105,24 @@ test_single_step (enum test_kind kind)
 
       wl_surface_attach (wayland_surface, buffer, 0, 0);
       submit_frame_callback (wayland_surface, kind);
+      submit_surface_damage (wayland_surface, 0, 0,
+			     INT_MAX, INT_MAX);
       wl_surface_commit (wayland_surface);
       wl_buffer_destroy (buffer);
       break;
     }
 }
 
+static void
+test_next_step (enum test_kind kind)
+{
+  switch (kind)
+    {
+    default:
+      break;
+    }
+}
+
 
 
 static void
@@ -116,9 +153,15 @@ handle_wl_callback_done (void *data, struct wl_callback *callback,
 {
   enum test_kind kind;
 
+  /* kind is not a pointer.  It is an enum test_kind stuffed into a
+     pointer.  */
   kind = (intptr_t) data;
+
   wl_callback_destroy (callback);
   verify_single_step (kind);
+
+  /* Now run the next test in this sequence.  */
+  test_next_step (kind);
 }
 
 static const struct wl_callback_listener wl_callback_listener =
@@ -134,9 +177,18 @@ submit_frame_callback (struct wl_surface *surface, enum test_kind kind)
   struct wl_callback *callback;
 
   callback = wl_surface_frame (surface);
-  wl_callback_set_user_data (callback, (void *) (intptr_t) kind);
   wl_callback_add_listener (callback, &wl_callback_listener,
-			    NULL);
+			    (void *) (intptr_t) kind);
+}
+
+static void
+submit_surface_damage (struct wl_surface *surface, int x, int y, int width,
+		       int height)
+{
+  test_log ("damaging surface by %d, %d, %d, %d", x, y, width,
+	    height);
+
+  wl_surface_damage (surface, x, y, width, height);
 }
 
 static void

tests/svnignore.txt

@@ -2,4 +2,6 @@
 *-*.c
 vgcore*
 simple_test
+damage_test
 Makefile
+Makefile.bak

tests/test_harness.c

@@ -751,3 +751,10 @@ test_init (void)
   write_image_data_instead
     = getenv ("TEST_WRITE_REFERENCE") != NULL;
 }
+
+void __attribute__ ((noreturn))
+test_complete (void)
+{
+  test_log ("test ran successfully");
+  exit (0);
+}

tests/test_harness.h

@@ -20,6 +20,7 @@ along with 12to11.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <stdint.h>
 #include <stdbool.h>
 #include <unistd.h>
+#include <limits.h>
 
 #include <wayland-client.h>
 
@@ -70,7 +71,7 @@ struct test_interface
   uint32_t version;
 };
 
-extern void die (const char *);
+extern void die (const char *) __attribute__ ((noreturn));
 extern struct test_display *open_test_display (struct test_interface *, int);
 extern int get_shm_file_descriptor (void);
 extern size_t get_image_stride (struct test_display *, int, int);
@@ -87,5 +88,6 @@ extern bool make_test_surface (struct test_display *, struct wl_surface **,
 extern struct wl_buffer *load_png_image (struct test_display *, const char *);
 extern void verify_image_data (struct test_display *, Window, const char *);
 extern void test_init (void);
+extern void test_complete (void) __attribute__ ((noreturn));
 
 #define ARRAYELTS(arr) (sizeof (arr) / sizeof (arr)[0])

xdg_surface.c

@@ -46,6 +46,7 @@ enum
     StateTemporaryBounds	= (1 << 8),
     StateFrameStarted		= (1 << 9),
     StateAllowUnredirection	= (1 << 10),
+    StatePendingBufferRelease   = (1 << 11),
   };
 
 typedef struct _XdgRole XdgRole;
@@ -124,9 +125,8 @@ struct _XdgRole
   /* Various role state.  */
   int state;
 
-  /* Queue of buffers to release later (when the X server is done with
+  /* Buffer release helper.  */
-     them).  */
+  BufferReleaseHelper *release_helper;
-  ReleaseLaterRecord *release_records;
 
   /* The frame clock.  */
   FrameClock *clock;
@@ -166,25 +166,6 @@ struct _XdgRole
   XdgRoleImplementationType type;
 };
 
-struct _ReleaseLaterRecord
-{
-  /* A monotonically (overflow aside) increasing identifier.  */
-  uint64_t id;
-
-  /* The buffer that should be released upon receiving this
-     message.  */
-  ExtBuffer *buffer;
-
-  /* The idle callback.  */
-  IdleCallbackKey key;
-
-  /* The XdgRole.  */
-  XdgRole *role;
-
-  /* The next and last records.  */
-  ReleaseLaterRecord *next, *last;
-};
-
 struct _PingEvent
 {
   /* Function called to reply to this event.  */
@@ -197,61 +178,6 @@ struct _PingEvent
 /* Event base of the XShape extension.  */
 int shape_base;
 
-static void
-DeleteRecord (ReleaseLaterRecord *record)
-{
-  /* Removing the sentinel record is invalid.  */
-  XLAssert (record->buffer != NULL);
-
-  /* First, make the rest of the list skip RECORD.  */
-  record->last->next = record->next;
-  record->next->last = record->last;
-
-  /* Finally, free RECORD.  */
-  XLFree (record);
-}
-
-static void
-FreeRecords (ReleaseLaterRecord *records)
-{
-  ReleaseLaterRecord *last, *tem;
-
-  tem = records->next;
-
-  while (tem != records)
-    {
-      last = tem;
-      tem = tem->next;
-
-      /* Cancel the idle callback if it already exists.  */
-      if (last->key)
-	RenderCancelIdleCallback (last->key);
-
-      /* Release the buffer now.  */
-      XLReleaseBuffer (last->buffer);
-
-      /* Before freeing the record itself.  */
-      XLFree (last);
-    }
-
-  XLFree (records);
-}
-
-static ReleaseLaterRecord *
-AddRecordAfter (ReleaseLaterRecord *start)
-{
-  ReleaseLaterRecord *record;
-
-  record = XLMalloc (sizeof *record);
-  record->next = start->next;
-  record->last = start;
-
-  start->next->last = record;
-  start->next = record;
-
-  return record;
-}
-
 static ReconstrainCallback *
 AddCallbackAfter (ReconstrainCallback *start)
 {
@@ -345,8 +271,7 @@ RunFrameCallbacks (Surface *surface, XdgRole *role)
 static void
 RunFrameCallbacksConditionally (XdgRole *role)
 {
-  if (role->release_records->last == role->release_records
+  if (!(role->state & StatePendingBufferRelease))
-      && role->role.surface)
     RunFrameCallbacks (role->role.surface, role);
   else if (role->role.surface)
     /* weston-simple-shm seems to assume that a frame callback can
@@ -355,24 +280,20 @@ RunFrameCallbacksConditionally (XdgRole *role)
 }
 
 static void
-BufferIdleCallback (RenderBuffer buffer, void *data)
+AllBuffersReleased (void *data)
 {
-  ReleaseLaterRecord *record;
   XdgRole *role;
   Surface *surface;
 
-  record = data;
+  role = data;
-  role = record->role;
-
-  XLReleaseBuffer (record->buffer);
-  DeleteRecord (record);
-
   surface = role->role.surface;
 
-  /* Run frame callbacks now, if no more buffers are waiting to be
+  /* Clear the buffer release flag.  */
+  role->state &= ~StatePendingBufferRelease;
+
+  /* Run frame callbacks now, as no more buffers are waiting to be
      released.  */
-  if (surface && role->state & StatePendingFrameCallback
+  if (surface && role->state & StatePendingFrameCallback)
-      && role->release_records->next == role->release_records)
     {
       RunFrameCallbacks (surface, role);
 
@@ -794,9 +715,7 @@ ReleaseBacking (XdgRole *role)
   /* Release all buffers pending release.  The sync is necessary
      because the X server does not perform operations immediately
      after the Xlib function is called.  */
-
+  FreeBufferReleaseHelper (role->release_helper);
-  XSync (compositor.display, False);
-  FreeRecords (role->release_records);
 
   /* Now release the reference to any toplevel implementation that
      might be attached.  */
@@ -858,7 +777,6 @@ static void
 ReleaseBuffer (Surface *surface, Role *role, ExtBuffer *buffer)
 {
   RenderBuffer render_buffer;
-  ReleaseLaterRecord *record;
   XdgRole *xdg_role;
 
   render_buffer = XLRenderBufferFromBuffer (buffer);
@@ -870,13 +788,9 @@ ReleaseBuffer (Surface *surface, Role *role, ExtBuffer *buffer)
   else
     {
       /* Release the buffer once it is destroyed or becomes idle.  */
-      record = AddRecordAfter (xdg_role->release_records);
+      ReleaseBufferWithHelper (xdg_role->release_helper,
-      record->buffer = buffer;
+			       buffer, xdg_role->target);
-      record->key = RenderAddIdleCallback (render_buffer,
+      xdg_role->state |= StatePendingBufferRelease;
-					   xdg_role->target,
-					   BufferIdleCallback,
-					   record);
-      record->role = xdg_role;
     }
 }
 
@@ -1518,24 +1432,12 @@ XLGetXdgSurface (struct wl_client *client, struct wl_resource *resource,
 
   memset (role, 0, sizeof *role);
 
-  role->release_records
-    = XLSafeMalloc (sizeof *role->release_records);
-
-  if (!role->release_records)
-    {
-      XLFree (role);
-      wl_client_post_no_memory (client);
-
-      return;
-    }
-
   role->role.resource = wl_resource_create (client, &xdg_surface_interface,
 					    wl_resource_get_version (resource),
 					    id);
 
   if (!role->role.resource)
     {
-      XLFree (role->release_records);
       XLFree (role);
       wl_client_post_no_memory (client);
 
@@ -1579,17 +1481,14 @@ XLGetXdgSurface (struct wl_client *client, struct wl_resource *resource,
   attrs.cursor = InitDefaultCursor ();
   flags = CWColormap | CWBorderPixel | CWEventMask | CWCursor;
 
-  /* Sentinel node.  */
-  role->release_records->next = role->release_records;
-  role->release_records->last = role->release_records;
-  role->release_records->buffer = NULL;
-
   role->window = XCreateWindow (compositor.display,
 				DefaultRootWindow (compositor.display),
 				0, 0, 20, 20, 0, compositor.n_planes,
 				InputOutput, compositor.visual, flags,
 				&attrs);
   role->target = RenderTargetFromWindow (role->window, DefaultEventMask);
+  role->release_helper = MakeBufferReleaseHelper (AllBuffersReleased,
+						  role);
 
   role->subcompositor = MakeSubcompositor ();
   role->clock = XLMakeFrameClockForWindow (role->window);