✨(backend) make LiveKit Egress recording encoding configurable#1288
Conversation
damienlaine
force-pushed
the
feat/configurable-recording-encoding
branch
from
dd3e28e to
7080c42
Compare
lebaudantoine
left a comment
lebaudantoine
left a comment
There was a problem hiding this comment.
Thank you for your contribution, I haven’t tested it locally yet, but it looks promising.
Feel free to add more tests if any additional edge cases come to mind. We’re getting very close to something that’s ready to be merged.
On a related note, I’d like to allow users to pass recording encoding configuration from the frontend so they can customize their recording preferences. I’ll start working on that once your PR is merged.
| | Balanced | 1280×720 | 20 | 1000 | 96 | 6 | ~240 MB | ~67 % | Mixed content, moderate motion | | ||
| | **Low CPU / small file** | 1280×720 | 15 | 600 | 64 | 10 | **~150 MB** | ~50 % | Videoconference (talking heads + screen share) ★ | | ||
| | Minimum CPU | 960×540 | 15 | 500 | 64 | 10 | ~125 MB | ~30 % | Voice-first meetings, readable text not required | | ||
| | Audio-heavy fallback | 1280×720 | 10 | 400 | 96 | 15 | ~110 MB | ~35 % | Long webinars, low motion | |
There was a problem hiding this comment.
Are 6+ (s) keyframes a bit aggressive for a meeting recording that's usually replayed?
There was a problem hiding this comment.
Good catch — aligned all profiles to 4s (standard VOD value). Side note: I also fixed an inaccurate claim in the docs that said larger keyframe intervals reduce file size by 10–15 %; in ABR mode (which LiveKit Egress uses) the bitrate budget is fixed, so keyframe interval only trades seek granularity for slightly higher quality at the same bitrate.
| width=opts["width"], | ||
| height=opts["height"], | ||
| framerate=opts["framerate"], | ||
| video_codec=livekit_api.VideoCodec.H264_MAIN, | ||
| video_bitrate=opts["video_bitrate"], | ||
| audio_codec=livekit_api.AudioCodec.AAC, | ||
| audio_bitrate=opts["audio_bitrate"], | ||
| audio_frequency=48000, | ||
| key_frame_interval=opts["key_frame_interval"], |
There was a problem hiding this comment.
This mixes hardcoded values, LiveKit defaults explicitly passed, and values that can be overridden by Django settings.
There was a problem hiding this comment.
Refactored — all the EncodingOptions kwargs (operator-tunable values + pinned codec/frequency constants) are now assembled in WorkerServiceConfig.from_settings(), and the services layer is a thin protobuf adapter (EncodingOptions(**opts)). Single source of truth, no more mix at the call site.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR makes LiveKit Egress recording encoding configurable. It adds new settings and environment documentation for enabling encoding overrides and specifying width, height, framerate, video/audio bitrates, and key-frame interval. The worker service config gains an optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/backend/meet/settings.py (1)
704-709: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winComment should clarify these settings only apply to video composite egress (screen recordings), not audio-only transcript recordings.
AudioCompositeEgressService.start()never calls_build_encoding_options(), soRECORDING_ENCODING_*values are silently ignored for audio-only recordings regardless ofRECORDING_ENCODING_ENABLED.✏️ Suggested comment update
- # Recording encoding options for LiveKit Egress. - # When disabled, LiveKit falls back to its built-in H264_720P_30 preset - # (1280x720, 30 fps, 3000 kbps H.264 MAIN video, 128 kbps AAC audio). - # When enabled, the values below are passed to LiveKit as EncodingOptions - # (advanced) and replace the preset. Lowering framerate and bitrate reduces - # output file size and CPU load on the egress worker. + # Recording encoding options for LiveKit Egress (video composite egress only). + # These settings affect screen recordings (VideoCompositeEgressService) and are + # silently ignored for audio-only transcript recordings (AudioCompositeEgressService). + # When disabled, LiveKit falls back to its built-in H264_720P_30 preset + # (1280x720, 30 fps, 3000 kbps H.264 MAIN video, 128 kbps AAC audio). + # When enabled, the values below are passed to LiveKit as EncodingOptions + # (advanced) and replace the preset. Lowering framerate and bitrate reduces + # output file size and CPU load on the egress worker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/meet/settings.py` around lines 704 - 709, Update the doc comment near the RECORDING_ENCODING_* settings to state explicitly that these encoding options apply only to video composite egress (e.g., screen recordings) and do not affect audio-only transcript recordings; mention that AudioCompositeEgressService.start() never calls _build_encoding_options(), so when RECORDING_ENCODING_ENABLED is set the encoding values are ignored for audio-only egress. Reference the RECORDING_ENCODING_* settings and AudioCompositeEgressService.start()/_build_encoding_options() so reviewers can verify the behavior.docs/features/recording.md (1)
177-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScreen-share caveat contradicts the ★ recommendation.
Line 177 marks the 600 kbps preset as the recommended starting point for "talking heads + screen share", while the caveat on line 197 warns that quality degrades at "below ~600 kbps". The recommended value sits exactly at that threshold, which can produce blocky text in shared slides. Consider either:
- Bumping the "Low CPU / small file" video bitrate to 800 kbps and updating the caveat boundary accordingly, or
- Noting in the table that this preset is best for talking-head content and that screen-share-heavy sessions should start at 800–1000 kbps.
Also applies to: 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/recording.md` at line 177, The table's "Low CPU / small file" row currently recommends 600 kbps for "Videoconference (talking heads + screen share)" which conflicts with the caveat at the ~600 kbps threshold; update the table row to use 800 kbps instead of 600 and change the caveat text (the note referenced on line 197) to use "below ~800 kbps" so the recommended preset no longer sits at the degradation threshold; ensure you update the numeric bitrate cell in the "Low CPU / small file" row and the corresponding caveat sentence that mentions the threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/features/recording.md`:
- Line 177: The table's "Low CPU / small file" row currently recommends 600 kbps
for "Videoconference (talking heads + screen share)" which conflicts with the
caveat at the ~600 kbps threshold; update the table row to use 800 kbps instead
of 600 and change the caveat text (the note referenced on line 197) to use
"below ~800 kbps" so the recommended preset no longer sits at the degradation
threshold; ensure you update the numeric bitrate cell in the "Low CPU / small
file" row and the corresponding caveat sentence that mentions the threshold.
In `@src/backend/meet/settings.py`:
- Around line 704-709: Update the doc comment near the RECORDING_ENCODING_*
settings to state explicitly that these encoding options apply only to video
composite egress (e.g., screen recordings) and do not affect audio-only
transcript recordings; mention that AudioCompositeEgressService.start() never
calls _build_encoding_options(), so when RECORDING_ENCODING_ENABLED is set the
encoding values are ignored for audio-only egress. Reference the
RECORDING_ENCODING_* settings and
AudioCompositeEgressService.start()/_build_encoding_options() so reviewers can
verify the behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f232b34-e3df-4434-b9d4-c14ac9141357
📒 Files selected for processing (8)
CHANGELOG.mddocs/features/recording.mdenv.d/development/common.distsrc/backend/core/recording/worker/factories.pysrc/backend/core/recording/worker/services.pysrc/backend/core/tests/recording/worker/test_factories.pysrc/backend/core/tests/recording/worker/test_services.pysrc/backend/meet/settings.py
damienlaine
force-pushed
the
feat/configurable-recording-encoding
branch
from
7080c42 to
87b04ed
Compare
|
Thanks for the review @lebaudantoine! Pushed 87b04ed (force-push, rebased onto latest main) addressing every thread:
Tests |
lebaudantoine
left a comment
lebaudantoine
left a comment
There was a problem hiding this comment.
LGTM thank @damienlaine
| # Codec / frequency constants matching LiveKit's H264_720P_30 preset. | ||
| # Kept fixed because changing them would shift the goal-post away from the | ||
| # "safe drop-in replacement for the default preset" contract of this feature. | ||
| _RECORDING_VIDEO_CODEC = livekit_api.VideoCodec.H264_MAIN | ||
| _RECORDING_AUDIO_CODEC = livekit_api.AudioCodec.AAC | ||
| _RECORDING_AUDIO_FREQUENCY_HZ = 48000 | ||
|
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/tests/recording/worker/test_services.py`:
- Around line 265-284: Add an assertion in
test_video_composite_egress_start_with_encoding_options to verify the protobuf
field advanced.audio_frequency is preserved by EncodingOptions: after retrieving
advanced from the request, assert advanced.audio_frequency equals the pinned
audio frequency used by config_with_encoding (e.g., 48000) so regressions that
drop this field are caught; update only the test referenced
(test_video_composite_egress_start_with_encoding_options) and keep the other
existing assertions intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c66e64f0-a224-4c74-9037-4382e964f3c7
📒 Files selected for processing (8)
CHANGELOG.mddocs/features/recording.mdenv.d/development/common.distsrc/backend/core/recording/worker/factories.pysrc/backend/core/recording/worker/services.pysrc/backend/core/tests/recording/worker/test_factories.pysrc/backend/core/tests/recording/worker/test_services.pysrc/backend/meet/settings.py
| def test_video_composite_egress_start_with_encoding_options(config_with_encoding): | ||
| """Custom encoding options are forwarded as `advanced` EncodingOptions.""" | ||
| service = VideoCompositeEgressService(config_with_encoding) | ||
| service._handle_request = Mock(return_value=Mock(egress_id="eg-2")) | ||
|
|
||
| service.start("test-room", "rec-2") | ||
|
|
||
| request = service._handle_request.call_args[0][0] | ||
| assert request.WhichOneof("options") == "advanced" | ||
|
|
||
| advanced = request.advanced | ||
| assert advanced.width == 1280 | ||
| assert advanced.height == 720 | ||
| assert advanced.framerate == 15 | ||
| assert advanced.video_bitrate == 600 | ||
| assert advanced.audio_bitrate == 64 | ||
| assert advanced.key_frame_interval == pytest.approx(10.0) | ||
| assert advanced.video_codec == livekit_api.VideoCodec.H264_MAIN | ||
| assert advanced.audio_codec == livekit_api.AudioCodec.AAC | ||
|
|
There was a problem hiding this comment.
advanced.audio_frequency is not asserted.
The test verifies 8 of 9 encoding fields but omits audio_frequency, the only pinned constant that flows through EncodingOptions. A regression that accidentally drops it from the encoding_options dict would pass undetected here (the factory test only checks the dict, not the protobuf field value).
🧪 Proposed addition
assert advanced.video_codec == livekit_api.VideoCodec.H264_MAIN
assert advanced.audio_codec == livekit_api.AudioCodec.AAC
+ assert advanced.audio_frequency == 48000📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_video_composite_egress_start_with_encoding_options(config_with_encoding): | |
| """Custom encoding options are forwarded as `advanced` EncodingOptions.""" | |
| service = VideoCompositeEgressService(config_with_encoding) | |
| service._handle_request = Mock(return_value=Mock(egress_id="eg-2")) | |
| service.start("test-room", "rec-2") | |
| request = service._handle_request.call_args[0][0] | |
| assert request.WhichOneof("options") == "advanced" | |
| advanced = request.advanced | |
| assert advanced.width == 1280 | |
| assert advanced.height == 720 | |
| assert advanced.framerate == 15 | |
| assert advanced.video_bitrate == 600 | |
| assert advanced.audio_bitrate == 64 | |
| assert advanced.key_frame_interval == pytest.approx(10.0) | |
| assert advanced.video_codec == livekit_api.VideoCodec.H264_MAIN | |
| assert advanced.audio_codec == livekit_api.AudioCodec.AAC | |
| def test_video_composite_egress_start_with_encoding_options(config_with_encoding): | |
| """Custom encoding options are forwarded as `advanced` EncodingOptions.""" | |
| service = VideoCompositeEgressService(config_with_encoding) | |
| service._handle_request = Mock(return_value=Mock(egress_id="eg-2")) | |
| service.start("test-room", "rec-2") | |
| request = service._handle_request.call_args[0][0] | |
| assert request.WhichOneof("options") == "advanced" | |
| advanced = request.advanced | |
| assert advanced.width == 1280 | |
| assert advanced.height == 720 | |
| assert advanced.framerate == 15 | |
| assert advanced.video_bitrate == 600 | |
| assert advanced.audio_bitrate == 64 | |
| assert advanced.key_frame_interval == pytest.approx(10.0) | |
| assert advanced.video_codec == livekit_api.VideoCodec.H264_MAIN | |
| assert advanced.audio_codec == livekit_api.AudioCodec.AAC | |
| assert advanced.audio_frequency == 48000 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tests/recording/worker/test_services.py` around lines 265 -
284, Add an assertion in test_video_composite_egress_start_with_encoding_options
to verify the protobuf field advanced.audio_frequency is preserved by
EncodingOptions: after retrieving advanced from the request, assert
advanced.audio_frequency equals the pinned audio frequency used by
config_with_encoding (e.g., 48000) so regressions that drop this field are
caught; update only the test referenced
(test_video_composite_egress_start_with_encoding_options) and keep the other
existing assertions intact.
There was a problem hiding this comment.
Good catch — added assert advanced.audio_frequency == 48000 in 61d68b6. Tests pass locally (26/26).
damienlaine
force-pushed
the
feat/configurable-recording-encoding
branch
from
87b04ed to
61d68b6
Compare
Expose RECORDING_ENCODING_* settings to override the default LiveKit Egress preset (H264_720P_30). When RECORDING_ENCODING_ENABLED is True, the provided width/height/framerate/bitrate/keyframe values are passed as advanced EncodingOptions. Lowering framerate and bitrate reduces recording file size and egress worker CPU load. Disabled by default, preserving current behaviour.
lebaudantoine
force-pushed
the
feat/configurable-recording-encoding
branch
from
61d68b6 to
c8b8152
Compare
|
2 participants
Purpose
Recordings produced by LiveKit Egress currently use the hard-coded
H264_720P_30preset (1280×720 / 30 fps / 3000 kbps H.264 MAIN + 128 kbps AAC). That's ~690 MB per 30-minute meeting. For typical La Suite Meet workloads (talking heads + screen share, low motion) this is significantly heavier than necessary, and the fixed 30 fps / veryfast real-time encoding also keeps egress worker CPU higher than it needs to be.Operators have no way to tune this today without patching the backend.
Proposal
Expose seven
RECORDING_ENCODING_*settings that, whenRECORDING_ENCODING_ENABLED=True, are sent to LiveKit asEncodingOptions.advancedinRoomCompositeEgressRequest. Default: disabled → current behaviour unchanged.Values pass straight through to the GStreamer pipeline (
x264enc/faac), no hidden conversions. LoweringFRAMERATEis the main CPU lever (x264speed-presetstaysveryfaston the LiveKit side); loweringVIDEO_BITRATE_KBPSis the main file-size lever.Documentation includes a GStreamer mapping table and a set of reference profiles (Default / Balanced / Low CPU / Minimum CPU / Audio-heavy) with estimated sizes and CPU cost. The recommended "Low CPU / small file" profile lands at ~150 MB for 30 min with ~50 % of the default CPU load.
RECORDING_ENCODING_*settings (opt-in, defaults match the currentH264_720P_30preset)advancedEncodingOptionsonly when enabled, so disabled-path behaviour is byte-identicalWorkerServiceConfig.from_settings()env.d/development/common.distdocumented examplesdocs/features/recording.md— configuration table + "Tuning recording encoding" section with GStreamer mapping and reference-profile table[Unreleased]