Skip to content

Ogg Vorbis decoding fails to truncate final packet in short (single-page) files #106

@nyanpasu64

Description

@nyanpasu64

I found that when running lewton 0.10.2 on short audio files and pulling data using OggStreamReader::read_dec_packet_itl(), the tail does not properly get truncated so gapless playback/looping fails.

Screenshot_20241030_025545-sine

The issue is that if audio starts and stops in the first OggStreamReader::read_dec_packet_generic() call to return data, the OggStreamReader::cur_absgp (assumed to be initialized in the previous page and incremented per packet?) is None so decoded_pck.truncate never gets called.

To test this theory, I managed to get this .ogg file to loop properly by patching lewton:

@@ -190,7 +190,8 @@ impl<T: Read + Seek> OggStreamReader<T> {
 		// 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()) {
+		if pck.last_in_stream() {
+			let absgp = self.cur_absgp.unwrap_or(0);
 			let target_length = pck.absgp_page().saturating_sub(absgp) as usize;
 			decoded_pck.truncate(target_length);
 		}
@@ -198,6 +199,8 @@ impl<T: Read + Seek> OggStreamReader<T> {
 			self.cur_absgp = Some(pck.absgp_page());
 		} else if let &mut Some(ref mut absgp) = &mut self.cur_absgp {
 			*absgp += decoded_pck.num_samples() as u64;
+		} else {
+			self.cur_absgp = Some(decoded_pck.num_samples() as u64);
 		}
 
 		return Ok(Some(decoded_pck));

The program appears to still decode longer files properly, but I did not look into how it affects seeking or such.

I'm guessing that we should assume cur_absgp starts at 0 for single-page audio files (or the first page of any audio file?). That seems like the most obvious way to make short .ogg files play at the same length as I exported it from in Audacity, and loops seamlessly in Audacious. I'm not sure how libvorbis handles this case.

Perhaps prior to the first page end (when we don't know the "starting" granule position), this "zero-based" absgp should not be made visible to the user (OggStreamReader::get_last_absgp() returns None still), but only tracked internally? (My proof-of-concept hack does not achieve this.)

Possibly related: #94 seems to rewrite the code in ways different from my ideas, and I'm not sure how it would interact with short files.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions