fix(recording): count the tap's catch-up apart from audio it lost - #424
Merged
Conversation
`droppedSeconds` counted two things that mean opposite things. A source that has never delivered is not in the quorum that can hold the cursor back, so the track starts moving before the audio tap has said anything at all. Whatever the tap hands over first describes a span the cursor has already crossed, and is discarded — that is the design working, and the origin staying anchored to the video. The same counter also held audio discarded after a source went quiet past `emissionGraceFrames` and then handed over the period it was quiet for. That one is captured audio genuinely lost, and the only thing here worth failing on. Against a live ScreenCaptureKit the first kind is never zero, so "dropped is zero" was unsatisfiable and the check meant nothing. Measured on an M1 Mac mini (macOS 26.5): 0.00s to 0.05s per take, varying from run to run, which made the macOS timeline test fail intermittently rather than outright. `hasPlaced` is the line between them: whether the source ever reached the mix, not where its frames sit. Frame zero was the tempting answer and the wrong one — splitting there reported `trimmedSeconds: 0` while the loss stayed on the dropped side, because this audio does not predate the writer session, it merely arrives after the cursor has passed it. `droppedSeconds` now keeps only real loss and is zero on live takes; `trimmedSeconds` reports the catch-up. Both directions are pinned by unit tests. Also records what the timeline summary was written to find out, now that a take has been run against the real API: ScreenCaptureKit's system-audio tap streams silence rather than gapping it the way the WASAPI loopback tap does. A 12.05s take with nothing playing reported 0.07s undelivered, 0.6% of the track, in a single hole at the head while the tap came up.
…ency Three things kept this test red for reasons that were never the timeline, on a machine where the timeline itself was correct throughout. The dropped-audio check now reads a `droppedSeconds` that no longer includes the tap's start-up catch-up, so it can be compared against zero — the only threshold that means anything for it. The catch-up is reported beside it instead. The tone's position is compared against the instant playback was asked for, so the budget carries afplay's start-up and the output device waking up, neither of which is charged to the code under test. Warming the device before the take removes the second term. The first is a floor: +0.22s on an M1 Mac mini, flat whether the tone is asked for at 1.5s, 4s or 7s, which left 30ms inside the old 0.25s tolerance for the thing actually being measured. 0.40s still fails the regression this exists to catch, a 250ms start-up rebase landing at 0.47s. The warm-up has to make a sound. Digital silence was tried first and measured no better than no warm-up at all, because nothing starts the device until there are samples to play. It runs before the helper starts, so it cannot be found by `firstAudibleSecond` and mistaken for the tone. ffmpeg's own error is reported rather than swallowed. A shared-library build whose dylibs are not on the loader path fails there with a linker error, and "Could not synthesise the test tone." sends you looking at the audio pipeline.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe audio mixer now distinguishes initial trimmed audio from later dropped audio. ChangesAudio timeline diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change separates initial audio catch-up from genuinely lost audio and updates the macOS timeline checks accordingly; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant SourceTimeline
participant AudioTrackMixer
participant DeliveryReport
SourceTimeline->>AudioTrackMixer: Classify pre-cursor frames
AudioTrackMixer->>SourceTimeline: Track source placement
AudioTrackMixer->>DeliveryReport: Report trimmedSeconds and droppedSeconds
🚥 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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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.
Follow-up to #410, which was written and reviewed entirely on Windows. This is what running it against a real ScreenCaptureKit turned up.
What was wrong
droppedSecondscounted two things that mean opposite things.A source that has never delivered is not in the quorum that can hold the cursor back, so the track starts moving before the audio tap has said anything at all. Whatever the tap hands over first describes a span the cursor has already crossed, and is discarded — that is the design working, and the origin staying anchored to the video. The same counter also held audio discarded after a source went quiet past
emissionGraceFramesand then handed over the period it was quiet for, which is captured audio genuinely lost.Against a live tap the first kind is never zero, so
droppedSeconds == 0was unsatisfiable and the check meant nothing. It measured 0.00s–0.05s per take, varying run to run, which is whytest:sck-audio-timeline:macfailed intermittently rather than outright.hasPlacedis the line between them: whether the source ever reached the mix, not where its frames sit.The hypothesis that was wrong
Worth recording, because it is the one the code invites.
SourceTimelinedocuments a negative frame index as "audio captured before the writer session started", so the obvious split is at frame zero.That was tried and refuted by measurement: splitting there reported
trimmedSeconds: 0while the whole loss stayed on the dropped side. This audio does not predate the session — it arrives after the cursor has passed it, which is a different thing and needs a different boundary.The question #410 left open
AudioTrackMixerasked, in a comment, whether ScreenCaptureKit's system-audio tap gaps its silence the way the WASAPI loopback tap does. A take has now been run: it streams silence. 12.05s with nothing playing at all reported 0.07s undelivered — 0.6% of the track, in a single hole at the head while the tap came up. So on macOS the clock only carries the take's edges. The answer is now recorded in the source rather than in a review thread.Also in here
test:sck-audio-timeline:macfailed for two further reasons that were never the timeline:Verification
npm run test:swift:mac— 13/13, on Swift 6.3.2 / macOS 26.5. Both directions of the split are pinned: pre-session →trimmed 0.1/dropped 0, late arrival →dropped 0.5/trimmed 0.npm run test:sck-audio-timeline:mac— 8 consecutive live recordings green before rebasing,droppedSecondszero throughout.Two caveats
Everything here was measured on one machine — an M1 Mac mini, macOS 26.5, no microphone. The 0.22s afplay floor and the 0.00s–0.05s catch-up are solid there; I have no data for Intel, a laptop, or an external output device.
An unrelated flake showed up while verifying, and it is not fixed here. The recording's video track sometimes comes up short — 10.48s and once 3.40s against an 11.0s audio track, roughly 2 runs in 6, while the audio side stayed healthy (
droppedSeconds: 0). Since this test's tightest assertion compares audio length against video length, that flake fails it for reasons outside the audio path. It is not from this change: the helper sources are byte-identical between the base this was written on and currentmain, and the diff here adds two counters and one bool without touching control flow. Worth its own look.Summary by CodeRabbit