-
Notifications
You must be signed in to change notification settings - Fork 246
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 support for exporting replays as MP4 #199
Conversation
I rebased this on the
|
You want me to proceed with the review or you want to fix conflicts first? I don't mind either. |
Some changes will be required to make GDI work, so reviewing now would be pointless. I'll probably make the changes sometime this week and ping you for review again. |
Rebased and ready for review. |
Handled the conflicts, starting to review $now. |
Build failures are due to PyAV not being able to build on Ubuntu 18.04 because its ffmpeg is too old for latest PyAV. Ubuntu 18.04 carries:
PyAV dropped support for ffmpeg < 4.x in 7.0. Since Ubuntu 18.04 is Ubuntu's latest LTS, I suggest we pin PyAV to latest 6.x+ for the time being. Note that Ubuntu 20.04 is unreleased but even if it would, it would be too new for a last a few months. Objections? Side note: Coverage tests didn't fail because we don't pull the full package there. I'll adjust it in another PR. |
I'm fine with pinning PyAV version, although I will need to retest before doing that. |
789a675
to
53ff569
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool work! Nicely integrated. One general comment: should we have a smoke test conversion with pre-built results? Video might not make it but pcap to replay file should do. What do you think?
Otherwise, just a handful of comments.
def __iter__(self): | ||
return self | ||
|
||
def __next__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a large method that makes it hard to approach. Can't we refactor it in logical subparts like _performPduReassembly()
, and so on?
Fixed latest conflicts and the new pinned dependencies were studied a little bit before deciding which branch of versions to pin. Oh I realize now I forgot to update the requirements files. I got to go, I'll do it later. |
I looked into why the CI is not working for this PR: Basically we need to install avcodec as a requirement. For Linux this is going to be trivial, but the Windows test run will make this complicated. I'll investigate what can be done. To make things even more fun, the docker build fails because of invalid certificates:
|
…ot be correctly saved in the replay file. - Add two optional arguments to the RDPMITM constructor to allow setting a custom Recorder to it instead of the default MITMRecorder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go
In the rebase process / force-push some of the code has been removed. Let's be more careful to avoid that in the future. To be crystal clear: I'm also to blame since I approved the merge. Regression tracked by #250. |
Another note to say that the PyAV version pin to deploy to Ubuntu 18.04 (#199 (comment)) has been removed too and it's causing issues on our Docker builds. Still related to #250. I'll see if I can update to 20.04. |
This is the foundational work to support mp4 export.
There will be several changes that must be done once #187 lands, since I've refactored the player event handler hierarchy to make implementation easier.
Right now the code will live in the player, but my hope is to have this code be part of the PCAP extractor script or as another standalone tool. #188.
This Pull Request refactors pyrdp-replay-extractor to
pyrdp-convert
and adds support for:It also preserves the OSI Layer 7 (plaintext exported PDU) export capabilities from the previous tool.
Closes #170