Add initial support for Core Audio Format / CAF files #232
Conversation
|
I tested it with files from https://filesamples.com/formats/caf and https://www.dwsamplefiles.com/download-caf-sample-files/. Code also looks good to me. Good stuff. |
|
Thanks submitting this, I can see a lot of work was put into it and we'd be happy to have it. Generally, I think it's okay to pull in a decoder/demuxer with a minimum set of functionality. It looks like you have PCM encapsulation down, so this would be a good start. Unfortunately, I have no knowledge or experience with CAF, so would you be willing to address any bugs that come up with it (we could assign them to you)? Also, would you like to continue extending the support to other encapsulated codecs? |
|
Hi @dedobbin + @pdeljanov, thanks for taking a look, and I'm glad to hear that you'd be happy to accept the PR.
Compressed codec support is implemented (see the last commit), but maybe I misunderstood something? What's missing from this implementation is support for metadata chunks like markers or annotations, but I wasn't sure which features would be available through Symphonia, and I figured that support should be added with specific use cases in mind.
Sure, I'd be happy to take a look if/when bugs come up, and I can review PRs with feature additions if you like. |
|
@dedobbin I rebased the branch onto the latest master because it's been a few months (things still seem to be working correctly!), and I also fixed some clippy warnings. |
|
Just a heads up, the format check is failing because our config requires the nightly toolchain to adjust the bracket style. If you install the nightly toolchain you can use it by adding
Got it! I probably just misunderstood what you mean't! |
|
@pdeljanov I've run |
There was a problem hiding this comment.
I've completed a more thorough review of this PR. While going over this I used Apple's specification as a reference.
Overall, I think this is very nicely done! Hopefully we can get this in very soon. 😄
Most of my concerns are related to data validation. The media stream should be considered untrustworthy and even malicious. Therefore, it's important we don't blindly trust what is being read. Less sinister scenarios could include damaged files. Ultimately, we don't ever want to panic or get thrown into an infinite loop due to bad input. Consider checking out the fuzzing setup used for MP3 if you really want to stress your code. That being said, I'm not going to gate this review on fuzz tests.
symphonia-format-caf/src/chunks.rs
Outdated
| format_id, | ||
| bytes_per_packet: reader.read_be_u32()?, | ||
| frames_per_packet: reader.read_be_u32()?, | ||
| channels_per_frame: reader.read_be_u32()?, |
There was a problem hiding this comment.
It is illegal for channels_per_frame to be 0.
There was a problem hiding this comment.
There's a check for the channel count being zero in CafReader::read_audio_description_chunk(), but I think it makes sense to report an error as soon as possible. Does it make sense to leave the extra check in place (I guess the alternative is replacing it with unreachable!())?
There was a problem hiding this comment.
Either way works for me.
In such cases, I think I'd only strongly prefer the latter over the former if the additional check was in a hot code path. Mainly because we already setup the precondition that the field will never be 0 so why waste the cycles? However, I don't think FormatReaders are ever really "hot" so you can choose whatever you think is best for maintainability.
There was a problem hiding this comment.
I think for maintainability replacing the error with an assertion makes sense, otherwise the maintainer is left wondering if there's ever a case where the first check could pass and then the second check could fail. There's no such case, so an assertion communicates that more clearly.
symphonia-format-caf/src/demuxer.rs
Outdated
| } | ||
| else { | ||
| error!("Invalid packet index: {}", current_packet_index); | ||
| return decode_error("Invalid packet index"); |
There was a problem hiding this comment.
We have some conventions regarding the messages used for error message and logging. Use only use lowercase for both types.
For errors, since the source module is not known, please prefix the format or decoder name. In this case, please use the prefix caf: . You don't need to do this with the logging macros because they record the module emitting the log.
Please adjust accordingly.
There was a problem hiding this comment.
Done, although I couldn't build with 1.53 to check that I haven't used any other post-1.53 features (there's a problem with finding a supported version of regex).
There was a problem hiding this comment.
Ah, I see. The regex crate is being pulled in by env_logger which is used by symphonia-play and symphonia-check, but not the main libraries. I tried building with 1.53.0 though, and it seems the log crate now requires 1.60.0, so this is unfortunate. We can ignore this then.
|
Thanks for the review @pdeljanov, I've followed up on your feedback and pushed the changes, although I haven't double checked against my test files yet. I should have time to check them out over the weekend, I'll follow up when I'm done. |
symphonia-format-caf/src/chunks.rs
Outdated
| let chunk_size_u64 = chunk_size as u64; | ||
| let edit_count_offset = size_of::<u32>() as u64; | ||
|
|
||
| if chunk_size != -1 && chunk_size_u64 < edit_count_offset { |
There was a problem hiding this comment.
Suggestion: I feel this check is a bit too clever and harms readability a bit. Atleast until one recalls signed two's complement. Consider doing it the more "naive" way.
There was a problem hiding this comment.
You're right, I've tried to simplify this a bit.
symphonia-format-caf/src/chunks.rs
Outdated
| format_id, | ||
| bytes_per_packet: reader.read_be_u32()?, | ||
| frames_per_packet: reader.read_be_u32()?, | ||
| channels_per_frame: reader.read_be_u32()?, |
There was a problem hiding this comment.
Either way works for me.
In such cases, I think I'd only strongly prefer the latter over the former if the additional check was in a hot code path. Mainly because we already setup the precondition that the field will never be 0 so why waste the cycles? However, I don't think FormatReaders are ever really "hot" so you can choose whatever you think is best for maintainability.
|
Hey @irh, Thanks, looks good to me! We can merge this when you're ready. Before then, you should also add yourself to the |
|
OK thanks for taking the time to review this and for the great feedback, I think we're good to go 👍 |
|
Sounds good @irh! Do you mind re-basing this to resolve the conflicts? |
Done 👍 |
|
Merged! Thank you for the work you put into this! |
This PR introduces support for decoding
.caffiles.The implementation is derived from the spec, and tested against a local collection of audio files that use various encoding formats.
I haven't added decoding support for all chunk types, but hopefully this PR acts as a good starting point for further development?
Things seem to be working correctly, but please let me know if I've misunderstood some aspect of the format reader API, I'll be happy to make changes as needed.