ruuda/claxon

A FLAC decoder in Rust

claxon's Issues

A blueprint for seeking feature

I'm planning to add seeking feature to this library, as I mentioned in #28 . This issue has been opened for tracking the design plan for this new feature.

The methods to add

First, we are going to add new constructors new_seekable and new_seekable_ext in impl <R: Read + Seek> FlacReader {. The purpose of creating a new struct is that we have to do an additional process right after reading all the metadata blocks: save the position of the reader. This enables the reader to determine the range for binary searching for every seek. This position is going to be stored in a new field of FlacReader, audio_block_start_position: Option<u64>.

Next, we are going to add new function seek to FrameReader and FlacSamples, both of which takes the sample number (0-indexed).

• FrameReader::seek seeks the reader to the beginning of the desired frame. Here, the "desired frame" is the last frame whose frame number is less than or equal to the given sample number (in case of fixed block size stream, it is automatically converted appropriately). Therefore, the next frame (this is equivalent to claxon::frame::Block, right?) yielded from FrameReader will include the desired sample (if the given sample number was less than the range of the length of the audio stream).
  • By default, this function uses binary search, assuming that any frame header is not broken (contains correct sample/frame number).
  • The range of search is between the beginning of audio frame (that was obtained in the constructor described above) and the end of the stream std::io::SeekFrom::End(0). However, if possible, this function uses SEEKTABLE metadata to narrow down the range, and thus speed up the seeking process.
    • By the way, I found that claxon::metadata::SeekTable does not expose any API to access the data. Shall I add some as well?
    • Also, I think that the data should be stored with something like BTreeMap rather than Vec.
  • For every step of binary searching, we find the first occurrence of FRAME_HEADER, by matching both sync code and 8-bit CRC provided at last. Calculating CRC-16 for entire frame makes it even more robust, but I think it is excessive work, and there are very low possibility that CRC-8 accidentally matches.
  • In the course of seeking, when the search range has been narrowed down enough, we switch to linear searching. This is in order to avoid searching frame header for almost the same place in the last phase of binary search (it is hard to explain with words; it's rather better to write some diagrams, but I'm reluctant to draw that. Tell me if you want more detailed explanation).
• FlacSamples::seek seeks the reader to the desired sample. That is, the next sample yielded from the iterator will precisely has the sample index that equal to the provided argument. Internally, it calls FrameReader::seek and then discards the first few frames in the first block obtained.

Concerns and future tasks

• As I explained above, audio_block_start_position will be stored as Option<u64>, as it may be vacant for non-seekable reader while filled for seekable one. But this is independent of whether the reader is actually seekable or not. That is, one can use the original constructor FrameReader::new while providing seekable reader, and then call seek function. So, the field may be None for the first time seek function is called, in which case we have to go back to the beginning of the stream and actually fill it with appropriate value. This may not be a big problem, but something that can be avoided by "type puzzle": For example, we can define a marker trait IsSeekable, create two marker structs Seekable and NonSeekable, accept S: IsSeekable as additional type parameter for FlacReader, FrameReader and FlacSamples, and define associated type IsSeekable::AudioBlockStartPosition which is set to () for NonSeekable and u64 for Seekable. Whew. Personally I love this kind of type puzzle but I doubt it's suitable for public library. Alternatively, we can define seekable variants for FlacReader, FrameReader and FlacSamples, which may make API less understandable and make code confusing.
• As a future task, we can provide another function that scans entire audio stream and build "complete" seek table. This allows us to avoid any binary searching, because we can jump to desired frame. However it's not a necessary feature, so I'll leave it laters and not implement it at first.

Thanks for reading this lengthy draft! Feel free to make questions or oppositions. I'll get started in a few days anyway.

About the metadata issue

Vorbis comment headers appear in quite many places, in vorbis, opus and in flac. Even though parsing them is only very little code, I think one could create a crate to do the parsing. What do you think? Would you use it? If yes, I'll extract the parsing code from my lewton crate :).

Support reading flac tag

Hello,
Is it possible to decode "flac tag" (or vorbis comments) with your library ?
It seems that it is exposed in the library through MetadataBlock, but I am not sure.
Thanks !

Some FLAC files don't decode because LPC order > 12 is not supported

I want to play some audio files in rust, so I used the rodio crate. Rodio uses claxon for decoding FLAC files, and some files didn't work with rodio.

When I tried the decode_simple.rs example's code from this repo, I got this error: thread 'main' panicked at 'failed to decode FLAC stream: Unsupported("LPC order > 12 is not supported")', src/libcore/result.rs:999:5. Here's a file that doesn't work. (Edit by @ruuda: link to private file removed.)

There is a fairly complicated workaround I came up with, here's the code. This method is, however, pretty slow. Can this feature be implemented in claxon?

MSRV for this crate?

I'm thinking of contributing this crate (specifically, add seek feature!). Do you have MSRV for this crate in mind? Also, do I have to stick to Edition 2015 instead of the latest 2018?

Reading on uninitialized memory may cause UB

Hello 🦀 ,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

• metadata::read_vorbis_comment_block() method

claxon/src/metadata.rs

Lines 432 to 438 in 2f05385

	let mut vendor_bytes = Vec::with_capacity(vendor_len as usize);
	// We can safely set the lenght of the vector here; the uninitialized memory
	// is not exposed. If `read_into` succeeds, it will have overwritten all
	// bytes. If not, an error is returned and the memory is never exposed.
	unsafe { vendor_bytes.set_len(vendor_len as usize); }
	try!(input.read_into(&mut vendor_bytes));

claxon/src/metadata.rs

Lines 473 to 475 in 2f05385

	let mut comment_bytes = Vec::with_capacity(comment_len as usize);
	unsafe { comment_bytes.set_len(comment_len as usize); }
	try!(input.read_into(&mut comment_bytes));

• metadata::read_application_block() method

claxon/src/metadata.rs

Lines 544 to 546 in 2f05385

	let mut data = Vec::with_capacity(length as usize - 4);
	unsafe { data.set_len(length as usize - 4); }
	try!(input.read_into(&mut data));

Methods metadata::read_vorbis_comment_block() & metadata::read_application_block() create an uninitialized buffer and passes it to user-provided ReadBytes implementation. This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).

