From 579044882be3cceb8031882cc1fbf448e10a41d0 Mon Sep 17 00:00:00 2001 From: TonalidadeHidrica <47710717+TonalidadeHidrica@users.noreply.github.com> Date: Sun, 22 Nov 2020 02:56:01 +0900 Subject: [PATCH 1/6] Simplify the logic of finding a new page header --- src/reading.rs | 199 +++---------------------------------------------- 1 file changed, 12 insertions(+), 187 deletions(-) diff --git a/src/reading.rs b/src/reading.rs index 53ddd7e..50bf8e7 100644 --- a/src/reading.rs +++ b/src/reading.rs @@ -12,7 +12,7 @@ Reading logic use std::error; use std::io; -use std::io::{Cursor, Read, Write, SeekFrom, Error, ErrorKind}; +use std::io::{Cursor, Write, SeekFrom, Error, ErrorKind}; use byteorder::{ReadBytesExt, LittleEndian}; use std::collections::HashMap; use std::collections::hash_map::Entry; @@ -20,7 +20,6 @@ use std::fmt::{Display, Formatter, Error as FmtError}; use std::mem::replace; use crc::vorbis_crc32_update; use Packet; -use std::io::Seek; /// Error that can be raised when decoding an Ogg transport. #[derive(Debug)] @@ -59,7 +58,7 @@ impl error::Error for OggReadError { fn cause(&self) -> Option<&dyn error::Error> { match *self { - OggReadError::ReadError(ref err) => Some(err as &error::Error), + OggReadError::ReadError(ref err) => Some(err as &dyn error::Error), _ => None } } @@ -528,180 +527,6 @@ impl BasePacketReader { } } -#[derive(Clone, Copy)] -enum UntilPageHeaderReaderMode { - Searching, - FoundWithNeeded(u8), - SeekNeeded(i32), - Found, -} - -enum UntilPageHeaderResult { - Eof, - Found, - ReadNeeded, - SeekNeeded, -} - -struct UntilPageHeaderReader { - mode :UntilPageHeaderReaderMode, - /// Capture pattern offset. Needed so that if we only partially - /// recognized the capture pattern, we later on only check the - /// remaining part. - cpt_of :u8, - /// The return buffer. - ret_buf :[u8; 27], - read_amount :usize, -} - -impl UntilPageHeaderReader { - pub fn new() -> Self { - UntilPageHeaderReader { - mode : UntilPageHeaderReaderMode::Searching, - cpt_of : 0, - ret_buf : [0; 27], - read_amount : 0, - } - } - /// Returns Some(off), where off is the offset of the last byte - /// of the capture pattern if it's found, None if the capture pattern - /// is not inside the passed slice. - /// - /// Changes the capture pattern offset accordingly - fn check_arr(&mut self, arr :&[u8]) -> Option { - for (i, ch) in arr.iter().enumerate() { - match *ch { - b'O' => self.cpt_of = 1, - b'g' if self.cpt_of == 1 || self.cpt_of == 2 => self.cpt_of += 1, - b'S' if self.cpt_of == 3 => return Some(i), - _ => self.cpt_of = 0, - } - } - return None; - } - /// Do one read exactly, and if it was successful, - /// return Ok(true) if the full header has been read and can be extracted with - /// - /// or return Ok(false) if the - pub fn do_read(&mut self, mut rdr :R) - -> Result { - use self::UntilPageHeaderReaderMode::*; - use self::UntilPageHeaderResult as Res; - // The array's size is freely choseable, but must be > 27, - // and must well fit into an i32 (needs to be stored in SeekNeeded) - let mut buf :[u8; 1024] = [0; 1024]; - - let rd_len = tri!(rdr.read(if self.read_amount < 27 { - // This is an optimisation for the most likely case: - // the next page directly follows the current read position. - // Then it would be a waste to read more than the needed amount. - &mut buf[0 .. 27 - self.read_amount] - } else { - match self.mode { - Searching => &mut buf, - FoundWithNeeded(amount) => &mut buf[0 .. amount as usize], - SeekNeeded(_) => return Ok(Res::SeekNeeded), - Found => return Ok(Res::Found), - } - })); - - if rd_len == 0 { - // Reached EOF. - if self.read_amount == 0 { - // If we have read nothing yet, there is no data - // but ogg data, meaning the stream ends legally - // and without corruption. - return Ok(Res::Eof); - } else { - // There is most likely a corruption here. - // I'm not sure, but the ogg spec doesn't say that - // random data past the last ogg page is allowed, - // so we just assume it's not allowed. - tri!(Err(OggReadError::NoCapturePatternFound)); - } - } - self.read_amount += rd_len; - - // 150 kb gives us a bit of safety: we can survive - // up to one page with a corrupted capture pattern - // after having seeked right after a capture pattern - // of an earlier page. - let read_amount_max = 150 * 1024; - if self.read_amount > read_amount_max { - // Exhaustive searching for the capture pattern - // has returned no ogg capture pattern. - tri!(Err(OggReadError::NoCapturePatternFound)); - } - - let rd_buf = &buf[0 .. rd_len]; - - use std::cmp::min; - let (off, needed) = match self.mode { - Searching => match self.check_arr(rd_buf) { - // Capture pattern found - Some(off) => { - self.ret_buf[0] = b'O'; - self.ret_buf[1] = b'g'; - self.ret_buf[2] = b'g'; - self.ret_buf[3] = b'S'; // (Not actually needed) - (off, 24) - }, - // Nothing found - None => return Ok(Res::ReadNeeded), - }, - FoundWithNeeded(needed) => { - (0, needed as usize) - }, - _ => unimplemented!(), - }; - - let fnd_buf = &rd_buf[off..]; - - let copy_amount = min(needed, fnd_buf.len()); - let start_fill = 27 - needed; - (&mut self.ret_buf[start_fill .. copy_amount + start_fill]) - .copy_from_slice(&fnd_buf[0 .. copy_amount]); - if fnd_buf.len() == needed { - // Capture pattern found! - self.mode = Found; - return Ok(Res::Found); - } else if fnd_buf.len() < needed { - // We still have to read some content. - let needed_new = needed - copy_amount; - self.mode = FoundWithNeeded(needed_new as u8); - return Ok(Res::ReadNeeded); - } else { - // We have read too much content (exceeding the header). - // Seek back so that we are at the position - // right after the header. - - self.mode = SeekNeeded(needed as i32 - fnd_buf.len() as i32); - return Ok(Res::SeekNeeded); - } - } - pub fn do_seek(&mut self, mut skr :S) - -> Result { - use self::UntilPageHeaderReaderMode::*; - use self::UntilPageHeaderResult as Res; - match self.mode { - Searching | FoundWithNeeded(_) => Ok(Res::ReadNeeded), - SeekNeeded(offs) => { - tri!(skr.seek(SeekFrom::Current(offs as i64))); - self.mode = Found; - Ok(Res::Found) - }, - Found => Ok(Res::Found), - } - } - pub fn into_header(self) -> [u8; 27] { - use self::UntilPageHeaderReaderMode::*; - match self.mode { - Found => self.ret_buf, - _ => panic!("wrong mode"), - } - } -} - /** Reader for packets from an Ogg stream. @@ -767,18 +592,18 @@ impl PacketReader { /// Ok(None) is returned if the stream has ended without an uncompleted page /// or non page data after the last page (if any) present. fn read_until_pg_header(&mut self) -> Result, OggReadError> { - let mut r = UntilPageHeaderReader::new(); - use self::UntilPageHeaderResult::*; - let mut res = tri!(r.do_read(&mut self.rdr)); - loop { - res = match res { - Eof => return Ok(None), - Found => break, - ReadNeeded => tri!(r.do_read(&mut self.rdr)), - SeekNeeded => tri!(r.do_seek(&mut self.rdr)) + let mut ret = [0u8; 27]; + ret[..4].copy_from_slice(b"OggS"); + let mut len = 0; + while len < 4 { + match (tri!(self.rdr.read_u8()), len) { + (b'O', _) => len = 1, + (b'g', 1) | (b'g', 2) | (b'S', 3) => len += 1, + _ => len = 0, } } - Ok(Some(r.into_header())) + tri!(self.rdr.read_exact(&mut ret[4..27])); + Ok(Some(ret)) } /// Parses and reads a new OGG page From 22c31379709866018729e29baed125bc07f057b2 Mon Sep 17 00:00:00 2001 From: TonalidadeHidrica <47710717+TonalidadeHidrica@users.noreply.github.com> Date: Sun, 22 Nov 2020 04:45:59 +0900 Subject: [PATCH 2/6] Remove requirement for io::Seek for reader --- src/reading.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/reading.rs b/src/reading.rs index 50bf8e7..5d22556 100644 --- a/src/reading.rs +++ b/src/reading.rs @@ -537,13 +537,13 @@ consistent when it encounters the `WouldBlock` error kind. If you desire async functionality, consider enabling the `async` feature and look into the async module. */ -pub struct PacketReader { +pub struct PacketReader { rdr :T, base_pck_rdr :BasePacketReader, } -impl PacketReader { +impl PacketReader { /// Constructs a new `PacketReader` with a given `Read`. pub fn new(rdr :T) -> PacketReader { PacketReader { rdr, base_pck_rdr : BasePacketReader::new() } @@ -628,7 +628,9 @@ impl PacketReader { Ok(Some(tri!(pg_prs.parse_packet_data(packet_data)))) } +} +impl PacketReader { /// Seeks the underlying reader /// /// Seeks the reader that this PacketReader bases on by the specified From a197365188047079738e33a41f33874a1798571e Mon Sep 17 00:00:00 2001 From: TonalidadeHidrica <47710717+TonalidadeHidrica@users.noreply.github.com> Date: Sun, 22 Nov 2020 06:30:40 +0900 Subject: [PATCH 3/6] Add additional notes about the reader on docs --- src/reading.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/reading.rs b/src/reading.rs index 5d22556..b9554c5 100644 --- a/src/reading.rs +++ b/src/reading.rs @@ -536,6 +536,9 @@ This reader is not async ready. It does not keep its internal state consistent when it encounters the `WouldBlock` error kind. If you desire async functionality, consider enabling the `async` feature and look into the async module. + +The reader passed to this packet reader should be buffered properly, +since `Read::read` will be called many times. */ pub struct PacketReader { rdr :T, From 5f4137498c8fb2802956cbbacf6911406c3791df Mon Sep 17 00:00:00 2001 From: TonalidadeHidrica <47710717+TonalidadeHidrica@users.noreply.github.com> Date: Mon, 23 Nov 2020 22:47:30 +0900 Subject: [PATCH 4/6] Return None if no new page header is fully available ...instead of returning an error. --- src/reading.rs | 45 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/src/reading.rs b/src/reading.rs index b9554c5..6c233e0 100644 --- a/src/reading.rs +++ b/src/reading.rs @@ -20,6 +20,7 @@ use std::fmt::{Display, Formatter, Error as FmtError}; use std::mem::replace; use crc::vorbis_crc32_update; use Packet; +use std::io::Read; /// Error that can be raised when decoding an Ogg transport. #[derive(Debug)] @@ -527,6 +528,23 @@ impl BasePacketReader { } } +#[derive(Default)] +struct MagicFinder { + len: usize, +} +impl MagicFinder { + fn feed(&mut self, b: u8){ + match (b, self.len) { + (b'O', _) => self.len = 1, + (b'g', 1..=2) | (b'S', 3) => self.len += 1, + _ => self.len = 0, + } + } + fn found(&self) -> bool { + self.len == 4 + } +} + /** Reader for packets from an Ogg stream. @@ -597,16 +615,27 @@ impl PacketReader { fn read_until_pg_header(&mut self) -> Result, OggReadError> { let mut ret = [0u8; 27]; ret[..4].copy_from_slice(b"OggS"); - let mut len = 0; - while len < 4 { - match (tri!(self.rdr.read_u8()), len) { - (b'O', _) => len = 1, - (b'g', 1) | (b'g', 2) | (b'S', 3) => len += 1, - _ => len = 0, + let mut finder = MagicFinder::default(); + while !finder.found() { + let next = match self.rdr.by_ref().bytes().next() { + Some(b) => tri!(b), + None => return Ok(None), + }; + finder.feed(next); + } + let mut target = &mut ret[4..]; + loop { + let read = tri!(self.rdr.read(target)); + if read == 0 { + break; } + target = &mut target[read..]; + } + if target.is_empty() { + Ok(Some(ret)) + } else { + Ok(None) } - tri!(self.rdr.read_exact(&mut ret[4..27])); - Ok(Some(ret)) } /// Parses and reads a new OGG page From 8373a62ec8cab6d0f00745f81fb3c76e6908e963 Mon Sep 17 00:00:00 2001 From: TonalidadeHidrica <47710717+TonalidadeHidrica@users.noreply.github.com> Date: Mon, 23 Nov 2020 22:54:07 +0900 Subject: [PATCH 5/6] Modify the code so that it compiles on 1.27.2 --- src/reading.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/reading.rs b/src/reading.rs index 6c233e0..0f5fc2d 100644 --- a/src/reading.rs +++ b/src/reading.rs @@ -623,15 +623,15 @@ impl PacketReader { }; finder.feed(next); } - let mut target = &mut ret[4..]; + let mut pos = 4; loop { - let read = tri!(self.rdr.read(target)); + let read = tri!(self.rdr.read(&mut ret[pos..])); if read == 0 { break; } - target = &mut target[read..]; + pos += read; } - if target.is_empty() { + if pos == ret.len() { Ok(Some(ret)) } else { Ok(None) From c1fe4dd5c2333496245f6a53fde37a0eeab25aa5 Mon Sep 17 00:00:00 2001 From: est31 Date: Tue, 11 May 2021 00:34:10 +0200 Subject: [PATCH 6/6] Apply suggestions from code review --- src/reading.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/reading.rs b/src/reading.rs index 0f5fc2d..afe29be 100644 --- a/src/reading.rs +++ b/src/reading.rs @@ -530,10 +530,10 @@ impl BasePacketReader { #[derive(Default)] struct MagicFinder { - len: usize, + len :usize, } impl MagicFinder { - fn feed(&mut self, b: u8){ + fn feed(&mut self, b :u8) { match (b, self.len) { (b'O', _) => self.len = 1, (b'g', 1..=2) | (b'S', 3) => self.len += 1,