-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
common: rename utils-xiph crate to common #318
Conversation
Ah, I hate do this to you, but I actually also have a WIP in commit for MKV that's basically this (including the HEVCDecoderConfigurationRecord) plus a bit more. I thought you were only interested in working on MP4 so I didn't push it. This is my last WIP commit though.. 😆 I'm not sure if a rebase is worthwhile in this case. I can push it later today if that's okay with you. Thing is, if you are interested in integrating in both MP4 and MKV, we may need to think about code deduplication. While these record parsers are simple right now, I feel like the entire record should be validated which will increase the complexity. Likewise, any NAL parsing that both the demuxer and (possible) future decoders do should be shared. My thought is that we will need a |
:-) no problem at all. I still learn the code. Hope you don't have plans for .ts files. I have some of those too. I will wait for your commit to fix some exceptional cases from my mkv files. |
Thanks! I pushed my MKV changes.
Yeah, this looks a bit weird. It's because MKV integers are variable width, but the maximum width is 64-bit. So the name reflects the largest possible integer it can return.
Can't say that one crossed my mind, but I do have a few laying around. In terms of additional formats, I've been particularly interested in seeing AVI and FLV added.
Hmm, that's odd, but perhaps not surprising. Movies usually use DTS/DCA and AC3 codecs which we don't support. I'm also particularly interested in seeing support for those as well. |
It might also contain VP8/VP9 codec configuration box parsers or other non mpeg parsers that might be reused across formats.
|
Hmm, yeah, so there are really two ways this could go:
If we go the latter, I'd like to see If we go with option 2, then here are some names I think are decent alternatives, but open to others:
If we go option 1, then the names can be adapted per family:
What do you think? |
I prefer the second option—one crate with multiple modules.
However, someone might find them useful if they want to implement their own format. "common" seems too generic as a name. While this might be fine at this stage, when it comes to code reuse, there could be multiple levels:
The initial intention was to have shared code just for formats, possibly named something like
Since this library was originally developed for audio, I imagine people will want the option to remove non-audio code during compilation. At the same time, things like Dolby Vision box parsing or HDR detection are common across different codecs like AV2, VP9, HEVC, and VVC but maybe too small for a dedicated module. Please suggest module names. For video, I'd include all related codecs in a single module for now to avoid creating too many small modules, not sure if you want the same for audio. |
Yeah, someone might. Though, in this case, we will need to be mindful of documenting the crate and to keep that use-case in mind while developing it. The current Xiph utils crate is not great in this regard.
My thought was that items in the former would be in sub-modules of But I will agree that "common" is indeed very generic. However, I'm struggling to come up with a good name.
I don't think a common crate for formats would solve the problem because some stuff will be used by both demuxers and decoders. For example, Initially, this was actually why I suggested individual crates per family. It side stepped the problem:
I'm still partial towards this solution. Unless you see anything significantly wrong with it, we could probably just go with it. We can loop back around before 0.6 is released to determine if we should merge them into one crate or not.
I was just thinking of a simple hierarchy like this:
I don't know enough about the video codecs to comment about any deeper levels for those. However, for audio, the above is sufficient. If multiple crates are used, then the family will be namespaced by the crate so it can be omitted from the hierarchy. |
Ok. I just renamed of utils-xiph to common, no cfg options. I would like to have small PRs for each independent change if you don't mind. Let me know if you prefer bigger PR with several commits. |
@sscobici, LGTM, merged! Thanks for taking care of this. I made a note on my todo to loop back around to this later and add the feature flags. We can also then see if we're still happy with the naming. For future PRs, I would prefer bigger PRs with a clean commit history that I can apply directly to the |
Adding basic video detection for MKV files, there are more fixes needed for some files, but want to keep those in separate PRs.
Please note that I copied the code for AVCDecoderConfigurationRecord and HEVCDecoderConfigurationRecord parsing from the isomp4 to avoid adding additional dependencies to the format crate.