graphics/MultiplexingDisplay: Add HW cursor support. #3206
Conversation
Create a HW cursor on each `Display`; if `Display` cannot support a HW cursor, bail. Our existing HW cursor implementations already handle setting the position outside of their display area, so we can just directly delegate to all multiplexed `Cursor` implementations without needing to track which platform(s) should actually be handling the event.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3206 +/- ##
==========================================
+ Coverage 77.81% 77.87% +0.05%
==========================================
Files 1072 1063 -9
Lines 75097 74680 -417
==========================================
- Hits 58436 58155 -281
+ Misses 16661 16525 -136 ☔ View full report in Codecov by Sentry. |
| cursors.push_back(display->create_hardware_cursor()); | ||
| if (cursors.back() == nullptr) | ||
| { | ||
| BOOST_THROW_EXCEPTION((std::runtime_error{"Platform failed to create hardware cursor"})); | ||
| } |
There was a problem hiding this comment.
Probably not a problem here, but thinking forward: this means that when adding a virtual display we offer different behaviour. (Obviously, fixing that would need something more complex than "hardware cursor" xor "software cursor")
| mir::logging::Severity::informational, | ||
| "display", | ||
| "Failed to create hardware cursor"); | ||
| return nullptr; |
There was a problem hiding this comment.
Not sure why, but I prefer default constructing a shared_ptr<...> (original code) to implicitly constructing it with nullptr.
| for (auto display : platform_displays) | ||
| { | ||
| cursors.push_back(display->create_hardware_cursor()); | ||
| if (cursors.back() == nullptr) |
There was a problem hiding this comment.
operator!() seems more idiomatic operator==(T*)
|
Something's not right here: |
@RAOF if I understand your theory in that issue correctly, then PR is "merely" exposing an underlying bug, not itself causing problems? |
My theory is that the HW cursor (specifically, I think the last time we tried to use this API was in 2.13, so testing a 2.13-based frame and checking in the logs that it enables the hardware cursor would be a way to verify this. |
That brings back memories... # Workaround for https://github.com/MirServer/mir-kiosk/issues/23
# "Mouse offset from image on Raspberry Pi 3 with Ubuntu Core 18"
if grep -q snap_core=core18 /proc/cmdline && grep -q snap_kernel=pi-kernel /proc/cmdline
then cursor=software
fi(I know it's a different bug and doesn't help here) |
@Saviq that seems to have been resolved. We should land unless you have thoughts on my "nits"? |
I think we need confirmation on what goes on with the hw cursor on a Pi - did it ever work? I'll have a look with 2.13 as Chris suggests. |
Like this? So yeah, should work? This is with |
Three days is enough chance |
graphics/MultiplexingDisplay: Add HW cursor support.
graphics/MultiplexingDisplay: Add HW cursor support.
Create a HW cursor on each
Display; if anyDisplaycannot support a HW cursor, bail.Our existing HW cursor implementations already handle setting the position outside of their display area, so we can just directly delegate to all multiplexed
Cursorimplementations without needing to track which platform(s) should actually be handling the event.Fixes: #3198