Repository navigation
feat(voice): add bit_rate and constant_bit_rate options to RecorderIO - #7740
Conversation
RecorderIO always encoded with the Opus encoder's defaults (variable bit rate, ~96 kb/s for stereo). Silence at the start of a session packs into tiny VBR packets, so players that estimate an Ogg Opus duration from the file size (macOS Finder, QuickLook, Preview) show recordings as twice their real length. There was also no way to match Egress's quality/size. Add two keyword arguments whose defaults keep today's behaviour: - bit_rate: sets codec_context.bit_rate (None keeps the encoder default) - constant_bit_rate: sets the Opus "vbr=off" option Both libopus and ffmpeg's native opus fallback honour them. Closes livekit#7737
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Reject a bit_rate outside 500..512000 (the range libopus accepts for stereo) or not an int in __init__. The encoder opens on its own thread, so a bad value was only logged there: out of range it left no audio file, and a non-int raised before the thread's try, so aclose() hung. - Set the "vbr" option only on libopus. FFmpeg's native opus encoder has no such option, so constant_bit_rate now logs a warning there instead of being silently ignored. - Skip the packet-size tests when libopus is not available.
ANDROIDHASSAN
changed the title
…trate Resolve conflicts with livekit#7741: apply bit_rate and vbr inside the encode thread's try block, and keep both sets of tests.
f80d007 to
90e05de
Compare
|
longcw
left a comment
longcw
left a comment
There was a problem hiding this comment.
looks good to me. the gap is reach: AgentSession creates its own RecorderIO(agent_session=self), and RecorderIO is not exported from livekit.agents.voice, so anyone who records with session.start(record=True) still gets VBR.
maybe add the option to RecordingOptions?
RecorderIO got bit_rate and constant_bit_rate options (prior commit), but
AgentSession's own session.start(record=...) path never passed them: it
builds its RecorderIO directly (self._recorder_io = RecorderIO(agent_session=self)),
and RecorderIO itself isn't exported from livekit.agents.voice, so a caller
using session.start(record=True) had no way to reach either option and
always got VBR.
Add bit_rate and constant_bit_rate to RecordingOptions and read them (via
.get(), since the TypedDict is total=False) at the one place AgentSession
constructs its RecorderIO.
Added two cases to tests/test_recording.py: record={...} with both options
set reaches RecorderIO's constructor unchanged, and record=True (neither
given) leaves RecorderIO's own defaults (None, False) alone rather than
forcing a specific bit rate. _patch_job_ctx now yields the mocked RecorderIO
class so a test can assert on its constructor kwargs.
make check (format, lint, type-check) and the full tests/test_recording.py
suite (62 tests) pass.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressed the new startup-cleanup finding in the latest commits on this branch.
This should prevent the partial-startup/leaked-room-task scenario described in the review. I haven't been able to run the repository test suite in this environment; please re-run CI and let me know if you'd like the validation placed differently. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| bit_rate = self._opts.recording_options.get("bit_rate") | ||
| recorder_may_start = self._opts.recording_options["audio"] or (c.enabled and c.record) | ||
| invalid_bit_rate = not isinstance(bit_rate, int) or ( | ||
| bit_rate < MIN_BIT_RATE or bit_rate > MAX_BIT_RATE |
There was a problem hiding this comment.
the bit-rate range check exists in both RecorderIO.__init__ and AgentSession.start(). Moving the session-side check into _resolve_recording_options would keep it in one place before the primary session is claimed, and would drop the recorder_may_start condition and the import of the private constants.
…tions Keeps the range check in one helper shared with RecorderIO and drops the recorder_may_start condition, so an invalid bit_rate is rejected before the primary session is claimed whether or not audio recording is on.
Closes #7737.
Problem:
RecorderIOalways encodes with the Opus encoder's defaults: variable bit rate, about 96 kb/s for stereo. A session almost always opens on silence, which VBR packs into 3-byte packets. Players that estimate an Ogg Opus duration from the file size (macOS Finder, QuickLook, Preview) then show a 3:32 recording as 6:53 or longer. The measurements in #7737 show that only a constant bit rate fixes the estimate. There was also no way to match Egress recordings, which are constant 128 kb/s.Fix: two keyword arguments on
RecorderIO, as proposed in the issue. The defaults keep today's behaviour:bit_rate: int | None = Nonesetscodec_context.bit_rate. Any value that is not an int between 500 and 512000 (the range libopus accepts for stereo) raisesValueErrorin__init__. The encoder opens on its own thread, where a rejected value is only logged. The session then ends with no audio file.constant_bit_rate: bool = Falsesets the libopusvbr=offoption. FFmpeg's nativeopusencoder has no such option, so on that fallback the recorder logs a warning instead of silently ignoring the setting.Tests
New cases in
tests/test_recorder_io.py:test_the_default_encoding_packs_silence_into_smaller_packetschecks that the default stays VBR.test_a_constant_bit_rate_encodes_every_packet_at_the_same_size: at 128 kb/s every 20 ms packet is 320 bytes, silence or tone, as in Egress recordings.test_a_bit_rate_the_encoder_would_reject_fails_at_constructionis parametrized over499,512001,-1,True,128000.0and"128000". Separately, I checked by hand that libopus accepts exactly 500 and 512000.test_constant_bit_rate_without_libopus_warnsforces the native fallback.The two packet-size tests are skipped when libopus is unavailable. The validation and fallback cases fail on
main.ruff format,ruff checkandmake type-checkare clean.Scope: only
recorder_io.pychanges. The branch is merged withmainafter #7741, so the options are now applied inside the encode thread'stry. The new options are read only in__init__and in the codec setup of_encode_thread.Open question:
AgentSessionbuilds its ownRecorderIO(agent_session=self)(agent_session.py:1088), sosession.start(record=True)users can't reach these options yet. I kept the PR to the scope proposed in #7737. If you'd like the options exposed throughRecordingOptions, I can add that here or in a follow-up.