frontend/wayland: `wp_viewporter` support #3445

RAOF · 2024-06-26T08:27:14Z

Accidentally all squashed together into a single mega-commit, here's wp_viewporter.

You can test with the weston-scaler client in... weston, and also with canonical/wlcs#340

mattkae

Overall makes sense, but some initial comments about error handling

include/platform/mir/graphics/renderable.h

src/server/frontend_wayland/wl_surface.cpp

AlanGriffiths

Some nits

include/platform/mir/graphics/renderable.h

AlanGriffiths · 2024-07-01T14:29:18Z

src/gl/tessellation_helpers.cpp

+    coords.top = sample_rect.top().as_value() / buffer_size.height.as_uint32_t();
+    coords.bottom = sample_rect.bottom().as_value() / buffer_size.height.as_uint32_t();
+    coords.left = sample_rect.left().as_value() / buffer_size.width.as_uint32_t();
+    coords.right = sample_rect.right().as_value() / buffer_size.width.as_uint32_t();


Hmm. All this .as_value() ... .as_uint32_t() makes me wonder if there's an operator missing in our geometry. This would look safer:

coords.top = sample_rect.top() / buffer_size;

I've ended up with sample_rect.top() / buffer_size.width (as sample_rect.top() / buffer_size is ambiguous).

AlanGriffiths · 2024-07-01T14:30:33Z

src/gl/tessellation_helpers.cpp

@@ -14,32 +14,58 @@
 * along with this program. If not, see <http://www.gnu.org/licenses/>.
 */
 #include "mir/gl/tessellation_helpers.h"
+#include "mir/geometry/forward.h"


It seems odd that we'd want forward.h in an implementation file

Oops, we don't, but clangd always wants to pull it in. I missed this one!

src/server/frontend_wayland/wp_viewporter.cpp

src/server/frontend_wayland/wp_viewporter.h

AlanGriffiths · 2024-07-01T14:39:32Z

src/server/frontend_wayland/wp_viewporter.h

+    /**
+     * Have the source or destination values changed since the last call to dirty()?
+     */
+    auto dirty() -> bool;


Not sure "dirty" is the best name here. Obvious interpretations are "is dirty" and "make dirty", but this is neither. And the actual meaning won't be obvious reading the code that calls it. Even the comment isn't clear about what the first call does.

Maybe unchanged_since_last_call()?

Or, unreported_changes()?

I've gone for changed_since_last_resolve(), and updated the implementation slightly to make that true.

mattkae

This looks to be a good implementation to me! I will let Alan get it over the line

AlanGriffiths · 2024-07-09T10:53:37Z

"This branch has conflicts that must be resolved"

…s of the code

We can provide an interface to the rest of the frontend to calculate the details `WlSurface` needs and keep all the protocol error generation code in with the `wp_viewporter` rather than spread across multiple files.

We now have a requirement for both the actual value of `scale` and for its reciprocal. Rather than storing the reciprocal and calculating the original value when needed, store the original value and calculate the reciprocal when needed.

Co-authored-by: Alan Griffiths <[email protected]>

…types. It's fine to implicitly convert `Size` -> `SizeD` or `SizeF` -> `SizeD`; these conversions lose no information. Use this to simplify some `RectangleD` constructions in the code

We change this to `changed_since_last_resolve()`.

AlanGriffiths · 2024-07-11T09:16:41Z

src/include/gl/mir/gl/tessellation_helpers.h

@@ -18,6 +18,7 @@
 #define MIR_GL_TESSELLATION_HELPERS_H_
 #include "mir/gl/primitive.h"
 #include "mir/geometry/displacement.h"
+#include "mir/geometry/rectangle.h"


Looks spurious

AlanGriffiths

OK now

Fuseteam · 2024-08-08T20:15:48Z

This closes #2599 i think

RAOF requested a review from a team as a code owner June 26, 2024 08:27

mattkae requested changes Jun 26, 2024

View reviewed changes

include/platform/mir/graphics/renderable.h Show resolved Hide resolved

src/server/frontend_wayland/wl_surface.cpp Outdated Show resolved Hide resolved

src/server/frontend_wayland/wl_surface.cpp Outdated Show resolved Hide resolved

