vm_tools: Refactors sommelier-output to batch up and group send updates

This is a pre-patch for the introduction of xdg-output. By batching up the output/update information inside one function it'll make it easier to integrate the logic of forwarding output and direct scale together. Also added some comments to help explain what all of the various scale numbers are in sl_context and sl_host_output. BUG=b:289158040 TEST=Unittests run, tested on device. Change-Id: Ib43853af6ef5a11ea0d4a6dcd28293333f443140 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/4891392 Reviewed-by: Chloe Pelling <[email protected]> Commit-Queue: Justin Huang <[email protected]> Tested-by: Justin Huang <[email protected]>
WhatAmISupposedToPutHere · Oct 19, 2023 · 2a9193c · 2a9193c
1 parent 40d56de
commit 2a9193c
Show file tree

Hide file tree

Showing 7 changed files with 84 additions and 85 deletions.
diff --git a/sommelier-ctx.h b/sommelier-ctx.h
@@ -67,6 +67,7 @@ enum {
   ATOM_LAST = ATOM_SOMMELIER_QUIRK_APPLIED,
 };
 
+// A series of configurations and objects shared globally.
 struct sl_context {
   char** runprog;
 
@@ -144,7 +145,11 @@ struct sl_context {
 #ifdef GAMEPAD_SUPPORT
   struct wl_list gamepads;
 #endif
+  // The scale provided by the --scale argument to sommelier.
   double desired_scale;
+  // The calculated value of desired_scale * default_scale. Default scale is the
+  // device_scale_factor * preferred_scale (usually preferred_scale = 1) of the
+  // internal screen (which is assumed to be the first output).
   double scale;
 
   // These scale factors are used for the direct scaling mode.

diff --git a/sommelier-output-test.cc b/sommelier-output-test.cc
@@ -53,12 +53,10 @@ TEST_F(X11DirectScaleTest,
   output = ctx.host_outputs[0];
   EXPECT_EQ(output->x, 0);
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[1];
   EXPECT_EQ(output->x, 1920);
   EXPECT_EQ(output->virt_x, 1920);
-  EXPECT_EQ(output->virt_y, 0);
 
   // Advertise another output to the left.
   AdvertiseOutputs(xwayland.get(), {{.x = -800,
@@ -71,17 +69,14 @@ TEST_F(X11DirectScaleTest,
   output = ctx.host_outputs[0];
   EXPECT_EQ(output->x, -800);
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[1];
   EXPECT_EQ(output->x, 0);
   EXPECT_EQ(output->virt_x, 1080);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[2];
   EXPECT_EQ(output->x, 1920);
   EXPECT_EQ(output->virt_x, 3000);
-  EXPECT_EQ(output->virt_y, 0);
 }
 
 TEST_F(X11Test, AddingMultipleOutputsPositionsCorrectly) {
@@ -102,12 +97,10 @@ TEST_F(X11Test, AddingMultipleOutputsPositionsCorrectly) {
   output = ctx.host_outputs[0];
   EXPECT_EQ(output->x, 0);
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[1];
   EXPECT_EQ(output->x, 1920);
   EXPECT_EQ(output->virt_x, 1920);
-  EXPECT_EQ(output->virt_y, 0);
 
   // Act
   AdvertiseOutputs(xwayland.get(), {{
@@ -123,17 +116,14 @@ TEST_F(X11Test, AddingMultipleOutputsPositionsCorrectly) {
   output = ctx.host_outputs[0];
   EXPECT_EQ(output->x, -800);
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[1];
   EXPECT_EQ(output->x, 0);
   EXPECT_EQ(output->virt_x, 1440);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[2];
   EXPECT_EQ(output->x, 1920);
   EXPECT_EQ(output->virt_x, 3360);
-  EXPECT_EQ(output->virt_y, 0);
 }
 
 TEST_F(X11Test, OutputsPositionedCorrectlyAfterRemovingLeftOutput) {
@@ -159,12 +149,10 @@ TEST_F(X11Test, OutputsPositionedCorrectlyAfterRemovingLeftOutput) {
   output = ctx.host_outputs[0];
   EXPECT_EQ(output->x, 1920);
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[1];
   EXPECT_EQ(output->x, 2000);
   EXPECT_EQ(output->virt_x, 540);
-  EXPECT_EQ(output->virt_y, 0);
 
   // outputs has length 2.
   EXPECT_EQ(ctx.host_outputs.size(), 2u);
@@ -193,12 +181,10 @@ TEST_F(X11Test, OutputsPositionedCorrectlyAfterRemovingMiddleOutput) {
   output = ctx.host_outputs[0];
   EXPECT_EQ(output->x, 0);
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[1];
   EXPECT_EQ(output->x, 2000);
   EXPECT_EQ(output->virt_x, 1920);
-  EXPECT_EQ(output->virt_y, 0);
 
   // outputs has length 2.
   EXPECT_EQ(ctx.host_outputs.size(), 2u);
@@ -220,7 +206,6 @@ TEST_F(X11Test, OtherOutputUnchangedAfterRemovingRightOutput) {
   output = ctx.host_outputs[0];
   EXPECT_EQ(output->x, 0);
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
   // outputs has length 1.
   EXPECT_EQ(ctx.host_outputs.size(), 1u);
 }
@@ -240,13 +225,11 @@ TEST_F(X11Test, RotatingOutputsShiftsNeighbouringOutputs) {
 
   // Assert: Output is rotated and next output is shifted left.
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
   EXPECT_EQ(output->virt_rotated_width, 1080);
   EXPECT_EQ(output->virt_rotated_height, 1920);
 
   output = ctx.host_outputs[1];
   EXPECT_EQ(output->virt_x, 1080);
-  EXPECT_EQ(output->virt_y, 0);
 }
 
 TEST_F(X11Test, MovingOutputsShiftsOutputs) {
@@ -266,12 +249,10 @@ TEST_F(X11Test, MovingOutputsShiftsOutputs) {
   output = ctx.host_outputs[0];
   EXPECT_EQ(output->x, -1920);
   EXPECT_EQ(output->virt_x, 0);
-  EXPECT_EQ(output->virt_y, 0);
 
   output = ctx.host_outputs[1];
   EXPECT_EQ(output->x, 0);
   EXPECT_EQ(output->virt_x, 1920);
-  EXPECT_EQ(output->virt_y, 0);
 }
 
 }  // namespace sommelier

diff --git a/sommelier-output.cc b/sommelier-output.cc
@@ -276,48 +276,16 @@ void sl_output_get_dimensions_original(struct sl_host_output* host,
 }
 
 // Recalculates the virt_x coordinates of outputs when an output is
-// add/removed/changed. skip_host is false if the host is being removed
-// (as it should not exist in the list already) and true if it's being
-// added/changed.
-void sl_output_shift_output_x(struct sl_host_output* host, bool skip_host) {
-  // Outputs are positioned in a line from left to right ordered base on its
-  // x position.
+// add/removed/changed.
+void sl_output_update_output_x(struct sl_context* ctx) {
+  // Outputs are positioned in a line from left to right based off their x
+  // position.
   int next_output_x = 0;
-  for (auto output : host->ctx->host_outputs) {
+  for (auto output : ctx->host_outputs) {
+    // Update the value and mark for sending.
     if (output->virt_x != next_output_x) {
       output->virt_x = next_output_x;
-      // Skipping sending current output's details here if skip host is set
-      // as they are sent after this method.
-      if (!skip_host || output != host) {
-        // virt_physical_width/height may have not been set yet.
-        if (!output->virt_physical_width || !output->virt_physical_height) {
-          int scale;
-          int virt_physical_width;
-          int virt_physical_height;
-          int virt_width;
-          int virt_height;
-
-          if (host->ctx->use_direct_scale) {
-            sl_output_init_dimensions_direct(host, &scale, &virt_physical_width,
-                                             &virt_physical_height, &virt_width,
-                                             &virt_height);
-          } else {
-            sl_output_get_dimensions_original(
-                host, &scale, &virt_physical_width, &virt_physical_height,
-                &virt_width, &virt_height);
-          }
-          host->virt_physical_width = virt_physical_width;
-          host->virt_physical_height = virt_physical_height;
-        }
-
-        wl_output_send_geometry(output->resource, output->virt_x,
-                                output->virt_y, output->virt_physical_width,
-                                output->virt_physical_height, output->subpixel,
-                                output->make, output->model, output->transform);
-        if (wl_resource_get_version(output->resource) >=
-            WL_OUTPUT_DONE_SINCE_VERSION)
-          wl_output_send_done(output->resource);
-      }
+      output->needs_update = true;
     }
     next_output_x += output->virt_rotated_width;
   }
@@ -393,7 +361,7 @@ struct sl_host_output* sl_infer_output_for_guest_position(
   return last;
 }
 
-void sl_output_send_host_output_state(struct sl_host_output* host) {
+void sl_output_calculate_virtual_dimensions(struct sl_host_output* host) {
   int scale;
   int virt_physical_width;
   int virt_physical_height;
@@ -410,26 +378,36 @@ void sl_output_send_host_output_state(struct sl_host_output* host) {
                                       &virt_height);
   }
 
+  host->scale_factor = scale;
+  host->virt_width = virt_width;
+  host->virt_height = virt_height;
   host->virt_physical_width = virt_physical_width;
   host->virt_physical_height = virt_physical_height;
 
   sl_output_apply_rotation(host->transform, virt_width, virt_height,
                            &host->virt_rotated_width,
                            &host->virt_rotated_height);
+  host->needs_update = true;
+}
 
-  // Shift all outputs that are to the right of host to the right if needed.
-  sl_output_shift_output_x(host, true);
-  host->virt_y = 0;
-  wl_output_send_geometry(host->resource, host->virt_x, host->virt_y,
-                          host->virt_physical_width, host->virt_physical_height,
-                          host->subpixel, host->make, host->model,
-                          host->transform);
-  wl_output_send_mode(host->resource, host->flags | WL_OUTPUT_MODE_CURRENT,
-                      virt_width, virt_height, host->refresh);
-  if (wl_resource_get_version(host->resource) >= WL_OUTPUT_SCALE_SINCE_VERSION)
-    wl_output_send_scale(host->resource, scale);
-  if (wl_resource_get_version(host->resource) >= WL_OUTPUT_DONE_SINCE_VERSION)
-    wl_output_send_done(host->resource);
+// Function which pushes the state of an output to the client.
+void sl_output_send_host_output_state(struct sl_host_output* host) {
+  // Could be more granular, but the current implementation means that if one
+  // value changes, everything should be impacted.
+  if (host->needs_update) {
+    wl_output_send_geometry(host->resource, host->virt_x, 0,
+                            host->virt_physical_width,
+                            host->virt_physical_height, host->subpixel,
+                            host->make, host->model, host->transform);
+    wl_output_send_mode(host->resource, host->flags | WL_OUTPUT_MODE_CURRENT,
+                        host->virt_width, host->virt_height, host->refresh);
+    if (wl_resource_get_version(host->resource) >=
+        WL_OUTPUT_SCALE_SINCE_VERSION)
+      wl_output_send_scale(host->resource, host->scale_factor);
+    if (wl_resource_get_version(host->resource) >= WL_OUTPUT_DONE_SINCE_VERSION)
+      wl_output_send_done(host->resource);
+    host->needs_update = false;
+  }
 }
 
 static void sl_output_geometry(void* data,
@@ -487,6 +465,7 @@ static void sl_output_mode(void* data,
   host->width = width;
   host->height = height;
   host->refresh = refresh;
+  host->needs_update = true;
 }
 
 static void sl_output_done(void* data, struct wl_output* output) {
@@ -497,6 +476,10 @@ static void sl_output_done(void* data, struct wl_output* output) {
   if (host->expecting_scale)
     return;
 
+  // Recalculate according to any information that's been modified.
+  sl_output_calculate_virtual_dimensions(host);
+  // Shift all outputs that are to the right of host to the right if needed.
+  sl_output_update_output_x(host->ctx);
   sl_output_send_host_output_state(host);
 
   // Expect scale if aura output exists.
@@ -573,7 +556,7 @@ static void sl_destroy_host_output(struct wl_resource* resource) {
   free(host->make);
   free(host->model);
   // Shift all outputs to the right of the deleted output to the left.
-  sl_output_shift_output_x(host, false);
+  sl_output_update_output_x(host->ctx);
   delete host;
 }
 
@@ -623,10 +606,11 @@ static void sl_bind_host_output(struct wl_client* client,
   host->aura_output = nullptr;
   // We assume that first output is internal by default.
   host->internal = ctx->host_outputs.empty();
+  // We'll always need to forward this information.
+  host->needs_update = true;
   host->x = 0;
   host->y = 0;
   host->virt_x = 0;
-  host->virt_y = 0;
   host->logical_x = 0;
   host->logical_y = 0;
   host->physical_width = 0;
@@ -640,6 +624,8 @@ static void sl_bind_host_output(struct wl_client* client,
   host->flags = 0;
   host->width = 1024;
   host->height = 768;
+  host->virt_width = 1024;
+  host->virt_height = 768;
   host->virt_rotated_width = 0;
   host->virt_rotated_height = 0;
   host->logical_width = 1024;

diff --git a/sommelier-transform.cc b/sommelier-transform.cc
@@ -401,7 +401,6 @@ struct sl_host_output* sl_transform_guest_position_to_host_position(
 
   // Translate from global to output-relative guest coordinates
   (*x) -= output->virt_x;
-  (*y) -= output->virt_y;
 
   // Convert to host logical scale
   sl_transform_guest_to_host(ctx, surface, x, y);
@@ -427,7 +426,6 @@ struct sl_host_output* sl_transform_host_position_to_guest_position(
 
   // Translate to global guest coordinates
   (*x) += output->virt_x;
-  (*y) += output->virt_y;
 
   return output;
 }

diff --git a/sommelier-window.cc b/sommelier-window.cc
@@ -305,7 +305,6 @@ static void sl_internal_toplevel_configure(struct sl_window* window,
         window->next_config.values[i++] =
             output->virt_x + (output->virt_rotated_width - width_in_pixels) / 2;
         window->next_config.values[i++] =
-            output->virt_y +
             (output->virt_rotated_height - height_in_pixels) / 2;
       } else {
         window->next_config.values[i++] =

diff --git a/sommelier.cc b/sommelier.cc
@@ -200,8 +200,7 @@ static void sl_adjust_window_position_for_screen_size(
   if (output) {
     window->x =
         output->virt_x + (output->virt_rotated_width - window->width) / 2;
-    window->y =
-        output->virt_y + (output->virt_rotated_height - window->height) / 2;
+    window->y = (output->virt_rotated_height - window->height) / 2;
   } else {
     // Center horizontally/vertically.
     window->x = ctx->screen->width_in_pixels / 2 - window->width / 2;
@@ -3282,6 +3281,9 @@ static void sl_calculate_scale_for_xwayland(struct sl_context* ctx) {
   ctx->scale = MIN(MAX_SCALE, MAX(MIN_SCALE, scale));
 
   // Scale affects output state. Send updated output state to xwayland.
+  for (auto output : ctx->host_outputs)
+    sl_output_calculate_virtual_dimensions(output);
+  sl_output_update_output_x(ctx);
   for (auto output : ctx->host_outputs)
     sl_output_send_host_output_state(output);
 }