This part from the Read trait documentation explains the issue:

It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

Suggested Fix

It is safe to zero-initialize the newly allocated u8 buffer before read_into(), in order to prevent user-provided Read from accessing old contents of the newly allocated heap memory.

Also, there are two nightly features for handling such cases.

• https://doc.rust-lang.org/std/io/struct.Initializer.html
• https://rust-lang.github.io/rfcs/2930-read-buf.html

Thank you for checking out this issue 👍

Support alternative containers

Flac doesn't have to be inside its own custom container, it can also be encapsulated inside ogg or inside the mp4 container. It would be cool if this crate supported those use cases, maybe through optional cargo features.

My lewton crate is organized in a very generic fashion: its main function only takes a byte slice and treats that as one packet, regardless of the actual container format. The entire crate knows nothing about the container format, except for the inside_ogg module. Having such a setup for the claxon crate would be really cool.

Emtpy Vorbis Comment

I got a few files containing tailing empty vorbis comment. I am aware that no "specification" explicitly allows this, but all applications (vlc, rythmbox, totem, my old mp3-player, etc.) that i tested, are able to correctly process these files.
So my suggestion would be to just ignore empty comments.

[wishlist] Drop the unsafe ensure_buffer_len()

It would be nice to refactor ensure_buffer_len() into safe code so that bugs such as #10 are statically ruled out by the compiler, without sacrificing performance.

This is interesting from the API design and best practices perspective. Writing data into a preallocated buffer is a very common thing to do in systems programming, so the compiler and stdlib should support a safe and idiomatic way to do it. This may be a good case study for unsafe code guidelines WG and/or the emerging security working group.

Some ideas to get the ball rolling:

1. Use buffer initialization such as vec![0, len] that the stdlib optimizes into as cheap memory zeroing as possible. Sadly this is still going to involve some overhead, unless we convince the optimizer that the entire buffer is always overwritten.
2. Instead of setting the buffer length in advance, preallocate a vector and use the Write trait or extend_from_slice() to safely write into it without zeroing memory.

version of hound required by claxon does not compile

Latest version of claxon requires hound 0.4.0, which does not compile.

I get the following errors:

Compiling hound v0.4.0
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84:5: 84:81 error: the trait `core::marker::Sized` is not implemented for the type `Self` [E0277]
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84     fn read<R: io::Read>(reader: &mut R, bytes: u16, bits: u16) -> Result<Self>;
                                                                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84:5: 84:81 help: run `rustc --explain E0277` to see a detailed explanation
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84:5: 84:81 note: `Self` does not have a constant size known at compile-time
path/.cargo/registry/src/github.com-88ac128001ac3a9a/hound-0.4.0/src/lib.rs:84:5: 84:81 note: required by `core::result::Result`
error: aborting due to previous error
Could not compile `hound`.

