Skip to content

Commit

Permalink
Fix chain-test1.ogg decoding mismatch
Browse files Browse the repository at this point in the history
Prior to this commit, the individual
streams that make up chain-test1.ogg
all had no difference to libvorbis
in isolation, but when their concatenation
was played back, a decoding mismatch
occured for every packet of the second
stream.
The issue was that the last packet of
the first stream should have been shorter
than the one lewton outputted.
E.g. if you put code like

if n == 2022 {
	dec_data.truncate(1044);
}

right before the let mut diffs = 0;
in the `cmp_output` function,
you'd get a match because that'd
have leveled out the mismatch.

The vorbis spec says that the absgp
of each page marks the last sample
inside the last packet that ends
it. It also says that if the absgp
of the last page of the entire
stream is before the actually
decoded length of the last packet,
truncation is neccessary.

This truncation was the cause for
the mismatch as vorbis implemented
and we didn't.
This commit implements the truncation
and thus fixes the mismatch for
chain-test1.ogg.

Addresses parts of #26.
  • Loading branch information
est31 committed Oct 28, 2018
1 parent 310f34c commit dbfaae5
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 10 deletions.
3 changes: 1 addition & 2 deletions dev/cmp/tests/vals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,7 @@ fn test_xiph_vals_2() {
println!();

cmp_output!("bimS-silence.ogg", 0);
// TODO fix these
//cmp_output!("chain-test1.ogg", 1);
cmp_output!("chain-test1.ogg", 0);
cmp_output!("chain-test2.ogg", 0);
cmp_output!("chain-test3.ogg", 1);
cmp_output!("highrate-test.ogg", 0);
Expand Down
37 changes: 29 additions & 8 deletions src/inside_ogg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub struct OggStreamReader<T: Read + Seek> {
pub comment_hdr :CommentHeader,
pub setup_hdr :SetupHeader,

absgp_of_last_read :Option<u64>,
cur_absgp :Option<u64>,
}

impl<T: Read + Seek> OggStreamReader<T> {
Expand Down Expand Up @@ -96,7 +96,7 @@ impl<T: Read + Seek> OggStreamReader<T> {
comment_hdr,
setup_hdr,
stream_serial,
absgp_of_last_read : None,
cur_absgp : None,
});
}
pub fn into_inner(self) -> PacketReader<T> {
Expand Down Expand Up @@ -126,7 +126,7 @@ impl<T: Read + Seek> OggStreamReader<T> {
self.comment_hdr = comment_hdr;
self.setup_hdr = setup_hdr;
self.stream_serial = pck.stream_serial();
self.absgp_of_last_read = None;
self.cur_absgp = None;

// Now, read the first audio packet to prime the pwr
// and discard the packet.
Expand All @@ -136,7 +136,7 @@ impl<T: Read + Seek> OggStreamReader<T> {
};
let _decoded_pck = try!(read_audio_packet(&self.ident_hdr,
&self.setup_hdr, &pck.data, &mut self.pwr));
self.absgp_of_last_read = Some(pck.absgp_page());
self.cur_absgp = Some(pck.absgp_page());

return Ok(try!(self.rdr.read_packet()));
} else {
Expand All @@ -161,9 +161,30 @@ impl<T: Read + Seek> OggStreamReader<T> {
Some(p) => p,
None => return Ok(None),
};
let decoded_pck = try!(read_audio_packet(&self.ident_hdr,
let mut decoded_pck = try!(read_audio_packet(&self.ident_hdr,
&self.setup_hdr, &pck.data, &mut self.pwr));
self.absgp_of_last_read = Some(pck.absgp_page());

// If this is the last packet in the logical bitstream,
// we need to truncate it so that its ending matches
// the absgp of the current page.
// This is what the spec mandates and also the behaviour
// of libvorbis.
if let (Some(absgp), true) = (self.cur_absgp, pck.last_in_stream()) {
let target_length = pck.absgp_page().saturating_sub(absgp) as usize;
for ch in decoded_pck.iter_mut() {
if target_length < ch.len() {
ch.truncate(target_length);
}
}
}
if pck.last_in_page() {
self.cur_absgp = Some(pck.absgp_page());
} else if let &mut Some(ref mut absgp) = &mut self.cur_absgp {
if let Some(v) = decoded_pck.get(0) {
*absgp += v.len() as u64;
}
}

return Ok(Some(decoded_pck));
}
/// Reads and decompresses an audio packet from the stream (interleaved).
Expand Down Expand Up @@ -210,7 +231,7 @@ impl<T: Read + Seek> OggStreamReader<T> {
/// In the case of ogg/vorbis, the absolute granule position is given
/// as number of PCM samples, on a per channel basis.
pub fn get_last_absgp(&self) -> Option<u64> {
self.absgp_of_last_read
self.cur_absgp
}

/// Seeks to the specified absolute granule position, with a page granularity.
Expand All @@ -223,7 +244,7 @@ impl<T: Read + Seek> OggStreamReader<T> {
pub fn seek_absgp_pg(&mut self, absgp :u64) -> Result<(), VorbisError> {
try!(self.rdr.seek_absgp(None, absgp));
// Reset the internal state after the seek
self.absgp_of_last_read = None;
self.cur_absgp = None;
self.pwr = PreviousWindowRight::new();
Ok(())
}
Expand Down

0 comments on commit dbfaae5

Please sign in to comment.