From b54ea3f4fd043547714773bbe9a9b0bdea9c5982 Mon Sep 17 00:00:00 2001 From: Scott Lamb Date: Wed, 28 Sep 2022 21:38:46 -0700 Subject: [PATCH] v0.4.3: upgrade h264-reader crate * match a couple h264-reader semantic changes. E.g., decode_nal now expects the full NALU including header byte. * adjust test data in depacketize_parameter_change. The SDP parameters I used were invalid in a way caught only by the new h264-reader crate. That's not the intended scenario, so use different parameters. But mention the accidental scenario in CHANGELOG.md. * initialize logging in h264 tests to ease debugging problems like the bullet above. * in AAC code, match h264-reader's switch from bitreader to bitstream-io. This keeps dependencies down, in addition to the reasons for h264-reader's switch, described here: https://github.com/dholroyd/h264-reader/commit/7a02a3b7bd739e96ce262c852ea8b177e226f7f3 --- CHANGELOG.md | 8 ++++++++ Cargo.lock | 26 +++++++++++++++----------- Cargo.toml | 6 +++--- fuzz/Cargo.lock | 26 +++++++++++++++----------- src/client/mod.rs | 17 +---------------- src/codec/aac.rs | 22 +++++++++++++--------- src/codec/h264.rs | 24 +++++++++++++++++------- src/testutil.rs | 17 ++++++++++++++++- 8 files changed, 88 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08e0af4..f02fbd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +## `v0.4.3` (2022-09-28) + +* upgrade version of `h264-reader` crate. Compatibility note: Retina may now + be stricter about parsing H.264 parameters (SPS/PPS). In practice, with some + cameras this means unparseable "out-of-line" parameters (specified in the + SDP) will be ignored in favor of parseable "in-line" parameters (specified + within the RTP data stream). + ## `v0.4.2` (2022-09-28) * ignore unparseable SDP media, improving compatibility with TP-Link cameras, diff --git a/Cargo.lock b/Cargo.lock index 2485396..de6ec82 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -190,13 +190,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" [[package]] -name = "bitreader" -version = "0.3.6" +name = "bitstream-io" +version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d84ea71c85d1fe98fe67a9b9988b1695bc24c0b0d3bfb18d4c510f44b4b09941" -dependencies = [ - "cfg-if", -] +checksum = "97d524fdb78bf6dc6d2dc4c02043e4b4962ede0a17ae3e13f0ed211a7eda5897" [[package]] name = "bitvec" @@ -1047,11 +1044,12 @@ dependencies = [ [[package]] name = "h264-reader" -version = "0.5.0" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c8d87669bdeca3d51902f1bf1f2c71c8f514a8f3011d9b81e63719b374091da1" +checksum = "a3c095862f1b74a6021f766321767e64fbec34fa76503debbe1da2c04ce23c2c" dependencies = [ - "bitreader", + "bitstream-io", + "hex-slice", "log", "memchr", "rfc6381-codec", @@ -1087,6 +1085,12 @@ version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" +[[package]] +name = "hex-slice" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5491a308e0214554f07a81d8944abe45f552871c12e3c3c6e7e5d354039a6c4c" + [[package]] name = "hmac" version = "0.10.1" @@ -1780,10 +1784,10 @@ checksum = "a3f87b73ce11b1619a3c6332f45341e0047173771e8b8b73f87bfeefb7b56244" [[package]] name = "retina" -version = "0.4.2" +version = "0.4.3" dependencies = [ "base64", - "bitreader", + "bitstream-io", "bytes", "criterion", "futures", diff --git a/Cargo.toml b/Cargo.toml index bfeb1e5..098e715 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ default-members = ["."] [package] name = "retina" -version = "0.4.2" +version = "0.4.3" authors = ["Scott Lamb "] license = "MIT/Apache-2.0" edition = "2021" @@ -17,10 +17,10 @@ rust-version = "1.59" [dependencies] base64 = "0.13.0" -bitreader = "0.3.3" +bitstream-io = "1.1" bytes = "1.0.1" futures = "0.3.14" -h264-reader = "0.5.0" +h264-reader = "0.6.0" hex = "0.4.3" http-auth = "0.1.2" log = "0.4.8" diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index cf0311f..63664f8 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -21,13 +21,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "904dfeac50f3cdaba28fc6f57fdcddb75f49ed61346676a78c4ffe55877802fd" [[package]] -name = "bitreader" -version = "0.3.4" +name = "bitstream-io" +version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9178181a7d44239c6c8eaafa8688558a2ab5fa04b8855381f2681e9591fb941b" -dependencies = [ - "cfg-if", -] +checksum = "97d524fdb78bf6dc6d2dc4c02043e4b4962ede0a17ae3e13f0ed211a7eda5897" [[package]] name = "block-buffer" @@ -230,11 +227,12 @@ dependencies = [ [[package]] name = "h264-reader" -version = "0.5.0" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c8d87669bdeca3d51902f1bf1f2c71c8f514a8f3011d9b81e63719b374091da1" +checksum = "a3c095862f1b74a6021f766321767e64fbec34fa76503debbe1da2c04ce23c2c" dependencies = [ - "bitreader", + "bitstream-io", + "hex-slice", "log", "memchr", "rfc6381-codec", @@ -246,6 +244,12 @@ version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" +[[package]] +name = "hex-slice" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5491a308e0214554f07a81d8944abe45f552871c12e3c3c6e7e5d354039a6c4c" + [[package]] name = "http-auth" version = "0.1.5" @@ -511,10 +515,10 @@ checksum = "6c230d73fb8d8c1b9c0b3135c5142a8acee3a0558fb8db5cf1cb65f8d7862132" [[package]] name = "retina" -version = "0.4.1" +version = "0.4.3" dependencies = [ "base64", - "bitreader", + "bitstream-io", "bytes", "futures", "h264-reader", diff --git a/src/client/mod.rs b/src/client/mod.rs index e105ee6..2e2c62b 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -2508,10 +2508,8 @@ impl futures::Stream for Demuxed { #[cfg(test)] mod tests { - use std::str::FromStr; - use super::*; - use crate::testutil::response; + use crate::testutil::{init_logging, response}; /// Cross-platform, tokio equivalent of `socketpair(2)`. async fn socketpair() -> (tokio::net::TcpStream, tokio::net::TcpStream) { @@ -2537,19 +2535,6 @@ mod tests { (client, server) } - fn init_logging() { - let h = mylog::Builder::new() - .set_format( - ::std::env::var("MOONFIRE_FORMAT") - .map_err(|_| ()) - .and_then(|s| mylog::Format::from_str(&s)) - .unwrap_or(mylog::Format::Google), - ) - .set_spec(::std::env::var("MOONFIRE_LOG").as_deref().unwrap_or("info")) - .build(); - let _ = h.install(); - } - /// Receives a request and sends a response, filling in the matching `CSeq`. async fn req_response( server: &mut crate::tokio::Connection, diff --git a/src/codec/aac.rs b/src/codec/aac.rs index 4d48c05..8d6e352 100644 --- a/src/codec/aac.rs +++ b/src/codec/aac.rs @@ -14,6 +14,7 @@ //! ISO base media file format. //! * ISO/IEC 14496-14: MP4 File Format. +use bitstream_io::BitRead; use bytes::{BufMut, Bytes, BytesMut}; use std::{ convert::TryFrom, @@ -66,14 +67,14 @@ const CHANNEL_CONFIGS: [Option; 8] = [ impl AudioSpecificConfig { /// Parses from raw bytes. fn parse(raw: &[u8]) -> Result { - let mut r = bitreader::BitReader::new(raw); + let mut r = bitstream_io::BitReader::endian(raw, bitstream_io::BigEndian); let audio_object_type = match r - .read_u8(5) + .read::(5) .map_err(|e| format!("unable to read audio_object_type: {}", e))? { 31 => { 32 + r - .read_u8(6) + .read::(6) .map_err(|e| format!("unable to read audio_object_type ext: {}", e))? } o => o, @@ -81,7 +82,7 @@ impl AudioSpecificConfig { // ISO/IEC 14496-3 section 1.6.3.3. let sampling_frequency = match r - .read_u8(4) + .read::(4) .map_err(|e| format!("unable to read sampling_frequency: {}", e))? { 0x0 => 96_000, @@ -101,13 +102,13 @@ impl AudioSpecificConfig { return Err(format!("reserved sampling_frequency_index value 0x{:x}", v)) } 0xf => r - .read_u32(24) + .read::(24) .map_err(|e| format!("unable to read sampling_frequency ext: {}", e))?, 0x10..=0xff => unreachable!(), }; let channels = { let c = r - .read_u8(4) + .read::(4) .map_err(|e| format!("unable to read channels: {}", e))?; CHANNEL_CONFIGS .get(usize::from(c)) @@ -117,7 +118,7 @@ impl AudioSpecificConfig { }; if audio_object_type == 5 || audio_object_type == 29 { // extensionSamplingFrequencyIndex + extensionSamplingFrequency. - if r.read_u8(4) + if r.read::(4) .map_err(|e| format!("unable to read extensionSamplingFrequencyIndex: {}", e))? == 0xf { @@ -125,7 +126,7 @@ impl AudioSpecificConfig { .map_err(|e| format!("unable to read extensionSamplingFrequency: {}", e))?; } // audioObjectType (a different one) + extensionChannelConfiguration. - if r.read_u8(5) + if r.read::(5) .map_err(|e| format!("unable to read second audioObjectType: {}", e))? == 22 { @@ -142,7 +143,7 @@ impl AudioSpecificConfig { // GASpecificConfig, ISO/IEC 14496-3 section 4.4.1. let frame_length_flag = r - .read_bool() + .read_bit() .map_err(|e| format!("unable to read frame_length_flag: {}", e))?; let frame_length = match (audio_object_type, frame_length_flag) { (3 /* AAC SR */, false) => NonZeroU16::new(256).expect("non-zero"), @@ -787,14 +788,17 @@ mod tests { let dahua = AudioSpecificConfig::parse(&[0x11, 0x88]).unwrap(); assert_eq!(dahua.parameters.clock_rate, 48_000); assert_eq!(dahua.channels.name, "mono"); + assert_eq!(dahua.parameters.rfc6381_codec(), Some("mp4a.40.2")); let bunny = AudioSpecificConfig::parse(&[0x14, 0x90]).unwrap(); assert_eq!(bunny.parameters.clock_rate, 12_000); assert_eq!(bunny.channels.name, "stereo"); + assert_eq!(bunny.parameters.rfc6381_codec(), Some("mp4a.40.2")); let rfc3640 = AudioSpecificConfig::parse(&[0x11, 0xB0]).unwrap(); assert_eq!(rfc3640.parameters.clock_rate, 48_000); assert_eq!(rfc3640.channels.name, "5.1"); + assert_eq!(rfc3640.parameters.rfc6381_codec(), Some("mp4a.40.2")); } #[test] diff --git a/src/codec/h264.rs b/src/codec/h264.rs index 994ed7a..cef7a17 100644 --- a/src/codec/h264.rs +++ b/src/codec/h264.rs @@ -677,16 +677,18 @@ impl InternalParameters { } fn parse_sps_and_pps(sps_nal: &[u8], pps_nal: &[u8]) -> Result { - let sps_rbsp = h264_reader::rbsp::decode_nal(&sps_nal[1..]); - if sps_rbsp.len() < 4 { + let sps_rbsp = h264_reader::rbsp::decode_nal(sps_nal).map_err(|_| "bad sps")?; + if sps_rbsp.len() < 5 { return Err("bad sps".into()); } let rfc6381_codec = format!( "avc1.{:02X}{:02X}{:02X}", sps_rbsp[0], sps_rbsp[1], sps_rbsp[2] ); - let sps = h264_reader::nal::sps::SeqParameterSet::from_bytes(&sps_rbsp) - .map_err(|e| format!("Bad SPS: {:?}", e))?; + let sps = h264_reader::nal::sps::SeqParameterSet::from_bits( + h264_reader::rbsp::BitReader::new(&*sps_rbsp), + ) + .map_err(|e| format!("Bad SPS: {:?}", e))?; debug!("sps: {:#?}", &sps); let pixel_dimensions = sps @@ -1011,6 +1013,7 @@ enum PacketizerState { mod tests { use std::num::NonZeroU32; + use crate::testutil::init_logging; use crate::{codec::CodecItem, rtp::ReceivedPacketBuilder}; /* @@ -1068,6 +1071,7 @@ mod tests { #[test] fn depacketize() { + init_logging(); let mut d = super::Depacketizer::new(90_000, Some("packetization-mode=1;profile-level-id=64001E;sprop-parameter-sets=Z2QAHqwsaoLA9puCgIKgAAADACAAAAMD0IAA,aO4xshsA")).unwrap(); let timestamp = crate::Timestamp { timestamp: 0, @@ -1176,6 +1180,7 @@ mod tests { /// suppress incorrect access unit changes after the SPS and PPS. #[test] fn depacketize_reolink_bad_framing_at_start() { + init_logging(); let mut d = super::Depacketizer::new(90_000, Some("packetization-mode=1;profile-level-id=640033;sprop-parameter-sets=Z2QAM6wVFKCgL/lQ,aO48sA==")).unwrap(); let ts1 = crate::Timestamp { timestamp: 0, @@ -1259,6 +1264,7 @@ mod tests { /// suppress incorrect access unit changes after the SPS and PPS. #[test] fn depacketize_reolink_gop_boundary() { + init_logging(); let mut d = super::Depacketizer::new(90_000, Some("packetization-mode=1;profile-level-id=640033;sprop-parameter-sets=Z2QAM6wVFKCgL/lQ,aO48sA==")).unwrap(); let ts1 = crate::Timestamp { timestamp: 0, @@ -1358,12 +1364,13 @@ mod tests { #[test] fn depacketize_parameter_change() { - let mut d = super::Depacketizer::new(90_000, Some("a=fmtp:96 profile-level-id=420029; packetization-mode=1; sprop-parameter-sets=Z01AHppkBYHv/lBgYGQAAA+gAAE4gBA=,aO48gA==")).unwrap(); + init_logging(); + let mut d = super::Depacketizer::new(90_000, Some("a=fmtp:96 packetization-mode=1;profile-level-id=4d002a;sprop-parameter-sets=Z00AKp2oHgCJ+WbgICAoAAADAAgAAAMAfCA=,aO48gA==")).unwrap(); match d.parameters() { Some(crate::codec::ParametersRef::Video(v)) => { - assert_eq!(v.pixel_dimensions(), (704, 480)); + assert_eq!(v.pixel_dimensions(), (1920, 1080)); } - _ => unreachable!(), + o => panic!("{:?}", o), } let timestamp = crate::Timestamp { timestamp: 0, @@ -1438,6 +1445,7 @@ mod tests { /// (Mostly that it should not panic.) #[test] fn depacketize_empty() { + init_logging(); assert!(super::InternalParameters::parse_format_specific_params("").is_err()); assert!(super::InternalParameters::parse_format_specific_params(" ").is_err()); } @@ -1446,6 +1454,7 @@ mod tests { /// an Annex B NAL separator at the end of each of the `sprop-parameter-sets` NALs. #[test] fn gw_security_params() { + init_logging(); let params = super::InternalParameters::parse_format_specific_params( "packetization-mode=1;\ profile-level-id=5046302;\ @@ -1461,6 +1470,7 @@ mod tests { #[test] fn bad_format_specific_params() { + init_logging(); // These bad parameters are taken from a VStarcam camera. The sprop-parameter-sets // don't start with proper NAL headers. (They look almost like the raw RBSP of each // NAL plus extra trailing NUL bytes?) diff --git a/src/testutil.rs b/src/testutil.rs index d124703..e44961c 100644 --- a/src/testutil.rs +++ b/src/testutil.rs @@ -1,8 +1,23 @@ -// Copyright (C) 2021 Scott Lamb +// Copyright (C) 2022 Scott Lamb // SPDX-License-Identifier: MIT OR Apache-2.0 +use std::str::FromStr; + use bytes::Bytes; +pub(crate) fn init_logging() { + let h = mylog::Builder::new() + .set_format( + ::std::env::var("MOONFIRE_FORMAT") + .map_err(|_| ()) + .and_then(|s| mylog::Format::from_str(&s)) + .unwrap_or(mylog::Format::Google), + ) + .set_spec(::std::env::var("MOONFIRE_LOG").as_deref().unwrap_or("info")) + .build(); + let _ = h.install(); +} + pub(crate) fn response(raw: &'static [u8]) -> rtsp_types::Response { let (msg, len) = rtsp_types::Message::parse(raw).unwrap(); assert_eq!(len, raw.len());