I'm using rust 1.7.0

FLAC file reading performance

Hello!

First thank you for this library. I've found it very approachable as a Rust beginner :)

I've written some code that reads a Flac file into memory as a Vec<f32> based off one of the given examples.

const CHANNELS: usize = 2;
type Sample = f32;
type Frame = [Sample; CHANNELS];
type Media = Vec<Frame>;

pub fn read_flac(path: String) -> Media {
    let mut reader = claxon::FlacReader::open(path).expect("Unable to open FLAC file");
    let mut samples = reader.samples();
    let capacity = (audio_engine::SAMPLE_HZ * 60.0 * 10.0) as usize;
    let mut frames = Vec::with_capacity(capacity);

    while let (Some(Ok(l)), Some(Ok(r))) = (samples.next(), samples.next()) {
        frames.push([l as Sample, r as Sample]);
    }

    frames.shrink_to_fit();
    frames
}

fn main() {
    println!("About to read file");
    let _media = media::read_flac("../media/music.flac".to_string());
    println!("Done");
}

When I run this it takes longer than I would expect.

louis ~/projects/decksterity/rust [master *] $ time cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/decksterity`
about to read file
done

real    0m21.641s
user    0m21.604s
sys     0m0.028s

Here the given input file was 26MiB and it took 22 seconds to read it.

This is slower than I would expect, other programs (not written by me) on the same machine seem to be able to read the entire file in less than a second.

Have I made a mistake in my implementation here?

Thanks,
Louis

Issues with privacy

When I wanted to update audrey to claxon version 0.3, I saw that claxon::FlacSamples now depends on only one type, which is generic on a Trait. Now there are a few problems:

1. in order for audrey to be able to store the samples struct, it needs to be generic with that type as well. But the type is private!
2. the only type that impls the trait is BufferedReader, which is opaque, too.

So what about a facade instead? If there was a public facade/wrapper for FrameReader which is generic on the io::Read trait, my problems would be solved.

Maybe there are further issues that can occur, not sure but if you pushed an updated version to github I will try it out :).

Frame CRC mismatch

I have a FLAC file with this frame: FF F8 C9 18 E0 B2 95 8E 01 80 01 00 00 03 28 92 followed by this frame: FF F8 C9 18 E0 B2 96 87 01 80 01 00 00 03 11 08.

Using flac --analyze I get the following for this 2 frames:

frame=3221	offset=26178049	bits=128	blocksize=4096	sample_rate=44100	channels=2	channel_assignment=INDEPENDENT
	subframe=0	wasted_bits=1	type=CONSTANT	value=1
	subframe=1	wasted_bits=0	type=CONSTANT	value=3
frame=3222	offset=26178065	bits=128	blocksize=4096	sample_rate=44100	channels=2	channel_assignment=INDEPENDENT
	subframe=0	wasted_bits=1	type=CONSTANT	value=1
	subframe=1	wasted_bits=0	type=CONSTANT	value=3

But with claxon I get the following:

Frame Number: FrameNumber(3221)
Block Size: 4096
Sample Rate: Some(44100)
Channel Assignment: Independent(2)
Bits per Sample: Some(16)
CRC8: 8e (Expected: 8e)

Subframe Type: Constant
Wasted Bits: 1
Constant Value: 2

Subframe Type: Constant
Wasted Bits: 0
Constant Value: 6

CRC16: 9200 (Expected: 92ff)

As you can see it parses wrong the two subframes, reading past the frame (it reads the value 92 FF for the frame CRC instead of 28 92 as it should).

Checking the reference implementation I found that the wasted bits are subtracted from the frames' bits per sample (source), something that claxon doesn't do and that's why it reads more bits than it should, resulting in frame CRC mismatch.

Adding this snippet

    let mut bps = bps;
    if header.wasted_bits_per_sample > 0 {
        bps -= header.wasted_bits_per_sample;
    }

here, the file is decoded fine, although there should be a check that the wasted bits are not more than the bits per sample.

Reading undecoded FLAC frames

I'm working on a thing that splits FLAC files with embedded CUE sheets into separate tracks, and would love to use this crate to do that: It's got most of the building blocks (a nice StreamInfo representation, good metadata support, a fine data model!), but it doesn't look like I can make use of the library getting decoded samples back.

Would this kind of usage mode be a useful contribution?

`read_next_or_eof`'s API prevents buffer reuse

Hi!
I'm trying to use claxon in an embedded environment and would like to reuse the decode buffer, but I'm not figuring out how to do this when I might have multiple frames to decode in a single buffer:

impl Decode for FlacDecoder {
    fn decode_sample(&mut self, buf: &[u8], out: &mut [i16]) -> Result<usize, anyhow::Error> {
        let mut fr = FrameReader::new(std::io::Cursor::new(buf));
        let mut c = 0;
        let mut v = std::mem::take(&mut self.dec_buf);
        loop {
            if let Ok(Some(block)) = fr.read_next_or_eof(v) {
                for (a, b) in block.stereo_samples() {
                    out[c + 0] = a as i16;
                    out[c + 1] = b as i16;
                    c += 2;
                }
                v = block.into_buffer();
            } else {
                break;
            }
        }
        // self.dec_buf = v;
        // can't do this, `v` was moved into  `read_next_or_eof` 
        // and both the `Err` and `Ok(None)` cases would lose the buffer
        Ok(c)
    }
}

I've made a branch with an API change which allows passing a mutable reference, but this breaks the FlacSamples and FlacIntoSamples iterators

How to use in async code

Hi,

Excellent library!

It's defined over io::Read but I need to use it on a WASM target that will receive data from javascript Promises. What would be the best way to use the library in an environment where future bytes may not exist yet?

Would returning io::ErrorKind::WouldBlock from io::Read make the reads for samples retryable?

Contents of uninitialized memory are leaked into the output on malformed inputs

On certain malformed inputs contents of uninitialized memory are written to the decoded audio.

This is a security vulnerability. Examples of similar vulnerabilities in C code and discussion of the potential impact can be found here. I have also discussed a similar bug in Rust png crate in my Auditing popular crates post.

This issue has been discovered using differential fuzzing with afl-fuzz, similar to the C vulnerabilities linked above. I shall relay further details on the issue to the maintainer privately by email.

The trivial hotfix is replacing the following line

claxon/src/frame.rs

Line 618 in b4a89e4

unsafe { buffer.set_len(new_len); }

with buffer.resize(new_len, 0);, but is likely to degrade performance. There are some tricks that can reduce the cost of zeroing the memory, but this approach is bound to be slower than using uninitialized memory.

However, these kinds of issues can be ruled out systemically by passing a reference to the vector to subframe::decode instead of a slice with uninitialized memory and using something like Write trait or extend_from_slice() to write to the vector safely without zeroing the memory first.

Once a fix is published, the issue should be added to the Rust security advisory database.

Last call on FLAC specification

Sorry for bothering you all this way, I really hope you don't mind.

To implement FLAC decoding in Claxon, you might have used the FLAC format specification on the FLAC website. If so, you probably needed to dive into some actual code (libFLAC or ffmpeg) to understand the finer points. An IETF working group has tried to improve this document, trying to include those finer points. In fact, I know @ruuda contributed to this a few years ago. However, all but a few lines have changed since and it is on the verge of being turned in for final checks and publication as an RFC. A so-called working group last call has been issued, which lasts 2 weeks.

As you've had first-hand experience implementing FLAC from scratch, you probably know best what implementation details needed extra attention. It would be very valuable to have your feedback on the FLAC format for this document.

I hope you can provide some insights.

The latest draft can be found here https://datatracker.ietf.org/doc/draft-ietf-cellar-flac/

Frame with block size shorter than 16 samples

I have a FLAC file encoded with the reference implementation (libFLAC) which has a frame (probably the last) with block size 8. The file has minimum and maximum block size of 4096 (fixed block size stream). All other frames have block size 4096.

When I parse it with claxon, though, I get the error invalid block size, must be at least 16.

If I understand correctly the specification (Note 3), this is allowed, as the last frame of a fixed block size stream can be shorter than the minimum block size defined in STREAMINFO block and thus shorter than 16 as well.

Consider adding encoding support

Given that Claxon is used by Rodio as it's FLAC support, I was wondering if ya'll have considered adding encoding support?

My use case is effectively that I want to convert a wav file to FLAC, but it doesn't seem that there is any Rust FLAC encoders out there yet.