Cursor Scaling Support #3735

tarek-y-ismail · 2025-02-04T16:41:51Z

Currently only implements cursor scaling for software cursors. Missing the actual configuration portion.

src/server/CMakeLists.txt

src/server/graphics/software_cursor.cpp

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

Works around lifetime issues

Since the order of initialization is the same as the order of declaration, the config file was initialized before the mutex. But it would use the mutex during its construction since it loads on construction, causing an error to be thrown and the application to crash.

cursor options

Duplicate of the last commit

In the grand scheme of things, the cursor isn't scaled nor is it shown/hidden that much. So this check doesn't help much, while breaking tests

visible

tarek-y-ismail · 2025-02-06T12:40:47Z

src/platform/CMakeLists.txt

@@ -1,3 +1,5 @@
+pkg_check_modules(PIXMAN REQUIRED IMPORTED_TARGET pixman-1)


Not super sure about this but putting this line in src/platform/graphics/CMakeLists.txt causes a cmake error so this is the best I had in mind.

src/platforms/gbm-kms/server/kms/cursor.h

include/platform/mir/graphics/pixman_image_scaling.h

src/include/server/mir/default_server_configuration.h

`create_scaled_renderable_for_current_cursor`

tarek-y-ismail · 2025-02-07T06:40:40Z

SoftwareCursor can be scaled by updating the width and height returned from screen_position

mir/src/server/graphics/software_cursor.cpp

Lines 81 to 85 in fbf87c7

    
           geom::Rectangle screen_position() const override 
        
           { 
        
               std::lock_guard lock{position_mutex}; 
        
               return {position, buffer_->size()}; 
        
           }

Edit: done

Allows both `Cursor::{show, set_scale}` to lock to avoid races while showing/scaling the cursor, while also not requiring a recursive lock. We previously needed a recursive lock since `set_scale` called `show`, and both would lock the mutex, requiring either a recursive lock, or adding a non-locking version of `set_scale`. No other need for a recursive lock.

tarek-y-ismail · 2025-02-07T09:12:07Z

Bug: If you change the cursor scale from inside the running compositor, a copy of the new cursor is displayed where the old cursor was.

Only happens if the cursor is over kgx/weston-terminal

Edit: Doesn't seem to happen anymore. Not sure why it came up in the first place

tarek-y-ismail · 2025-02-07T09:22:51Z

Another bug: the cursor doesn't work properly with x apps

False alarm, xterm launched from kgx inside mir_demo_sever was opening outside the intended compositor. Ported the changes to miral-shell and started xterm from there and the cursor works properly.

AlanGriffiths

A bunch of "housekeeping"...

AlanGriffiths · 2025-02-07T16:28:06Z

src/platform/symbols.map

