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

Add --hid-capture & --hid-replay options #4556

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Rob--W
Copy link

@Rob--W Rob--W commented Dec 31, 2023

This PR introduces the --hid-record and --hid-replay flags to support the use case of capturing events and replaying it (#4468). I have designed it such that it can also replay from a stream, currently through a fifo pipe to support the use case of targeting multiple devices at once (#4179). FIFO pipes are UNIX only, but the implementation is easily extensible to support streaming over a local socket (e.g. TCP, which would also work on Windows).

Documentation and unit tests are included.

Tested as follows:

  • The file format is plain text, line-based [timestamp] [accessory id] [hex-encoded space-separated event data]. This simple format is chosen to make it easier to edit the event data manually if desired. Blank lines and lines with # are ignored, everything else is rejected.
  • The parsing and serialization of the file format is fully covered by unit tests.
  • I have manually tested the functionality with real devices connected to a Linux laptop via USB.
  • Test devices were: Google Pixel 8, Samsung Galaxy A52s, Samsung Galaxy A53.
  • Test scenarios: record to one file, replay from one file, record (overwrite) existing file, replay from non-existing file, replay and record in one command, replay from a FIFO pipe, where input is recorded to. In all cases I also tested the effect of interrupting the replay early, with the expected behavior (program closes without issues).
  • I made sure that there are no memory leaks or memory safety issue by compiling with ASAN:
    meson setup x --buildtype=debug --strip -Db_lto=true -Dprebuilt_server=/usr/share/scrcpy/scrcpy-server -Db_sanitize=address,undefined && ninja -Cx

A note on the design:

  • The implementation utilizes up to three extra threads to minimize the impact on the main and event thread (aoa).
  • To record events, one thread is used (to perform I/O - write to file).
  • To replay data, two threads are used: one to perform I/O (read from file/stream) and another to replay (waiting as needed to ensure that enough time has elapsed to replay the event). I chose two separate threads to make sure that the I/O and replay-sleep do not block each other from progressing as quick as possible. For ordinary local files, this does not really matter, but for data streamed from a fifo pipe (or in the future: sockets), blocking the pipe or socket could result in undesired interruptions or blocked pipes or closed sockets.

This also moves the `sc_hid_event` type to a separate file, to enable
the other classes to use it without requiring the several other object
files to be linked.
@Rob--W
Copy link
Author

Rob--W commented Dec 31, 2023

@rom1v Note: I have tested it extensively on Linux, but I haven't tried cross-compiling to Windows yet. I'll check whether it can compile successfully on Windows after your code review.

@Rob--W
Copy link
Author

Rob--W commented Mar 13, 2024

@rom1v The branch currently has merge conflicts due to changes in the past two months. Before I rebase, I'd like to ask whether you have unsubmitted feedback or any other blockers to merging the PR.

@rom1v
Copy link
Collaborator

rom1v commented Mar 14, 2024

First of all, thank you for your PR. It's a lot of work, with a lot attention to details (as per your PR description) ❤️

Sorry for the delay (the UHID feature took priority 😉).

Streaming

Initially, I did not consider the "live" recording/replay of events, so loading the whole file (which should be small) then replaying it would be ok. But I like the idea to support this use case 👍

Replay

During the replay, keyboard and mouse should be disabled (a mouse move during a replay breaks everything).
I think that replay does not even require mirroring, and there is even no need for a window (like OTG).
This could be a separate code path which just configures AOA/HID and replays synchronously (with some feedback in the console).

For me, the most important use case for this feature is to enable USB debugging on a device with a broken screen (but other people might have other use cases).

Threading

To minimize timing errors, the wait should be as close as possible as the input injection. So IMO, the thread waiting during ms_to_sleep (btw we should use µS instead of ms) should not post an event to another thread (sc_aoa_push_hid_event()) but should call sc_aoa_send_hid_event() directly immediately. In other words, the thread waiting for the deadline should be the thread which writes AOA over USB directly.

The thread which parses the file might be a separate thread posting the parsed events in a queue along with their timings, as you said:

I chose two separate threads to make sure that the I/O and replay-sleep do not block each other from progressing as quick as possible.

TBH, I think it might as well be the same thread, since the "injection" needs to wait for I/O to get the data anyway, so unless parsing takes a long time (in which case pipelining could improve the progress), I'm not sure it could make a difference. But why not.

Format

I think a binary format would be simpler. Parsing text in C is a PITA. I already parse some text because I have no choice in adb_parser.c, but if we can avoid that (more complexity, more code to maintain, more bugs…), that would be great. In another language (like Rust or Python), parsing text would be ok, but in C I really really prefer binary.

What do you think?

And I'm not sure there is an important need to edit HID event data manually in text.

What I have in mind is something like:

mouse:    |xx xx xx xx xx xx xx|02|xx xx xx xx|
keyboard: |xx xx xx xx xx xx xx|01|xx xx xx xx xx xx xx xx|
           \------------------/ ^^ \---------------------/
                 timestamp    device    HID event

(the size of the HID event might be deduced from the device)

I think we should add a protocol version at the beginning of the file, to be able to make changes later but still be able to replay old recorded files.

Also, a minor detail: the timestamps should start at 0 in the recorded file.

AOA/UHID

When you implemented the PR, HID was only supported via AOA. Now there is UHID.
But I think the feature should still target AOA specifically, because timings are captured and replayed on the client side (UHID would require to capture and replay on the server side, so the code could not be common).


Your contribution is great! I'm sorry that the codebase changed a lot since your PR was written (causing conflicts).

Also, I'm aware that changing from a text format to a binary format is quite a big change, but the binary version should be simpler (I'd really prefer not to maintain more code which parses text in C when this is not absolutely necessary).

Thank you again 😉

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

Successfully merging this pull request may close these issues.

None yet

2 participants