Updates for exporting video .mp4 type#438
Updates for exporting video .mp4 type#438nguyenducdongdev wants to merge 2 commits intosiddharthvaddem:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces hardcoded Opus audio handling with selectable AAC or Opus paths: new exported codec types, constructors now accept codec choices, muxer and processor use the selected codec, and video exporter probes encoder support to pick AAC or Opus (or fail if none supported). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a0d3c530b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /* reader already closed */ | ||
| } | ||
| } | ||
| await this.processTrimOnlyAudio(demuxer, muxer, [], undefined); |
There was a problem hiding this comment.
Routing muxRenderedAudioBlob through processTrimOnlyAudio changes this path from chunk streaming to full decode/re-encode buffering, because processTrimOnlyAudio accumulates all decoded AudioData frames before encoding/muxing. For long videos with speed regions, this can drive memory usage to uncompressed PCM scale and cause export stalls/OOMs that did not occur with the previous streamed chunk path.
Useful? React with 👍 / 👎.
| if (hasAudio && !audioCodec) { | ||
| throw new Error( | ||
| "Source has audio, but no supported audio encoder was found for MP4 export.", | ||
| ); |
There was a problem hiding this comment.
This new hard failure blocks the entire MP4 export whenever the input has an audio stream but neither AAC nor Opus encoding is supported in the runtime. That is a functional regression for environments with limited WebCodecs audio support, where a video-only export is still possible and was previously handled by skipping audio in AudioProcessor.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/exporter/audioEncoder.ts (1)
9-17: Share the codec union withVideoMuxer.This redefines the same
"aac" | "opus"union asMp4AudioCodecinsrc/lib/exporter/muxer.ts. importing the existing type keeps the exporter path on one source of truth and avoids drift the next time this list changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/audioEncoder.ts` around lines 9 - 17, Replace the locally defined TargetAudioCodec union with the shared type from the muxer module: remove the export type TargetAudioCodec = "aac" | "opus"; and import Mp4AudioCodec from the muxer file, then use Mp4AudioCodec as the type for the AudioProcessor.targetCodec and the constructor parameter (i.e. update references to TargetAudioCodec to Mp4AudioCodec in class AudioProcessor) so both modules share the same single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/videoExporter.ts`:
- Around line 71-103: pickAudioCodec currently probes only
AudioEncoder.isConfigSupported for AAC/Opus and returns null if no encoder is
supported; update pickAudioCodec to also call AudioDecoder.isConfigSupported
(using the decoded AudioDecoderConfig from demuxer) before probing encoders so
you gate encoding checks on decoder support, and if neither decoder+encoder
combos work return null; then change the hard throw in the export flow that
handles a null audio codec (the error raised when audio cannot be encoded) to a
console.warn and continue the export as video-only so the operation degrades
gracefully instead of failing completely.
---
Nitpick comments:
In `@src/lib/exporter/audioEncoder.ts`:
- Around line 9-17: Replace the locally defined TargetAudioCodec union with the
shared type from the muxer module: remove the export type TargetAudioCodec =
"aac" | "opus"; and import Mp4AudioCodec from the muxer file, then use
Mp4AudioCodec as the type for the AudioProcessor.targetCodec and the constructor
parameter (i.e. update references to TargetAudioCodec to Mp4AudioCodec in class
AudioProcessor) so both modules share the same single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f90a6ee-e170-43c7-860a-2fbbcf40521b
📒 Files selected for processing (4)
src/lib/exporter/audioEncoder.tssrc/lib/exporter/muxer.tssrc/lib/exporter/videoExporter.tsvitest.config.ts
| /* reader already closed */ | ||
| } | ||
| } | ||
| await this.processTrimOnlyAudio(demuxer, muxer, [], undefined); |
There was a problem hiding this comment.
This makes speed-region exports much heavier in memory.
Line 332 now sends the rendered blob through processTrimOnlyAudio(), and that path buffers the whole track twice: once in decodedFrames and again in encodedChunks before muxing. for longer recordings that's a pretty easy way to balloon memory on top of the already-recorded blob. If re-encode is required here, it'd be safer to stream decode → encode → mux instead of materializing both arrays.
| private async pickAudioCodec(demuxer: WebDemuxer | null): Promise<Mp4AudioCodec | null> { | ||
| if (!demuxer) return null; | ||
|
|
||
| let audioConfig: AudioDecoderConfig; | ||
| try { | ||
| audioConfig = (await demuxer.getDecoderConfig("audio")) as AudioDecoderConfig; | ||
| } catch { | ||
| return null; | ||
| } | ||
|
|
||
| const sampleRate = audioConfig.sampleRate || 48000; | ||
| const channels = audioConfig.numberOfChannels || 2; | ||
|
|
||
| const aacConfig: AudioEncoderConfig = { | ||
| codec: "mp4a.40.2", | ||
| sampleRate, | ||
| numberOfChannels: channels, | ||
| bitrate: AUDIO_BITRATE, | ||
| }; | ||
| const aacSupport = await AudioEncoder.isConfigSupported(aacConfig); | ||
| if (aacSupport.supported) return "aac"; | ||
|
|
||
| const opusConfig: AudioEncoderConfig = { | ||
| codec: "opus", | ||
| sampleRate, | ||
| numberOfChannels: channels, | ||
| bitrate: AUDIO_BITRATE, | ||
| }; | ||
| const opusSupport = await AudioEncoder.isConfigSupported(opusConfig); | ||
| if (opusSupport.supported) return "opus"; | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "video exporter codec probing:"
rg -n -C3 'pickAudioCodec|AudioEncoder\.isConfigSupported|return "aac"|return "opus"|return null' src/lib/exporter/videoExporter.ts
echo '---'
echo "audio processor decode/encode gates:"
rg -n -C3 'AudioDecoder\.isConfigSupported|Audio codec not supported|Audio encoding not supported' src/lib/exporter/audioEncoder.ts
echo '---'
echo "hard failure on missing audio codec:"
rg -n -C2 'Source has audio, but no supported audio encoder was found for MP4 export' src/lib/exporter/videoExporter.tsRepository: siddharthvaddem/openscreen
Length of output: 2198
Add decoder gate to pickAudioCodec() and gracefully degrade on audio encode failures.
The codec picker at lines 71–103 only probes AudioEncoder.isConfigSupported() for AAC and Opus, but audioEncoder.ts separately checks AudioDecoder.isConfigSupported() at line 71. If source audio can be decoded (demuxer config parses fine) but can't be encoded to any supported format, pickAudioCodec() returns null at line 102. Then lines 193–196 throw a hard error instead of falling back to video-only export, which tanks the whole operation for files with oddball audio tracks.
Suggested fix:
- Add
AudioDecoder.isConfigSupported()check insidepickAudioCodec()before probing encoders - Change the throw at lines 193–196 to
console.warn()so export continues without audio
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/exporter/videoExporter.ts` around lines 71 - 103, pickAudioCodec
currently probes only AudioEncoder.isConfigSupported for AAC/Opus and returns
null if no encoder is supported; update pickAudioCodec to also call
AudioDecoder.isConfigSupported (using the decoded AudioDecoderConfig from
demuxer) before probing encoders so you gate encoding checks on decoder support,
and if neither decoder+encoder combos work return null; then change the hard
throw in the export flow that handles a null audio codec (the error raised when
audio cannot be encoded) to a console.warn and continue the export as video-only
so the operation degrades gracefully instead of failing completely.
|
@nguyenducdongdev can you provide a better pr desc |
2 participants
Pull Request Template
Description
Motivation
Type of Change
Related Issue(s)
Screenshots / Video
Screenshot (if applicable):
Video (if applicable):
Testing
Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests