refactor hvc1 and hev1 to shared implementation, add hvc2 and hev2#109
refactor hvc1 and hev1 to shared implementation, add hvc2 and hev2#109bradh wants to merge 1 commit intokixelated:mainfrom
Conversation
WalkthroughThis pull request refactors HEVC atom handling by consolidating four HEVC sample entry types (Hvc1, Hvc2, Hev1, Hev2) through a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs`:
- Around line 3-70: The match arms in the decode_body of the
define_hevc_sample_entry macro incorrectly use `.into()` to set Option fields
(e.g., hvcc, btrt, colr, pasp, taic, fiel, ccst); replace those assignments with
explicit Some(atom) (e.g., Any::Hvcc(atom) => hvcc = Some(atom)) inside the impl
Atom::decode_body for the generated types so each extracted atom is
idiomatically wrapped in Option; leave the
hvcc.ok_or(Error::MissingBox(Hvcc::KIND)) check as-is and do not change other
logic in encode_body or the Visual/Hvcc fields.
kixelated
left a comment
There was a problem hiding this comment.
Seems fine. Although I wonder if it would be possible to have an enum for the FourCC instead of copy-paste with a macro. Maybe I'll mock something up tomorrow?
|
Shouldn't the ccst box be available for basically all image/video codecs, not just HEVC boxes ? |
It is available for Uncv and Hvc1, and Hev1 was intended but omitted. I see no reason why
Could be, but I'm not aware of anyone doing (say) VP9 as a HEIF image sequence. So I think it only makes sense to support it in codecs where its valid / common. |
The code you added to to GStreamer's isomp4mux will happily produce image sequences with VP8 or VP9. The same would apply to the JPEG/J2K family as well |
I don't think I'll have time, and yeah it would be for naught if there are subtle differences between the boxes outside of the FourCC. |
Perhaps the VP8/VP9 code should get fixed in gstreamer then. JPEG and JPEG 2000 are valid (along with the AV1 and AVC cases). |
Are you OK with the macro implementation for now? |
I tried to find in the various specs why VP8/VP9 image sequences wouldn't be valid, or MPEG-4 Visual (Part 2), or even H.263 ? Reading the spec, there doesn't seem to have any restrictions on which codecs can be used for image sequences stored in tracks. |
I think its "is this codec valid in HEIF"? ISO 23008-12:2025 makes that the case for HEVC (see Annex B), AVC (see Annex E), JPEG (see Annex H), VVC (see Annex L) and EVC (see Annex M). ISO 15444-16:2025 Section 7 covers JPEG 2000. AVIF covers that at https://aomediacodec.github.io/av1-avif/#image-sequences I'm not aware of anything that makes VP8 or VP9 valid in HEIF. Equally, I'm not aware of anything that explicitly excludes them. I lean to "if its not in the standard, its outside it", although a different interpretation could well be valid. |
I missed the
ccstonhev1the first time around, so its probably better to have a shared implementation. There are more boxes to go in (e.g.lhvC).Took the opportunity to add
hvc2andhev2which are the same (at least from https://mpeggroup.github.io/FileFormatConformance/?query=%3D%22hvc2%22 ).If this works out, I'll do the
avc2,avc3, andavc4in the same way.