setup of riff module, support for PCM aiff #193
Conversation
symphonia-format-riff/src/chunks.rs
Outdated
| // | ||
| // Select the appropriate codec using bits per sample. Samples are always interleaved and | ||
| // little-endian encoded for the PCM format. | ||
| let codec = match bits_per_sample { |
There was a problem hiding this comment.
As mentioned, setting bits_per_coded_sample and bits_per_sample correctly in the CodecParameters will make PcmDecoder properly read non-multiple-of-8 samples. The codec type will need to be round up to the nearest multiple, though (e.g., 17 bits = CODEC_TYPE_S24_BE).
There was a problem hiding this comment.
I tried this and it does allow for playback, but the audio is clipping. I manually compared the values in the binary and what Symphonia was reading as samples, and it seemed to be multiplied by 16, but i haven't spent that much time digging into it
The CodecParameter value was properly set to 12 though, so something somewhere else is misbehaving.
There was a problem hiding this comment.
Got it, it could be the decoder then.
symphonia-format-riff/src/chunks.rs
Outdated
| let mut sample_rate: [u8; 10] = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]; | ||
| let _res = reader.read_buf(sample_rate.as_mut()); | ||
| let sample_rate = Extended::from_be_bytes(sample_rate); | ||
| let sample_rate = sample_rate.to_f64() as u32; |
There was a problem hiding this comment.
I guess 0 is valid sample rate?? It doens't make much sense, but the spec doesn't mention much about this field. Not really sure how to validate this value haha.
There was a problem hiding this comment.
I'd probably assume a sample rate of 0 is an error. At least, it's unplayable since samples would never be consumed.
symphonia-format-riff/src/lib.rs
Outdated
| const AIFC_RIFF_FORM: [u8; 4] = *b"AIFC"; | ||
|
|
||
| /// The maximum number of frames that will be in a packet. | ||
| /// Took this from symphonia-format-wav, but I don't know if it's correct. |
There was a problem hiding this comment.
It's arbitrary since there are no packets in WAVE. 1152 is the same as MP3.
There was a problem hiding this comment.
Updated comment to reflect this
symphonia/src/lib.rs
Outdated
| @@ -161,6 +161,8 @@ pub mod default { | |||
| pub use symphonia_format_mkv::MkvReader; | |||
| #[cfg(feature = "ogg")] | |||
| pub use symphonia_format_ogg::OggReader; | |||
| #[cfg(feature = "riff")] | |||
| pub use symphonia_format_riff::RiffReader; | |||
There was a problem hiding this comment.
I think you are still experimenting with this. But presumably this should be an aiff feature, enabling the AiffReader.
There was a problem hiding this comment.
Hmm yes, i did it like because in this iteration, the RiffReader only supports Aiff. In an updated version that supports AIFC and lays some groundwork to get WAVE in this structure, there is/should be more separate readers.
Maybe i can even call the feature "aiff" but use the RiffReader. Another option is to rename it here to AiffReader, that bit more clean perhaps.
There was a problem hiding this comment.
Alright, i ended up doing it the way you posted here.
In another branch i extracted similar logic from symphonia-format-aiff and symphonia-format-wav to get rid of the copy pasting while still having separate features . Maybe it's best i also incorporate that commit into this branch before we merge. It will make the MR slightly more complex.
There was a problem hiding this comment.
Sorry, I'm a bit confused. I think there are 3 ways forward:
- Have a
symphonia-format-aiffcrate with a single reader:AiffReader. KeepWaveReaderin it's ownsymphonia-format-wavecrate. Other RIFF based formats would also have their own crates and readers. - Have generic RIFF crate
symphonia-format-riffwith a singleRiffReaderthat can read AIFF, WAVE, etc. - Have a generic RIFF crate
symphonia-format-riffwith multiple readers:AiffReader,WaveReader, etc.
I got the impression that we narrowed our options between 2 and 3, but it seems you landed on 1 in the current iteration. Have I missed anything?
There was a problem hiding this comment.
As discussed, 3 it is!
|
You'll need a nightly toolchain to properly format the code! Once you install the nightly toolchain you can run |
|
Thanks for your work on this! |
I set up a RIFF module to support AIFF files. It's heavily based on the wav module.
As discussed, in a future release the WAVE logic should be moved in to this RIFF module, since both WAVE and AIFF are contained in a RIFF file