mp4-atom icon indicating copy to clipboard operation
mp4-atom copied to clipboard

Initial avc3 support

Open fcancela opened this issue 1 month ago • 6 comments

This PR aims to add avc3 support.

It's an initial implementation, reusing all the avc1 codebase. Tested it with a bbc test stream.

Tested with this live stream: https://vs-dash-ww-rd-live.akamaized.net/pl/testcard2020/avc-full.m3u8

fcancela avatar Nov 18 '25 20:11 fcancela

Walkthrough

Adds an Avc3 sample-entry variant and wires it throughout the codebase. Introduces Avc3 as a public type alias (AvcSampleEntry<{ AVC3_CODE }>), the private AVC3_CODE, and unit tests exercising base and extended forms. Exposes avc3 in the H.264 module and adds Any::Avc3 handling. Extends the Codec enum with Avc3(Avc3) and updates decode/encode paths to convert between Any::Avc3 and Codec::Avc3. Also generalizes AVC sample entries via AvcSampleEntry<const KIND_CODE: u32> and adds FourCC::from_u32.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Initial avc3 support' directly summarizes the main change: adding avc3 atom support to the MP4 parser.
Description check ✅ Passed The description explains the PR's purpose (adding avc3 support), implementation approach (reusing avc1 codebase), and testing validation with live streams.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

[!TIP]

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions: | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context. Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 18 '25 20:11 coderabbitai[bot]

Looks generally OK. Didn't check the specs yet.

Its a bit annoying that we have to duplicate code like this, but not clear there is a significantly better way.

Can you fix the formatting issue?

I said they should start by copy-pasting AVC1 and sure enough, it decodes properly. But it's definitely not implementing the full spec and yeah the code reuse is frustrating.

Maybe until there's interest in full Avc3 support, a type alias might just work? pub type Avc3 = Avc1?

kixelated avatar Nov 18 '25 23:11 kixelated

Maybe until there's interest in full Avc3 support, a type alias might just work? pub type Avc3 = Avc1?

Not an expert in Rust, so after a quick Cursor investigation, I got the following answer:

  • Atom::KIND must be b"avc3" for Avc3 while Avc1 stays b"avc1". With a type alias there is only a single impl block, so we can’t publish two different constants or decode_body registrations. The dispatcher (Any, Codec) would see both boxes as avc1 and treat avc3 bytes as unknown.
  • Traits implemented for an alias are the same as for the underlying type, so we couldn’t even add a second impl Atom for Avc3. Rust forbids duplicate impls on aliases.

My latest commit looks to minimize code reuse and plan for future additions to other avc types by:

  • Adding one data structure AvcSampleEntry and one implementation for AVC sample entries, eliminating the previous wrapper struct and duplicate logic.
  • Adding future FourCCs (e.g., avc2/avc4) is a single-line alias instead of a new module or macro.
  • Public API stays the same (Avc1 { … } literals still work), so there’s no user-facing churn.

Happy to defer the decision to you guys! Or if we want to leave this open for now...

fcancela avatar Nov 20 '25 14:11 fcancela

Not an expert in Rust, so after a quick Cursor investigation, I got the following answer:

* Atom::KIND must be b"avc3" for Avc3 while Avc1 stays b"avc1". With a type alias there is only a single impl block, so we can’t publish two different constants or decode_body registrations. The dispatcher (Any, Codec) would see both boxes as avc1 and treat avc3 bytes as unknown.

* Traits implemented for an alias are the same as for the underlying type, so we couldn’t even add a second impl Atom for Avc3. Rust forbids duplicate impls on aliases.

I'm going to guess you have some background in an object-oriented language. Trying to force everything to be an object is making it hard to see alternatives. (Maybe I'm just projecting here, because I sometimes still have this problem).

I haven't actually tried it, but we can probably just turn the Visual parsing into a fn outside the impl block and call that.

bradh avatar Nov 21 '25 22:11 bradh

You guessed correctly 😂

I'll take a look at those suggestions during the weekend.

Thanks for the suggestions!

fcancela avatar Nov 22 '25 02:11 fcancela

I've fixed the formatting nit. The other parts are messier than I'd hoped, and won't get looked at until the weekend (UTC+11 time).

bradh avatar Nov 27 '25 23:11 bradh