-
Notifications
You must be signed in to change notification settings - Fork 23
Add recording #193
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
base: main
Are you sure you want to change the base?
Add recording #193
Conversation
468393a to
d953d15
Compare
pyproject.toml
Outdated
| dependencies = [ | ||
| "aiortc>=1.13.0", | ||
| "fastapi>=0.116.1", | ||
| "ffmpeg-python>=0.2.0", |
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.
Can you confirm that this does not require the user to install a system copy of ffmpeg separately?
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.
You do need the FFmpeg binary installed separately, since ffmpeg-python is just a wrapper. I’ve previously tried replacing it with other libraries, but it wasn’t very straightforward. That said, we only use FFmpeg for MP4 concatenation, so replacing it should be doable.
I’ll try again to remove ffmpeg-python and evaluate alternatives. Some options that avoid concatenation entirely (mentioning them here, since maybe we'll like them more and it'd solve the ffmpeg-python issue):
- Return multiple MP4 files (listed in the UI or zipped on the backend if the stream was paused).
- Rethink the “pause” feature, since this issue only affects that path.
- Adopt a WYSIWYG approach: the recording matches exactly what the user sees, including pause, so some frames will be repeated.
- Revisit recording delivery: instead of enforcing timeline timing, record frames and let the user choose the FPS when constructing the final video.
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.
I'm inclined to do 3 in this PR if it means an easy way to avoid requiring system ffmpeg installation and then take a separate pass at improving how we handle recording (4 feels promising).
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.
I'm inclined to do 3 in this PR if it means an easy way to avoid requiring system ffmpeg installation and then take a separate pass at improving how we handle recording (4 feels promising).
Ok, let me do some time-boxed exercise (a few hours max) and if getting rid of ffmpeg is not trivial, then I'll just do 3.
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.
I've spent some time exploring MP4 concatenation and ultimately decided to go with Option 3.
Some notes:
- MP4 concatenation turned out to be significantly more complex than I initially expected.
- I do have a working pure-Python concatenation branch, but the Claude-generated implementation is very long and complex (>1k lines) and still doesn't handle certain edge cases (e.g., varying FPS, long mp4 files).
- My suggestion is to revisit this if a real need arises. For now, maintaining AI-generated MP4 concatenation code seems unnecessary.
See the PR Description and the attached demo to see how it works now.
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.
I don't see an attached demo anywhere.
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.
Sorry, added now with the comment. PTAL.
4134dca to
648ee01
Compare
Signed-off-by: Rafal Leszko <rafal@livepeer.org>
Signed-off-by: Rafal Leszko <rafal@livepeer.org>
648ee01 to
0feb3d1
Compare
Env variables
RECORDING_ENABLEDtruefalse, recording is disabled.RECORDING_MAX_LENGTH1h1h,30m,120s, or plain seconds (e.g.,3600)RECORDING_STARTUP_CLEANUP_ENABLEDtrueAssumptions / Comments
aiortc.contrib.media.MediaRecorder.Recording input may be tricky because of the pause/resume (we don't pause input stream!)
30because60FPS won't work with MediaRecorder, because MediaRecorder calculates PTS/DTS on its own and if we start delivering frames at60FPS, it fails because PTS/DTS are duplicatedDemo with comments
demo_recording.mp4
fix #56