fix(audio): prevent audio session interference during recording #4548
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rabble
left a comment
There was a problem hiding this comment.
Review — PR #4548: prevent audio session interference during recording
Overview
Targeted fix for #4539 (iOS recording-mic volume ramp-up). Roots out the cause: just_audio's AudioPlayer was calling setCategory(.playback) on the camera-owned shared AVAudioSession when the countdown beep / lip-sync playback player was constructed, which triggered attachAudioToSessionIfNeeded() on the native camera controller, resetting VPIO/AGC mid-capture. The fix passes handleAudioSessionActivation: false to every just_audio player used during recording, via two new injectable default factories on VideoRecorderNotifier.
What's good
- Right layer. The fix sits at the construction site of the players the recorder owns — not buried in the recorder's hot path. No
awaitis introduced between session change and capture start (percode_style.md'sFuture.delayedban). - No state pollution. No mutable instance vars hold session state; the factories are
finalconfiguration (architecture.md / state_management.md compliant). - Documentation is excellent. Each factory's dartdoc explains the why —
setCategory(.playback)->attachAudioToSessionIfNeeded()-> VPIO/AGC reset — with a backlink to #4539. Future maintainers will not silently drophandleAudioSessionActivation: false. - Tests anchor the wiring. The new regression group ("VideoRecorderNotifier - Audio Service Factories (#4539)") asserts: (a) default factories return the expected service types, (b)
startRecordingactually invokes the injectedCountdownSoundServicefactory exactly once when the timer is enabled. Renaming or removing the factories breaks the constructor and the tests. This is the right shape — the behaviour under test (factory wiring) is what protects the fix. coverage:ignoreis justified inline. TheJustAudioSimplePlayerconstructor comment explicitly explains why the thin pass-through can't be unit-tested and points to the indirect coverage via the recorder factories. Strict-coverage gates won't be silently subverted.- Permission flow is untouched — mic permission gating is not in this diff. Recording hot path unchanged structurally.
Minor observations (non-blocking)
- Unicode escape in dartdoc.
simple_audio_player.dartline ~33 has a literal—in the doc comment ("the associated tests — see #4539") rather than the em dash itself. Cosmetic — renders as—in IDE hover. Trivial follow-up. - No direct assertion on the boolean. The tests verify the factory is invoked, not that
handleAudioSessionActivation: falseis forwarded. The forwarding is exercised only at integration. Acceptable given the documentation and the small surface, but a future iteration could expose a getter onJustAudioSimplePlayer(test-only) to assert the flag explicitly. Not a blocker — would add coverage cost for a one-line wiring. - Positional constructor params keep growing.
VideoRecorderNotifier([CameraService?, CountdownSoundServiceFactory?, AudioPlaybackServiceFactory?])— three optional positionals. Migrating to named parameters in a follow-up would improve readability at call sites, but the existing positional-cameraService convention pre-dates this PR and changing it would broaden scope. - Test timeout is
Duration(seconds: 30)for the countdown test because the 3-second timer runs in real time. The comment is candid about this. Long-term, threading a fake clock into the countdown would let this drop back to default. Out of scope here.
Risk
Low. The change is additive (new optional params with defaults preserving the safer behaviour), platform-side code is untouched (no AVAudioSession code in Swift/Kotlin), and the test suite locks the wiring. CI is 14/14 green. The only platform whose behaviour changes is iOS, which is exactly the regression surface.
Verdict
Approve. Well-scoped, well-documented, and the test seam is positioned to catch the exact regression this PR fixes.
3e5295a to
e93aaf6
Compare
|
Pushed a follow-up commit to tighten the regression guard on the lip-sync playback path. What changed:
Local verification I ran on the rebased branch:
@hm21 can you confirm the on-device iOS verification as well:
Once that confirmation is in and CI is green, this looks ready to approve. |
This comment has been minimized.
This comment has been minimized.
Mobile PR PreviewPreview refreshed for Last refresh:
|
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Added the missing playback-path regression coverage, then isolated it into its own skip-optimization test file so the optimized CI run stays stable. Local targeted verification and the refreshed GitHub suite are green. I left a PR comment asking for explicit iOS device confirmation on countdown audibility and stable recorded volume.
Description
Resolve the issue of the recording-audio on iOS devices getting progressively louder during recording.
Related Issue: Closes #4539
Type of Change