Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scrcpy v >= 3.0.0: UI elements (e.g. brightness slider) not reacting to mouse when downsizing video (--max-size/-m) #5605

Closed
rodrigofd opened this issue Dec 4, 2024 · 4 comments

Comments

@rodrigofd
Copy link

Environment

  • OS: [e.g. Debian, Windows, macOS...]
  • Scrcpy version: 3.0.0, 3.0.1, 3.0.2
  • Installation method: Windows x64 release zip from Github releases page
  • Device model: Samsung Galaxy S23 Ultra (SM-S918B, EUX CSC)
  • Android version: 14. One UI 6.1

Describe the bug

  • When I try to use/run scrcpy to control my primary phone (S23 Ultra as stated above), although I experience the exact same issue with some of my other Samsung devices (S22 Ultra, Galaxy Tab S9+...):

  • Connection method: via tcpip + wifi, using bundled adb binary

  • I've been using scrcpy for at least 1.5 years now, for all of my android devices, never experiencing this problem that I recall.

  • It specifically appeared since 3.0.0 release

  • Every time I downsize the rendered video stream (with --max-size/-m), to anything lower than my SM-S918B native resolution (1440x3088) (e.g. --max-size=2000), and after ensuring that the SDL window is automatically resized to match the video surface (i.e. ModifierKey + G), it does what's expected (in my case, height drops from 3088 to 2000), but ** certain ui elements (seems to be happening with SystemUI-related elements but not sure if it's limited to just that), do not react to mouse clicks/button down/ups (best example is the brightness slider shown inside the quicksettings/notification panel pulled down from top border) **

  • I have reproduced this multiples times, with different combinations of switches, encoders, video-renderers, and with my other devices. The same cmd. line, with the same device, but with scrcpy older than 3.0.0, does not reproduce this bug.

Let me know if you need a snapshot/other info. for further troubleshooting.

Thanks!

@rom1v
Copy link
Collaborator

rom1v commented Dec 4, 2024

This is probably related to #5545.

Definitely, the problem is introduced by d193967.

What's interesting is that you can reproduce it only when the mirroring size is not the same as the display size.

If we inject clicks from a downscaled "virtual display" mirroring the main display, the locations must be related to the virtual display. But it seems that some UI widgets receive the clicks with coordinates relative to the original display. I'd say this is a bug in Android (at least for some devices), and I don't know how it could be solved.

We could make a specific case for the main display (and always inject clicks to the main display, not the "mirroring virtual display"), but that would fix the problem only for the main display. And it's a dirty hack.

@rom1v
Copy link
Collaborator

rom1v commented Dec 4, 2024

We could make a specific case for the main display

Could you please try with this change:

