Repository navigation
Conversation
|
Thank you 👍 I will review later. Just a quick question:
Doesn't it cause a regression for an issue fixed by 5bd7514 on some Android versions? |
I see #4074 is caused by scrcpy/server/src/main/java/com/genymobile/scrcpy/Workarounds.java Lines 119 to 134 in dc2fcc4 So If I change static InputManager create() {
try {
Object im = android.hardware.input.InputManager.class.getDeclaredMethod("getInstance").invoke(null);
return new InputManager(im);
} catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
throw new RuntimeException(e);
}
}It still works (and calls |
I just tested to simplify But then, when I paste: |
|
For |
|
Indeed, thank you, it works: a899752 |
| try { | ||
| FileDescriptor old = fds.put(id, fd); | ||
| // Must be unique across the system | ||
| String inputPort = "scrcpy:" + Os.getpid() + ":" + id; |
There was a problem hiding this comment.
There is only one virtual display per scrcpy session, do we need one input port per UHID device?
I think we could just use String inputPort = "scrcpy:" + Os.getpid(); and call addUniqueIdAssociationByPort() only once (and all UHID devices would use the same input port associated to the single display id). That way, storing an inputPort per device would be unnecessary.
I'm not asking you to make any change, I would just like to know what you think about it.
(Currently, I cannot test virtual displays because of #5523 on my current device.)
There was a problem hiding this comment.
I never thought about that. Initially I saw Android Studio is using different phys values for each device: https://cs.android.com/android-studio/platform/tools/adt/idea/+/mirror-goog-studio-main:streaming/screen-sharing-agent/app/src/main/cpp/virtual_input_device.cc;l=66-68;drc=fc06f05ca98ff7d3c8558976fd7eaf1b5480e2e8
But I believe multiple devices can have the same phys value, it means they are in one physical device, but with multiple input features. And addUniqueIdAssociationByPort still works.
The original commit has been backed up at https://github.com/yume-chan/scrcpy/tree/feat/associate-input-port-2
0ef8f64 to
6d17ee6
Compare
I tested on two devices:
On both of them, running either your initial branch or your new one, the UHID mouse pointer does not appear on the virtual display (but it correctly disappears from the main display). I run with: scrcpy --new-display -M
scrcpy --new-display -M --start-app=org.videolan.vlc # to get an app running |
|
I see. So Android 14 still only has one mouse pointer across all displays. We need not only associate the UHID device to the display, but also move the mouse cursor to the display, which is impossible because the API is not available outside system server (same as my original comment #4829 (comment)). I guess adding association alone is enough for virtual touchscreen devices Only Android 15 added seperated mouse cursors for each display (https://cs.android.com/android/_/android/platform/frameworks/native/+/da10dd34cb41f0f117fe87f568f16f6025ab3d72) |
|
Thank you. Here is a branch:
TODO:
|
It would require to rework the parsing, as a single regex is not sufficient (depending on the device, the values are not always in the same order). Since it's just a fallback for some old devices, it is probably useless, so I will not do it until necessary. So I think the branch can be considered ready. Could you please review/test? I managed to test it successfully on a Pixel 6a. |
|
The timing between adding port association and creating UHID devices doesn't matter, so I think it could be simplified a little (just adding the association when initializing UHID manager and remove it on exit). The association will NOT be removed by Android OS when Scrcpy process exits, although it won't cause any harm, if we want to make sure it's 100% clean, maybe we need to add it to the CleanUp process. Otherwise looks good to me and tests OK on my Redmi K70 Pro running Android 15. |
|
Thank you for your review.
When scrcpy runs without UHID device, I want to avoid adding a useless association. So it must be done only when at least one UHID device is created (and only once the virtual display id is known, whichever happens first). Also, I think that my branch only adds the association for "new virtual displays" (not when What makes it more complicated is that currently, |
Fixed, now it should be called both for virtual displays and non-main display (I disabled the association for the main display, since it works by default).
I added synchronization and now call
OK. Since it is harmless, for simplicity I just remove the association when it closes normally, but not if the process is killed. New version: |
|
I rebased: @yume-chan Could you please test and confirm that it works correctly before I merge? I was able to test with an older version of the branch, but the device I used has been updated and is now impacted by #5523). |
|
I tested |
|
@yume-chan Thank you for your tests 👍 I just noticed that 3b36f29 breaks copy-paste from the Android device to the computer: the callback passed to |
|
@rom1v Just a side remark in scrcpy/server/src/main/java/com/genymobile/scrcpy/control/Controller.java Lines 698 to 702 in e0f37f8 if (timeout <= 0) because if timeout is zero thenObject.wait(long) waits for a notification (or a spurious wakeup).
Other classes in the |
|
@anotheruserofgithub Indeed, we should not call
Since this is on Even if it does not change anything in practice, I would not test diff --git server/src/main/java/com/genymobile/scrcpy/control/Controller.java server/src/main/java/com/genymobile/scrcpy/control/Controller.java
index 5e64a4c5b..24d827fda 100644
--- server/src/main/java/com/genymobile/scrcpy/control/Controller.java
+++ server/src/main/java/com/genymobile/scrcpy/control/Controller.java
@@ -699,7 +699,9 @@ public class Controller implements AsyncProcessor, VirtualDisplayListener {
if (timeout < 0) {
return null;
}
- displayDataAvailable.wait(timeout);
+ if (timeout > 0) {
+ displayDataAvailable.wait(timeout);
+ }
data = displayData.get();
}
|
I don't wish to have authorship, this is such a minor thing.
You're right! So it's definitely your authorship. ;) |
Calling wait(0) results in waiting without a timeout, which is unintended. Refs #6009 comment <#6009 (comment)>
|
@anotheruserofgithub OK, thanks. Merged: 52f5d08 |
yume-chan
changed the title
yume-chan
changed the title
I see. I can either
Still simpler than old (While the main looper is running, it can also be used elsewhere so we don't need to create so many |
Yes, I think it would be a good solution (even for other Android features that might use the main looper). Since this is a separate issue, I will probably merge this PR without the clipboard refactor, and this can be done afterwards. |
|
Merged on I will open a new PR with a looper. |
This will allow the main thread to process any event posted to the main looper. Refs #6009 comment <#6009 (comment)>
Instead of blocking the main thread until completion, run a looper. This will allow the main thread to process any event posted to the main looper. Refs #6009 comment <#6009 (comment)>
Instead of blocking the main thread until completion, run a looper. This will allow the main thread to process any event posted to the main looper. Refs #6009 comment <#6009 (comment)> PR #6129 <#6129>
Fixes #4829
Fixes #5547
Fixes #5557
Only works on Android 15 and newer because it requires
ASSOCIATE_INPUT_DEVICE_TO_DISPLAYpermission.When running multiple instances of
scrcpy --display-idorscrcpy --new-display, each display will have its own mouse pointer.Has no effect on keyboard, the whole system still only has one input focus.
I used
FakeContext.get().getSystemService(Context.INPUT_SERVICE)to getInputManager, because initially I was testingaddPortAssociation, which doesn't exist onInputManagerGlobal. This simplifies the code, especially forClipboardManager, where we can use the stable public API (https://github.com/yume-chan/leap-scrcpy/blob/f9aaf1b05118261d75b82ba88b462f08e37eecdc/server/app/src/main/java/leap/scrcpy/server/FakeContext.kt#L62-L72)