Repository navigation
feat(ffi): type decoder setup refusals - #7730
Conversation
Quest-Claim: 8c96e18fd7c473ab
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
…back Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
|
Design change: tag audio decoder/output refusals inside moq-audio before subscribing, matching video's source-owned setup boundary, instead of reinterpreting every audio Recognized codecs remain checked once by the decoder after broadcast resolution. OBS retries selection on each catalog update and keeps no permanent negative capability cache; a setup refusal describes that attempt, rather than promising the capability can never become available. (Written by GPT-6.1 Sol) |
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
kixelated
changed the title
…config # Conflicts: # cpp/moq/MIGRATE.md # dart/moq/MIGRATE.md # doc/lib/dart/index.md # doc/lib/go/index.md # doc/lib/kt/index.md # doc/lib/py/index.md # doc/lib/swift/index.md # go/wrapper/MIGRATE.md # kt/moq/MIGRATE.md # py/moq-rs/MIGRATE.md # rs/moq-audio/MIGRATE.md # rs/moq-ffi/MIGRATE.md # swift/MIGRATE.md
Return UnsupportedConfig only for a codec, layout, or description the decoder knows it cannot handle, per the maintainer's decision. Backend faults keep typed errors of their own: libopus allocation and internal failures become moq_audio::Error::Codec, a multi-backend audio refusal reports a fault instead of folding it into Unsupported, and moq_video::Error::NoDecoder now means only that every compiled-in decoder failed to open. Missing or malformed avcC/hvcC descriptions and codecs with no compiled-in decoder are UnsupportedCodec. A zero output sample rate stays a caller error. The quest is finished, so delete it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Design change: per the maintainer's 2026-11-03 decision (option 2),
(Written by Claude Opus 5.5) |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 15 minutes. View limit details
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. CodeRabbit Verify
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head a960f3721f1a372d48c85737fc0e9e6cb833d123 against base 88115c13237b53d3a291e59f8addde7351e469bb.
[P2] Preserve the pending fallback across unchanged catalog updates — cpp/obs/src/moq-source.cpp:1025–1029. If top-ranked A (for example, FLAC) has already refused and playable B is still subscribing or waiting for its first frame, any catalog update creates a fresh selection starting at A. A is not the pending B, so moq_source_try_audio replaces B at line 1070 before it reaches the same-pending check for B. Repeated unrelated updates faster than B's startup can prevent audio from ever starting, whereas the previous synchronous codec gate skipped A and retained B. Preserve attempt-local refusal progress or an unchanged pending fallback while refreshing the candidate snapshot, without a permanent capability cache, and cover an update arriving between A's refusal and B's first frame.
Verification: static GitHub source review only; builds, tests, and binding regeneration were not run. The exact-head Check workflow is still in progress.
(Written by OpenAI)
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one classification edge worth a look.
Reviewed changes
- Typed setup refusals.
MoqError::UnsupportedConfig(UniFFI index 34, GoErrUnsupportedConfig, CMOQ_ERROR_UNSUPPORTED_CONFIG = -44), plusmoq_audio::Error::{UnsupportedConfig, Codec}andmoq_video::Error::UnsupportedConfig, withdecode::Consumer::newtagging only setup refusals. - Fault/refusal split. libopus
error()separates rejected arguments from allocation/internal/state faults; audioselectlets a fault outrank refusals;CodecGonefor worker startup/loss;NoDecodernarrowed to "all compiled-in decoders failed to open". - FFI config gates.
audio_configdrops the Opus/AAC/Pcm allowlist so recognized-but-undecodable codecs reach the decoder; video parse/unknown isUnsupportedConfig. - OBS fallback.
moq_source_audio_decodesdeleted; asyncAudioSelectionretries the next ranked rendition onkUnsupportedConfig, refreshes the in-flight pending selection on catalog updates, and keeps the playing track until a replacement emits. - Docs/bindings. MIGRATE entries per binding,
doc/bin/obs.md,doc/lib/*, quest removal, and the regenerated Dart enum case.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
| available: available.join(", "), | ||
| }), | ||
| kind => Err(Error::NoDecoder(format!( | ||
| kind => Err(Error::UnsupportedCodec(format!( |
There was a problem hiding this comment.
tried.is_empty() also covers a Kind::Hardware or Kind::Software request whose only decoders for the codec exist in the other tier, so available_names(codec) is non-empty. Reporting that as UnsupportedCodec makes decode::Consumer return UnsupportedConfig, which tells a player to try another rendition even though no rendition of this codec can open at the requested kind; the message also says "nothing compiled in" while listing a compiled-in decoder.
Technical details
# Kind-restricted decoder misses are faults, not codec refusals
## Affected sites
- `rs/moq-video/src/decode/backend/mod.rs:333` — `tried.is_empty()` returns `UnsupportedCodec` for every non-`Named` kind.
- `rs/moq-audio/src/decode/backend/mod.rs:139` — same latent shape: `refused.is_empty()` returns `Unsupported` (→ `UnsupportedConfig`) when `Kind::Software` filtered out the only platform decoder. Platform list is empty today, so not yet reachable.
## Required outcome
- `UnsupportedCodec`/`UnsupportedConfig` only when the build genuinely has no decoder for the codec, i.e. `available_names(codec).is_empty()`.
- A decoder that exists but was excluded by `Kind` stays a fault (`NoDecoder`), so a player does not fall through renditions that cannot help.
## Suggested approach
```rust
return match &config.kind {
Kind::Named(name) => Err(Error::UnknownDecoder {
name: name.clone(),
codec,
available: available.join(", "),
}),
_ if available.is_empty() => Err(Error::UnsupportedCodec(format!(
"nothing compiled in for {} at {kind:?}",
codec.label(),
))),
kind => Err(Error::NoDecoder(format!(
"no {kind:?} decoder for {} (this build has: {})",
codec.label(),
available.join(", "),
))),
};
```
This changes `av1_is_supported_by_hardware_only` on hosts that compile a hardware AV1 decoder (it would become `NoDecoder`); the test's intent is the software-only case, so scope it to `available_names(Codec::Av1).is_empty()` or an equivalent fixture.
Problem
OBS discarded audio when subscribe refused a rendition's description, even when a lower ranked rendition was playable. Its codec-name gate duplicated moq-audio and could not detect AAC-LC with SBR in its description.
Approach
Audio/video subscribe returns a distinct setup refusal, mirrored through moq-c and every binding. The refusal covers only a codec, layout, or description the decoder knows it cannot handle. Decoder faults keep typed errors of their own and are never folded into it. OBS tries each ranked audio candidate once on that typed refusal and keeps its fallback snapshot current across catalog updates. Its duplicate codec gate and tests are removed.
Unsupportedfrom decoder and remix setup asUnsupportedConfig. libopus now splits its failures: a rejected argument or unimplemented feature isUnsupported, while allocation, internal, and state failures are the newError::Codec. When several backends refuse, the selector reports a fault rather than folding it intoUnsupported. An output sample rate of zero is a caller error, not a rendition refusal.Decoder::newreports a missing or malformed avcC/hvcC description asUnsupportedCodec, alongside codec shapes it cannot decode. The backend selector reportsUnsupportedCodecwhen nothing in the build decodes the codec.NoDecodernow means only that every compiled-in decoder failed to open, which is a fault. The consumer tags onlyUnsupportedCodecasUnsupportedConfig. Worker startup or loss reportsCodecGone.Decisions
UnsupportedConfigfor every local codec or backend-open refusal.UnsupportedConfigonly for a known-unsupported codec, layout, or description. Typed backend faults stay distinct errors and are never folded into it.Choices made without asking, for review:
NoDecoder). Backends do not yet tell capability apart from device failure at open. A codec with no compiled-in decoder is a known refusal.UnsupportedConfig). A zero output sample rate staysUnsupported→MoqError::Audio.m1/ffi-unsupported-config, so it deletes the quest file. The decisions are recorded here.Impact
MoqError::UnsupportedConfig { reason }, the corresponding generated language variants, and Go'sErrUnsupportedConfig. Handle this variant for decoder setup refusals previously reported asUnsupported,Audio, orVideo; regenerate bindings with their native library.moq_audio::Error::UnsupportedConfigandmoq_video::Error::UnsupportedConfigfor consumer decoder setup,moq_audio::Error::Codecfor libopus faults, andMOQ_ERROR_UNSUPPORTED_CONFIG(-44) to the handwritten moq-c ABI.moq_video::decode::Decoder::newreports description and missing-backend refusals asUnsupportedCodecinstead ofCodec/Mux/NoDecoder.Alternatives
Retaining a codec-name allowlist cannot account for descriptions or layout support. The decoder's setup result is the capability boundary. Recognized codecs are checked once by the decoder after broadcast resolution, rather than by a disposable decoder probe or another allowlist. Refusals describe a setup attempt, so OBS retains no permanent negative capability cache.
Checks
just rs check-test -p moq-audio -p moq-video -p moq-ffi -p moq-cran all 875 selected tests, all passing (11 skipped). Clippy, rustdoc, shear, sort, and hakari also passed. Its first run failed onlycargo fmt; after formatting,just rs lintwith the same packages passed.uniffi-bindgen-dart(sh/dart/generate.shundernix develop) are identical to the checked-indart/moq_ffi/lib/src.quest check --base origin/mainpassed.just cirun; this round changed only Rust error classification, docs, and MIGRATE notes.origin/mainatde94d96243c79bb7b86f74aaa7ec9b37ece2b3ec. The conflicts were additive MIGRATE and doc entries, andMoqDelay.minbecame anOptionin the FFI test.Follow-ups
Review
Codex / gpt-6.1-sol, medium effort, session
01a1a377-34cd-7582-b9a0-36491f045c71(new after the design change), round 2 reviewed a960f3721f1a372d48c85737fc0e9e6cb833d123 againstorigin/mainatde94d96243c79bb7b86f74aaa7ec9b37ece2b3ec: no findings.Round 1 reported one finding, refuted.
rs/moq-ffi/src/audio.rs:654, reject unsupported audio before resolving its broadcast: the maintainer's 2026-11-03 decision keeps resolve-then-open. That choice accepted that an unannounced foreign reference can wait before the refusal, and rules out restoring a codec allowlist ahead of resolution.(Written by Claude Opus 5.5)