feat(recording): add AudioOnly capture target and recording pipeline …#1881
feat(recording): add AudioOnly capture target and recording pipeline …#1881ManthanNimodiya wants to merge 6 commits into
Conversation
…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. |
| 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.
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 adds the
AudioOnlyvariant toScreenCaptureTargetand wires it through the instant and studio recording pipelines, skipping screen/camera capture and building audio-only output. It also makesdisplay: Option<VideoMeta>in segment structs with serde defaults for backward compatibility, addsaudio_only: booltoRecordingMeta, and addsCurrentRecordingTarget::Audiofor the Tauri state layer.ScreenCaptureTarget::AudioOnlyis added and propagated through all match sites (telemetry, screenshot, capture pipeline, shareable content acquisition).displayis madeOption<VideoMeta>inSingleSegmentandMultipleSegmentswith backward-compatible serde defaults; all callers updated withas_ref().map(...).unwrap_or_default()fallbacks.Pipeline::screenis madeOption<OutputPipeline>in the studio pipeline, but the cancel-guard task inspawn_watcherimmediately cancels audio tracks whenscreenis absent.Confidence Score: 3/5
Not safe to merge without addressing the cancel-guard regression in the studio pipeline and the several audio-only finalization failures flagged across review rounds.
The cancel-guard task in
Pipeline::spawn_watcherimmediately cancels the microphone pipeline for every audio-only studio recording, making studio audio-only recordings non-functional. Combined with still-unresolved issues from earlier rounds —ProjectRecordingsMeta::newreturningErrfor audio-only (blocking finalization),audio_only: falsehardcoded inpersist_final_recording_meta, andAudioOnlyincorrectly opening the camera window — the audio-only path is not end-to-end functional.crates/recording/src/studio_recording.rs (cancel-guard in
spawn_watcher), crates/rendering/src/project_recordings.rs (SegmentRecordings.displaystill non-optional), apps/desktop/src-tauri/src/recording.rs (camera window triggered for AudioOnly,audio_only: falsehardcoded in meta writers)Important Files Changed
screenoptional throughout the studio pipeline to support audio-only mode; introduces a P1 bug where the cancel-guard task inspawn_watcherimmediately cancels mic/audio pipelines whenscreenisNone.Errreturns for audio-only, butSegmentRecordings.displayis stillVideo(non-optional), soProjectRecordingsMeta::newpropagated errors still prevent audio-only finalization.AudioOnlythrough recording start/finish/finalize paths;unwrap_or_default()on missing display paths is safe but firescreate_screenshotagainst the recording directory for audio-only.AudioOnlyvariant toScreenCaptureTargetwith correctNonereturns for display, area, rect, and name lookups.DashSegmentedAudioMuxer;video_infomade optional and handled correctly at stop time.displayoptional inSingleSegmentandMultipleSegmentwith backward-compatible serde defaults;min_fps/max_fpsupdated withunwrap_or(0)fallbacks.CurrentRecordingTarget::Audiovariant and mapsScreenCaptureTarget::AudioOnlyto it correctly.displayfield accesses to useOption; addsaudio_only: falseto all imported recording metas.screen_fpsanddisplay_start_offsetaccesses to useOption; falls back to 0 for audio-only.Comments Outside Diff (4)
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
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "fix(recording): skip audio-only segments..." | Re-trigger Greptile