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

Multiple Player improvements #190

Merged
merged 5 commits into from
Mar 24, 2020
Merged

Multiple Player improvements #190

merged 5 commits into from
Mar 24, 2020

Conversation

alxbl
Copy link
Collaborator

@alxbl alxbl commented Mar 13, 2020

  • Optional GUI dependencies (pip install -U -e '.[GUI]')
  • Auto-play single replay files
  • Headless pyrdp-player (--headless)

Closes #151, Closes #163

@alxbl alxbl added the enhancement New feature or request label Mar 13, 2020
@alxbl alxbl marked this pull request as ready for review March 13, 2020 18:32
Copy link
Member

@obilodeau obilodeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! That's going to be really good for our docker image size and embedding opportunities.

Question: I think the existing replay event handler didn't log file operations between the client and the server or mouse clicks (w/ coordinates) am I correct?

Remarks:

  • You can bump copyright year on all files you touched that had ~5+ lines changed

bin/pyrdp-player.py Outdated Show resolved Hide resolved
pyrdp/player/HeadlessEventHandler.py Outdated Show resolved Hide resolved
@alxbl
Copy link
Collaborator Author

alxbl commented Mar 17, 2020

Question: I think the existing replay event handler didn't log file operations between the client and the server or mouse clicks (w/ coordinates) am I correct?

It doesn't. The mouse is rendered visually and it's pretty difficult/spammy to render the coordinates nicely in text only. We could try to log mouse clicks.

For the file events, we can add support in both places whenever we add it.

I'll make the modifications and merge this.

Cheers,

@alxbl
Copy link
Collaborator Author

alxbl commented Mar 17, 2020

Oh lord. I just realized that the entire keyboard.py file which processes scancodes depends on Qt. I had reinstalled PySide2 so it didn't error, but my last tests are failing. I'm going to have to do a bit more work on this.

@obilodeau
Copy link
Member

Oh lord. I just realized that the entire keyboard.py file which processes scancodes depends on Qt. I had reinstalled PySide2 so it didn't error, but my last tests are failing. I'm going to have to do a bit more work on this.

Great catch 👍

I was under the impression that we borrowed scan codes from FreeRDP but it could not be the case. Maybe if not using QT we could re-import them from FreeRDP. I let you investigate all the options.

@obilodeau
Copy link
Member

It doesn't. The mouse is rendered visually and it's pretty difficult/spammy to render the coordinates nicely in text only. We could try to log mouse clicks.

Yes, I was thinking just on clicks you log the X, Y at that moment. Might seem not to add a lot of value but the way I see it is if I replay capture and have no event, I would be wondering if the user is doing everything through mouse whereas if I see the clicks then I would know for sure. This is out of scope for this PR btw.

For the file events, we can add support in both places whenever we add it.

👍

@alxbl
Copy link
Collaborator Author

alxbl commented Mar 17, 2020

I was under the impression that we borrowed scan codes from FreeRDP but it could not be the case. Maybe if not using QT we could re-import them from FreeRDP. I let you investigate all the options.

We have the scan code mappings to Qt.Key enums, I just need to make them not rely on the Qt import, so it should just be a matter of a few editor macros and minor code changes to use the new mappings.

@alxbl
Copy link
Collaborator Author

alxbl commented Mar 18, 2020

Tested without PySide2 installed end-to-end to make sure I didn't miss anything. I fixed a few things in the process and added mouse click reporting in the replays (both headless and GUI).

I'll give you some time to look my latest commit over before merging.

I can see a few opportunities for refactoring (like having a better event handler hierarchy: Headless -> Player -> Live) and some code de-duplication, but I feel like this is clean enough right now.

@alxbl alxbl added this to the vNext milestone Mar 24, 2020
@obilodeau obilodeau merged commit 0c7f6ac into master Mar 24, 2020
@obilodeau obilodeau mentioned this pull request Mar 25, 2020
7 tasks
@robeving
Copy link
Contributor

I'm excited to test this. I'm currently processing PCAPs of thousands of connections through some hacked up changed to pyrdp that use a framebuffered X server.

@alxbl
Copy link
Collaborator Author

alxbl commented Mar 25, 2020

@robeving That sounds like a very creative workaround! Let me know how headless mode goes for you. Feel free to open issues if you run into any problems when trying it.

@robeving
Copy link
Contributor

It works and about 1000x faster!

@obilodeau obilodeau deleted the headless-mode branch August 6, 2021 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pyrdp-player.py without a GUI but with logs Make the GUI an optional feature?
3 participants