AlanGriffiths requested changes Jul 1, 2024

View reviewed changes

mattkae approved these changes Jul 2, 2024

View reviewed changes

RAOF and others added 11 commits July 10, 2024 17:25

wayland: Add wp_viewporter protocol definition & wrapper

d6bf592

frontend_wayland: Add WpViewporter support

40b2ecc

tests/Tessellation: Make the MockRenderable fit the new expectation…

791cdb1

…s of the code

frontend_wayland/WpViewporter: Encapsulate Viewport better.

71f77df

We can provide an interface to the rest of the frontend to calculate the details `WlSurface` needs and keep all the protocol error generation code in with the `wp_viewporter` rather than spread across multiple files.

frontend_wayland/WlSurface: Store scale directly.

15f7a54

We now have a requirement for both the actual value of `scale` and for its reciprocal. Rather than storing the reciprocal and calculating the original value when needed, store the original value and calculate the reciprocal when needed.

WpViewporter: Single argument constructors should be explicit

7457c7e

Co-authored-by: Alan Griffiths <[email protected]>

Drop unnecessary and accidentally-added geometry/forward.h #include

fbe9df2

geometry/Size: Add non-lossy implicit conversions between underlying …

629f063

…types. It's fine to implicitly convert `Size` -> `SizeD` or `SizeF` -> `SizeD`; these conversions lose no information. Use this to simplify some `RectangleD` constructions in the code

geometry/Dimensions: Add X<T>/Width<U> and Y<T>/Height<U> operators.

279c854

frontend_wayland/WpViewporter: Better name for dirty()

2e8880d

We change this to `changed_since_last_resolve()`.

Use the implicit Size -> SizeD conversion a bit more

3c948ff

RAOF force-pushed the MIRENG-215-MIRENG-418-wp-viewporter-support branch from 054fda9 to 3c948ff Compare July 10, 2024 07:26

AlanGriffiths reviewed Jul 11, 2024

View reviewed changes

Revert spurious change

45d7c8d

AlanGriffiths approved these changes Jul 11, 2024

View reviewed changes

AlanGriffiths enabled auto-merge July 11, 2024 09:24

AlanGriffiths added this pull request to the merge queue Jul 11, 2024

Merged via the queue into main with commit ef638b6 Jul 11, 2024
34 of 36 checks passed

AlanGriffiths deleted the MIRENG-215-MIRENG-418-wp-viewporter-support branch July 11, 2024 10:39

AlanGriffiths mentioned this pull request Aug 9, 2024

Support wp_viewporter #2599

Closed

AlanGriffiths mentioned this pull request Sep 9, 2024

Release 2.18.0 #3532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontend/wayland: `wp_viewporter` support #3445

frontend/wayland: `wp_viewporter` support #3445

RAOF commented Jun 26, 2024

mattkae left a comment

AlanGriffiths left a comment

AlanGriffiths Jul 1, 2024

RAOF Jul 9, 2024

AlanGriffiths Jul 1, 2024

RAOF Jul 2, 2024

AlanGriffiths Jul 1, 2024

RAOF Jul 9, 2024

mattkae left a comment

AlanGriffiths commented Jul 9, 2024

AlanGriffiths Jul 11, 2024

AlanGriffiths left a comment

Fuseteam commented Aug 8, 2024

frontend/wayland: wp_viewporter support #3445

frontend/wayland: wp_viewporter support #3445

Conversation

RAOF commented Jun 26, 2024

mattkae left a comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Jul 1, 2024

Choose a reason for hiding this comment

RAOF Jul 9, 2024

Choose a reason for hiding this comment

AlanGriffiths Jul 1, 2024

Choose a reason for hiding this comment

RAOF Jul 2, 2024

Choose a reason for hiding this comment

AlanGriffiths Jul 1, 2024

Choose a reason for hiding this comment

RAOF Jul 9, 2024

Choose a reason for hiding this comment

mattkae left a comment

Choose a reason for hiding this comment

AlanGriffiths commented Jul 9, 2024

AlanGriffiths Jul 11, 2024

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

Fuseteam commented Aug 8, 2024

frontend/wayland: `wp_viewporter` support #3445

frontend/wayland: `wp_viewporter` support #3445