diff --git server/src/main/java/com/genymobile/scrcpy/video/ScreenCapture.java server/src/main/java/com/genymobile/scrcpy/video/ScreenCapture.java
index 47425d090..846755028 100644
--- server/src/main/java/com/genymobile/scrcpy/video/ScreenCapture.java
+++ server/src/main/java/com/genymobile/scrcpy/video/ScreenCapture.java
@@ -129,10 +129,17 @@ public class ScreenCapture extends SurfaceCapture {
         try {
             virtualDisplay = ServiceManager.getDisplayManager()
                     .createVirtualDisplay("scrcpy", inputSize.getWidth(), inputSize.getHeight(), displayId, surface);
-            virtualDisplayId = virtualDisplay.getDisplay().getDisplayId();
 
-            // The positions are relative to the virtual display, not the original display (so use inputSize, not deviceSize!)
-            positionMapper = PositionMapper.create(videoSize, transform, inputSize);
+            if (displayId == 0) {
+                // Main display: send all events to the original display, relative to the device size
+                Size deviceSize = displayInfo.getSize();
+                positionMapper = PositionMapper.create(videoSize, transform, deviceSize);
+                virtualDisplayId = 0;
+            } else {
+                // The positions are relative to the virtual display, not the original display (so use inputSize, not deviceSize!)
+                positionMapper = PositionMapper.create(videoSize, transform, inputSize);
+                virtualDisplayId = virtualDisplay.getDisplay().getDisplayId();
+            }
             Ln.d("Display: using DisplayManager API");
         } catch (Exception displayManagerException) {
             try {

Here is a binary to replace in scrcpy 3.0.2:

  • scrcpy-server SHA-256: 19854dab6826731edb40f5469e32a7d50a9961baedaa1458f21d25630c6f663

rom1v added a commit that referenced this issue Dec 5, 2024
When mirroring a secondary display, touch and scroll events must be sent
to the mirroring virtual display id (with coordinates relative to the
virtual display size), rather than to the original display (with
coordinates relative to the original display size).

This behavior, introduced by d193967,
was also applied for the main display for consistency.

However, this mechanism has been found to cause some UI elements to
become unclickable.

To minimize inconveniences, restore the previous behavior when mirroring
the main display: send all events to the original display id (0) with
coordinates relative to the original display size.

Fixes #5545 <#5545>
Fixes #5605 <#5605>
Refs #4598 <#4598>
Refs #5137 <#5137>
Refs #5370 <#5370>
rom1v added a commit that referenced this issue Dec 5, 2024
Following the changes from the previous commit, the behavior is now
identical when mirroring the main display or using the SurfaceControl
API.

Factorize the code to perform the initialization in a single location.

Refs #5605 <#5605>
rom1v added a commit that referenced this issue Dec 5, 2024
When mirroring a secondary display, touch and scroll events must be sent
to the mirroring virtual display id (with coordinates relative to the
virtual display size), rather than to the original display (with
coordinates relative to the original display size).

This behavior, introduced by d193967,
was also applied for the main display for consistency. However, it has
been found to cause some UI elements to become unclickable.

To minimize inconveniences, restore the previous behavior when mirroring
the main display: send all events to the original display id (0) with
coordinates relative to the original display size.

Fixes #5545 <#5545>
Fixes #5605 <#5605>
Refs #4598 <#4598>
Refs #5137 <#5137>
Refs #5370 <#5370>
rom1v added a commit that referenced this issue Dec 5, 2024
Following the changes from the previous commit, the behavior is now
identical when mirroring the main display or using the SurfaceControl
API.

Factorize the code to perform the initialization in a single location.

Refs #5605 <#5605>
@rom1v
Copy link
Collaborator

rom1v commented Dec 5, 2024

Please test #5614.

@rodrigofd
Copy link
Author

We could make a specific case for the main display

Could you please try with this change:

diff --git server/src/main/java/com/genymobile/scrcpy/video/ScreenCapture.java server/src/main/java/com/genymobile/scrcpy/video/ScreenCapture.java
index 47425d090..846755028 100644
--- server/src/main/java/com/genymobile/scrcpy/video/ScreenCapture.java
+++ server/src/main/java/com/genymobile/scrcpy/video/ScreenCapture.java
@@ -129,10 +129,17 @@ public class ScreenCapture extends SurfaceCapture {
         try {
             virtualDisplay = ServiceManager.getDisplayManager()
                     .createVirtualDisplay("scrcpy", inputSize.getWidth(), inputSize.getHeight(), displayId, surface);
-            virtualDisplayId = virtualDisplay.getDisplay().getDisplayId();
 
-            // The positions are relative to the virtual display, not the original display (so use inputSize, not deviceSize!)
-            positionMapper = PositionMapper.create(videoSize, transform, inputSize);
+            if (displayId == 0) {
+                // Main display: send all events to the original display, relative to the device size
+                Size deviceSize = displayInfo.getSize();
+                positionMapper = PositionMapper.create(videoSize, transform, deviceSize);
+                virtualDisplayId = 0;
+            } else {
+                // The positions are relative to the virtual display, not the original display (so use inputSize, not deviceSize!)
+                positionMapper = PositionMapper.create(videoSize, transform, inputSize);
+                virtualDisplayId = virtualDisplay.getDisplay().getDisplayId();
+            }
             Ln.d("Display: using DisplayManager API");
         } catch (Exception displayManagerException) {
             try {

Here is a binary to replace in scrcpy 3.0.2:

  • scrcpy-server SHA-256: 19854dab6826731edb40f5469e32a7d50a9961baedaa1458f21d25630c6f663

3.0.2 with this scrcpy-server binary did the trick, issue fixed! 🎉 thanks!

rom1v added a commit that referenced this issue Dec 7, 2024
Following the changes from the previous commit, the behavior is now
identical when mirroring the main display or using the SurfaceControl
API.

Factorize the code to perform the initialization in a single location.

Refs #5605 <#5605>
PR #5614 <#5614>
@rom1v rom1v closed this as completed in 97fa77c Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants