Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of re-generating and overwriting the last 5 HLS segments with a fade-out (causing audible glitches), generate new music-only segments with fade-out appended after the last streamed segment. No segment is ever overwritten. - Add _append_fade_segments() replacing _apply_fade_to_segments() - Add normalize=0 to all amix filters for consistent -15dB music level - Add explicit codec params (-ar 44100 -ac 2 -b:a 128k) across segment boundaries - Scale trailing pad by duration tier (base 7s + 5s per tier) - Add unit tests for new fade method Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds CLAUDE.md documentation; introduces three HLS constants; refactors FFmpegAudioService HLS fade logic from rewriting to append-only fade segments and updates amix normalization; adds unit tests for the new fade behavior; changes CI trigger to run on pull_request. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FFmpegAudioService
participant FFmpeg
participant HLSStorage as HLS Storage
participant HLSService
Client->>FFmpegAudioService: start streaming job
FFmpegAudioService->>FFmpeg: start streaming (no fade, amix normalize=0)
FFmpeg->>HLSStorage: write HLS segments & playlist
HLSStorage->>HLSService: notify/upload complete segments
HLSService-->>FFmpegAudioService: upload watcher events
note over FFmpegAudioService,HLSService: after streaming completes
FFmpegAudioService->>FFmpeg: generate appended fade segments (using music offset, trailing pad)
FFmpeg->>HLSStorage: upload fade segments & update playlist
HLSStorage->>HLSService: publish updated playlist
HLSService-->>Client: final playlist available
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/src/services/ffmpeg_audio_service.py`:
- Around line 580-583: The code calls
self.hls_service.upload_segment_from_file(...) and discards its boolean result,
then always appends seg_duration to segment_durations; instead check the
returned bool and handle failure the same way combine_voice_and_music_hls does
(e.g., if not upload_segment_from_file(...): raise/return an error) so failed
uploads are not counted in segment_durations and the playlist never references
missing segments; locate the call to upload_segment_from_file and the subsequent
segment_durations.append and add the conditional check and error handling around
it.
In `@backend/tests/unit/test_services.py`:
- Line 1148: The test's assertion expects a fade duration of 20s but the
implementation uses HLS_FADE_DURATION_SECONDS (10s); update the assertion in
backend/tests/unit/test_services.py to check for "afade=t=out:st=0:d=10" (or
interpolate HLS_FADE_DURATION_SECONDS) instead of "afade=t=out:st=0:d=20" so the
test aligns with _append_fade_segments and the HLS_FADE_DURATION_SECONDS
constant referenced by the code under test.
🧹 Nitpick comments (1)
CLAUDE.md (1)
38-43: Add a language specifier to the fenced code block.The ASCII directory layout block has no language tag. Adding
```textsatisfies markdownlint (MD040) and makes intent explicit.Suggested fix
-``` +```text ├── frontend/ # Expo 52 / React Native 0.74 / TypeScript ├── backend/ # Python 3.13 / AWS Lambda / Pydantic ├── tests/ # Frontend test suites (Jest) └── docs/ # API.md, ARCHITECTURE.md -``` +```
Summary by CodeRabbit
New Features
Documentation
Tests
Chores