moq-mux: replace anyhow with thiserror#1495
Open
kixelated wants to merge 8 commits into
Open
Conversation
moq-mux is a library crate; per the workspace convention (thiserror for
libraries, anyhow for binaries) it shouldn't be exporting anyhow types
in its public API.
Each codec, container, and catalog format module now exports its own
Error enum (codec::{annexb,aac,opus,h264,h265,av1}::Error,
container::{fmp4,mkv,hls}::Error, catalog::msf::Error), flattened into
crate::Error via #[from]. Pure parsing functions (Config::parse,
Sps::parse, Avcc::parse, Avc1/Hvc1::transform, etc.) return their
module-local Result; importer/exporter structs return crate::Result so
they can mix codec errors with moq_net::Error and the other leaf
errors that flow through track operations.
Downstream:
- libmoq::Error::{InitFailed,DecodeFailed} now wrap Arc<moq_mux::Error>
instead of Arc<anyhow::Error>; added BufferNotConsumed for the one
ad-hoc anyhow!() site that didn't fit either bucket.
- moq-cli's PublishDecoder bridges moq_mux::Result into anyhow::Result
with explicit `?` since the binary still uses anyhow.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR replaces untyped anyhow errors with structured Error enums and Result aliases across the codebase. It adds module- and crate-level Error/Result types, converts public APIs and internal helpers in codecs (AAC, Opus, AV1, H.264, H.265, AnnexB), containers (fMP4, HLS, MKV), MSF catalog consumer, and import/stream dispatchers to use typed errors, introduces BufferNotConsumed/UnknownFormat variants, updates libmoq FFI mappings and moq-cli call sites, and removes the anyhow dependency. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
- Drop unused crate::Error::NotInitialized variant.
- Consolidate fmp4::Error::{NoMoof,MissingMoof} and {NoMoov,MissingMoov}
duplicates; both pairs described the same condition. Tighten the
remaining messages to match the Missing* sibling variants.
- Add Result<T> type aliases to container::{fmp4,mkv,hls} and
catalog::msf so all per-module error enums expose one. fmp4's Wire
Container impl falls back to std::result::Result because it needs the
two-arg form for Self::Error.
- Reorder libmoq::Error::ReturnCode so BufferNotConsumed sits next to
its numeric neighbors.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…drasekhar-da73a5 # Conflicts: # rs/libmoq/src/error.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-mux/src/container/fmp4/mod.rs (1)
192-227:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn an explicit bounds error instead of silently truncating decoded samples.
On Line 225,
decodereturnsOk(frames)when sample bounds exceedmdatlength. That silently accepts truncated/corrupt fragments. This should fail withError::SampleRangeOutOfBounds.Proposed fix
- if end > mdat_data.len() { - return Ok(frames); - } + if end > mdat_data.len() { + return Err(Error::SampleRangeOutOfBounds { + start: offset, + end, + len: mdat_data.len(), + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/container/fmp4/mod.rs` around lines 192 - 227, The decode function currently returns Ok(frames) when a sample's end index exceeds mdat_data length, silently truncating; change this to return Err(Error::SampleRangeOutOfBounds) instead. In the loop inside decode (for trun in &traf.trun { for entry in &trun.entries { ... }}), replace the early return Ok(frames) when end > mdat_data.len() with an Err(...) using Error::SampleRangeOutOfBounds so malformed/corrupt fragments fail rather than being silently accepted; ensure the error is propagated from decode's Result<Vec<Frame>> signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rs/moq-mux/src/container/fmp4/mod.rs`:
- Around line 192-227: The decode function currently returns Ok(frames) when a
sample's end index exceeds mdat_data length, silently truncating; change this to
return Err(Error::SampleRangeOutOfBounds) instead. In the loop inside decode
(for trun in &traf.trun { for entry in &trun.entries { ... }}), replace the
early return Ok(frames) when end > mdat_data.len() with an Err(...) using
Error::SampleRangeOutOfBounds so malformed/corrupt fragments fail rather than
being silently accepted; ensure the error is propagated from decode's
Result<Vec<Frame>> signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9a25ca6-6721-4931-9a1e-34b19c872c8f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
rs/libmoq/src/error.rsrs/libmoq/src/publish.rsrs/moq-cli/src/publish.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog/msf/consumer.rsrs/moq-mux/src/catalog/msf/mod.rsrs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/codec/aac/mod.rsrs/moq-mux/src/codec/annexb.rsrs/moq-mux/src/codec/av1/import.rsrs/moq-mux/src/codec/av1/mod.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h264/mod.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/codec/h265/mod.rsrs/moq-mux/src/codec/opus/import.rsrs/moq-mux/src/codec/opus/mod.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/fmp4/mod.rsrs/moq-mux/src/container/hls/import.rsrs/moq-mux/src/container/hls/mod.rsrs/moq-mux/src/container/mkv/export.rsrs/moq-mux/src/container/mkv/import.rsrs/moq-mux/src/container/mkv/mod.rsrs/moq-mux/src/container/source.rsrs/moq-mux/src/error.rsrs/moq-mux/src/import.rs
💤 Files with no reviewable changes (1)
- rs/moq-mux/Cargo.toml
decode() previously returned Ok(frames) when a sample's end index exceeded mdat length, silently truncating the frame list. By the time decode() runs the full CMAF fragment is in hand, so a size mismatch is a corrupt-publisher signal rather than a streaming-truncation case worth tolerating. Returning Error::SampleRangeOutOfBounds lets the caller see the failure instead of receiving a misleading partial decode. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Addressed the out-of-bounds-samples finding in 4737fac: |
…drasekhar-da73a5 Conflicts in moq-mux Import structs where main (#1515) added a `seek` method using anyhow::Result; reconciled to return the per-module crate::Result/Result variant matching the rest of this branch's refactor. The h264/h265/av1 seek variants swap `.context("not initialized")` for `.ok_or(Error::NotInitialized)?` using each module's NotInitialized variant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-mux/src/codec/h264/import.rs`:
- Around line 462-467: The seek method currently only calls track.seek and
ignores any buffered avc3 access unit, which can cause pre-seek NALs to be
flushed into the post-seek group; update seek(&mut self, sequence: u64) in
import.rs to either (A) reject seeks while an avc3 AU is buffered by checking
self.state for State::Avc3 and that State::Avc3.current.is_some() and returning
a clear error (e.g., Error::InvalidSeekWhileBufferedAvc3) or (B) atomically
discard/reset the pending AU before calling track.seek (clear
State::Avc3.current and related per-AU state and ensure maybe_start_frame() will
not re-flush it); reference the symbols seek, State::Avc3.current,
maybe_start_frame, and track.seek when making the change so the behavior
prevents cross-boundary flushing.
🪄 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: d51c9d6d-8b80-457b-bfbc-5c26e3857532
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
rs/moq-mux/Cargo.tomlrs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/codec/av1/import.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/codec/opus/import.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/mkv/import.rsrs/moq-mux/src/import.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rs/moq-mux/src/codec/aac/import.rs
- rs/moq-mux/src/codec/av1/import.rs
- rs/moq-mux/src/import.rs
- rs/moq-mux/src/container/mkv/import.rs
- rs/moq-mux/src/codec/h265/import.rs
h264 (avc3), h265, and av1 buffer NALs/OBUs in a `current` access-unit struct that's flushed on the next maybe_start_frame(). Previously seek() only advanced the underlying track; pre-seek bytes survived and would later be emitted into the post-seek group with the wrong timestamp. Reset `current` (and h264's avc3 State) before calling track.seek() so the discontinuity is honored on both sides. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kixelated
commented
May 28, 2026
| /// Media decoder initialization failed. | ||
| #[error("init failed: {0}")] | ||
| InitFailed(Arc<anyhow::Error>), | ||
| InitFailed(Arc<moq_mux::Error>), |
Collaborator
Author
There was a problem hiding this comment.
Could we make the moq_mux::Error Clone so we don't need to wrap it in an Arc?
And maybe combine InitFailed and DecodeFailed into a single Mux(#[from] moq_mux::Error)? Don't worry about backwards compatibility for error codes.
Collaborator
Author
There was a problem hiding this comment.
Done in 4196dd5: every moq-mux Error now derives Clone (non-Clone leaves like std::io::Error, mp4_atom::Error, reqwest::Error, and TagWriterError are wrapped in Arc), and libmoq drops InitFailed/DecodeFailed in favor of Mux(#[from] moq_mux::Error) with plain ? at the call sites.
All moq-mux per-module Error enums now derive Clone. Non-Clone leaf errors (mp4_atom::Error, std::io::Error, reqwest::Error, webm_iterable TagWriterError) are wrapped in Arc with a manual From<T> impl, mirroring the pattern hang::Error already uses for serde_json::Error. moq-loc::Error has only unit variants, so it gains a plain Clone derive. With moq_mux::Error: Clone, libmoq no longer needs to Arc-wrap it. The InitFailed/DecodeFailed variants collapse into the existing Mux(#[from] moq_mux::Error), and the explicit .map_err sites in libmoq::publish disappear in favor of plain `?`. Return codes -10 and -11 are gone; per discussion on the PR, callers are not matching on them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Errorenum, flattened intocrate::Errorvia#[from]:codec::{annexb,aac,opus,h264,h265,av1}::Error,container::{fmp4,mkv,hls}::Error,catalog::msf::Error.Config::parse,Sps::parse,Avcc::parse,Avc1/Hvc1::transform, etc.) return their module-localResult. Importer/exporter struct methods returncrate::Resultbecause they interleave codec errors withmoq_net::Errorand the other leaf errors that flow through track operations.Downstream impact
libmoq::Error::{InitFailed,DecodeFailed}now wrapArc<moq_mux::Error>instead ofArc<anyhow::Error>. Added aBufferNotConsumedvariant for the one ad-hocanyhow!()site that didn't fit either bucket.moq-cli'sPublishDecoderkeepsanyhow::Resultand bridgesmoq_mux::Resultwith explicit?.Test plan
just check(cargo check + clippy -D warnings + fmt --check + doc + shear + sort) passescargo test -p moq-mux— 141 tests passcargo check --workspace --all-targets --exclude moq-gstbuilds (moq-gst is blocked by a local gstreamer install only, not by these changes)🤖 Generated with Claude Code
(Written by Claude)