Partial implementation of ext_image_copy_capture_v1 cursor sessions#4632
Partial implementation of ext_image_copy_capture_v1 cursor sessions#4632jhenstridge wants to merge 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements cursor session support for the ext_image_copy_capture_v1 Wayland protocol. The implementation adds cursor position tracking via CursorObserverMultiplexer and a backend architecture for transforming output-space coordinates to capture-space coordinates.
Changes:
- Extended the image copy capture manager to accept cursor observer and clock dependencies
- Implemented cursor session class with position tracking and visibility management
- Added backend abstraction for coordinate mapping between different capture sources
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wayland_default_configuration.cpp | Passes cursor observer multiplexer and clock to image copy capture manager factory |
| wayland_connector.h | Adds cursor observer multiplexer and clock to WaylandExtensions context |
| wayland_connector.cpp | Wires cursor observer multiplexer through connector constructor |
| ext_image_capture_v1.h | Updates function signatures to accept new dependencies |
| ext_image_capture_v1.cpp | Implements cursor session tracking, coordinate mapping, and placeholder cursor rendering |
| auto data = mapping->data(); | ||
| auto size = mapping->size(); | ||
| auto stride = mapping->stride(); | ||
| printf("Buffer length is %d (%dx%d, stride %d)\n", (int)mapping->len(), size.width.as_int(), size.height.as_int(), stride.as_int()); |
There was a problem hiding this comment.
Debug print statements should be removed before merging. Use proper logging infrastructure if diagnostic output is needed.
AlanGriffiths
left a comment
There was a problem hiding this comment.
There's some tidy-up that can be done now and not wait for a follow-up
| for (int y = 0; y < size.height.as_int(); y++) | ||
| { | ||
| for (int x = 0; x < size.width.as_int(); x++) | ||
| { | ||
| auto pos = y * stride.as_int() + x * 4; | ||
|
|
||
| if (x <= y) | ||
| { | ||
| data[pos] = std::byte{0xff}; | ||
| data[pos+1] = std::byte{0xff}; | ||
| data[pos+2] = std::byte{0xff}; | ||
| data[pos+3] = std::byte{0xff}; | ||
| } | ||
| else | ||
| { | ||
| data[pos] = std::byte{0x00}; | ||
| data[pos+1] = std::byte{0x00}; | ||
| data[pos+2] = std::byte{0x00}; | ||
| data[pos+3] = std::byte{0x00}; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I guess this temporary faking for image capture? I won't whine about the C-style loops
mattkae
left a comment
There was a problem hiding this comment.
Some structural thoughts, but looks mostly reasonable!
4f6c309 to
0ee4284
Compare
| // From CursorObserver | ||
| void cursor_moved_to(float abs_x, float abs_y); |
There was a problem hiding this comment.
What does this comment mean? ExtImageCopyCaptureCursorSessionV1 is not a CursorObserver
There was a problem hiding this comment.
@jhenstridge why do you keep marking this comment "resolved"? It still seems valid to me
There was a problem hiding this comment.
Still a confusing comment
AlanGriffiths
left a comment
There was a problem hiding this comment.
Can we correct the misleading comment on ExtImageCopyCaptureCursorSessionV1::cursor_moved_to()?
AlanGriffiths
left a comment
There was a problem hiding this comment.
Can we correct the misleading comment on the declaration of ExtImageCopyCaptureCursorSessionV1::cursor_moved_to()?
The session uses the CursorObserverMultiplexer to track the cursor position, and a source-dependent backend class to transform the output space coordinates to match those of the captured image. Currently it sends a white triangle as the cursor image rather than the actual cursor image.
* Remove some debug statements * Clean up cursor position transformation
…tion While the class could potentially be extended to send a shutdown event to the cursor session's image copy session, clients will likely act on the shutdown signal on the main image copy session. That leaves us with a class providing a single virtual method, so passing a closure seems simpler.
…essionV1::CursorObserver The observer will not be invoked after it has been unregistered and freed by the cursor session, so we don't need to repeat this protection.
837a8da to
e34b6b7
Compare
TICS Quality Gate✔️ Passedmir
|
mattkae
left a comment
There was a problem hiding this comment.
Cool by me! I'll let Alan's feedback take it over the line
| : public mi::CursorObserver | ||
| { | ||
| public: | ||
| CursorObserver(ExtImageCopyCaptureCursorSessionV1& session) |
There was a problem hiding this comment.
Nit:
| CursorObserver(ExtImageCopyCaptureCursorSessionV1& session) | |
| explicit CursorObserver(ExtImageCopyCaptureCursorSessionV1& session) |
There was a problem hiding this comment.
Yes, better to be consistent.
AlanGriffiths
left a comment
There was a problem hiding this comment.
Cool by me! I'll let Alan's feedback take it over the line
Just the same feedback I left two weeks ago
| // From CursorObserver | ||
| void cursor_moved_to(float abs_x, float abs_y); |
There was a problem hiding this comment.
Still a confusing comment
| : public mi::CursorObserver | ||
| { | ||
| public: | ||
| CursorObserver(ExtImageCopyCaptureCursorSessionV1& session) |
There was a problem hiding this comment.
Yes, better to be consistent.
What's new?
This implements cursor the cursor session parts of the ext_image_copy_capture_v1 protocol. At present, it sends a dummy cursor image, which will be replaced with the real cursor in a follow-up PR.
Cursor position tracking is done via the CursorObserverMultiplexer, with a source-dependent backend class responsible for transforming the output space cursor coordinates to those of the corresponding image capture, and indicating whether the cursor is within bounds.
This can be tested like so:
WAYLAND_DISPLAY=wayland-1 wayvncChecklist