Update AudioStreamTrack to use stateful av.AudioResampler for better quality#272
Open
dangusev wants to merge 6 commits into
Open
Update AudioStreamTrack to use stateful av.AudioResampler for better quality#272dangusev wants to merge 6 commits into
dangusev wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough
ChangesAudio frame pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AudioStreamTrack
participant FrameResampler
participant FrameDeque
participant FramePacer
AudioStreamTrack->>FrameResampler: Resample PCM input
FrameResampler-->>AudioStreamTrack: Return fixed-size frames
AudioStreamTrack->>FrameDeque: Buffer frames and drop oldest on overflow
AudioStreamTrack->>FramePacer: Request next PTS
FramePacer-->>AudioStreamTrack: Return paced PTS
AudioStreamTrack->>FrameDeque: Dequeue frame or generate silence
AudioStreamTrack-->>AudioStreamTrack: Pad and timestamp output frame
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_audio_stream_track.py (1)
3-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
_sine_chunks/SINE_FREQhelper.This helper and constant are byte-for-byte identical to
_sine_chunksintests/rtc/test_track_util.py:14-34(same signature, same body). Consider extracting to a shared test utility module (e.g.tests/audio_test_utils.pyor aconftest.pyfixture) so the two test suites don't drift independently.🤖 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 `@tests/test_audio_stream_track.py` around lines 3 - 34, Extract the identical SINE_FREQ constant and _sine_chunks helper into a shared test utility module, then update tests/test_audio_stream_track.py and tests/rtc/test_track_util.py to import and reuse them. Remove the duplicated local definitions while preserving the existing helper signature and behavior.
🤖 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 `@getstream/video/rtc/audio_track.py`:
- Around line 158-178: Update _FramePacer.next_pts to use time.monotonic() for
both the initial _start anchor and the current-time calculation used to derive
wait, while preserving the existing timestamp progression and sleep behavior.
- Around line 76-109: Move the _resampler.resample call in write() inside the
_frame_lock critical section, keeping frame enqueueing and buffer-cap
enforcement in that same section. This must serialize resampling with flush() so
an in-flight write cannot enqueue frames computed before the resampler reset;
preserve the existing empty-frame and final-flush behavior.
In `@getstream/video/rtc/track_util.py`:
- Around line 2631-2680: The _ensure_av_resampler method currently replaces the
existing av.AudioResampler on input signature changes without draining its
buffered samples. Before creating and assigning a new resampler, flush the old
instance and preserve those returned frames in the resample flow; ensure the new
resampler is then used for the current input. Add a regression test covering a
partial frame followed by a rate or format change and verify the buffered tail
is emitted.
In `@tests/test_audio_stream_track.py`:
- Around line 302-312: Loosen the real-time elapsed_ms upper bounds in
test_first_frame_is_immediate_and_pts_zero and
test_slow_consumer_does_not_accumulate_drift to tolerate CI scheduling and
runtime jitter while still detecting unintended pacing waits. Keep the
assertions for immediate first-frame behavior, pts=0, and drift semantics
unchanged.
---
Nitpick comments:
In `@tests/test_audio_stream_track.py`:
- Around line 3-34: Extract the identical SINE_FREQ constant and _sine_chunks
helper into a shared test utility module, then update
tests/test_audio_stream_track.py and tests/rtc/test_track_util.py to import and
reuse them. Remove the duplicated local definitions while preserving the
existing helper signature and behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4ce0ad2-b72d-492e-b323-9812a906d7fd
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
getstream/video/rtc/audio_track.pygetstream/video/rtc/track_util.pytests/rtc/test_track_util.pytests/test_audio_stream_track.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use the new
FrameResampler(a thin wrapper aroundav.AudioResampler) insideAudioStreamTrackto improve the output audio quality.Previous version defaulted to
PcmData.resample(), which uses a stateless algorithm under the hood for chunks <500ms.On such short chunks, the stateless transform introduces artifacts at the chunk boundaries.
av.AudioResampleris stateful, and it outputs much smoother audio.Performance-wise, both versions are very similar, with
FrameResamplerhaving slightly higher overhead when theAudioStreamTrack.write(..., final=True)is called because it re-inits the av.AudioResampler under the hood.What's changed
FrameResamplerthat acceptsPcmDataand outputs a list ofav.AudioFrameobjects.AudioStreamTrackis updated to use theFrameResamplerfor incoming PCM chunks and buffer theav.AudioFramesinstead of raw bytes under the hood.AudioStreamTrack.write()gainedfinal:boolparameter to signal the end-of-utterance events when the internal resampler state needs to be flushed and added to the track buffer for output.AudioStreamTrack._bufferwhich is discouraged).What has not changed
PcmData.resample()remains as-is. In fact, the stateless algorithm may be handy in some scenarios where quality is less of a concern (e.g., sending input to STT)Summary by CodeRabbit
Summary
Improvements
Bug Fixes / Behavior Changes
API Updates
write()now supportsfinalto control draining; output is restricted to S16 format.Testing