@@ -84,6 +84,7 @@ MIR_PLATFORM_2.19 {
    mir::graphics::initialise_egl_logger*;
    mir::graphics::operator*;
    mir::graphics::red_channel_depth*;
+    mir::graphics::scale_cursor_image*;


This PR breaks the platform ABI, everything should move to a 2.20 stanza

AlanGriffiths · 2025-02-07T16:29:53Z

src/include/server/mir/default_server_configuration.h

-    CachedPtr<shell::AccessibilityManager> accessibility_manager;
    CachedPtr<shell::decoration::Manager> decoration_manager;
    CachedPtr<scene::ApplicationNotRespondingDetector> application_not_responding_detector;
    CachedPtr<cookie::Authority> cookie_authority;
    CachedPtr<input::receiver::XKBMapperRegistrar> xkb_mapper_registrar;
    std::shared_ptr<ConsoleServices> console_services;
    std::shared_ptr<DecorationStrategy> decoration_strategy;
+    CachedPtr<shell::AccessibilityManager> accessibility_manager;


Looks like an unnecessary reordering change?

AlanGriffiths · 2025-02-07T16:33:27Z

src/server/symbols.map

+    mir::shell::AccessibilityManager::cursor_scale_changed*;
+    mir::shell::AccessibilityManager::set_cursor*;


Changing the layout of AccessibilityManager is an ABI break: we need to move to a 2.20 stanza

AlanGriffiths · 2025-02-07T16:36:13Z

tests/unit-tests/graphics/test_software_cursor.cpp

@@ -424,12 +426,12 @@ TEST_F(SoftwareCursor, handles_argb_8888_cursor_surface)
    ON_CALL(mock_buffer_allocator, supported_pixel_formats())
        .WillByDefault(Return(std::vector<MirPixelFormat>{ mir_pixel_format_argb_8888 }));

-    StubCursorImage test_image{{8, 8}};
+    auto test_image = std::make_shared<StubCursorImage>(geom::Displacement{8, 8});


Suggested change

auto test_image = std::make_shared<StubCursorImage>(geom::Displacement{8, 8});

auto const test_image = std::make_shared<StubCursorImage>(geom::Displacement{8, 8});

AlanGriffiths · 2025-02-07T16:36:32Z

tests/unit-tests/graphics/test_software_cursor.cpp

@@ -478,12 +480,12 @@ TEST_F(SoftwareCursor, handles_argb_8888_buffer_with_stride)
    ON_CALL(mock_buffer_allocator, supported_pixel_formats())
        .WillByDefault(Return(std::vector<MirPixelFormat>{ mir_pixel_format_argb_8888 }));

-    StubCursorImage test_image{{8, 8}};
+    auto test_image = std::make_shared<StubCursorImage>(geom::Displacement{8, 8});


Suggested change

auto test_image = std::make_shared<StubCursorImage>(geom::Displacement{8, 8});

auto const test_image = std::make_shared<StubCursorImage>(geom::Displacement{8, 8});

AlanGriffiths · 2025-02-07T16:36:49Z

tests/unit-tests/input/test_cursor_controller.cpp

 {
-    auto image = dynamic_cast<NamedCursorImage const*>(&i);
+    auto image = std::dynamic_pointer_cast<NamedCursorImage const>(i);


Suggested change

auto image = std::dynamic_pointer_cast<NamedCursorImage const>(i);

auto const image = std::dynamic_pointer_cast<NamedCursorImage const>(i);

AlanGriffiths · 2025-02-07T16:37:38Z

tests/unit-tests/platforms/gbm-kms/kms/test_cursor.cpp

@@ -321,7 +322,7 @@ struct MesaCursorTest : ::testing::Test
    }

    testing::NiceMock<mtd::MockDRM> mock_drm;
-    StubCursorImage stub_image;
+    std::shared_ptr<StubCursorImage> stub_image = std::make_shared<StubCursorImage>();


Suggested change

std::shared_ptr<StubCursorImage> stub_image = std::make_shared<StubCursorImage>();

auto const stub_image = std::make_shared<StubCursorImage>();

tarek-y-ismail self-assigned this Feb 4, 2025

Saviq requested changes Feb 4, 2025

View reviewed changes

src/server/CMakeLists.txt Outdated Show resolved Hide resolved

src/server/CMakeLists.txt Outdated Show resolved Hide resolved

Add and stub out mg::Cursor::set_scale

60c667e

tarek-y-ismail force-pushed the MIRENG-899/cursor-scaling-support branch 2 times, most recently from 9cd58cb to ed17b1f Compare February 5, 2025 12:12

AlanGriffiths reviewed Feb 5, 2025

View reviewed changes

src/server/graphics/software_cursor.cpp Outdated Show resolved Hide resolved

tarek-y-ismail force-pushed the MIRENG-899/cursor-scaling-support branch 6 times, most recently from 4f3b5dc to b5ac991 Compare February 6, 2025 14:09

tarek-y-ismail and others added 13 commits February 6, 2025 16:24

Include and link against pixman

e1ac97d

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

Use pixman to scale cursor images

7a544ab

Add bilinear filtering to scaled cursor images

8c0faf8

Use unique ptrs to free up pixman_image_t's

7172fa1

Swap accessibility_manager to be a shared pointer for the time being

e294a14

Works around lifetime issues

Hook up command line and input file configuration of cursor scale

ff38c67

Refactor SoftwareCursor to separate cursor initialization and scaling

a3bd7aa

Fix brace placement in accessibility_manager.cpp

1894915

Properly merge cli/env/.config cursor/keyboard/touch options with shell

e199fe2

cursor options

Implement Cursor::set_scale for trivial implmentations

83c29fa

Change Cursor::show to take a shared pointer to the cursor image

59e30c3

Move cursor scaling logic to pixman_image_scaling.{h,cpp}

f088dc6

tarek-y-ismail force-pushed the MIRENG-899/cursor-scaling-support branch from b5ac991 to 080c35b Compare February 6, 2025 14:25

tarek-y-ismail added 4 commits February 6, 2025 18:07

Implement cursor scaling for gbm-kms

71c707c

Implement cursor scaling for atomic-kms

9bbf1d7

Duplicate of the last commit

Implement cursor scaling for wayland cursors

010f167

Account for the special case of scale = 1 in scale_cursor_image

e2d9975

tarek-y-ismail added 2 commits February 6, 2025 18:07

Don't check for buffers or scale when Cursor::set_scale is called.

24b53e2

In the grand scheme of things, the cursor isn't scaled nor is it shown/hidden that much. So this check doesn't help much, while breaking tests

Set visiblity on Cursor::show, remove on {show, set_scale} if

bd302df

visible

tarek-y-ismail force-pushed the MIRENG-899/cursor-scaling-support branch from 080c35b to bd302df Compare February 6, 2025 16:07

tarek-y-ismail commented Feb 6, 2025

View reviewed changes

Take into account cursor hotspot scaling in

fbf87c7

`create_scaled_renderable_for_current_cursor`

tarek-y-ismail marked this pull request as ready for review February 6, 2025 16:25

tarek-y-ismail requested a review from a team as a code owner February 6, 2025 16:25

tarek-y-ismail requested review from Saviq and AlanGriffiths February 7, 2025 06:04

tarek-y-ismail added 2 commits February 7, 2025 09:57

Move duplication current_{scale, cursor_state} to mg::Cursor

f3eb7b9

tarek-y-ismail added 2 commits February 7, 2025 11:23

Revert the accessibility manager to a cached ptr and pass it the cursor

5b210ae

Use Renderable::screen_position to scale software cursors.

ce78b72

AlanGriffiths requested changes Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cursor Scaling Support #3735

Cursor Scaling Support #3735

tarek-y-ismail commented Feb 4, 2025

tarek-y-ismail Feb 6, 2025

tarek-y-ismail commented Feb 7, 2025 •

edited

Loading

tarek-y-ismail commented Feb 7, 2025 •

edited

Loading

tarek-y-ismail commented Feb 7, 2025 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths Feb 7, 2025

AlanGriffiths Feb 7, 2025

AlanGriffiths Feb 7, 2025

AlanGriffiths Feb 7, 2025

AlanGriffiths Feb 7, 2025

AlanGriffiths Feb 7, 2025

AlanGriffiths Feb 7, 2025

		@@ -1,3 +1,5 @@
		pkg_check_modules(PIXMAN REQUIRED IMPORTED_TARGET pixman-1)

		mir::shell::AccessibilityManager::cursor_scale_changed*;
		mir::shell::AccessibilityManager::set_cursor*;

	auto test_image = std::make_shared<StubCursorImage>(geom::Displacement{8, 8});
	auto const test_image = std::make_shared<StubCursorImage>(geom::Displacement{8, 8});

	auto image = std::dynamic_pointer_cast<NamedCursorImage const>(i);
	auto const image = std::dynamic_pointer_cast<NamedCursorImage const>(i);

	std::shared_ptr<StubCursorImage> stub_image = std::make_shared<StubCursorImage>();
	auto const stub_image = std::make_shared<StubCursorImage>();

Cursor Scaling Support #3735

Are you sure you want to change the base?

Cursor Scaling Support #3735

Conversation

tarek-y-ismail commented Feb 4, 2025

Choose a reason for hiding this comment

tarek-y-ismail commented Feb 7, 2025 • edited Loading

tarek-y-ismail commented Feb 7, 2025 • edited Loading

tarek-y-ismail commented Feb 7, 2025 • edited Loading

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarek-y-ismail commented Feb 7, 2025 •

edited

Loading

tarek-y-ismail commented Feb 7, 2025 •

edited

Loading

tarek-y-ismail commented Feb 7, 2025 •

edited

Loading