Skip to content

Commit

Permalink
Fix test-short.ogg and chain-test3.ogg
Browse files Browse the repository at this point in the history
Fixes comparison to libvorbis for two sample
files by fixing two bugs in lewton:

1. In cases where the last header packet
   was directly followed by audio packets,
   without any page boundary,
   libvorbis just ignored those packets.
   lewton didn't so it treated the packets
   as normal content. This created a
   misalignment between lewton and libvorbis
   when parsing the sample files.
   Such a situation occured in test-short.ogg
   as well as chain-test3.ogg.
   The spec says that audio packets
   must start in a fresh page, so this
   means that the files are technically invalid.
   However, libvorbis parses them just fine.

2. Second, we move the last = 0 resetting into
   the outer loop. This is what the spec says,
   and it is also needed in order to fix
   the comparison to libvorbis for the two files.
   Otherwise, there'd be just silence, as the
   floor0 samples would assume crazy values.

As we rely on a new function that has been added
to the ogg crate for the very purpose of using
it in this commit, we bump the version
requirement for the ogg crate to 0.6.1.

Both files get fixed at once as
chain-test3.ogg is just a concatenation of
test-short.ogg with another file where no
mismatch to libvorbis was present.

Transparency notice: this commit marks the
first and hopefully last time I've ever looked
into the vorbis source code for my work on the
lewton crate. I think that this creates no change
in the licensing situation of the lewton crate.
The changes of this commit are so small that
they don't fall under copyright.

This commit addresses parts of #26.
  • Loading branch information
est31 committed Oct 27, 2018
1 parent 0333ce5 commit e2b38e8
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 15 deletions.
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ async_ogg = ["ogg", "ogg/async", "futures", "tokio-io"]
[dependencies]
byteorder = "1.0"
smallvec = "0.6"
ogg = { version = "0.6", optional = true }
ogg = { version = "0.6.1", optional = true }
tokio-io = { version = "0.1", optional = true }
futures = { version = "0.1", optional = true }

[dev-dependencies]
ogg = "0.6"
ogg = "0.6.1"
alto = "3"

[package.metadata.docs.rs]
Expand Down
6 changes: 3 additions & 3 deletions dev/cmp/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 11 additions & 6 deletions dev/cmp/tests/vals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,15 @@ fn test_libnogg_vals() {
cmp_output!("6ch-long-first-packet.ogg", 0);
cmp_output!("6ch-moving-sine-floor0.ogg", 0);
cmp_output!("6ch-moving-sine.ogg", 0);
ensure_malformed!("bad-continued-packet-flag.ogg", OggError(InvalidData));
// NOTE: The bad-continued-packet-flag.ogg test is
// actually supposed to return an error in libnogg.
// However, libvorbis doesn't, nor does lewton.
// Given a (slightly) erroneous ogg file where there
// are audio packets following the last header packet,
// we follow libvorbis behaviour and simply ignore those packets.
// Apparently the test case has been created in a way
// where this behaviour doesn't evoke an error from lewton.
cmp_output!("bad-continued-packet-flag.ogg", 0);
cmp_output!("bitrate-123.ogg", 0);
cmp_output!("bitrate-456-0.ogg", 0);
cmp_output!("bitrate-456-789.ogg", 0);
Expand Down Expand Up @@ -113,8 +121,7 @@ fn test_xiph_vals_2() {
// TODO fix these
//cmp_output!("chain-test1.ogg", 1);
cmp_output!("chain-test2.ogg", 0);
// stb_vorbis can't open this file at all (gives VORBIS_invalid_setup)
//cmp_output!("chain-test3.ogg", 1);
cmp_output!("chain-test3.ogg", 1);
cmp_output!("highrate-test.ogg", 0);
}

Expand Down Expand Up @@ -157,9 +164,7 @@ fn test_xiph_vals_5() {

cmp_output!("singlemap-test.ogg", 0);
cmp_output!("sleepzor.ogg", 9);
// TODO fix this test as well
// stb_vorbis can't open this file at all (gives VORBIS_invalid_setup)
cmp_output!("test-short.ogg", 69);
cmp_output!("test-short.ogg", 1);
cmp_output!("test-short2.ogg", 0);
// Contains an out of bounds mode index
ensure_malformed!("unused-mode-test.ogg", BadAudio(AudioBadFormat));
Expand Down
2 changes: 1 addition & 1 deletion src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,8 @@ fn lookup_vec_val_decode(lup :&CodebookVqLookup, codebook_entries :u32, codebook
}
}
} else {
let mut last = 0.;
for lookup_offset in 0 .. codebook_entries {
let mut last = 0.;
let mut multiplicand_offset :usize = lookup_offset as usize * codebook_dimensions as usize;
for _ in 0 .. codebook_dimensions {
let vec_elem = lup.codebook_multiplicands[multiplicand_offset] as f32 *
Expand Down
1 change: 1 addition & 0 deletions src/inside_ogg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub fn read_headers<'a, T: Read + Seek + 'a>(rdr: &mut PacketReader<T>) ->
let setup_hdr = try!(read_header_setup(&pck.data, ident_hdr.audio_channels,
(ident_hdr.blocksize_0, ident_hdr.blocksize_1)));

rdr.delete_unread_packets();
return Ok(((ident_hdr, comment_hdr, setup_hdr), pck.stream_serial));
}

Expand Down

0 comments on commit e2b38e8

Please sign in to comment.