feat: record encoding #14188
feat: record encoding #14188
Conversation
github-actions
bot
added
app:electron
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce native-module-driven audio recording via Opus encoding in Ogg containers, replacing in-process Opus handling. A new recording state machine orchestrates lifecycle management, native audio capture integrates through an AudioCallback abstraction, and public APIs expose startRecording/stopRecording functions. Opus streaming encoder is removed from the frontend layer. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Electron Renderer
participant Feature as Recording Feature
participant StateMachine as State Machine
participant Native as Native Module
participant Capture as Audio Capture (macOS/Windows)
participant Encoder as Opus Encoder (Worker)
participant FileSystem as File System
rect rgb(100, 150, 200)
note over Renderer,Encoder: Start Recording Flow
Renderer->>Feature: startRecording(appGroup)
Feature->>StateMachine: dispatch CREATE_BLOCK
StateMachine-->>Feature: RecordingStatus (id, pending)
Feature->>Native: startRecording(opts)
Native->>Capture: create capture session with AudioCallback
Capture-->>Native: RecordingSessionMeta (id, filepath, startedAt)
Native-->>Feature: RecordingSessionMeta
Feature->>StateMachine: dispatch ATTACH_NATIVE_RECORDING
StateMachine-->>Feature: RecordingStatus (with nativeId, filepath, sampleRate)
Feature-->>Renderer: Promise<RecordingStatus>
end
rect rgb(150, 180, 220)
note over Capture,FileSystem: Recording & Encoding Flow
Capture->>Capture: capture audio frames
Capture->>Encoder: AudioCallback.call(samples) via channel
Encoder->>Encoder: encode frame (20ms Opus)
Encoder->>Encoder: track granule position
Encoder->>FileSystem: write Opus frames to Ogg file
end
rect rgb(100, 150, 200)
note over Renderer,FileSystem: Stop Recording Flow
Renderer->>Feature: stopRecording(id)
Feature->>Native: stopRecording(id)
Native->>Capture: stop capture session
Capture-->>Native: (capture halted)
Native->>Encoder: signal end-of-stream
Encoder->>Encoder: finalize Ogg (duration, size)
Encoder->>FileSystem: close file
FileSystem-->>Native: RecordingArtifact (filepath, size, durationMs)
Native-->>Feature: RecordingArtifact
Feature->>StateMachine: dispatch SAVE_RECORDING
StateMachine-->>Feature: RecordingStatus (saved, with metadata)
Feature-->>Renderer: Promise<SerializedRecordingStatus>
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
packages/frontend/native/media_capture/src/audio_callback.rs (1)
15-29: Consider implications of silently dropping audio samples.The
Channelvariant usestry_send, which drops samples when the channel is full. While this prevents blocking the audio thread, it could lead to gaps in recordings without any indication to the caller.Consider:
- Adding a counter or metric to track dropped samples for observability
- Using a bounded channel with appropriate capacity based on expected latency
- Documenting the expected consumer behavior to prevent buffer overruns
packages/frontend/native/nbstore/src/indexer_sync.rs (1)
55-111: Tests cover basic functionality.The unit tests validate the core CRUD operations for indexed clocks. Consider adding tests for:
- Updating an existing indexed clock (upsert behavior)
- Handling non-existent doc_ids in get/clear operations
- Concurrent updates if that's a concern
packages/frontend/native/media_capture/src/recording.rs (3)
104-168: Potential data loss in resampler warm-up phase.The resampler discards the first output block (lines 153-156) during the "warm-up" phase. While this is common for some resampler algorithms to avoid initial transients, it may cause a small amount of audio data loss at the start of recordings. Consider whether this is acceptable for your use case or if the warm-up samples should be handled differently.
384-385: Global state mutex could cause deadlock if held across async boundaries.The
ACTIVE_RECORDINGSmutex is locked duringstart_recordingandstop_recording. If the lock is held while calling blocking operations, it could cause contention. The current implementation appears safe since the lock is released before spawning the worker thread, but be cautious about future modifications.
434-478: Unreachable code warning on unsupported platforms.The
#[cfg(not(any(target_os = "macos", target_os = "windows")))]block (lines 472-477) returns an error but thelet _ = opts; let _ = tx;statements are needed to suppress unused variable warnings. This is fine but consider using#[allow(unused_variables)]for clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
Cargo.tomlpackages/common/nbstore/src/impls/sqlite/db.tspackages/common/nbstore/src/impls/sqlite/index.tspackages/common/nbstore/src/impls/sqlite/indexer-sync.tspackages/common/nbstore/src/storage/index.tspackages/frontend/apps/android/src/plugins/nbstore/definitions.tspackages/frontend/apps/android/src/plugins/nbstore/index.tspackages/frontend/apps/electron-renderer/src/background-worker/index.tspackages/frontend/apps/electron-renderer/src/popup/recording/index.tsxpackages/frontend/apps/electron/src/helper/nbstore/handlers.tspackages/frontend/apps/electron/src/main/protocol.tspackages/frontend/apps/electron/src/main/recording/feature.tspackages/frontend/apps/electron/src/main/recording/index.tspackages/frontend/apps/electron/src/main/recording/state-machine.tspackages/frontend/apps/electron/src/main/recording/types.tspackages/frontend/apps/ios/src/plugins/nbstore/definitions.tspackages/frontend/apps/ios/src/plugins/nbstore/index.tspackages/frontend/core/src/modules/workspace-engine/impls/cloud.tspackages/frontend/core/src/modules/workspace-engine/impls/local.tspackages/frontend/core/src/utils/opus-encoding.tspackages/frontend/native/index.d.tspackages/frontend/native/index.jspackages/frontend/native/media_capture/Cargo.tomlpackages/frontend/native/media_capture/src/audio_callback.rspackages/frontend/native/media_capture/src/lib.rspackages/frontend/native/media_capture/src/macos/screen_capture_kit.rspackages/frontend/native/media_capture/src/macos/tap_audio.rspackages/frontend/native/media_capture/src/recording.rspackages/frontend/native/media_capture/src/windows/audio_capture.rspackages/frontend/native/media_capture/src/windows/screen_capture_kit.rspackages/frontend/native/nbstore/src/doc.rspackages/frontend/native/nbstore/src/indexer_sync.rspackages/frontend/native/nbstore/src/lib.rspackages/frontend/native/schema/src/lib.rs
💤 Files with no reviewable changes (3)
- packages/frontend/core/src/utils/opus-encoding.ts
- packages/frontend/apps/electron/src/main/recording/index.ts
- packages/frontend/apps/electron-renderer/src/background-worker/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/frontend/apps/ios/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/frontend/apps/ios/AGENTS.md)
packages/frontend/apps/ios/**/*.{ts,tsx}: Use TypeScript as the programming language
Expose JavaScript APIs to native iOS code through window object for Capacitor integration
Implement getCurrentServerBaseUrl() API for native iOS bridge
Implement getCurrentI18nLocale() API for native iOS bridge
Implement getAiButtonFeatureFlag() API for native iOS bridge
Implement getCurrentWorkspaceId() API for native iOS bridge
Implement getCurrentDocId() API for native iOS bridge
Implement getCurrentDocContentInMarkdown() API for native iOS bridge
Implement createNewDocByMarkdownInCurrentWorkspace() API for native iOS bridge
Enable TypeScript strict mode
Files:
packages/frontend/apps/ios/src/plugins/nbstore/definitions.tspackages/frontend/apps/ios/src/plugins/nbstore/index.ts
packages/frontend/apps/ios/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/frontend/apps/ios/AGENTS.md)
Follow ESLint/Prettier configuration from workspace root
Files:
packages/frontend/apps/ios/src/plugins/nbstore/definitions.tspackages/frontend/apps/ios/src/plugins/nbstore/index.ts
🧠 Learnings (9)
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement getCurrentWorkspaceId() API for native iOS bridge
Applied to files:
packages/frontend/core/src/modules/workspace-engine/impls/cloud.tspackages/frontend/core/src/modules/workspace-engine/impls/local.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement getCurrentDocId() API for native iOS bridge
Applied to files:
packages/frontend/apps/ios/src/plugins/nbstore/definitions.tspackages/frontend/apps/android/src/plugins/nbstore/index.tspackages/common/nbstore/src/impls/sqlite/db.tspackages/frontend/apps/ios/src/plugins/nbstore/index.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement createNewDocByMarkdownInCurrentWorkspace() API for native iOS bridge
Applied to files:
packages/frontend/apps/ios/src/plugins/nbstore/definitions.tspackages/frontend/apps/ios/src/plugins/nbstore/index.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement getCurrentDocContentInMarkdown() API for native iOS bridge
Applied to files:
packages/frontend/apps/ios/src/plugins/nbstore/definitions.tspackages/frontend/apps/electron/src/main/recording/feature.tspackages/frontend/apps/ios/src/plugins/nbstore/index.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Expose JavaScript APIs to native iOS code through window object for Capacitor integration
Applied to files:
packages/frontend/apps/ios/src/plugins/nbstore/definitions.tspackages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement getCurrentI18nLocale() API for native iOS bridge
Applied to files:
packages/frontend/apps/ios/src/plugins/nbstore/definitions.tspackages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement getAiButtonFeatureFlag() API for native iOS bridge
Applied to files:
packages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Use TypeScript as the programming language
Applied to files:
packages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Enable TypeScript strict mode
Applied to files:
packages/frontend/apps/electron/src/main/recording/types.ts
🧬 Code graph analysis (17)
packages/frontend/native/index.js (1)
packages/common/y-octo/node/index.js (1)
nativeBinding(7-7)
packages/frontend/core/src/modules/workspace-engine/impls/cloud.ts (1)
packages/common/nbstore/src/impls/sqlite/indexer-sync.ts (1)
SqliteIndexerSyncStorage(8-38)
packages/common/nbstore/src/impls/sqlite/indexer-sync.ts (2)
packages/common/nbstore/src/impls/sqlite/db.ts (2)
NativeDBConnection(146-201)SqliteNativeDBOptions(12-16)packages/frontend/native/index.d.ts (1)
DocIndexedClock(141-145)
packages/frontend/apps/ios/src/plugins/nbstore/definitions.ts (1)
packages/frontend/native/index.d.ts (1)
DocIndexedClock(141-145)
packages/common/nbstore/src/impls/sqlite/index.ts (1)
packages/common/nbstore/src/impls/sqlite/indexer-sync.ts (1)
SqliteIndexerSyncStorage(8-38)
packages/frontend/core/src/modules/workspace-engine/impls/local.ts (1)
packages/common/nbstore/src/impls/sqlite/indexer-sync.ts (1)
SqliteIndexerSyncStorage(8-38)
packages/frontend/native/index.d.ts (1)
packages/frontend/apps/electron/src/main/recording/feature.ts (2)
startRecording(474-516)stopRecording(526-583)
packages/frontend/native/nbstore/src/indexer_sync.rs (2)
packages/frontend/native/nbstore/src/lib.rs (4)
get_doc_indexed_clock(248-260)clear_doc_indexed_clock(279-286)new(101-105)new(589-593)packages/frontend/native/nbstore/src/doc.rs (1)
get_storage(265-270)
packages/frontend/native/nbstore/src/lib.rs (2)
packages/frontend/native/index.d.ts (1)
DocIndexedClock(141-145)packages/frontend/native/nbstore/src/indexer_sync.rs (3)
get_doc_indexed_clock(6-20)set_doc_indexed_clock(22-43)clear_doc_indexed_clock(45-52)
packages/frontend/apps/android/src/plugins/nbstore/index.ts (2)
packages/common/nbstore/src/storage/doc.ts (1)
CrawlResult(22-26)packages/frontend/native/index.d.ts (1)
DocIndexedClock(141-145)
packages/frontend/native/media_capture/src/windows/screen_capture_kit.rs (4)
packages/frontend/native/media_capture/src/macos/screen_capture_kit.rs (5)
tap_audio_with_callback(681-709)tap_audio(712-720)new(104-110)new(529-533)tap_global_audio(740-748)packages/frontend/native/media_capture/src/windows/audio_capture.rs (2)
start_recording(220-395)new(32-49)packages/frontend/native/media_capture/src/macos/tap_audio.rs (3)
new(69-116)new(580-593)new(955-961)packages/frontend/native/media_capture/src/windows/microphone_listener.rs (2)
new(202-219)new(363-393)
packages/common/nbstore/src/impls/sqlite/db.ts (1)
packages/frontend/native/index.d.ts (1)
DocIndexedClock(141-145)
packages/frontend/apps/android/src/plugins/nbstore/definitions.ts (1)
packages/frontend/native/index.d.ts (1)
DocIndexedClock(141-145)
packages/frontend/native/media_capture/src/macos/tap_audio.rs (1)
packages/frontend/native/media_capture/src/recording.rs (1)
start_capture(434-478)
packages/frontend/apps/electron-renderer/src/popup/recording/index.tsx (2)
packages/frontend/apps/electron/src/preload/electron-api.ts (1)
apis(278-281)packages/frontend/apps/electron/src/main/recording/state-machine.ts (1)
status(61-63)
packages/frontend/apps/ios/src/plugins/nbstore/index.ts (1)
packages/frontend/native/index.d.ts (1)
DocIndexedClock(141-145)
packages/frontend/native/media_capture/src/macos/screen_capture_kit.rs (2)
packages/frontend/native/media_capture/src/windows/screen_capture_kit.rs (5)
tap_audio_with_callback(235-242)process_id(130-132)tap_audio(245-256)new(66-72)new(192-203)packages/frontend/native/media_capture/src/macos/tap_audio.rs (3)
new(69-116)new(580-593)new(955-961)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: y-octo binding test on x86_64-apple-darwin
- GitHub Check: E2E Test (8)
- GitHub Check: E2E Test (10)
- GitHub Check: Build AFFiNE native (aarch64-apple-darwin)
- GitHub Check: E2E Test (2)
- GitHub Check: Build AFFiNE native (x86_64-apple-darwin)
- GitHub Check: E2E Test (1)
- GitHub Check: E2E Test (5)
- GitHub Check: E2E Test (6)
- GitHub Check: Build AFFiNE native (aarch64-pc-windows-msvc)
- GitHub Check: Build AFFiNE native (x86_64-pc-windows-msvc)
- GitHub Check: E2E Mobile Test (4)
- GitHub Check: E2E Mobile Test (5)
- GitHub Check: miri code check
- GitHub Check: E2E BlockSuite Cross Browser Test (1, chromium)
- GitHub Check: E2E BlockSuite Cross Browser Test (1, firefox)
- GitHub Check: Lint
- GitHub Check: Analyze (typescript, affine)
- GitHub Check: Analyze (javascript, affine)
- GitHub Check: Typecheck
🔇 Additional comments (55)
packages/frontend/apps/electron-renderer/src/popup/recording/index.tsx (1)
139-139: LGTM!The empty dependency array is correct here since the effect only needs to run once on mount to set up the recording status subscription. The imported
apisandeventsare stable module-level imports, and all other variables are defined within the effect's scope.packages/frontend/native/media_capture/Cargo.toml (3)
14-14: Good refactoring: crossbeam-channel is now cross-platform.Moving
crossbeam-channelfrom Windows-specific dependencies to global dependencies indicates it's now used across platforms, which aligns with the broader audio recording capabilities introduced in this PR.Also applies to: 19-19
17-17: LGTM: ogg dependency addition.The
oggworkspace dependency correctly supports the Ogg container format for the new encoding functionality.
18-18: No action needed—opus-codec 0.1.2 is the latest stable version with no known security vulnerabilities.The dependency is already pinned to the latest release on crates.io and has no published RustSec/GHSA advisories. The version is secure and current.
Cargo.toml (1)
67-67: Version 0.9 is current with no known security vulnerabilities.The ogg crate latest stable version is 0.9.2, so the
0.9specification will resolve correctly. No security advisories are present.packages/frontend/native/nbstore/src/doc.rs (2)
44-48: LGTM! Consistent with existing update pattern.The indexer_sync table update correctly follows the established pattern for maintaining referential integrity when the space_id changes. The placement between clocks and peer_clocks updates is appropriate.
215-218: LGTM! Proper cleanup within transaction.The indexer_sync deletion is correctly included in the delete_doc transaction, ensuring atomic cleanup of all related records.
packages/frontend/native/nbstore/src/lib.rs (3)
7-7: LGTM! Module and struct follow existing patterns.The new
indexer_syncmodule andDocIndexedClockstruct are well-defined and consistent with the codebase conventions. The#[napi(object)]attribute correctly exposes the struct to JavaScript.Also applies to: 59-65
247-276: LGTM! Methods follow established patterns.The
get_doc_indexed_clockandset_doc_indexed_clockmethods correctly delegate to the storage layer and maintain consistency with the existing API surface.
278-286: LGTM! Clear method completes the API surface.The
clear_doc_indexed_clockmethod provides the necessary deletion functionality and maintains consistency with the rest of the codebase.packages/frontend/native/nbstore/src/indexer_sync.rs (3)
6-20: LGTM! Safe and type-safe query implementation.The
get_doc_indexed_clockmethod correctly usessqlx::query!with proper type annotations and safely handles optional results.
22-43: LGTM! Correct upsert implementation.The
set_doc_indexed_clockmethod properly implements an upsert pattern usingON CONFLICT, ensuring idempotent updates to the indexed clock state.
45-52: LGTM! Clean delete implementation.The
clear_doc_indexed_clockmethod correctly deletes the indexed clock record for the specified document.packages/common/nbstore/src/storage/index.ts (1)
95-95: LGTM! Consistent with existing exports.The indexer-sync export follows the established pattern and correctly expands the public API surface.
packages/frontend/native/media_capture/src/lib.rs (1)
11-13: LGTM! Module declarations follow Rust conventions.The new
audio_callbackandrecordingmodules are correctly declared as public, expanding the library's API surface appropriately.packages/frontend/native/schema/src/lib.rs (1)
89-100: LGTM! Migration adds indexer_sync table.The new migration correctly creates the
indexer_synctable with appropriate schema. Note that this migration is irreversible (no down migration), which is acceptable for a new table that doesn't affect existing data.packages/frontend/apps/electron/src/main/protocol.ts (3)
130-135: LGTM! Domain patterns correctly match affine domains.The
affineDomainsregex patterns properly match all affine-related domains including subdomains, using case-insensitive matching.
231-246: LGTM! Logic correctly applies CORS for affine domains.The updated
onHeadersReceivedhandler correctly identifies requests to affine domains and applies CORS headers. The logic properly maintains separation between assets protocol handling and HTTP/HTTPS affine domain handling.
175-184: TheAccess-Control-Allow-Origin: *is scoped to affine-controlled domains only (lines 241-245 restrictallowCorscalls to responses matchingaffineDomainsregex), so it doesn't expose resources to arbitrary origins. This wildcard policy is intentional for supporting signed blob redirects to*.usercontent.affine.prowithin the Electron app's protocol handler. The backend confirms blob URLs are cryptographically signed and time-limited (1-hour expiration), mitigating exposure risk. No action needed.packages/frontend/native/index.js (1)
582-583: LGTM!The new
startRecordingandstopRecordingexports follow the established pattern for exposing native bindings. This is auto-generated code by NAPI-RS, and the additions are consistent with the existing export structure.packages/frontend/apps/electron/src/helper/nbstore/handlers.ts (1)
33-35: LGTM!The new
getDocIndexedClock,setDocIndexedClock, andclearDocIndexedClockhandlers follow the established binding pattern and are correctly bound to thePOOLinstance.packages/common/nbstore/src/impls/sqlite/index.ts (1)
7-7: LGTM!The
SqliteIndexerSyncStorageis properly imported, exported, and registered in thesqliteStoragesarray, following the established pattern for other storage types in this module.Also applies to: 15-15, 23-23
packages/frontend/apps/electron/src/main/recording/types.ts (1)
43-45: LGTM!The new optional fields (
nativeId,sampleRate,numberOfChannels) onRecordingStatusappropriately support the refactored state-machine-driven recording architecture. These fields are correctly marked optional since they're populated during different recording lifecycle stages (e.g., afterATTACH_NATIVE_RECORDINGandSAVE_RECORDINGactions).packages/common/nbstore/src/impls/sqlite/db.ts (1)
33-43: LGTM!The new
getDocIndexedClock,setDocIndexedClock, andclearDocIndexedClockmethods extend theNativeDBApisinterface consistently with the existing clock-related methods. The signatures properly useDatefor timestamps and align with theDocIndexedClocktype structure.packages/frontend/native/index.d.ts (2)
44-73: LGTM!The new recording interfaces (
RecordingArtifact,RecordingSessionMeta,RecordingStartOptions) and function declarations (startRecording,stopRecording) are well-structured. The synchronous signatures are appropriate for native bindings that return immediately with session metadata or artifacts.
99-101: LGTM!The
DocIndexedClockinterface and correspondingDocStoragePoolmethods are properly declared. The async Promise-based signatures for pool operations are consistent with the existing clock-related methods in the class.Also applies to: 141-145
packages/frontend/apps/ios/src/plugins/nbstore/definitions.ts (2)
1-1: LGTM!The import of
DocIndexedClockfrom@affine/nbstorealigns the iOS plugin with the shared type definitions.
190-203: LGTM!The new
getDocIndexedClock,setDocIndexedClock, andclearDocIndexedClockmethods extend theNbStorePlugininterface consistently with the existing API patterns. The use ofindexedClock: number(instead ofDate) for the timestamp parameter is consistent with how other methods in this file handle timestamps for the Capacitor bridge serialization.packages/frontend/native/media_capture/src/windows/audio_capture.rs (2)
220-223: LGTM!The refactored
start_recordingfunction correctly accepts the newAudioCallbackabstraction and optionalSampleRate. The target rate selection logic properly defaults to the minimum of microphone and loopback sample rates when not specified.
346-349: LGTM!The callback invocation is correctly updated to use the unified
AudioCallback::callmethod, aligning with the cross-platform callback abstraction.packages/frontend/apps/electron/src/main/recording/state-machine.ts (3)
16-24: LGTM!The new
ATTACH_NATIVE_RECORDINGevent is well-structured with all necessary fields for native recording metadata attachment.
181-208: LGTM!The
handleAttachNativeRecordingmethod correctly validates both the recording ID and status before attaching native metadata. The error logging for invalid states is appropriate for debugging.
282-301: LGTM!The
handleSaveRecordingmethod is correctly updated to accept and propagate optionalsampleRateandnumberOfChannelsparameters.packages/frontend/apps/ios/src/plugins/nbstore/index.ts (1)
425-443: LGTM!The
setDocIndexedClockcorrectly convertsDateto timestamp viagetTime(), andclearDocIndexedClockproperly delegates to the native store.packages/frontend/apps/android/src/plugins/nbstore/index.ts (1)
341-346: LGTM!The
crawlDocDataexplicit typing,setDocIndexedClockDate-to-timestamp conversion, andclearDocIndexedClockdelegation are correctly implemented.Also applies to: 425-443
packages/common/nbstore/src/impls/sqlite/indexer-sync.ts (1)
1-38: LGTM!The
SqliteIndexerSyncStorageimplementation correctly extendsIndexerSyncStorageBaseand follows the established patterns of other SQLite storage implementations:
- Shared connection via
share()utility- Static identifier for storage registration
- Methods properly delegate to the wrapped native DB APIs
packages/frontend/apps/android/src/plugins/nbstore/definitions.ts (1)
1-1: LGTM!The
NbStorePlugininterface extensions correctly define the native bridge contract:
DocIndexedClocktype import for return type specification- The
ftsGetMatchesreturn type syntax change is a minor cleanup (...[]vsArray<...>)- New methods correctly use
numberforindexedClockparameter to match native bridge serializationNote: The
DocIndexedClockreturn type ingetDocIndexedClockexpects aDatefortimestamp, but the native bridge will return a number. The transformation should occur inindex.ts(see related comment on that file).Also applies to: 187-187, 190-203
packages/frontend/native/media_capture/src/windows/screen_capture_kit.rs (2)
235-256: LGTM!The
tap_audio_with_callbackvariant correctly:
- Converts optional
u32toOption<SampleRate>- Delegates to the unified
start_recordingfunctionThe existing
tap_audiois cleanly refactored to wrap the callback inAudioCallback::Jsand delegate to the new method.
258-279: LGTM!The
tap_global_audio_with_callbackfollows the same pattern and correctly delegates tostart_recording. The refactor aligns Windows behavior with the macOS callback-based architecture.packages/frontend/native/media_capture/src/macos/screen_capture_kit.rs (3)
681-709: LGTM - Clean internal API for callback abstraction.The internal
tap_audio_with_callbackmethod properly accepts theAudioCallbackabstraction, allowing both JavaScript and channel-based callbacks. This aligns well with the Windows implementation pattern shown in the relevant snippets.
711-720: LGTM - Public API preserved with clean delegation.The public N-API methods (
tap_audioandtap_global_audio) correctly wrap theThreadsafeFunctioninAudioCallback::Jsbefore delegating to the internal callback methods. This maintains backward compatibility while enabling the new native recording flow.Also applies to: 739-748
722-737: LGTM - Global audio tap refactored consistently.The
tap_global_audio_with_callbackmethod follows the same pattern astap_audio_with_callback, providing a unified internal API for both single-app and global audio capture scenarios.packages/frontend/native/media_capture/src/recording.rs (4)
559-581: LGTM - Stop recording handles cleanup correctly.The
stop_recordingfunction properly:
- Acquires the lock and removes the entry atomically
- Stops the capture
- Drops the sender to signal EOF to the worker
- Joins the worker thread to get the artifact
170-230: LGTM - OggOpusWriter implementation is solid.The encoder setup correctly:
- Handles mono/stereo channels
- Creates resampler when source rate differs from 48kHz encode rate
- Writes proper Opus headers with vendor string
- Uses 20ms frame size (standard for Opus)
279-320: The end-of-stream implementation is spec-compliant with RFC 7845.The empty packet with
EndStreamflag properly marks the end of the logical Ogg/Opus stream per RFC 7845, which states the stream SHOULD end with a page marked with the "end of stream" flag. The granule position is correctly passed through, enabling proper end-trimming signaling. This approach is standard and will be correctly handled by spec-compliant Ogg/Opus players.
356-376: Verifyunsafe impl Sendis sound for PlatformCapture.The
unsafe impl Send for PlatformCapture(line 363) asserts thread-safety. Ensure that bothAudioCaptureSessiontypes (macOS and Windows) are actually safe to send between threads.#!/bin/bash # Check if AudioCaptureSession types implement Send or contain non-Send types rg -n "pub struct AudioCaptureSession" packages/frontend/native/media_capture/src/ rg -n "Arc<.*Mutex" packages/frontend/native/media_capture/src/macos/tap_audio.rs packages/frontend/native/media_capture/src/windows/audio_capture.rspackages/frontend/native/media_capture/src/macos/tap_audio.rs (3)
240-306: LGTM - AudioCallback integration in audio processing loop.The
startmethod correctly uses the newAudioCallback::call(mixed_samples)API. The callback is cloned into theRcBlockclosure, which is appropriate for the callback's use in the audio I/O block.
611-652: LGTM - start_capture properly stores and uses AudioCallback.The method correctly:
- Stores the callback for potential device switch handling (line 614)
- Clones the callback when starting the initial stream (line 633)
- Uses the stored callback reference in device change listeners
573-576: LGTM - Field type update is consistent.The
audio_callbackfield type change fromOption<Arc<ThreadsafeFunction<...>>>toOption<AudioCallback>aligns with the broader refactoring effort.packages/frontend/apps/electron/src/main/recording/feature.ts (6)
69-73: LGTM - Centralized native module access.The
getNativeModule()helper provides a single point for dynamic require of@affine/native, avoiding load on unsupported platforms while maintaining type safety.
75-95: LGTM - Improved cleanup with proper error handling.The cleanup function now:
- Properly iterates both
appStateSubscribersandsubscribers- Catches and ignores unsubscribe errors
- Resets arrays correctly (
subscribers.length = 0and reassignment forappStateSubscribers)- Clears the observables
526-583: stopRecording should handle case where native stop fails but state already transitioned.At line 538-541, the state machine is dispatched with
STOP_RECORDINGbefore calling the nativestopRecording. If the native call fails (line 570), the state has already transitioned to "stopped" but no artifact was saved. The error handling dispatchesCREATE_BLOCK_FAILED, which is appropriate, but consider whether the early state transition is intentional.
559-567: Async window focus should not block recording stop.The
getMainWindow().then(...)call is fire-and-forget, which is correct for not blocking the return. However, errors are only logged. This is acceptable behavior.
585-594: Good path traversal protection.The
assertRecordingFilepathfunction properly validates that the filepath is withinSAVED_RECORDINGS_DIRusing normalized path comparison, preventing path traversal attacks.
257-272: getRecording now reads from state machine - verify callers expect this behavior.The function now returns data from
recordingStateMachine.statusinstead of a Map. Callers should be aware that only the current active recording can be retrieved, not historical ones.
| getDocIndexedClock: function ( | ||
| id: string, | ||
| docId: string | ||
| ): Promise<DocIndexedClock | null> { | ||
| return NbStore.getDocIndexedClock({ id, docId }); | ||
| }, |
There was a problem hiding this comment.
Missing timestamp conversion for getDocIndexedClock return value.
Same issue as the iOS implementation: the native response is returned directly without converting the timestamp field from a number to a Date object, as required by the DocIndexedClock interface.
🔎 Proposed fix
getDocIndexedClock: function (
id: string,
docId: string
): Promise<DocIndexedClock | null> {
- return NbStore.getDocIndexedClock({ id, docId });
+ return NbStore.getDocIndexedClock({ id, docId }).then(clock =>
+ clock
+ ? {
+ docId: clock.docId,
+ timestamp: new Date(clock.timestamp),
+ indexerVersion: clock.indexerVersion,
+ }
+ : null
+ );
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getDocIndexedClock: function ( | |
| id: string, | |
| docId: string | |
| ): Promise<DocIndexedClock | null> { | |
| return NbStore.getDocIndexedClock({ id, docId }); | |
| }, | |
| getDocIndexedClock: function ( | |
| id: string, | |
| docId: string | |
| ): Promise<DocIndexedClock | null> { | |
| return NbStore.getDocIndexedClock({ id, docId }).then(clock => | |
| clock | |
| ? { | |
| docId: clock.docId, | |
| timestamp: new Date(clock.timestamp), | |
| indexerVersion: clock.indexerVersion, | |
| } | |
| : null | |
| ); | |
| }, |
🤖 Prompt for AI Agents
In packages/frontend/apps/android/src/plugins/nbstore/index.ts around lines 419
to 424, the native result from NbStore.getDocIndexedClock is returned directly
without converting the numeric timestamp into a Date as required by the
DocIndexedClock interface; change the function to await the native call, check
for null, and return the object with the timestamp field converted via new
Date(numericTimestamp) (preserving other fields) so callers receive a Date
instance.
| apis?.recording | ||
| .getCurrentRecording() | ||
| .then(status => { | ||
| if (status) { | ||
| return handleRecordingStatusChanged(status); | ||
| } | ||
| return; | ||
| }) | ||
| .catch(console.error); | ||
|
|
There was a problem hiding this comment.
Duplicate getCurrentRecording call may cause race condition.
The getCurrentRecording API is called twice on mount: once in the useRecordingStatus hook (line 34) and again here (line 118). This creates redundant API calls and potential race conditions since both execute concurrently.
The useRecordingStatus hook already handles the initial status by calling getCurrentRecording and setting the status state. The tracking logic in handleRecordingStatusChanged (which only processes 'new' status) could instead be triggered by the status change event subscription (line 129) or moved into useRecordingStatus.
🔎 Suggested refactor to remove duplicate call
Consider removing the duplicate getCurrentRecording call and relying on the useRecordingStatus hook and event subscription:
- apis?.recording
- .getCurrentRecording()
- .then(status => {
- if (status) {
- return handleRecordingStatusChanged(status);
- }
- return;
- })
- .catch(console.error);
-
// allow processing stopped event in tray menu as well:
const unsubscribe = events?.recording.onRecordingStatusChanged(status => {
if (status) {
handleRecordingStatusChanged(status).catch(console.error);
}
});Alternatively, if initial tracking is critical, move the tracking logic into useRecordingStatus or trigger it when status changes using a separate useEffect that depends on status.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| apis?.recording | |
| .getCurrentRecording() | |
| .then(status => { | |
| if (status) { | |
| return handleRecordingStatusChanged(status); | |
| } | |
| return; | |
| }) | |
| .catch(console.error); | |
| // allow processing stopped event in tray menu as well: | |
| const unsubscribe = events?.recording.onRecordingStatusChanged(status => { | |
| if (status) { | |
| handleRecordingStatusChanged(status).catch(console.error); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/frontend/apps/electron-renderer/src/popup/recording/index.tsx around
lines 118 to 127, remove the redundant apis.recording.getCurrentRecording() call
to avoid duplicate concurrent API requests and potential race conditions;
instead rely on the existing useRecordingStatus hook (which already calls
getCurrentRecording) and the event subscription to trigger
handleRecordingStatusChanged, or move the tracking logic into useRecordingStatus
(or a separate useEffect that reacts to status changes) so the initial status is
handled only once and tracking runs when the hook's status updates.
| export async function startRecording( | ||
| appGroup?: AppGroupInfo | number | ||
| ): RecordingStatus | null { | ||
| const state = recordingStateMachine.dispatch( | ||
| { | ||
| type: 'START_RECORDING', | ||
| appGroup: normalizeAppGroupInfo(appGroup), | ||
| }, | ||
| false | ||
| ); | ||
| ): Promise<RecordingStatus | null> { | ||
| const state = recordingStateMachine.dispatch({ | ||
| type: 'START_RECORDING', | ||
| appGroup: normalizeAppGroupInfo(appGroup), | ||
| }); | ||
|
|
||
| if (state?.status === 'recording') { | ||
| createRecording(state); | ||
| if (!state || state.status !== 'recording') { | ||
| return state; | ||
| } | ||
|
|
||
| recordingStateMachine.status$.next(state); | ||
| try { | ||
| fs.ensureDirSync(SAVED_RECORDINGS_DIR); | ||
|
|
||
| const meta = getNativeModule().startRecording({ | ||
| appProcessId: state.app?.processId, | ||
| outputDir: SAVED_RECORDINGS_DIR, | ||
| format: 'opus', | ||
| id: String(state.id), | ||
| }); | ||
|
|
||
| const filepath = assertRecordingFilepath(meta.filepath); | ||
| const nextState = recordingStateMachine.dispatch({ | ||
| type: 'ATTACH_NATIVE_RECORDING', | ||
| id: state.id, | ||
| nativeId: meta.id, | ||
| startTime: meta.startedAt ?? state.startTime, | ||
| filepath, | ||
| sampleRate: meta.sampleRate, | ||
| numberOfChannels: meta.channels, | ||
| }); | ||
|
|
||
| return state; | ||
| return nextState; | ||
| } catch (error) { | ||
| logger.error('failed to start recording', error); | ||
| return recordingStateMachine.dispatch({ | ||
| type: 'CREATE_BLOCK_FAILED', | ||
| id: state.id, | ||
| error: error instanceof Error ? error : undefined, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Error in startRecording leaves capture running on ATTACH_NATIVE_RECORDING failure.
If startRecording succeeds at line 489 but the ATTACH_NATIVE_RECORDING dispatch fails or the state machine rejects it, the native recording continues running without being tracked. Consider stopping the native recording if attachment fails.
🔎 Suggested fix: Stop native recording on attachment failure
try {
fs.ensureDirSync(SAVED_RECORDINGS_DIR);
const meta = getNativeModule().startRecording({
appProcessId: state.app?.processId,
outputDir: SAVED_RECORDINGS_DIR,
format: 'opus',
id: String(state.id),
});
const filepath = assertRecordingFilepath(meta.filepath);
const nextState = recordingStateMachine.dispatch({
type: 'ATTACH_NATIVE_RECORDING',
id: state.id,
nativeId: meta.id,
startTime: meta.startedAt ?? state.startTime,
filepath,
sampleRate: meta.sampleRate,
numberOfChannels: meta.channels,
});
+ if (!nextState || nextState.status !== 'recording') {
+ // Attachment failed, stop the native recording to avoid orphaned capture
+ try {
+ getNativeModule().stopRecording(meta.id);
+ } catch (stopErr) {
+ logger.error('failed to stop orphaned native recording', stopErr);
+ }
+ }
+
return nextState;
} catch (error) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/frontend/apps/electron/src/main/recording/feature.ts around lines 474
to 516: the code can start a native recording (meta from startRecording) but if
the ATTACH_NATIVE_RECORDING dispatch fails you leave the native recorder
running; fix by capturing the meta after startRecording, then wrap the
ATTACH_NATIVE_RECORDING dispatch in a try/catch (or check its return) and on any
failure call getNativeModule().stopRecording(meta.id) (and log any stop errors)
before returning the failed state; ensure meta is in scope for the catch, and
still return the recordingStateMachine dispatch for CREATE_BLOCK_FAILED with the
original error.
| getDocIndexedClock: function ( | ||
| id: string, | ||
| docId: string | ||
| ): Promise<DocIndexedClock | null> { | ||
| return NbStore.getDocIndexedClock({ id, docId }); | ||
| }, |
There was a problem hiding this comment.
Missing timestamp conversion for getDocIndexedClock return value.
Per the DocIndexedClock interface in packages/frontend/native/index.d.ts (lines 140-144), the timestamp field should be a Date object. However, this implementation returns the native response directly without converting the timestamp from a number to a Date. Other clock methods like getDocClock (lines 112-126) perform this conversion.
🔎 Proposed fix
getDocIndexedClock: function (
id: string,
docId: string
): Promise<DocIndexedClock | null> {
- return NbStore.getDocIndexedClock({ id, docId });
+ return NbStore.getDocIndexedClock({ id, docId }).then(clock =>
+ clock
+ ? {
+ docId: clock.docId,
+ timestamp: new Date(clock.timestamp),
+ indexerVersion: clock.indexerVersion,
+ }
+ : null
+ );
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getDocIndexedClock: function ( | |
| id: string, | |
| docId: string | |
| ): Promise<DocIndexedClock | null> { | |
| return NbStore.getDocIndexedClock({ id, docId }); | |
| }, | |
| getDocIndexedClock: function ( | |
| id: string, | |
| docId: string | |
| ): Promise<DocIndexedClock | null> { | |
| return NbStore.getDocIndexedClock({ id, docId }).then(clock => | |
| clock | |
| ? { | |
| docId: clock.docId, | |
| timestamp: new Date(clock.timestamp), | |
| indexerVersion: clock.indexerVersion, | |
| } | |
| : null | |
| ); | |
| }, |
🤖 Prompt for AI Agents
In packages/frontend/apps/ios/src/plugins/nbstore/index.ts around lines 419 to
424, the getDocIndexedClock implementation returns the native object directly
without converting the numeric timestamp to a Date; update the function to await
NbStore.getDocIndexedClock, return null if the native result is null, and
otherwise return an object that spreads the native result but replaces
timestamp: new Date(native.timestamp) (keeping other fields intact) so the
function returns Promise<DocIndexedClock | null> with timestamp as a Date.
| #[napi] | ||
| pub fn start_recording(opts: RecordingStartOptions) -> Result<RecordingSessionMeta> { | ||
| if let Some(fmt) = opts.format.as_deref() { | ||
| if fmt.to_ascii_lowercase() != "opus" { | ||
| return Err(RecordingError::InvalidFormat(fmt.to_string()).into()); | ||
| } | ||
| } | ||
|
|
||
| let output_dir = validate_output_dir(&opts.output_dir)?; | ||
| let id = sanitize_id(opts.id.clone()); | ||
| let filepath = output_dir.join(format!("{id}.opus")); | ||
| if filepath.exists() { | ||
| fs::remove_file(&filepath)?; | ||
| } | ||
|
|
||
| let (tx, rx) = bounded::<Vec<f32>>(32); | ||
| let (capture, capture_rate, capture_channels) = | ||
| start_capture(&opts, tx.clone()).map_err(|e| RecordingError::Start(e.to_string()))?; | ||
|
|
||
| let encoding_channels = match opts.channels { | ||
| Some(1) => 1, | ||
| Some(2) => 2, | ||
| _ => capture_channels, | ||
| }; | ||
|
|
||
| let worker = spawn_worker( | ||
| id.clone(), | ||
| filepath.clone(), | ||
| rx, | ||
| capture_rate, | ||
| encoding_channels, | ||
| ); | ||
|
|
||
| let meta = RecordingSessionMeta { | ||
| id: id.clone(), | ||
| filepath: filepath.to_string_lossy().to_string(), | ||
| sample_rate: ENCODE_SAMPLE_RATE.as_i32() as u32, | ||
| channels: encoding_channels, | ||
| started_at: now_millis(), | ||
| }; | ||
|
|
||
| let mut recordings = ACTIVE_RECORDINGS | ||
| .lock() | ||
| .map_err(|_| RecordingError::Start("lock poisoned".into()))?; | ||
|
|
||
| if recordings.contains_key(&id) { | ||
| return Err(RecordingError::Start("duplicate recording id".into()).into()); | ||
| } | ||
|
|
||
| recordings.insert( | ||
| id, | ||
| ActiveRecording { | ||
| sender: Some(tx), | ||
| capture, | ||
| worker: Some(worker), | ||
| }, | ||
| ); | ||
|
|
||
| Ok(meta) | ||
| } |
There was a problem hiding this comment.
Recording ID collision not fully prevented.
Line 543-545 checks for duplicate recording IDs, but the ID is generated from opts.id or timestamp before acquiring the lock (line 507). In a race condition, two calls with the same user-provided ID could both pass validation before either acquires the lock. Consider generating the ID inside the critical section or using a more robust ID generation strategy.
🔎 Suggested fix: Move ID generation inside the lock
#[napi]
pub fn start_recording(opts: RecordingStartOptions) -> Result<RecordingSessionMeta> {
if let Some(fmt) = opts.format.as_deref() {
if fmt.to_ascii_lowercase() != "opus" {
return Err(RecordingError::InvalidFormat(fmt.to_string()).into());
}
}
let output_dir = validate_output_dir(&opts.output_dir)?;
- let id = sanitize_id(opts.id.clone());
- let filepath = output_dir.join(format!("{id}.opus"));
- if filepath.exists() {
- fs::remove_file(&filepath)?;
- }
let (tx, rx) = bounded::<Vec<f32>>(32);
let (capture, capture_rate, capture_channels) =
start_capture(&opts, tx.clone()).map_err(|e| RecordingError::Start(e.to_string()))?;
let encoding_channels = match opts.channels {
Some(1) => 1,
Some(2) => 2,
_ => capture_channels,
};
+ let mut recordings = ACTIVE_RECORDINGS
+ .lock()
+ .map_err(|_| RecordingError::Start("lock poisoned".into()))?;
+
+ let id = sanitize_id(opts.id.clone());
+ if recordings.contains_key(&id) {
+ return Err(RecordingError::Start("duplicate recording id".into()).into());
+ }
+
+ let filepath = output_dir.join(format!("{id}.opus"));
+ if filepath.exists() {
+ fs::remove_file(&filepath)?;
+ }
+
let worker = spawn_worker(
id.clone(),
filepath.clone(),
rx,
capture_rate,
encoding_channels,
);
let meta = RecordingSessionMeta {
id: id.clone(),
filepath: filepath.to_string_lossy().to_string(),
sample_rate: ENCODE_SAMPLE_RATE.as_i32() as u32,
channels: encoding_channels,
started_at: now_millis(),
};
- let mut recordings = ACTIVE_RECORDINGS
- .lock()
- .map_err(|_| RecordingError::Start("lock poisoned".into()))?;
-
- if recordings.contains_key(&id) {
- return Err(RecordingError::Start("duplicate recording id".into()).into());
- }
-
recordings.insert(
id,
ActiveRecording {
sender: Some(tx),
capture,
worker: Some(worker),
},
);
Ok(meta)
}🤖 Prompt for AI Agents
In packages/frontend/native/media_capture/src/recording.rs around lines 498 to
557, the recording ID and derived filepath are created before acquiring the
ACTIVE_RECORDINGS lock, allowing a race where two callers supply the same ID and
both pass the duplicate check; move ID generation/sanitization
(sanitize_id(opts.id.clone())) and any logic that depends on that ID (filepath
construction, existence check, and removal) inside the critical section that
holds ACTIVE_RECORDINGS, so you acquire the lock first, compute/sanitize the id,
check recordings.contains_key(&id) and only then create filepath/remove existing
file and insert the ActiveRecording; adjust order so start_capture and
spawn_worker occur after the insert or ensure the worker/sender are created in a
way that won’t leak on failure, and keep error handling consistent (map lock
poisoning to RecordingError::Start).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## canary #14188 +/- ##
========================================
Coverage 56.50% 56.51%
========================================
Files 2775 2775
Lines 142431 142433 +2
Branches 21696 21701 +5
========================================
+ Hits 80483 80496 +13
+ Misses 60165 59601 -564
- Partials 1783 2336 +553
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
darkskygit
force-pushed
the
darksky/native-sync-state
branch
from
5d4a3b5 to
7ef550a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/frontend/apps/electron-renderer/src/popup/recording/index.tsx (1)
118-127: DuplicategetCurrentRecordingcall still present.This concern was raised in a previous review. The
getCurrentRecordingAPI is called twice on mount: once inuseRecordingStatus(line 34) and again here (line 118). This creates redundant API calls and potential race conditions since both execute concurrently.packages/frontend/native/media_capture/src/recording.rs (1)
498-557: Recording ID collision allows resource leak on duplicate detection.This issue was flagged in a previous review. The current flow starts capture and spawns the worker (lines 513-529) before acquiring the lock and checking for duplicate IDs (lines 539-545). If a duplicate is detected, the capture session and worker thread are orphaned.
The suggested fix from the previous review to move ID generation and duplicate checking inside the critical section (before starting capture) would resolve this.
packages/frontend/apps/electron/src/main/recording/feature.ts (1)
486-515: Native recording may be orphaned if state machine dispatch fails.This was flagged in a previous review. If
startRecordingsucceeds (line 489) but theATTACH_NATIVE_RECORDINGdispatch fails or the state machine rejects it (e.g., status changed between dispatches), the native recording continues running without being tracked.Consider stopping the native recording if attachment fails:
🔎 Suggested fix
const nextState = recordingStateMachine.dispatch({ type: 'ATTACH_NATIVE_RECORDING', id: state.id, nativeId: meta.id, startTime: meta.startedAt ?? state.startTime, filepath, sampleRate: meta.sampleRate, numberOfChannels: meta.channels, }); + if (!nextState || nextState.status !== 'recording') { + try { + getNativeModule().stopRecording(meta.id); + } catch (stopErr) { + logger.error('failed to stop orphaned native recording', stopErr); + } + } + return nextState;
🧹 Nitpick comments (3)
packages/frontend/native/media_capture/src/windows/audio_capture.rs (1)
246-248: Consider documenting the default sample rate selection logic.The default selection of
min(mic_sample_rate, lb_sample_rate)is reasonable to avoid upsampling, but a brief comment explaining this choice would help future maintainers understand the rationale.🔎 Suggested documentation
let mic_sample_rate = mic_config.sample_rate(); let lb_sample_rate = lb_config.sample_rate(); + // Default to the lower of the two rates to avoid unnecessary upsampling let target_rate = target_sample_rate.unwrap_or(SampleRate(mic_sample_rate.min(lb_sample_rate).0));packages/frontend/native/media_capture/Cargo.toml (1)
18-18: Add opus-codec to workspace.dependencies for consistency.The
opus-codecdependency is pinned to a specific version"0.1.2"while all other dependencies use{ workspace = true }. Align this with the project's dependency management pattern by addingopus-codec = "0.1.2"to the workspace.dependencies section in the root Cargo.toml and using{ workspace = true }in this package.packages/frontend/native/media_capture/src/recording.rs (1)
131-138: Edge case: partial interleaved frames may silently drop samples.If
interleavedends with a partial frame (length not divisible byself.channels), the inner loop silently skips samples whereidx >= self.channels. This could happen if the audio source delivers misaligned buffers.Consider adding a debug assertion or handling the remainder explicitly.
🔎 Suggested diagnostic
fn feed(&mut self, interleaved: &[f32]) -> Vec<f32> { + debug_assert!( + interleaved.len() % self.channels == 0, + "interleaved buffer length {} not divisible by channels {}", + interleaved.len(), + self.channels + ); for frame in interleaved.chunks(self.channels) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlpackages/frontend/apps/electron-renderer/src/popup/recording/index.tsxpackages/frontend/apps/electron/src/main/recording/feature.tspackages/frontend/apps/electron/src/main/recording/index.tspackages/frontend/apps/electron/src/main/recording/state-machine.tspackages/frontend/apps/electron/src/main/recording/types.tspackages/frontend/core/src/utils/opus-encoding.tspackages/frontend/native/index.d.tspackages/frontend/native/index.jspackages/frontend/native/media_capture/Cargo.tomlpackages/frontend/native/media_capture/src/audio_callback.rspackages/frontend/native/media_capture/src/lib.rspackages/frontend/native/media_capture/src/macos/screen_capture_kit.rspackages/frontend/native/media_capture/src/macos/tap_audio.rspackages/frontend/native/media_capture/src/recording.rspackages/frontend/native/media_capture/src/windows/audio_capture.rspackages/frontend/native/media_capture/src/windows/screen_capture_kit.rs
💤 Files with no reviewable changes (2)
- packages/frontend/core/src/utils/opus-encoding.ts
- packages/frontend/apps/electron/src/main/recording/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/frontend/native/media_capture/src/lib.rs
- Cargo.toml
- packages/frontend/native/media_capture/src/macos/screen_capture_kit.rs
- packages/frontend/apps/electron/src/main/recording/state-machine.ts
- packages/frontend/native/index.d.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Use TypeScript as the programming language
Applied to files:
packages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Expose JavaScript APIs to native iOS code through window object for Capacitor integration
Applied to files:
packages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Enable TypeScript strict mode
Applied to files:
packages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement getAiButtonFeatureFlag() API for native iOS bridge
Applied to files:
packages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement getCurrentI18nLocale() API for native iOS bridge
Applied to files:
packages/frontend/apps/electron/src/main/recording/types.ts
📚 Learning: 2025-11-27T03:23:11.880Z
Learnt from: CR
Repo: toeverything/AFFiNE PR: 0
File: packages/frontend/apps/ios/AGENTS.md:0-0
Timestamp: 2025-11-27T03:23:11.880Z
Learning: Applies to packages/frontend/apps/ios/**/*.{ts,tsx} : Implement getCurrentDocContentInMarkdown() API for native iOS bridge
Applied to files:
packages/frontend/apps/electron/src/main/recording/feature.ts
🧬 Code graph analysis (6)
packages/frontend/native/index.js (1)
packages/common/y-octo/node/index.js (1)
nativeBinding(7-7)
packages/frontend/apps/electron-renderer/src/popup/recording/index.tsx (2)
packages/frontend/apps/electron/src/preload/electron-api.ts (1)
apis(278-281)packages/frontend/apps/electron/src/main/recording/state-machine.ts (1)
status(61-63)
packages/frontend/native/media_capture/src/recording.rs (1)
packages/frontend/native/index.d.ts (3)
RecordingStartOptions(61-69)RecordingSessionMeta(53-59)RecordingArtifact(44-51)
packages/frontend/native/media_capture/src/macos/tap_audio.rs (1)
packages/frontend/native/media_capture/src/recording.rs (1)
start_capture(434-478)
packages/frontend/native/media_capture/src/windows/screen_capture_kit.rs (2)
packages/frontend/native/media_capture/src/macos/screen_capture_kit.rs (4)
tap_audio_with_callback(681-709)tap_audio(712-720)new(104-110)new(529-533)packages/frontend/native/media_capture/src/windows/audio_capture.rs (2)
start_recording(220-395)new(32-49)
packages/frontend/apps/electron/src/main/recording/feature.ts (3)
packages/frontend/apps/electron/src/main/recording/state-machine.ts (2)
recordingStateMachine(356-356)status(61-63)packages/frontend/apps/electron/src/main/recording/types.ts (2)
AppGroupInfo(12-19)RecordingStatus(21-46)packages/frontend/apps/electron/src/main/windows-manager/main-window.ts (1)
getMainWindow(251-253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: y-octo binding test on aarch64-pc-windows-msvc
- GitHub Check: y-octo binding test on x86_64-apple-darwin
- GitHub Check: Build AFFiNE native (aarch64-pc-windows-msvc)
- GitHub Check: Build AFFiNE native (x86_64-apple-darwin)
- GitHub Check: Build AFFiNE native (x86_64-pc-windows-msvc)
- GitHub Check: Lint
🔇 Additional comments (31)
packages/frontend/native/media_capture/src/audio_callback.rs (1)
1-29: Clean abstraction for dual-target audio delivery.The
AudioCallbackenum provides a clean abstraction for routing audio to either JS or native channels. The non-blocking semantics with intentional error suppression are appropriate for real-time audio processing where blocking would cause worse problems than occasional sample drops.One consideration: for debugging purposes, you might want to track dropped samples via a counter or occasional log, especially during development.
packages/frontend/apps/electron-renderer/src/popup/recording/index.tsx (1)
103-139: Effect cleanup pattern is correct.The
removedflag pattern properly prevents state updates after unmount. The empty dependency array is appropriate since this effect should only run once on component mount.packages/frontend/native/index.js (1)
582-583: New recording API exports added correctly.The
startRecordingandstopRecordingexports are properly wired to the native binding, consistent with the pattern used for other exports in this auto-generated file.packages/frontend/native/media_capture/src/windows/audio_capture.rs (2)
220-223: Function signature updated correctly for AudioCallback abstraction.The updated signature accepting
AudioCallbackand optionaltarget_sample_ratealigns with the cross-platform abstraction introduced in this PR. The parameter ordering is consistent with the macOS implementation.
346-350: Callback invocation simplified correctly.The change from
func.call(Ok(mixed.clone().into()), ThreadsafeFunctionCallMode::NonBlocking)toaudio_buffer_callback.call(mixed)is cleaner and delegates the complexity to theAudioCallbackabstraction.packages/frontend/native/media_capture/Cargo.toml (1)
13-22: Dependency additions support the new recording architecture.The new dependencies (
crossbeam-channel,ogg,opus-codec,rand,thiserror) align with the PR's goal of moving Opus encoding to the native layer. Movingcrossbeam-channelfrom Windows-specific to global dependencies makes sense given the newAudioCallback::Channelvariant used across platforms.packages/frontend/native/media_capture/src/windows/screen_capture_kit.rs (3)
235-242: New callback-based entry point added correctly.The
tap_audio_with_callbackfunction properly convertsOption<u32>toOption<SampleRate>and delegates to the underlyingstart_recording. This mirrors the pattern used in the macOS implementation for cross-platform consistency.
244-256: Clean delegation to callback-based implementation.The public
tap_audioNAPI function now correctly delegates totap_audio_with_callback, wrapping theThreadsafeFunctioninAudioCallback::Js(Arc::new(...)). This maintains backward compatibility while enabling the new callback abstraction.
258-267: Global audio tap follows same pattern.The
tap_global_audio_with_callbackmirrorstap_audio_with_callbackcorrectly. Note that_excluded_processesis unused on Windows since per-application audio tapping isn't supported—the comment intap_audioat line 249-250 explains this platform limitation.packages/frontend/apps/electron/src/main/recording/types.ts (2)
1-1: Import cleanup reflects architectural shift.The removal of
WriteStreamandAudioCaptureSessionimports aligns with the PR's goal of moving recording lifecycle management to the native layer. The type now correctly depends only onApplicationInfofrom the native module.
43-45: New metadata fields support native recording integration.The addition of
nativeId,sampleRate, andnumberOfChannelsas optional fields provides the necessary metadata for the native recording flow while maintaining backward compatibility with existing code.packages/frontend/native/media_capture/src/macos/tap_audio.rs (4)
240-245: AggregateDevice::start updated for AudioCallback abstraction.The signature change accepting
AudioCallbackinstead ofArc<ThreadsafeFunction<...>>aligns with the cross-platform abstraction. Theoriginal_audio_statsparameter is retained for consistent target rate handling during device switches.
301-302: Callback invocation simplified and comment updated.The change from
audio_stream_callback.call(Ok(mixed_samples.into()), ThreadsafeFunctionCallMode::NonBlocking)toaudio_stream_callback.call(mixed_samples)properly delegates to theAudioCallbackabstraction. The comment change from "JavaScript sink" to "configured sink" accurately reflects the new flexibility.
612-633: start_capture correctly stores and uses AudioCallback.The method properly:
- Stores the callback for device switch scenarios (line 614)
- Clones it for the initial stream start (line 633)
- Makes it available in the device change listener closure (line 661)
This ensures the callback remains valid throughout the capture lifecycle.
674-736: Device change listener correctly handles AudioCallback.The listener closure captures
callback_arc(a clone ofAudioCallback) and uses it when recreating the stream on device changes (line 722). SinceAudioCallbackderivesCloneand containsArcorSender(both thread-safe), this usage is correct.packages/frontend/native/media_capture/src/recording.rs (10)
1-27: LGTM!Imports and constants are well-organized. The 48kHz sample rate is standard for Opus, and the 4096-byte max packet size provides adequate headroom for encoded frames.
30-58: LGTM!The N-API structs are well-defined and align with the TypeScript interfaces in
index.d.ts. Field types are appropriate for cross-language interop.
60-102: LGTM!Comprehensive error handling with clear error codes and proper conversion to N-API errors. The
thiserrorderive keeps the implementation clean.
170-230: LGTM!The
OggOpusWriterimplementation is well-structured. The encoder initialization with 20ms frames at 48kHz is appropriate for Opus, and the optional resampler integration is clean.
279-321: LGTM!The
finish()method correctly handles partial frames by zero-padding, properly writes the end-of-stream marker, and accurately calculates duration from samples written. Good defensive cleanup of empty recordings.
387-417: LGTM!Helper functions are well-implemented. The
validate_output_dircorrectly requires absolute paths and canonicalizes to prevent path traversal. Thesanitize_idprovides safe filename generation.
434-478: LGTM!Platform-specific capture initialization is cleanly abstracted. The sample rate clamping and channel extraction handle edge cases appropriately.
480-496: LGTM!The worker thread implementation is clean and straightforward. The channel-based iteration naturally terminates when the sender is dropped.
559-581: LGTM!The stop sequence correctly stops the capture first, then signals the worker by dropping the sender, and waits for the worker to finish. The error handling properly propagates both join failures and encoding errors.
Minor: Line 563 reuses
RecordingError::Startfor lock poisoning in stop context; a dedicated variant might improve clarity, but this is a minor nit.
363-363: Document or remove the unsafe Send implementation for PlatformCapture.The
unsafe impl Send for PlatformCaptureat line 363 lacks documentation justifying thread-safety. The macOS variant contains a raw pointer (Option<*mut c_void>) managed through CoreAudio C APIs, which does not automatically implement Send. Either add explicit documentation of the thread-safety guarantees and constraints under which this is safe, or derive Send/Sync bounds conditionally on the platform-specific types.packages/frontend/apps/electron/src/main/recording/feature.ts (6)
69-73: LGTM!The
getNativeModule()accessor pattern correctly handles dynamic loading of the native module to avoid loading it on unsupported platforms. The type alias provides proper type safety.
75-95: LGTM!Cleanup function properly resets all state including the new
appStateSubscribersarray. Silencing unsubscribe errors during cleanup is appropriate.
257-272: LGTM!The
getRecordingfunction now correctly derives recording metadata from the state machine status, aligning with the native module integration.
526-583: LGTM!The
stopRecordingfunction properly coordinates native module cleanup with state machine transitions. The error handling correctly falls back toCREATE_BLOCK_FAILEDstate, and the async window show doesn't block the return.
633-647: LGTM!The serialization now includes the new native recording metadata fields (
filepath,sampleRate,numberOfChannels) for IPC communication.
109-132: LGTM!The
createAppGroupfunction correctly usesgetNativeModule().ShareableContentfor platform-safe access to the native API.
| let mut opus_head = Vec::with_capacity(19); | ||
| opus_head.extend_from_slice(b"OpusHead"); | ||
| opus_head.push(1); // version | ||
| opus_head.push(channels.as_usize() as u8); | ||
| opus_head.extend_from_slice(&0u16.to_le_bytes()); // pre-skip | ||
| opus_head.extend_from_slice(&(sample_rate.as_i32() as u32).to_le_bytes()); | ||
| opus_head.extend_from_slice(&0i16.to_le_bytes()); // output gain | ||
| opus_head.push(0); // channel mapping |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Opus encoder delay pre-skip sample rate 48kHz RFC 7845 Ogg container specification
💡 Result:
Summary — Opus pre-skip in Ogg (RFC 7845)
- The OpusHead "pre-skip" field is a 16-bit unsigned little-endian count of samples at 48 kHz that MUST be discarded (decoded but not played) at stream start. It is used to remove encoder algorithmic delay from playback. [1] (datatracker.ietf.org)
- PCM sample position = granule_position − pre-skip; playback time = (PCM sample position) / 48000.0. RFC 7845 gives the example and the 48 kHz basis. [1] (datatracker.ietf.org)
- For cropping, RFC 7845 RECOMMENDS a pre-skip of at least 3,840 samples (80 ms) to ensure decoder convergence. [1] (datatracker.ietf.org)
- Encoders SHOULD set pre-skip = encoder lookahead + any extra LPC padding used. Reference-implementation encoders expose the lookahead via OPUS_GET_LOOKAHEAD (opus_encoder_ctl). [1][2] (datatracker.ietf.org)
Sources
[1] RFC 7845 — Ogg Encapsulation for the Opus Audio Codec. (datatracker.ietf.org)
[2] Opus API (opus_encoder_ctl / OPUS_GET_LOOKAHEAD). (opus-codec.org)
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc7845?utm_source=openai
- 2: https://datatracker.ietf.org/doc/html/rfc7845?utm_source=openai
- 3: https://datatracker.ietf.org/doc/html/rfc7845?utm_source=openai
- 4: https://datatracker.ietf.org/doc/html/rfc7845?utm_source=openai
- 5: https://datatracker.ietf.org/doc/html/rfc7845?utm_source=openai
- 6: https://www.opus-codec.org/docs/opus_api-1.6/group__opus__encoder.html?utm_source=openai
🌐 Web query:
libopus OPUS_GET_LOOKAHEAD samples 48kHz encoder delay
💡 Result:
OPUS_GET_LOOKAHEAD returns the total number of samples of algorithmic delay (lookahead + decoder-side delay) that the codec adds. The value is implementation- and config-dependent, so you should query it via opus_encoder_ctl(..., OPUS_GET_LOOKAHEAD, &n) rather than hard-coding a number. [1]
For the reference implementation with the normal (default) configuration at a 48 kHz decode rate the typical total algorithmic delay is 26.5 ms, which equals 26.5 ms * 48 000 = 1272 samples. Note this is a typical/default figure (the encoder part can vary with mode, version, and settings). [2][1]
Sources:
- libopus encoder CTLs (OPUS_GET_LOOKAHEAD). [1]
- RFC 6716 / Opus algorithmic delay (default 26.5 ms → 1272 samples at 48 kHz). [2]
🏁 Script executed:
cat -n packages/frontend/native/media_capture/src/recording.rs | head -80Repository: toeverything/AFFiNE
Length of output: 2681
🏁 Script executed:
sed -n '300,360p' packages/frontend/native/media_capture/src/recording.rsRepository: toeverything/AFFiNE
Length of output: 2203
🏁 Script executed:
sed -n '1,100p' packages/frontend/native/media_capture/src/recording.rs | grep -A 20 "Encoder::"Repository: toeverything/AFFiNE
Length of output: 45
🏁 Script executed:
grep -n "Encoder::" packages/frontend/native/media_capture/src/recording.rsRepository: toeverything/AFFiNE
Length of output: 140
🏁 Script executed:
grep -n "encoder" packages/frontend/native/media_capture/src/recording.rs | head -20Repository: toeverything/AFFiNE
Length of output: 202
🏁 Script executed:
sed -n '200,240p' packages/frontend/native/media_capture/src/recording.rsRepository: toeverything/AFFiNE
Length of output: 1110
🏁 Script executed:
grep -n "write_opus_headers" packages/frontend/native/media_capture/src/recording.rsRepository: toeverything/AFFiNE
Length of output: 169
🏁 Script executed:
find packages/frontend/native -name "Cargo.toml" -exec grep -l "opus" {} \;Repository: toeverything/AFFiNE
Length of output: 112
🏁 Script executed:
grep -A2 "opus" packages/frontend/native/media_capture/Cargo.tomlRepository: toeverything/AFFiNE
Length of output: 172
🌐 Web query:
opus-codec 0.1.2 Rust crate lookahead OPUS_GET_LOOKAHEAD encoder delay
💡 Result:
Short answer — what it is and how to get it
- The opus-codec 0.1.2 crate bundles libopus (tag v1.5.2) and exposes the underlying Opus CTL to query lookahead. [1][2]
- OPUS_GET_LOOKAHEAD returns the total number of samples of algorithmic delay added by the codec (encoder+decoder). Call the encoder CTL (opus_encoder_ctl with OPUS_GET_LOOKAHEAD) to get the value; the encoder-side contribution can vary by implementation, version, and configuration, so query it at runtime rather than hard-coding a value. [2]
- RFC guidance: use that lookahead value (plus any extra LPC pre-skip you add) as pre-skip when muxing/setting granule positions so decoded output is time-aligned. [3]
Sources
- opus-codec crate docs (v0.1.2, bundles libopus v1.5.2). [1]
- Opus API — OPUS_GET_LOOKAHEAD / encoder CTLs. [2]
- RFC 7845 (Opus Ogg/encapsulation) — encoder delay / pre-skip guidance. [3]
The pre-skip value should match the encoder's lookahead to comply with RFC 7845.
The OpusHead pre-skip is hardcoded to 0, but RFC 7845 states encoders SHOULD set pre-skip to the encoder's lookahead (plus any extra padding). For libopus at 48 kHz, the typical algorithmic delay is approximately 1272 samples (26.5 ms). The opus-codec crate exposes the OPUS_GET_LOOKAHEAD CTL to query this value at runtime.
Currently, write_opus_headers() (line 211) is called before the encoder is created (line 214), preventing access to the lookahead value. To fix this, either refactor write_opus_headers() to accept the encoder as a parameter, or move the header write after encoder initialization and query the lookahead via opus_encoder_ctl.
Without this, audio playback may have timing issues at the stream start on standards-compliant decoders.
🤖 Prompt for AI Agents
In packages/frontend/native/media_capture/src/recording.rs around lines 329-336,
the OpusHead pre-skip is hardcoded to 0 but must reflect the encoder's lookahead
per RFC 7845; change the code so the pre-skip written into the OpusHead equals
the encoder's lookahead (plus any padding) obtained via the opus-codec CTL
OPUS_GET_LOOKAHEAD. To do this either refactor write_opus_headers to accept a
reference to the created encoder or move the header write to after the encoder
is initialized (the comment indicates write_opus_headers() is called before
encoder creation at ~lines 211/214), then call the appropriate
opus_encoder_ctl/opus-codec wrapper to get the lookahead, convert to u16
little-endian and write that into the pre-skip bytes instead of 0.
Summary by CodeRabbit
Release Notes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.