From 5d648f1a729eb4d08ad129b38b41e8b8c7cd7fee Mon Sep 17 00:00:00 2001 From: Ririsoft Date: Fri, 8 Jul 2022 05:02:00 +0200 Subject: [PATCH] fix clippy & enhance CI/CD (#78) * fix clippy::unused_io_amount See related clippy documentation, but in short some partial reads can occur in particular with io on the networl. read_exact/write_all transparently handle such errors. The fix actually revealed a bug in 'mp4a' atom parsing, but this is a dangerous change: Parsing bugs that were transparently ignored are now failing with error (unattended io eof). * fix trivial clippy errors * fix clippy error with always 0 value * run ci/cd with clippy and latest rust version --- .github/workflows/rust.yml | 40 +++++++++++++++++++++++++++---- examples/mp4dump.rs | 11 ++++----- src/mp4box/avc1.rs | 4 ++-- src/mp4box/dinf.rs | 2 +- src/mp4box/edts.rs | 9 +++---- src/mp4box/hdlr.rs | 2 +- src/mp4box/mehd.rs | 12 +--------- src/mp4box/mod.rs | 13 +++++------ src/mp4box/mp4a.rs | 15 +++++++----- src/mp4box/tfhd.rs | 13 +---------- src/track.rs | 5 +--- src/types.rs | 48 +++++++++++++++++++------------------- tests/lib.rs | 2 +- 13 files changed, 90 insertions(+), 86 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 56d932b..e2d357d 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -13,8 +13,38 @@ jobs: build: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - name: Build - run: cargo build --verbose - - name: Run tests - run: cargo test --verbose + - name: Checkout sources + uses: actions/checkout@v2 + + - name: Install rust toolchain + uses: actions-rs/toolchain@v1 + with: + profile: minimal + toolchain: stable + override: true + components: rustfmt, clippy + + - name: Setup rust smart caching + uses: Swatinem/rust-cache@v1.3.0 + + - name: Cargo fmt + uses: actions-rs/cargo@v1 + with: + command: fmt + args: --all -- --check + + - name: Cargo clippy + uses: actions-rs/cargo@v1 + with: + command: clippy + args: --no-deps -- -D warnings + + - name: Cargo build + uses: actions-rs/cargo@v1 + with: + command: build + + - name: Cargo test + uses: actions-rs/cargo@v1 + with: + command: test diff --git a/examples/mp4dump.rs b/examples/mp4dump.rs index b5c9ee1..6a97d9a 100644 --- a/examples/mp4dump.rs +++ b/examples/mp4dump.rs @@ -45,12 +45,11 @@ fn get_boxes(file: File) -> Result> { let mp4 = mp4::Mp4Reader::read_header(reader, size)?; // collect known boxes - let mut boxes = Vec::new(); - - // ftyp, moov, mvhd - boxes.push(build_box(&mp4.ftyp)); - boxes.push(build_box(&mp4.moov)); - boxes.push(build_box(&mp4.moov.mvhd)); + let mut boxes = vec![ + build_box(&mp4.ftyp), + build_box(&mp4.moov), + build_box(&mp4.moov.mvhd), + ]; if let Some(ref mvex) = &mp4.moov.mvex { boxes.push(build_box(mvex)); diff --git a/src/mp4box/avc1.rs b/src/mp4box/avc1.rs index 89cfa06..81cf81a 100644 --- a/src/mp4box/avc1.rs +++ b/src/mp4box/avc1.rs @@ -283,13 +283,13 @@ impl NalUnit { fn read(reader: &mut R) -> Result { let length = reader.read_u16::()? as usize; let mut bytes = vec![0u8; length]; - reader.read(&mut bytes)?; + reader.read_exact(&mut bytes)?; Ok(NalUnit { bytes }) } fn write(&self, writer: &mut W) -> Result { writer.write_u16::(self.bytes.len() as u16)?; - writer.write(&self.bytes)?; + writer.write_all(&self.bytes)?; Ok(self.size() as u64) } } diff --git a/src/mp4box/dinf.rs b/src/mp4box/dinf.rs index 5124978..3025e38 100644 --- a/src/mp4box/dinf.rs +++ b/src/mp4box/dinf.rs @@ -289,7 +289,7 @@ impl WriteBox<&mut W> for UrlBox { write_box_header_ext(writer, self.version, self.flags)?; if !self.location.is_empty() { - writer.write(self.location.as_bytes())?; + writer.write_all(self.location.as_bytes())?; writer.write_u8(0)?; } diff --git a/src/mp4box/edts.rs b/src/mp4box/edts.rs index 67b2630..b8dea6f 100644 --- a/src/mp4box/edts.rs +++ b/src/mp4box/edts.rs @@ -55,12 +55,9 @@ impl ReadBox<&mut R> for EdtsBox { let header = BoxHeader::read(reader)?; let BoxHeader { name, size: s } = header; - match name { - BoxType::ElstBox => { - let elst = ElstBox::read_box(reader, s)?; - edts.elst = Some(elst); - } - _ => {} + if let BoxType::ElstBox = name { + let elst = ElstBox::read_box(reader, s)?; + edts.elst = Some(elst); } skip_bytes_to(reader, start + size)?; diff --git a/src/mp4box/hdlr.rs b/src/mp4box/hdlr.rs index 5a015d7..f0e77ec 100644 --- a/src/mp4box/hdlr.rs +++ b/src/mp4box/hdlr.rs @@ -92,7 +92,7 @@ impl WriteBox<&mut W> for HdlrBox { writer.write_u32::(0)?; } - writer.write(self.name.as_bytes())?; + writer.write_all(self.name.as_bytes())?; writer.write_u8(0)?; Ok(size) diff --git a/src/mp4box/mehd.rs b/src/mp4box/mehd.rs index bf01770..57de477 100644 --- a/src/mp4box/mehd.rs +++ b/src/mp4box/mehd.rs @@ -4,7 +4,7 @@ use std::io::{Read, Seek, Write}; use crate::mp4box::*; -#[derive(Debug, Clone, PartialEq, Serialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Default)] pub struct MehdBox { pub version: u8, pub flags: u32, @@ -28,16 +28,6 @@ impl MehdBox { } } -impl Default for MehdBox { - fn default() -> Self { - MehdBox { - version: 0, - flags: 0, - fragment_duration: 0, - } - } -} - impl Mp4Box for MehdBox { fn box_type(&self) -> BoxType { self.get_type() diff --git a/src/mp4box/mod.rs b/src/mp4box/mod.rs index c271360..9c35cfb 100644 --- a/src/mp4box/mod.rs +++ b/src/mp4box/mod.rs @@ -122,9 +122,9 @@ macro_rules! boxtype { } } - impl Into for BoxType { - fn into(self) -> u32 { - match self { + impl From for u32 { + fn from(b: BoxType) -> u32 { + match b { $( BoxType::$name => $value, )* BoxType::UnknownBox(t) => t, } @@ -212,7 +212,7 @@ impl BoxHeader { pub fn read(reader: &mut R) -> Result { // Create and read to buf. let mut buf = [0u8; 8]; // 8 bytes for box header. - reader.read(&mut buf)?; + reader.read_exact(&mut buf)?; // Get size. let s = buf[0..4].try_into().unwrap(); @@ -224,9 +224,8 @@ impl BoxHeader { // Get largesize if size is 1 if size == 1 { - reader.read(&mut buf)?; - let s = buf.try_into().unwrap(); - let largesize = u64::from_be_bytes(s); + reader.read_exact(&mut buf)?; + let largesize = u64::from_be_bytes(buf); Ok(BoxHeader { name: BoxType::from(typ), diff --git a/src/mp4box/mp4a.rs b/src/mp4box/mp4a.rs index 6a33763..8e49e50 100644 --- a/src/mp4box/mp4a.rs +++ b/src/mp4box/mp4a.rs @@ -89,14 +89,17 @@ impl ReadBox<&mut R> for Mp4aBox { reader.read_u32::()?; // pre-defined, reserved let samplerate = FixedPointU16::new_raw(reader.read_u32::()?); - let header = BoxHeader::read(reader)?; - let BoxHeader { name, size: s } = header; - let mut esds = None; - if name == BoxType::EsdsBox { - esds = Some(EsdsBox::read_box(reader, s)?); + let current = reader.seek(SeekFrom::Current(0))?; + if current < start + size { + let header = BoxHeader::read(reader)?; + let BoxHeader { name, size: s } = header; + + if name == BoxType::EsdsBox { + esds = Some(EsdsBox::read_box(reader, s)?); + } + skip_bytes_to(reader, start + size)?; } - skip_bytes_to(reader, start + size)?; Ok(Mp4aBox { data_reference_index, diff --git a/src/mp4box/tfhd.rs b/src/mp4box/tfhd.rs index c468e7d..68763af 100644 --- a/src/mp4box/tfhd.rs +++ b/src/mp4box/tfhd.rs @@ -4,7 +4,7 @@ use std::io::{Read, Seek, Write}; use crate::mp4box::*; -#[derive(Debug, Clone, PartialEq, Serialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Default)] pub struct TfhdBox { pub version: u8, pub flags: u32, @@ -12,17 +12,6 @@ pub struct TfhdBox { pub base_data_offset: u64, } -impl Default for TfhdBox { - fn default() -> Self { - TfhdBox { - version: 0, - flags: 0, - track_id: 0, - base_data_offset: 0, - } - } -} - impl TfhdBox { pub fn get_type(&self) -> BoxType { BoxType::TfhdBox diff --git a/src/track.rs b/src/track.rs index a88365f..2427a9d 100644 --- a/src/track.rs +++ b/src/track.rs @@ -502,10 +502,7 @@ impl Mp4Track { } if let Some(ref stss) = self.trak.mdia.minf.stbl.stss { - match stss.entries.binary_search(&sample_id) { - Ok(_) => true, - Err(_) => false, - } + stss.entries.binary_search(&sample_id).is_ok() } else { true } diff --git a/src/types.rs b/src/types.rs index b5d412b..7561592 100644 --- a/src/types.rs +++ b/src/types.rs @@ -206,9 +206,9 @@ impl TryFrom<&FourCC> for TrackType { } } -impl Into for TrackType { - fn into(self) -> FourCC { - match self { +impl From for FourCC { + fn from(t: TrackType) -> FourCC { + match t { TrackType::Video => HANDLER_TYPE_VIDEO_FOURCC.into(), TrackType::Audio => HANDLER_TYPE_AUDIO_FOURCC.into(), TrackType::Subtitle => HANDLER_TYPE_SUBTITLE_FOURCC.into(), @@ -252,9 +252,9 @@ impl TryFrom<&str> for MediaType { } } -impl Into<&str> for MediaType { - fn into(self) -> &'static str { - match self { +impl From for &str { + fn from(t: MediaType) -> &'static str { + match t { MediaType::H264 => MEDIA_TYPE_H264, MediaType::H265 => MEDIA_TYPE_H265, MediaType::VP9 => MEDIA_TYPE_VP9, @@ -264,9 +264,9 @@ impl Into<&str> for MediaType { } } -impl Into<&str> for &MediaType { - fn into(self) -> &'static str { - match self { +impl From<&MediaType> for &str { + fn from(t: &MediaType) -> &'static str { + match t { MediaType::H264 => MEDIA_TYPE_H264, MediaType::H265 => MEDIA_TYPE_H265, MediaType::VP9 => MEDIA_TYPE_VP9, @@ -290,7 +290,7 @@ impl TryFrom<(u8, u8)> for AvcProfile { type Error = Error; fn try_from(value: (u8, u8)) -> Result { let profile = value.0; - let constraint_set1_flag = value.1 & 0x40 >> 7; + let constraint_set1_flag = (value.1 & 0x40) >> 7; match (profile, constraint_set1_flag) { (66, 1) => Ok(AvcProfile::AvcConstrainedBaseline), (66, 0) => Ok(AvcProfile::AvcBaseline), @@ -503,20 +503,20 @@ impl TryFrom for SampleFreqIndex { impl SampleFreqIndex { pub fn freq(&self) -> u32 { - match self { - &SampleFreqIndex::Freq96000 => 96000, - &SampleFreqIndex::Freq88200 => 88200, - &SampleFreqIndex::Freq64000 => 64000, - &SampleFreqIndex::Freq48000 => 48000, - &SampleFreqIndex::Freq44100 => 44100, - &SampleFreqIndex::Freq32000 => 32000, - &SampleFreqIndex::Freq24000 => 24000, - &SampleFreqIndex::Freq22050 => 22050, - &SampleFreqIndex::Freq16000 => 16000, - &SampleFreqIndex::Freq12000 => 12000, - &SampleFreqIndex::Freq11025 => 11025, - &SampleFreqIndex::Freq8000 => 8000, - &SampleFreqIndex::Freq7350 => 7350, + match *self { + SampleFreqIndex::Freq96000 => 96000, + SampleFreqIndex::Freq88200 => 88200, + SampleFreqIndex::Freq64000 => 64000, + SampleFreqIndex::Freq48000 => 48000, + SampleFreqIndex::Freq44100 => 44100, + SampleFreqIndex::Freq32000 => 32000, + SampleFreqIndex::Freq24000 => 24000, + SampleFreqIndex::Freq22050 => 22050, + SampleFreqIndex::Freq16000 => 16000, + SampleFreqIndex::Freq12000 => 12000, + SampleFreqIndex::Freq11025 => 11025, + SampleFreqIndex::Freq8000 => 8000, + SampleFreqIndex::Freq7350 => 7350, } } } diff --git a/tests/lib.rs b/tests/lib.rs index 2cca9f3..39662d7 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -24,7 +24,7 @@ fn test_read_mp4() { for b in brands { let t = mp4.compatible_brands().iter().any(|x| x.to_string() == b); - assert_eq!(t, true); + assert!(t); } assert_eq!(mp4.duration(), Duration::from_millis(62));