mouse right click ACTION_UP is lost

[paweljasinski]

• I have read the FAQ.
• I have searched in existing issues.

Existing issues:
#2711, similar to my experience with right-click
#3070, related, if this is implemented right, the hoover events can be delivered

Environment

• OS: arch x86_64
• scrcpy version: 1.24, rebuild server with extra event logging
• installation method: pacman/yay
• device model, os:
  google pixel 6 pro, grapheneOS (android13)
  asus nexus7, android 6
  samsung SM-T510, android 11

Primary work on pixel, other devices used to make sure problem is not os/device specific

Describe the bug

scrcpy --max-fps=30  --forward-all-clicks --verbosity=verbose --display=2

When using scrcpy with aFreeRDP, the right and middle mouse button don't work as expected.
I don't use the hid (AOA). In case of pixel, mouse pointer appears on the primary display. In case of samsung tab, it shows up on the extra display. I have not found a way to control it.

Investigation
Initially I assumed that the problem is in the aFreeRDP and started by dumping all motion events delivered to dispatchTouchEvent and dispatchGenericMotionEvent.
The first impression: ACTION_UP is missing for right and middle click. The event is not delivered. But perhaps it is the app itself.
So, I took a physical mouse and captured events. Than captured events when delivered from scrcpy.

Right click events from physical mouse as seen by the application, the attributes which are always the same are in const section.

dispatchTouchEvent:         ACTION_DOWN,           actionButton=0, id[0]=0, x[0]=0.0, y[0]=2257.9397,                buttonState=BUTTON_SECONDARY, eventTime=11857929, ventId=507666466 }
dispatchGenericMotionEvent: ACTION_BUTTON_PRESS,   actionButton=BUTTON_SECONDARY, id[0]=0, x[0]=0.0, y[0]=2257.9397, buttonState=BUTTON_SECONDARY, eventTime=11857929, ventId=608438892 }
dispatchGenericMotionEvent: ACTION_BUTTON_RELEASE, actionButton=BUTTON_SECONDARY, id[0]=0, x[0]=0.0, y[0]=2257.9397, buttonState=0,                eventTime=11858273, ventId=612919901 }
dispatchTouchEvent:         ACTION_UP,             actionButton=0, id[0]=0, x[0]=0.0, y[0]=2257.9397,                buttonState=0,                eventTime=11858273, ventId=76737027 }
dispatchGenericMotionEvent: ACTION_HOVER_MOVE,     actionButton=0, id[0]=0, x[0]=0.0, y[0]=2257.9397,                buttonState=0,                eventTime=11858273, ventId=655777164 }

const:
toolType[0]=TOOL_TYPE_MOUSE,
classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0,
downTime=11857929, deviceId=6, source=0x2002, displayId=0,

Right click events when delivered from scrcpy, the attributes which are always the same are in const section.
In addition log contains scrcpy events just before injection point (Controller.java, injectTouch)

scrcpy  injectEvent: ACTION_DOWN, id[0]=0, x[0]=710.0, y[0]=1668.0, buttonState=BUTTON_SECONDARY, eventTime=14278106, deviceId=0,  source=0x2002, displayId=0, eventId=-1002372252 }
FreeRDP dispatchTou: ACTION_DOWN, id[0]=0, x[0]=710.0, y[0]=1668.0, buttonState=BUTTON_SECONDARY, eventTime=14278106, deviceId=-1, source=0x2002, displayId=0, eventId=-1002372252 }
scrcpy  injectEvent: ACTION_UP,   id[0]=0, x[0]=710.0, y[0]=1668.0, buttonState=0,                eventTime=14278149, deviceId=0,  source=0x1002, displayId=0, eventId=-687213207 }

const:
toolType[0]=TOOL_TYPE_FINGER,
actionButton=0,
classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0,
downTime=14278106,

The highlights are:

• missing touch ACTION_UP in the aFreeRDP, but can see the injected touch event in server
• missing motion events (generic motion events) which is probably not the real problem, but would be nice for consistency. Not sure about the performance, the hoover is send on each move.
• inconsistent deviceId, probably not a problem
• toolType is FINGER instead of MOUSE, again not a deal breaker
• wrong source (FINGER instead of MOUSE) in ACTION_UP event injected by the scrcpy

