Fix/vp8 vp9 codec normalization#501
Conversation
📝 WalkthroughWalkthroughThe streaming decoder enhances codec handling in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/exporter/streamingDecoder.ts (1)
305-306: Nit: gate verbose decoder diagnostics behind a debug flagLowkey useful while debugging, but these logs will fire on every decode path and dump raw
descriptionobjects. Consider guarding with a debug toggle (orconsole.debug) to keep prod consoles less cursed.diff suggestion
- console.log("[StreamingVideoDecoder] decoderConfig.codec:", decoderConfig.codec); - console.log("[StreamingVideoDecoder] decoderConfig.description:", decoderConfig.description); + const debugDecode = false; + if (debugDecode) { + console.debug("[StreamingVideoDecoder] decoderConfig.codec:", decoderConfig.codec); + console.debug( + "[StreamingVideoDecoder] decoderConfig.description:", + decoderConfig.description, + ); + } ... - console.log( - `[StreamingVideoDecoder] isConfigSupported for "${preferredDecoderConfig.codec}":`, - support.supported, - ); + if (debugDecode) { + console.debug( + `[StreamingVideoDecoder] isConfigSupported for "${preferredDecoderConfig.codec}":`, + support.supported, + ); + }Also applies to: 387-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/streamingDecoder.ts` around lines 305 - 306, The two console.log calls printing decoderConfig.codec and decoderConfig.description (and the similar logs at the other spot) are noisy and dump raw objects; wrap these diagnostics behind a debug toggle or replace with console.debug so they only run when verbose logging is enabled—e.g., add a check on a DEBUG/verbose flag or use console.debug inside the StreamingVideoDecoder decode/init path where decoderConfig is available to prevent spamming production consoles while keeping the logs accessible for debugging.
🤖 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/streamingDecoder.ts`:
- Around line 324-329: The codec normalization currently unconditionally
overwrites decoderConfig.codec to "avc1.640033" for /^avc1$/i or /^h264$/i and
later the fallback path attempts the same configure() again causing a misleading
warning; change both places to only set or attempt the avc1 fallback when
decoderConfig.codec (case-insensitive) is not already "avc1.640033" so you skip
the no-op rewrite and avoid calling configure()/emitting the fallback warning
redundantly.
---
Nitpick comments:
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 305-306: The two console.log calls printing decoderConfig.codec
and decoderConfig.description (and the similar logs at the other spot) are noisy
and dump raw objects; wrap these diagnostics behind a debug toggle or replace
with console.debug so they only run when verbose logging is enabled—e.g., add a
check on a DEBUG/verbose flag or use console.debug inside the
StreamingVideoDecoder decode/init path where decoderConfig is available to
prevent spamming production consoles while keeping the logs accessible for
debugging.
🪄 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: 67202809-4da3-488b-87de-c26fd29f75a5
📒 Files selected for processing (1)
src/lib/exporter/streamingDecoder.ts
![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: f9401f051c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
dont forget the debugging console logs |
Sync ui-rework with main, which includes: - fix/vp8-vp9-codec-normalization (PR siddharthvaddem#501) - fix/cursor-telemetry-session-isolation (PR siddharthvaddem#457) No source conflicts — changes are in disjoint areas.
3 participants
Description
Fixes H.264 video export failing with
"VideoDecoder error: Unknown or ambiguous codec name"/"Unsupported codec".Motivation
When exporting a recording, the
StreamingVideoDecodercallsweb-demuxer.getDecoderConfig("video")which returns acodecstring and an optionaldescription(the AVCDecoderConfigurationRecord /avcCbox). In some cases — particularly WebM containers repaired by@fix-webm-duration/fix— the extradata is missing (description: undefined), andweb-demuxerfalls back to a malformed codec string likeavc1.2420032. This string is rejected by Chromium's WebCodecsVideoDecoderwith"Unknown or ambiguous codec name".The same root cause can also produce bare four-character code strings (
av01,vp08,vp09,avc1,h264) that lack the full parametrized form WebCodecs requires.Type of Change
Related Issue(s)
#500
Changes
src/lib/exporter/streamingDecoder.tsH.264 codec fallback — When
VideoDecoder.isConfigSupported()returnsfalse(orconfigure()throws) for a codec string starting withavc1, the decoder now falls back toavc1.640033(H.264 High Profile Level 5.1, universally supported on Chromium).Rationale: The actual H.264 bitstream profile/level is conveyed in-band via SPS NAL units, so a decode-capable codec string (even a generic one) is sufficient for the decoder to configure itself.
Bare codec string normalization — Added normalization for four-character code strings returned by
web-demuxer:vp08→vp8vp09→vp9avc1→avc1.640033h264→avc1.640033These follow the existing
av01→ parametrized AV1 string pattern from PR fix: handle av1 VideoDecoder errors #334 .Extended
shouldPreferSoftwareDecode— Addedvp09andvp9to the software-decode preference (consistent with AV1, which also benefits from software decode for CPU-intensive codecs).Diagnostic logging — Added
console.log/console.warnstatements to surface the rawcodec,description, andisConfigSupportedresult at runtime. These can be removed once the fix is stable.Testing
fix/vp8-vp9-codec-normalizationbranchnpm run dev)"VideoDecoder error"or"Unsupported codec"in the DevTools consoleChecklist
Summary by CodeRabbit