feat(recording): add AudioOnly capture target and recording pipeline …#1881
feat(recording): add AudioOnly capture target and recording pipeline …#1881ManthanNimodiya wants to merge 12 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…dd Audio target variant
|
@greptileai please re-review |
|
@greptileai please re-review |
|
@greptileai please re-review |
|
@richiemcilroy, can you have a quick look and lmk if I can go for the phase 6 and 7, no hurry just in case this gets buried |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| let needs_remux = if fragmented { | ||
| segment_metas.iter().any(|seg| { | ||
| let display_path = seg.display.path.to_path(&recording_dir); | ||
| let display_path = seg | ||
| .display | ||
| .as_ref() | ||
| .map(|d| d.path.to_path(&recording_dir)) | ||
| .unwrap_or_default(); | ||
| display_path.is_dir() | ||
| }) |
There was a problem hiding this comment.
unwrap_or_default() here will treat display: None like an empty path; is_dir() can easily become true (e.g. current dir), which would incorrectly mark audio-only recordings as needing remux.
| let needs_remux = if fragmented { | |
| segment_metas.iter().any(|seg| { | |
| let display_path = seg.display.path.to_path(&recording_dir); | |
| let display_path = seg | |
| .display | |
| .as_ref() | |
| .map(|d| d.path.to_path(&recording_dir)) | |
| .unwrap_or_default(); | |
| display_path.is_dir() | |
| }) | |
| let needs_remux = if fragmented { | |
| segment_metas.iter().any(|seg| { | |
| seg.display | |
| .as_ref() | |
| .is_some_and(|d| d.path.to_path(&recording_dir).is_dir()) | |
| }) | |
| } else { | |
| false | |
| }; |
| sharing: None, | ||
| inner: RecordingMetaInner::Studio(Box::new(studio_meta.clone())), | ||
| upload: None, | ||
| audio_only: false, |
There was a problem hiding this comment.
This hardcodes audio_only: false when persisting the final meta. If recording-meta.json was initially written with audio_only: true, this will overwrite it at the end (same issue in write_in_progress_meta).
| audio_only: false, | |
| audio_only: RecordingMeta::load_for_project(recording_dir) | |
| .ok() | |
| .map(|m| m.audio_only) | |
| .unwrap_or(false), |
| .enumerate() | ||
| .map(|(index, segment)| { | ||
| let duration = get_video_duration_secs(&segment.display.path.to_path(project_path))?; | ||
| let duration = get_video_duration_secs(&segment.display.as_ref().map(|d| d.path.clone()).unwrap_or_default().to_path(project_path))?; |
There was a problem hiding this comment.
Using unwrap_or_default() here means a missing display falls back to project_path and you’ll try to read a directory as a video. I think this should stay an explicit error (like the full_timeline_for_source_segments path).
| let duration = get_video_duration_secs(&segment.display.as_ref().map(|d| d.path.clone()).unwrap_or_default().to_path(project_path))?; | |
| let display = segment.display.as_ref().ok_or("Missing display video")?; | |
| let duration = get_video_duration_secs(&display.path.to_path(project_path))?; |
| let display = s | ||
| .display | ||
| .as_ref() | ||
| .ok_or_else(|| "SingleSegment missing display".to_string()) |
There was a problem hiding this comment.
Now that display is optional, this returns an error whenever it’s None. If studio audio-only recordings are expected to finalize, callers need to skip ProjectRecordingsMeta::new (or this type needs to represent display as optional too) so audio-only paths don’t fail here.
| tokio::spawn(async move { | ||
| // When screen (video) finishes, cancel the other pipelines | ||
| let _ = screen_done.await; | ||
| if let Some(done) = screen_done { | ||
| let _ = done.await; | ||
| } |
There was a problem hiding this comment.
This still cancels the other pipelines immediately for audio-only (screen_done = None). If the intent is “cancel others when screen finishes”, the task should exit early when there’s no screen pipeline.
| tokio::spawn(async move { | |
| // When screen (video) finishes, cancel the other pipelines | |
| let _ = screen_done.await; | |
| if let Some(done) = screen_done { | |
| let _ = done.await; | |
| } | |
| tokio::spawn(async move { | |
| let Some(done) = screen_done else { | |
| return; | |
| }; | |
| let _ = done.await; |
| let display_output_path = match &updated_studio_meta { | ||
| StudioRecordingMeta::SingleSegment { segment } => { | ||
| segment.display.path.to_path(&recording_dir) | ||
| segment.display.as_ref().map(|d| d.path.clone()).unwrap_or_default().to_path(&recording_dir) |
There was a problem hiding this comment.
Minor footgun with the unwrap_or_default() change: for audio-only, this resolves to recording_dir and then gets passed into create_screenshot a few lines later. Might be worth guarding the screenshot generation behind if segment.display.is_some() (and same for the multiple-segment case) so audio-only doesn’t try to thumbnail a directory.
|
@greptileai @tembo please re-review |
re-reviewed pr #1881 at the latest commit (b04c68d) and posted my findings as a pr comment. two of the four flagged issues are fixed:
two are still open:
comment with details and concrete fix suggestions: #1881 (comment) want me to push a fix for the two remaining issues? |
Re-review (commit
|
| let recordings = if has_display { | ||
| ProjectRecordingsMeta::new(&recording_dir, &updated_studio_meta)? | ||
| } else { | ||
| ProjectRecordingsMeta { segments: vec![] } |
There was a problem hiding this comment.
One concern with the audio-only fallback here: ProjectRecordingsMeta { segments: vec![] } means project_config_from_recording will still persist timeline: Some with 0 segments, which can block later auto-timeline generation (it only runs when timeline is None).
Might be worth explicitly leaving timeline unset for audio-only (or gating timeline creation on !recordings.segments.is_empty()).
|
@tembo please re-review |
re-reviewed pr #1881 at the latest commit (b22097e) and posted a review on github. good progress since last round — most previously-flagged issues are fixed:
still open (left inline comments):
flagged as follow-up (not a regression here): recovery.rs analyze_incomplete skips any segment without display fragments, so a crashed audio-only studio recording is silently discarded. recovery.rs isn't touched by this pr, so it's a pre-existing gap audio-only newly exposes — fine to handle in a later phase but worth tracking. once 1 and 2 are addressed it looks ready to go. |
There was a problem hiding this comment.
Re-review @ b22097e
Good progress since the last round — most of the previously-flagged issues are resolved. Summary below.
✅ Fixed
needs_fragment_remux/needs_remux— audio-only segments (display: None) no longer resolve torecording_dirand get mis-flagged as fragmented; the empty-path fallback isfalseunderis_dir(). (studio_recording.rs:1143)- Cancel-guard — the spawned task now early-returns when
screen_doneisNone, so audio pipelines are no longer cancelled immediately for audio-only recordings. (studio_recording.rs:609) - Cross-track AV sync —
CROSS_TRACK_SNAP_SECSsnapping is preserved for screen recordings; the raw-fallback only fires whenscreenisNone. (studio_recording.rs:978) - Empty timeline —
project_config_from_recordingnow gatesconfig.timelineon!timeline_segments.is_empty(), so audio-only recordings no longer persist a 0-segment timeline that would block auto-timeline generation. (recording.rs:3328, commit b22097e) ProjectRecordingsMeta::new— callers guard withhas_displaybefore calling it, so audio-only finalization no longer hits theErr/panic path. (recording.rs:2853)
❌ Still open
audio_onlyflag clobbered tofalse(P1) —persist_final_recording_meta(line 1782) andwrite_in_progress_meta(line 1812) both hardcodeaudio_only: false, overwriting thetruewritten bystart_recording. Downstream consumers always readfalse. Inline comment with suggested fix.- Camera window opened for
AudioOnly(P2) —AudioOnlyshares theCameraOnlybranch and showsShowCapWindow::Camera, opening an empty preview and possibly prompting for camera permission. Inline comment atrecording.rs:1237.
⚠️ Follow-up worth tracking
- Audio-only crash recovery —
RecoveryManager::analyze_incomplete(crates/recording/src/recovery.rs:226)continues on any segment without display fragments, so a crashed audio-only studio recording is silently discarded.recovery.rsis not touched by this PR, so this is a pre-existing limitation that audio-only newly exposes rather than a regression here. Given the UI to trigger audio-only studio recordings is a follow-up phase, this is reasonable to address in a later PR — but it should be tracked so audio-only doesn't ship with silent data loss on crash.
Once (1) and (2) are addressed this looks ready. Nice work on the sync/cancel/timeline fixes.
| sharing: None, | ||
| inner: RecordingMetaInner::Studio(Box::new(studio_meta.clone())), | ||
| upload: None, | ||
| audio_only: false, |
There was a problem hiding this comment.
Still open (re-review): audio_only is hardcoded to false here. start_recording writes audio_only: true for an AudioOnly studio recording, but this final persist overwrites recording-meta.json at the end, so the editor/share page always read false. write_in_progress_meta (line 1812) has the same hardcode and runs before recording starts, clobbering the initial true as well.
Since RecordingMeta::load_for_project exists, the minimal fix is to read and preserve the existing value:
audio_only: RecordingMeta::load_for_project(recording_dir)
.ok()
.map(|m| m.audio_only)
.unwrap_or(false),| if matches!(inputs.capture_target, ScreenCaptureTarget::CameraOnly) { | ||
| if matches!( | ||
| inputs.capture_target, | ||
| ScreenCaptureTarget::CameraOnly | ScreenCaptureTarget::AudioOnly |
There was a problem hiding this comment.
Still open (re-review): AudioOnly shares this CameraOnly branch, so it calls ShowCapWindow::Camera { centered: true } and sets was_camera_only_recording = true. An audio-only recording has no camera feed, so this opens an empty camera preview window, attaches incorrect state metadata, and may trigger a camera permission prompt that audio-only users shouldn't see.
AudioOnly should skip the camera-window block. The capture_system_audio = false line is also worth a second look for audio-only — disabling system audio limits audio-only to mic-only capture, which may not be the intended behavior (vs. CameraOnly where it makes sense).
… window for AudioOnly
|
@tembo @greptileai please re-review |
re-reviewed pr #1881 at commit 801e689 and posted my review: #1881 (review) most of the round-1 issues are now fixed:
but i found a blocker that greptile missed: AudioOnly never actually reaches an audio-only pipeline. neither instant nor studio has a branch for it, so it falls through to screen-capture code that calls still open from round 1:
also worth noting: no tests exercise an actual audio-only recording start or audio-only crash recovery, so these slip past clippy/compile. happy to open a follow-up pr with the audio-only pipeline branches + the recovery fix if you want me to. |
| "displayVideo", | ||
| meta.path(&segment.display.path), | ||
| )); | ||
| if let Some(display) = &segment.display { |
There was a problem hiding this comment.
Should this be gated on meta.audio_only rather than segment.display.is_some()? As-is, a corrupted/non-audio-only studio recording with missing display metadata would get reported as valid since the required check is skipped.
| .display | ||
| .as_ref() | ||
| .map(|d| recording_meta.path(&d.path)) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
unwrap_or_default() here turns a missing display track into an empty path (often treated as .), which leads to pretty confusing decoder errors. Since this binary can’t render without a display track anyway, failing fast seems clearer.
| .unwrap_or_default(), | |
| .context("Missing display video")?, |
| .display | ||
| .as_ref() | ||
| .map(|d| recording_meta.path(&d.path)) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
Same idea as above for the multi-segment path — avoids passing an empty path into the screen decoder.
| .unwrap_or_default(), | |
| .context("Missing display video")?, |
| StudioRecordingMeta::MultipleSegments { inner, .. } => { | ||
| inner.segments[segment_i].display.fps | ||
| StudioRecordingMeta::SingleSegment { segment } => { | ||
| segment.display.as_ref().map(|d| d.fps).unwrap_or(0) |
There was a problem hiding this comment.
Defaulting screen_fps to 0 makes downstream failures harder to diagnose (and risks odd behavior if it slips through). Returning a clear error here keeps the invariant “rendering requires display metadata” explicit.
| segment.display.as_ref().map(|d| d.fps).unwrap_or(0) | |
| segment | |
| .display | |
| .as_ref() | |
| .map(|d| d.fps) | |
| .ok_or_else(|| "Display metadata missing".to_string())? |
| .display | ||
| .as_ref() | ||
| .map(|d| d.fps) | ||
| .unwrap_or(0), |
There was a problem hiding this comment.
Same for the multi-segment case.
| .unwrap_or(0), | |
| .map(|d| d.fps) | |
| .ok_or_else(|| "Display metadata missing".to_string())?, |
There was a problem hiding this comment.
Re-review (commit 801e689)
Re-reviewed after the two follow-up commits. Good progress — most of the previously-flagged issues are now resolved, but there's a core-functionality bug that blocks the whole feature plus the recovery gap that's still open.
✅ Fixed since last review
audio_onlyflag no longer overwritten —persist_final_recording_meta/write_in_progress_metanow read the existing value viaRecordingMeta::load_for_projectbefore re-writing (studio_recording.rs:1775,1802). 👍- Camera window no longer opened for AudioOnly —
start_recordingnow matchesCameraOnlyonly (recording.rs:1235). 👍 - Cancel-guard early-returns for audio-only — the spawned task bails when
screen_doneisNoneinstead of cancelling the mic/cam/sys pipelines (studio_recording.rs:610-613). 👍 - Finalization guarded —
handle_recording_finishuseshas_displayand substitutes an emptyProjectRecordingsMetainstead of callingProjectRecordingsMeta::newfor audio-only (recording.rs:2851); timeline generation is skipped when there are no segments (recording.rs:3328). 👍
🔴 Blocker (new) — AudioOnly never reaches an audio-only pipeline; it errors out at startup
Neither pipeline has a branch for AudioOnly, so it falls through to screen-capture code that calls target_to_display_and_crop, which returns Err("Camera-only target has no display") for AudioOnly (capture_pipeline.rs:553-555). Both recording modes fail before any audio is captured:
- Studio —
create_segment_pipelineonly special-casescamera_only;audio_onlyis computed (studio_recording.rs:1410) but never used in the screen tuple, so it hits theelsebranch and errors atstudio_recording.rs:1497-1498. - Instant —
match inputs.capture_targethas aCameraOnlyarm but noAudioOnlyarm, soAudioOnlyhits_ =>and errors atinstant_recording.rs:614.
This contradicts the PR's stated goal of "skipping screen/camera capture entirely and building audio-only output." Each pipeline needs a dedicated audio-only arm (build mic + optional system-audio pipelines, set screen/display to None, skip target_to_display_and_crop). Worth adding a smoke test that actually starts an audio-only recording in each mode — clippy/compile won't catch this since the failure is at runtime.
🔴 Still open — audio-only crash recovery silently discards all data
recovery.rs:226-232 still continues whenever display_fragments is empty, so every audio-only segment is skipped, recoverable_segments stays empty, and inspect_recording returns None. Any audio-only studio recording that ends in a crash/force-quit loses all captured audio. Fix as previously suggested: treat a segment as recoverable when mic_fragments/system_audio_fragments are present, and make display_fragments optional on RecoverableSegment. (No test added for this path either — tests/recovery.rs only covers display recordings.)
🟡 Latent — second ProjectRecordingsMeta::new call site is unguarded
finalize_studio_recording (the fragmented-remux path) still calls ProjectRecordingsMeta::new(...)? unconditionally at recording.rs:3105. It's currently unreachable for audio-only because needs_fragment_remux returns false when no segment has a display dir — but the moment audio-only recovery/remux is wired up (see the recovery bug above), this becomes a real finalization failure. Mirror the has_display guard used at recording.rs:2851 here too.
Net: the meta/camera/cancel/finalize fixes are correct, but as it stands an audio-only recording can't actually start in either mode, and the recovery path would drop data if it did. Happy to open a PR with the pipeline branches + recovery fix if useful.
… missing display in renderer
|
@tembo please re-review |
re-reviewed pr #1881 at the latest commit (d7f2fb0) and posted a review on github. the new commit since my last pass only hardens the renderer/cli to fail-fast on missing display — good cleanup, but the core blocker is still there. still blocking:
still open from round 1:
confirmed still fixed: audio_only flag preserved in meta writes, camera window skipped for AudioOnly, cancel-guard early-returns with no screen track, finalization guarded by has_display. the two studio/instant pipeline branches are the gating items. happy to push a fix for those + the recovery handling if you want. |
There was a problem hiding this comment.
Re-review (commit d7f2fb08e)
Re-reviewed against my last pass (801e689). The new commit hardens the renderer/CLI to fail-fast on missing display (crates/rendering/src/{lib,main}.rs, apps/cli/src/project.rs now gate on audio_only) — good hygiene, but it does not touch the core blocker.
❌ Still blocking — AudioOnly never reaches an audio-only pipeline
Unchanged since my last review. Neither recording mode has a branch for AudioOnly, so it falls through to screen-capture code that errors out before any audio is captured. Greptile flagged the same two spots (its comments 6 & 7).
Studio — crates/recording/src/studio_recording.rs:1415
The screen tuple is if camera_only { … } else { … }. AudioOnly hits the else, which calls target_to_display_and_crop at line 1498 → Err("Camera-only target has no display") (capture_pipeline.rs:553), and on macOS also requires shareable_content at line 1523 (None for audio-only). The audio_only flag computed at line 1410 is applied to the camera (1559) and cursor (1698) skips but never to the screen tuple. Needs an else if audio_only { (None, None, None) } arm parallel to camera_only.
Instant — crates/recording/src/instant_recording.rs:610
spawn_instant_recording_actor matches CameraOnly explicitly; AudioOnly falls into _ =>, calling target_to_display_and_crop at 614 (same Err) and requiring shareable_content at 635. Needs its own AudioOnly arm that builds a mic + system-audio pipeline with video_info: None, like the CameraOnly arm.
Net effect: starting an audio-only recording fails at startup in both modes, which contradicts the PR's stated "building audio-only output". Note there's no UI path to trigger AudioOnly yet (phase 6), so existing flows are unaffected — but the new capability is currently non-functional and not exercised by any test, so it slips past clippy/compile.
❌ Still open (from round 1)
recovery.rs:226-231—analyze_incompletestill doesif display_fragments.is_empty() { continue; }, so a crashed audio-only studio recording is silently discarded (mic/system-audio fragments are never considered recoverable).recovery.rsisn't touched by this PR, so this is a pre-existing gap that audio-only newly exposes — fine to defer to a later phase, but worth tracking.recording.rs:3105— the secondProjectRecordingsMeta::newinfinalize_studio_recordingis still unguarded (the one inhandle_recording_finish:2851is correctly gated onhas_display). Latent: only bites once audio-only remux/recovery is wired up.
✅ Confirmed still fixed
audio_only flag preserved in the studio meta writers (reads existing value via RecordingMeta::load_for_project), camera window skipped for AudioOnly, cancel-guard early-returns when there's no screen pipeline, and finalization is guarded by has_display with timeline gen skipped for empty segments.
The two studio/instant pipeline branches are the gating items. Happy to push a fix for those + the recovery handling if you'd like.
| let path = segment | ||
| .display | ||
| .as_ref() | ||
| .map_or_else(PathBuf::new, |d| meta.path(&d.path)); |
There was a problem hiding this comment.
PathBuf::new() here is a bit risky: an empty path often resolves to . so exists() can return true and accidentally mark a non-audio-only project as valid even when display metadata is missing.
Maybe prefer branching on the option and forcing exists: false when display is None:
| .map_or_else(PathBuf::new, |d| meta.path(&d.path)); | |
| let path = match segment.display.as_ref() { | |
| Some(d) => meta.path(&d.path), | |
| None => PathBuf::from("<missing display metadata>"), | |
| }; |
…t modes, fix recovery and finalize
|
@tembo please re-review |
re-reviewed pr #1881 at commit the headline blocker is finally fixed —
one new issue worth fixing before merge: recovery is only half-wired. minor follow-ups (non-blocking): linux camera branch doesn't exclude audio_only; studio audio-only has no mic-presence guard (instant does); no tests exercise an actual audio-only start or recovery. happy to push a fix for the recovery half-wiring if you want. review: #1881 (review) |
| bounds: LogicalBounds, | ||
| }, | ||
| CameraOnly, | ||
| AudioOnly, |
There was a problem hiding this comment.
With ScreenCaptureTarget::AudioOnly added, the #[cfg(target_os = "linux")] LinuxCaptureSource::from_target match now needs an AudioOnly arm too, otherwise this won’t compile on Linux (non-exhaustive match). Probably map it the same way as CameraOnly (or explicitly return an error, depending on desired behavior).
| } | ||
| } | ||
| ScreenCaptureTarget::CameraOnly => { | ||
| ScreenCaptureTarget::CameraOnly | ScreenCaptureTarget::AudioOnly => { |
There was a problem hiding this comment.
Minor: this CameraOnly | AudioOnly arm is effectively unreachable right now because the function calls target.display()? before the match, and both variants return None there. If you want the more specific error, consider early-returning on these variants before the display lookup.
Also the error text says “Camera-only…” but now includes AudioOnly.
There was a problem hiding this comment.
Re-review (commit 863fe1f2)
The headline blocker from the last few rounds is now fixed: AudioOnly finally reaches a real audio-only pipeline in both modes.
✅ Resolved
- Studio pipeline branch —
studio_recording.rs:1491now has anelse if audio_only { (None, None, None) }arm, so it no longer falls into the screen-capture branch /target_to_display_and_crop(which returnedErrforAudioOnly). The mic pipeline is still built frombase_inputs.mic_feed, and the segment writer already handlesscreen == None(display: None, falling back tomic_start_time). Theaudio_onlyflag is now actually consumed (camera/cursor skip + screen tuple). - Instant pipeline branch —
instant_recording.rs:610adds a dedicatedAudioOnlyarm building an audio-onlyoutput.mp4viaOutputPipelineBuilder<NoVideo>::build::<Mp4Muxer>. Verified this path is valid:Mp4Muxer::setuptoleratesvideo_config: None, and theNoVideobuilder requires ≥1 audio source (which is provided). Missing mic returns a clear error. - finalize_studio_recording — the second
ProjectRecordingsMeta::newatrecording.rs:3104is now guarded byhas_display, building an emptysegments: vec![]otherwise. The latent panic I flagged last round is closed. - Still-confirmed from prior rounds:
audio_onlyflag preserved in meta writes, camera window skipped forAudioOnly, cancel-guard early-returns with no screen track, macOS shareable-content acquisition skipped forAudioOnly.
❌ New issue — recovery is only half-wired
recovery.rs:226 now keeps audio-only segments (&& !meta.audio_only), but the downstream build_recovered_meta still unconditionally fabricates a display track:
// recovery.rs:1278-1286
MultipleSegment {
display: Some(VideoMeta {
path: RelativePathBuf::from(format!("{segment_base}/display.mp4")), // file never exists for audio-only
fps, // get_video_fps falls back to 30
start_time: display_start_time,
...
}),So recovering a crashed audio-only studio recording now produces a recording-meta.json that references a non-existent display.mp4 (and contributes no duration, since estimate_fragments_duration runs on the empty display_fragments). Before this commit the segment was silently dropped; now it's kept but points at a phantom display track, which the editor will fail to load. Either gate the display field on !display_fragments.is_empty() (mirroring the segment writer), or hold the :226 change until recovery is fully wired. As-is the two halves disagree.
Minor / follow-up (non-blocking)
- Linux camera not excluded for audio-only —
studio_recording.rs:1622guards the camera pipeline oncamera_onlyonly; macOS/Windows usecamera_only || audio_only. On Linux an audio-only recording with a camera feed selected would still record camera. - No mic-presence guard in studio audio-only — instant returns a friendly "requires a microphone" error; the studio
audio_onlybranch doesn't, so an audio-only studio recording with no mic yields a fully-empty segment (screen/mic/cam/sys allNone) rather than a clear error. - No tests exercise an actual audio-only recording start (studio or instant) or audio-only recovery, so these slip past clippy/compile.
- Several
matches!(.., ScreenCaptureTarget::CameraOnly)sites (telemetry, target mapping, upload mode) don't yet account forAudioOnly— fine while the UI mode selector is still phase 6-7, but worth auditing when that lands.
The pipeline blocker is genuinely closed — nice work. The recovery inconsistency (#1 above) is the one item I'd want addressed (or reverted) before merge; the rest are follow-ups. Happy to push a fix for the recovery half-wiring if useful.
…ining AudioOnly call sites
|
@tembo please re-review |
re-reviewed pr #1881 at the latest commit (c915932) and posted a review on github. the new commit since my last pass (
still confirmed from prior rounds: audio_only flag preserved in meta writes, camera window skipped, cancel-guard early-returns, finalization guarded by has_display, both modes reach a real audio-only pipeline. non-blocking notes: no end-to-end test for an actual audio-only start or crash recovery yet; the two leftover ci note: typecheck is failing but only on unrelated web files (Footer.tsx, caption-tracks.test.ts) — pre-existing, not from this pr. the rust clippy jobs were still running at review time, worth confirming they go green before merge. verdict: all four issues from my last review are resolved and i don't see a new blocker. the recording-pipeline foundation looks ready to merge once clippy is green (ui/editor/share-page work is explicitly deferred to phases 6–7). review: #1881 (review) |
| StudioRecordingMeta::SingleSegment { segment } => { | ||
| segment.display.path.to_path(&recovered.project_path) | ||
| } | ||
| StudioRecordingMeta::SingleSegment { segment } => segment |
There was a problem hiding this comment.
Minor: if segment.display is None (audio-only, or corrupted meta), this builds a default/empty display_output_path and then create_screenshot just logs an error. Gating the screenshot spawn on display.is_some() keeps recovery quieter and avoids confusing errors.
| StudioRecordingMeta::SingleSegment { segment } => segment | |
| let display_output_path = match &recovered.meta { | |
| StudioRecordingMeta::SingleSegment { segment } => segment | |
| .display | |
| .as_ref() | |
| .map(|d| d.path.to_path(&recovered.project_path)), | |
| StudioRecordingMeta::MultipleSegments { inner, .. } => inner.segments[0] | |
| .display | |
| .as_ref() | |
| .map(|d| d.path.to_path(&recovered.project_path)), | |
| }; | |
| if let Some(display_output_path) = display_output_path { | |
| let screenshots_dir = recovered.project_path.join("screenshots"); | |
| std::fs::create_dir_all(&screenshots_dir) | |
| .map_err(|e| format!("Failed to create screenshots directory: {e}"))?; | |
| let display_screenshot = screenshots_dir.join("display.jpg"); | |
| tokio::spawn(async move { | |
| if let Err(e) = create_screenshot(display_output_path, display_screenshot, None).await { | |
| tracing::error!("Failed to create screenshot during recovery: {}", e); | |
| } | |
| }); | |
| } |
There was a problem hiding this comment.
Re-review (commit c9159323)
The new commit since my last pass (c915932 — wire up recovery display gating, mic guard, and remaining AudioOnly call sites) closes every item I flagged last round. Nice — this addresses the recovery half-wiring that was the last real blocker.
✅ Fixed since last review
1. Recovery is now fully wired for audio-only (was half-wired)
analyze_incomplete(recovery.rs:226) keeps segments with no display fragments whenmeta.audio_only, instead ofcontinue-ing past them.recover(recovery.rs:744–788) only finalizes adisplay.mp4whendisplay_fragmentsis non-empty; mic/system-audio tracks are muxed independently.build_recovered_meta(recovery.rs:1279) now setsdisplay: Nonewhendisplay_fragments.is_empty()instead of unconditionally pointing at a non-existentdisplay.mp4. This was the specific bug I called out last time.create_project_config(recovery.rs:1399–1417) derives the timeline duration from the mic track when there's no display, so a recovered audio-only recording gets a valid timeline.
2. Studio audio-only mic guard added (studio_recording.rs:1491–1497) — the else if audio_only arm now errors early with a clear message if no mic is selected, matching the instant path (instant_recording.rs:611).
3. Linux camera branch (studio_recording.rs:1568/1599/1630) now excludes audio_only, so no camera pipeline is built for audio-only on Linux.
4. Extra hardening — AudioOnly is now treated like CameraOnly (skips screen-specific behavior) in spawn_current_desktop_background_snapshot, the instant upload_mode (desktopMP4), and apply_screen_recording_presentation_defaults, with a new unit test covering the presentation-defaults case.
✅ Still confirmed from prior rounds
audio_onlyflag preserved in both studio meta writers viaRecordingMeta::load_for_project(studio_recording.rs:1784/1811).- Camera window no longer opens for
AudioOnly(recording.rs:1238matchesCameraOnlyonly). - Cancel-guard early-returns when there's no screen track.
- Finalization guarded by
has_displayat both call sites (recording.rs:2857and:3110). - Both studio and instant reach a real audio-only pipeline; macOS shareable-content acquisition skipped.
Minor / non-blocking
- The two remaining
audio_only: falseliterals inrecording.rs(:2512,:3231) are in non-audio-only contexts — a screenshot-importSingleSegmentthat always has a display, and an in-memory tempRecordingMetaused only for zoom-segment generation. Both harmless; not regressions. - No automated test exercises an actual audio-only recording start or audio-only crash recovery end-to-end (the new test only covers presentation defaults). Worth adding in a follow-up, but these paths are now logically wired.
analyze_incompletewill keep an audio-only segment dir even if it has neither display nor audio fragments;recoverjust produces nothing for it. Negligible edge case.
CI note
Typecheck is failing, but on web-only files unrelated to this PR (apps/web/app/(site)/Footer.tsx, apps/web/__tests__/unit/caption-tracks.test.ts) — pre-existing, not introduced here. The Rust Clippy jobs (the relevant ones) were still in progress at review time; worth confirming they go green before merge.
Verdict
All four issues from my last review are resolved and I don't see a new blocker. With the desktop UI / editor / share-page work explicitly deferred to phases 6–7, the recording-pipeline foundation in this PR looks ready to merge once Clippy is green.
|
@greptileai please re-review |
1 participant
Summary
What's not in this PR
(Phase 6-7)
Desktop UI (mode selector), desktop editor (waveform view), and share page (AudioPlayer fallback) are follow-up PRs that build on this foundation.
Test plan
cargo clippy --workspace --all-targets -- -D warningspasses cleanrecording-meta.jsonfiles withoutdisplayoraudio_onlydeserialize correctly via serde defaultsGreptile Summary
This PR wires
AudioOnlyas a first-classScreenCaptureTargetthrough the full recording pipeline. The second-round re-review shows that all seven previously flagged blocking issues (missing AudioOnly pipeline arms, premature audio-pipeline cancellation,audio_onlyflag always written asfalse, cross-track AV sync regression, crash-recovery data loss, incorrectCurrentRecordingTarget::Cameramapping, andneeds_fragment_remuxfalse-positive) have been correctly resolved.SingleSegment.displayandMultipleSegment.displayare nowOption<VideoMeta>with serde defaults, leaving existing recordings fully backward-compatible.persist_final_recording_metaandwrite_in_progress_metanow read the existingaudio_onlyflag from disk rather than hardcodingfalse.EditorInstance::create(line 243) still callsProjectRecordingsMeta::newwithout ahas_displayguard; the rendering crate'sSegmentRecordings.displayis still a non-optionalVideo, so the editor window fails to open for any audio-only recording with an opaque "missing display" error.Confidence Score: 4/5
Safe to merge for the recording-pipeline scope of this PR; the editor path for audio-only recordings will still fail to open.
All previously flagged recording-pipeline defects are resolved. The one remaining defect is in
EditorInstance::create: it callsProjectRecordingsMeta::newwithout guarding for audio-only, andSegmentRecordings.displayin the rendering crate is still non-optional, so opening the editor for an audio-only recording returns a hard error to the user. No UI can create audio-only recordings yet, so the path cannot be triggered today — but the fix is a one-liner guard analogous to the one already added inrecording.rs.crates/editor/src/editor_instance.rs (unconditional ProjectRecordingsMeta::new call) and crates/rendering/src/project_recordings.rs (SegmentRecordings.display non-optional)
Important Files Changed
ScreenCaptureTarget::AudioOnlyarm; previously it fell into the screen-capture_arm and failed. Audio-only pipeline now writes tocontent/output.mp4viaMp4Muxer.audio_onlybranch tocreate_segment_pipeline, makesPipeline.screenoptional, fixes cancel-guard early-return, restores cross-track AV-sync, and readsaudio_onlyflag from disk in bothpersist_final_recording_metaandwrite_in_progress_meta.continuethat discarded segments with no display fragments is now guarded by!meta.audio_only; recovered meta correctly omitsdisplayfor audio-only segments.has_displayguard aroundProjectRecordingsMeta::newin finalization path; fixesneeds_fragment_remuxfor audio-only; setsaudio_only: truein initial meta; audio-only no longer triggers camera window or screen-permission check..expect()panics to properErrreturns for missing display, butSegmentRecordings.displayremains non-optional (VideonotOption<Video>), soProjectRecordingsMeta::newstill errors for audio-only recordings — callers outsiderecording.rsare unguarded.SingleSegment.displayandMultipleSegment.displayOption<VideoMeta>with serde defaults for backward compatibility; addsaudio_only: boolfield toRecordingMeta; updatesmin_fps/max_fpsto return 0 when no display segments exist.ScreenCaptureTarget::AudioOnlymapping fromCurrentRecordingTarget::CameratoCurrentRecordingTarget::Audio, so the frontend state correctly reflects an audio-only session.display: Option, but still callsProjectRecordingsMeta::newunconditionally at line 243 — this will fail for audio-only recordings with a missing-display error, crashing editor load.displayVideofile check with!meta.audio_onlyand handlesdisplay: Option<VideoMeta>correctly for both SingleSegment and MultipleSegments.Comments Outside Diff (8)
crates/recording/src/studio_recording.rs, line 1615-1636 (link)audio_onlyflag always written asfalseby the studio pipelinepersist_final_recording_metahardcodesaudio_only: falsein theRecordingMetait writes to disk. For audio-only studio recordings,start_recordingcorrectly writesaudio_only: trueinitially, but this function overwrites the file at the end of the recording, so downstream consumers (editor, share page) will always readaudio_only: false. The same problem exists inwrite_in_progress_meta(line 1656), which runs before recording even begins and overwrites the initial value. Both functions need to either accept the capture target as a parameter or read and preserve the existingaudio_onlyvalue from disk.Prompt To Fix With AI
apps/desktop/src-tauri/src/recording.rs, line 853-869 (link)The
AudioOnlytarget enters the same branch asCameraOnlyhere, which callsShowCapWindow::Camera { centered: true }and setswas_camera_only_recording = true. For an audio-only recording there is no camera feed, so this opens a camera preview window with nothing to show and attaches incorrect state metadata. If the camera permission has not been granted, this could also produce an unexpected permission prompt. TheAudioOnlycase should likely skip this block entirely.Prompt To Fix With AI
apps/desktop/src-tauri/src/recording.rs, line 2749-2750 (link)SegmentRecordings.displayis a non-optionalVideo, soProjectRecordingsMeta::newreturnsErr("SingleSegment/MultipleSegment missing display")whenever display isNone. At both this call site (line 2749) andhandle_recording_finish(line 2506), the result is propagated with?. For audio-only studio recordings this means neitherconfig.writenor any downstream steps run — the recording's project config is never written and the recording appears incomplete from the editor's perspective.Prompt To Fix With AI
crates/recording/src/studio_recording.rs, line 595-609 (link)When
screenisNone(audio-only),screen_doneisNone, so theif let Some(done) = screen_doneguard is skipped and the spawned task proceeds directly to callingmic_cancel.cancel()(andcam_cancel,sys_cancel). Because the task is spawned but not immediately polled, it fires at the very next async yield point after recording starts — effectively cancelling the microphone pipeline before meaningful audio is captured. Every audio-only studio recording would produce an empty or near-empty output.The cancellation task should only be spawned when there is an actual screen pipeline to act as the trigger. When
screenis absent, the audio pipelines should run untilPipeline::stopis called explicitly.Prompt To Fix With AI
crates/recording/src/recovery.rs, line 226-232 (link)analyze_incompleterequires display fragments before a segment can be considered recoverable. For audio-only recordings there is no display file, sodisplay_fragmentsis always empty and every segment is skipped by thiscontinue. The result is thatrecoverable_segmentsstays empty,inspect_recordingreturnsNone, and the recovery system treats every crashed audio-only recording as if nothing happened — all captured audio is silently discarded.Because audio-only studio recordings do use
FragmentedAudioMuxerwhen crash recovery (fragmented = true) is enabled, this is not a theoretical edge case: it affects every audio-only recording that ends with a crash or force-quit.A minimal fix is to treat a segment as recoverable when
mic_fragments(orsystem_audio_fragments) are present even with no display, and relax theRecoverableSegmenttype sodisplay_fragmentsbecomesOption<Vec<PathBuf>>for audio-only paths.Prompt To Fix With AI
crates/recording/src/studio_recording.rs, line 1415-1498 (link)create_segment_pipelinecheckscamera_onlyand, when false, falls through to theelsebranch that callstarget_to_display_and_crop(line 1497). That function now returnsErr("Camera-only target has no display")forAudioOnly. On macOS the else branch also calls.ok_or_else(|| anyhow!("Missing shareable content"))?(line 1523), which fails becauseshareable_contentisNoneforAudioOnly. As a result, any attempt to start an audio-only studio recording returns an error before a single byte of audio is captured.The
audio_onlyflag defined at line 1410 is used to skip camera and cursor setup but never applied to the screen pipeline branch. Anelse if audio_only { (None, None, None) }arm — parallel to the existingif camera_onlyarm — is required to skip screen capture entirely forAudioOnlytargets.Prompt To Fix With AI
crates/recording/src/instant_recording.rs, line 610-641 (link)AudioOnlyfalls into the screen-capture_arm — instant recording also failsspawn_instant_recording_actormatchesCameraOnlyexplicitly and handles everything else with_ =>. ForAudioOnly, the_ =>arm callstarget_to_display_and_crop(line 614), which returnsErr("Camera-only target has no display"). On macOS the arm also calls.ok_or_else(|| anyhow::anyhow!("Missing shareable content"))?(line 635) against aNonevalue, so the actor fails to build and returns an error before any audio pipeline is created.AudioOnlyneeds its own match arm that sets up an audio-only pipeline (mic + system audio) and setsvideo_info: None, analogous to theCameraOnlyarm.Prompt To Fix With AI
crates/editor/src/editor_instance.rs, line 243-246 (link)ProjectRecordingsMeta::newfails unconditionally for audio-only recordingsEditorInstance::createcallsProjectRecordingsMeta::newwithout any audio-only guard. For audio-only recordings the function returnsErr("SingleSegment/MultipleSegment missing display")(becauseSegmentRecordings.displayis still non-optional in the rendering crate) and the?propagates that error up, causing the editor window to fail to open.recording.rsadded ahas_displayguard before the same call, but this site was not updated. Any recording that reaches the editor path — for example if a user opens an audio-only.capfrom the recordings overlay — will produce an error dialog instead of an editor.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "fix(audio-only): wire up recovery displa..." | Re-trigger Greptile