When I hard-code source to 0x2002 in injectTouch, the ACTION_UP is delivered to the application.
But this is just to prove why android is swallowing the event.

[rom1v]

Thank you for your detailed bug report.

wrong source (FINGER instead of MOUSE) in ACTION_UP event injected by the scrcpy

When I hard-code source to 0x2002 in injectTouch, the ACTION_UP is delivered to the application.

Then the problem is here:

scrcpy/server/src/main/java/com/genymobile/scrcpy/Controller.java

Lines 212 to 214 in 48bb6f2

	// Right-click and middle-click only work if the source is a mouse
	boolean nonPrimaryButtonPressed = (buttons & ~MotionEvent.BUTTON_PRIMARY) != 0;
	int source = nonPrimaryButtonPressed ? InputDevice.SOURCE_MOUSE : InputDevice.SOURCE_TOUCHSCREEN;

(introduced by adc547f).

The input source must be a touchscreen unless a non-primary button is pressed (see f70359f, unfortunately, poor commit messages with a typo and no details).

But your report adds an additional constraint: an action_up must be sent with the same source as the matching action_down (which makes sense).

However, the fix is not trivial. For example consider the following sequence of events:

• action_down, buttons=left (left click) → touch screen
• action_down, buttons=left+right (right click) → mouse
• action_up, buttons=right (release left click) → mouse (issue: the source for releasing left click is not the same)
• action_up, buttons=0 (release right click) → touch screen (issue: the source for releasing right click is not the same)

I will think about it. If you have any suggestion, don't hesitate :)

[paweljasinski]

The input source must be a touchscreen unless a non-primary button is pressed (see f70359f, unfortunately, poor commit messages with a typo and no details).

This looks like workaround for devices which have problem with a mouse in general.

However, the fix is not trivial.

I have checked events for a real finger touch with 2 fingers and compared to 2 button mouse (coincidentally exactly what your example is). I believe simulating touches with a mouse is compatible only for a single button/finger. It is broken when more keys/fingers are involved. For multi finger, events contain multiple touch points. For the mouse, events always contain single touch with multiple keys.

How about a verbatim mouse mode, where events injected by scrcpy are as close as possible to what can be observed with physical mouse?
This of course should be an option and not default, so for existing users/devices/application nothing changes.
For my use case, the zoom makes no sense. "pitch-to-zoom" emulation should be an option or simply disabled in verbatim mode.

How about I hack something together and do a PR for review?

[rom1v]

This looks like workaround for devices which have problem with a mouse in general.

Maybe, but I would like not to break them again. It might also impact only some apps (I don't remember).

I believe simulating touches with a mouse is compatible only for a single button/finger. It is broken when more keys/fingers are involved.

(I would like to keep a common behavior between multi-touch and single click.)

How about a verbatim mouse mode, where events injected by scrcpy are as close as possible to what can be observed with physical mouse?

Here are the possible alternatives I considered since yesterday.

1. always use SOURCE_MOUSE: I don't like this solution, because this will break on some devices/apps, and semantically, we want to simulate touchscreen via the scrcpy window (even if we locally use a mouse)
2. split events into two sources: primary button → SOURCE_TOUCHSCREEN, other buttons → SOURCE_MOUSE, with correct handling of ACTION_DOWN/ACTION_UP; this adds complexity but would work for the example in my previous comment. However, on a click-and-move event with both left click and right click pressed, this would generate 2 events, one for each source, so it is inherently broken ❌
3. add an option to force ACTION_MOUSE (the verbatim mode you are suggesting): but when disabled, how to handle middle and right click correctly (when --forward-all-clicks is set)?
4. force ACTION_MOUSE when --forward-all-clicks is enabled: it would work correctly for your use case and the example above, the minor drawback is that enabling --forward-all-clicks would change the behavior of left click alone (using a SOURCE_MOUSE rather than a SOURCE_TOUCHSCREEN).

I think my current preference is 4. What do you think?

[paweljasinski]

Option 4 makes sense. I was trying to be conservative and avoid changes in the way things work today.

However, I would like to go one step further and also add the ACTION_BUTTON_PRESS, ACTION_BUTTON_RELEASE and ACTION_HOOVER_MOVE.
I don't thing the first 2 are a big problem. But for the hoover, I have seen code which disables the motion event injection without button pressed:

scrcpy/app/src/mouse_inject.c

Lines 61 to 64 in 72ba913

	if (!event->buttons_state) {
	// Do not send motion events when no click is pressed
	return;
	}

Any particular reason?

[rom1v]

However, I would like to go one step further and also add the ACTION_BUTTON_PRESS, ACTION_BUTTON_RELEASE

I don't know when it is necessary (when ACTION_UP/ACTION_DOWN is different), and we probably don't want to generate both. If there is a good reason, why not.

But for the hoover, I have seen code which disables the motion event injection without button pressed:
Any particular reason?

It represents maybe 99% of event generation (it floods the device), and hover alone typically does nothing. Also, I don't necessarily want to inject events on the device just because my mouse is over the scrcpy window on the computer.

[paweljasinski]

This is the capture with a physical mouse for: left-down, right-down, left-up, right-up
There is no motion, the coordinates are constant.

dispatchGenericMotionEvent: MotionEvent { action=ACTION_HOVER_MOVE,     actionButton=0,                id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0,                               classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39824687, downTime=0,        deviceId=5, source=0x2002, displayId=0, eventId=832795045 }
dispatchTouchEvent:         MotionEvent { action=ACTION_DOWN,           actionButton=0,                id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=BUTTON_PRIMARY,                  classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39826351, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=302901624 }
dispatchGenericMotionEvent: MotionEvent { action=ACTION_BUTTON_PRESS,   actionButton=BUTTON_PRIMARY,   id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=BUTTON_PRIMARY,                  classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39826351, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=926472458 }
dispatchTouchEvent:         MotionEvent { action=ACTION_MOVE,           actionButton=0,                id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=BUTTON_PRIMARY|BUTTON_SECONDARY, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39826647, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=212267199 }
dispatchGenericMotionEvent: MotionEvent { action=ACTION_BUTTON_PRESS,   actionButton=BUTTON_SECONDARY, id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=BUTTON_PRIMARY|BUTTON_SECONDARY, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39826647, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=685127339 }
dispatchGenericMotionEvent: MotionEvent { action=ACTION_BUTTON_RELEASE, actionButton=BUTTON_PRIMARY,   id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=BUTTON_SECONDARY,                classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39826887, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=274112588 }
dispatchTouchEvent:         MotionEvent { action=ACTION_MOVE,           actionButton=0,                id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=BUTTON_SECONDARY,                classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39826887, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=466066843 }
dispatchGenericMotionEvent: MotionEvent { action=ACTION_BUTTON_RELEASE, actionButton=BUTTON_SECONDARY, id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0,                               classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39827175, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=579098142 }
dispatchTouchEvent:         MotionEvent { action=ACTION_UP,             actionButton=0,                id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0,                               classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39827175, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=299287089 }
dispatchGenericMotionEvent: MotionEvent { action=ACTION_HOVER_MOVE,     actionButton=0,                id[0]=0, x[0]=308.3386, y[0]=2135.5417, toolType[0]=TOOL_TYPE_MOUSE, buttonState=0,                               classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=39827175, downTime=39826351, deviceId=5, source=0x2002, displayId=0, eventId=330091271 }

The first and last hoover is to show there was no motion before and after. You are absolutely right, there is no reason to implement them.
The touchEvents are as follows:
ACTION_DOWN (left down),
ACTION_MOVE (right down)
ACTION_MOVE (left up)
ACTION_UP (right up)

and the genericMotionEvents:
ACTION_BUTTON_PRESS (left down)
ACTION_BUTTON_PRESS (right down)
ACTION_BUTTON_RELEASE (left up)
ACTION_BUTTON_RELEASE (right up)

As far as I can tell, aFreeRDP is not using the genericMotionEvents at the moment neither handle properly touchEvents sequence as above.
At the same time, if I had to fix it, the genericMotionEvents are way easier to interpret.

[rom1v]

Thank you for the details.

So IIUC, there is currently no practical reason to implement ACTION_BUTTON_PRESS/ACTION_BUTTON_RELEASE, is it correct?

How about I hack something together and do a PR for review?

Option 4 makes sense.

Would you like to implement option 4 and submit a PR?

Thank you

[paweljasinski]

So IIUC, there is currently no practical reason to implement ACTION_BUTTON_PRESS/ACTION_BUTTON_RELEASE, is it correct?

yes, but ... I will create a separate issue for it when it comes

Would you like to implement option 4 and submit a PR?

yes

[paweljasinski]

How important is simulated zoom (ctrl-click) when in forward-all-clicks mode?

[rom1v]

Good question, ideally it should still work, but I understand this is a problem with solution (4) 😞

[paweljasinski]

I implemented it trying to mimic hardware mouse, so the events have source set to SOURCE_MOUSE and tool type set to TOOL_TYPE_MOUSE. In this configuration I have not found a way to simulate pinch. Tried 2 virtual fingers. But I have to cancel mouse events.
I will go back to original hack and check it again. The tool type was TOOL_TYPE_FINGER and source was SOURCE_MOUSE.

[paweljasinski]

So, if I mix TOOL_TYPE_FINGER and source SOURCE_MOUSE which was the original hack, the results are application dependent. This combination is not a valid combination observed on the device, so it is a best effort.

I have another idea, what if we modify pinch-to-zoom emulator logic.
In case of mouse, ctrl-click would not trigger simulation. Only after ctrl-click-move the 2 fingers are placed down and mouse event gets canceled.
What do you think?

[rom1v]

Hi,

Thank you for your work.

I reviewed your 2 proposals:

1. paweljasinski@4b77fe2
2. paweljasinski@14d7351

(2) is the most straightforward, but --forward-all-clicks is really a client-side option, I prefer that it does not "leak" to the server side.

(1) LGTM. I made minor changes, in particular I moved the simulate_touch flag from the events structs to the sc_mouse_inject struct (the sc_mouse_*_events represent how the events are received from SDL by scrcpy, and simulate_touch is specific to the sc_mouse_inject mouse processor).

I opened a PR: #3579 Please review and test 😉

Only after ctrl-click-move the 2 fingers are placed down and mouse event gets canceled.
What do you think?

How does the "mouse event get canceled"? If this implies to inject an ACTION_UP, the app will believe that a click pressed+released occurred. A priori, I don't like the fact that a mouse event is injected if in the end it should not.

[paweljasinski]

I opened a PR: #3579 Please review and test

testing right now

How does the "mouse event get canceled"? If this implies to inject an ACTION_UP, the app will believe that a click pressed+released occurred. A priori, I don't like the fact that a mouse event is injected if in the end it should not.

I am guilty of doing experiments with physical mouse and not documenting. Let's fix it. The experiment:

1. press the left mouse button
2. pinch screen with 2 fingers (2 fingers down, move, 2 fingers up)
3. release left button mouse button

And here are the events captured by the app, the finger motion events and generic motion events removed.

dispatchTouchEvent: MotionEvent         { action=ACTION_DOWN,            actionButton=0,              id[0]=0, x[0]=486.5354, y[0]=2306.577,  toolType[0]=TOOL_TYPE_MOUSE,  buttonState=BUTTON_PRIMARY, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=1033584, downTime=1033584, deviceId=5, source=0x2002, displayId=0, eventId=837052436 }
dispatchTouchEvent: MotionEvent         { action=ACTION_CANCEL,          actionButton=0,              id[0]=0, x[0]=486.5354, y[0]=2306.577,  toolType[0]=TOOL_TYPE_MOUSE,  buttonState=0, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=1034829, downTime=1033584, deviceId=5, source=0x2002, displayId=0, eventId=1801081690 }
dispatchTouchEvent: MotionEvent         { action=ACTION_DOWN,            actionButton=0,              id[0]=0, x[0]=383.0, y[0]=2564.0,       toolType[0]=TOOL_TYPE_FINGER, buttonState=0, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=1034815, downTime=1034815, deviceId=4, source=0x1002, displayId=0, eventId=801332124 }
dispatchTouchEvent: MotionEvent         { action=ACTION_POINTER_DOWN(1), actionButton=0,              id[0]=0, x[0]=383.0, y[0]=2564.0,       toolType[0]=TOOL_TYPE_FINGER, id[1]=1, x[1]=747.0, y[1]=2199.0, toolType[1]=TOOL_TYPE_FINGER, buttonState=0, classification=AMBIGUOUS_GESTURE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=2, historySize=0, eventTime=1034832, downTime=1034815, deviceId=4, source=0x1002, displayId=0, eventId=199526300 }
dispatchTouchEvent: MotionEvent         { action=ACTION_POINTER_UP(1),   actionButton=0,              id[0]=0, x[0]=411.0, y[0]=2633.0,       toolType[0]=TOOL_TYPE_FINGER, id[1]=1, x[1]=920.0, y[1]=1938.0, toolType[1]=TOOL_TYPE_FINGER, buttonState=0, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=2, historySize=0, eventTime=1035306, downTime=1034815, deviceId=4, source=0x1002, displayId=0, eventId=16401284 }
dispatchTouchEvent: MotionEvent         { action=ACTION_UP,              actionButton=0,              id[0]=0, x[0]=408.0, y[0]=2629.0,       toolType[0]=TOOL_TYPE_FINGER, buttonState=0, classification=NONE, metaState=0, flags=0x0, edgeFlags=0x0, pointerCount=1, historySize=0, eventTime=1035335, downTime=1034815, deviceId=4, source=0x1002, displayId=0, eventId=995215654 }

The interesting part, when finger touches, the old mouse click event get ACTION_CANCEL even when the mouse has its button down.
Assuming most of the mouse click events are interpreted on ACTION_UP, the mouse click never triggers application.

I don't like the fact that a mouse event is injected if in the end it should not.

Based on the above logic, it will not have mouse click effect.
But, this can be done better.
Scrcpy could hold back ACTION_DOWN from mouse until there is either key up or motion.
On key up, scrcpy sends ACTION_DOWN followed by ACTION_UP.
On motion, there is only finger pinch emulation and final mouse key up is discarded. Not sure what to do about the Ctrl up.
If the control key is released before emulation start, send ACTION_DOWN and continue as usually.
If the control key is released in pinch emulation, continue pinch.

I will try it when I have a moment.

[rom1v]

I implemented it trying to mimic hardware mouse, so the events have source set to SOURCE_MOUSE and tool type set to TOOL_TYPE_MOUSE. In this configuration I have not found a way to simulate pinch. Tried 2 virtual fingers. But I have to cancel mouse events.

In fact, the virtual finger may also be a mouse. Here is a diff to test against current dev branch, which forces all events (mouse or touch or virtual finger) to be a mouse, and pinch-to-zoom still works:

diff --git a/server/src/main/java/com/genymobile/scrcpy/Controller.java b/server/src/main/java/com/genymobile/scrcpy/Controller.java
index 95b64711..b9f193c1 100644
--- a/server/src/main/java/com/genymobile/scrcpy/Controller.java
+++ b/server/src/main/java/com/genymobile/scrcpy/Controller.java
@@ -45,7 +45,7 @@ public class Controller {
     private void initPointers() {
         for (int i = 0; i < PointersState.MAX_POINTERS; ++i) {
             MotionEvent.PointerProperties props = new MotionEvent.PointerProperties();
-            props.toolType = MotionEvent.TOOL_TYPE_FINGER;
+            props.toolType = MotionEvent.TOOL_TYPE_MOUSE;
 
             MotionEvent.PointerCoords coords = new MotionEvent.PointerCoords();
             coords.orientation = 0;
@@ -210,12 +210,7 @@ public class Controller {
         }
 
         // Right-click and middle-click only work if the source is a mouse
-        boolean nonPrimaryButtonPressed = (buttons & ~MotionEvent.BUTTON_PRIMARY) != 0;
-        int source = nonPrimaryButtonPressed ? InputDevice.SOURCE_MOUSE : InputDevice.SOURCE_TOUCHSCREEN;
-        if (source != InputDevice.SOURCE_MOUSE) {
-            // Buttons must not be set for touch events
-            buttons = 0;
-        }
+        int source = InputDevice.SOURCE_MOUSE;
 
         MotionEvent event = MotionEvent
                 .obtain(lastTouchDown, now, action, pointerCount, pointerProperties, pointerCoords, 0, buttons, 1f, 1f, DEFAULT_DEVICE_ID, 0, source,

[paweljasinski]

I recall inconsistent behavior. Not all applications worked.

[rom1v]

I recall inconsistent behavior. Not all applications worked.

With which commit? With 2 "virtual" mice?

[paweljasinski]

With which commit? With 2 "virtual" mice?

yes, but I didn't use your code, it was one of the experiments I did on my own.

[rom1v]

OK. When you have some time, could you please review and test this new version? (#3579)