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:
  7a02a3b7bd

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,

Cargo.lock

@@ -190,13 +190,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a"
 
 [[package]]
-name = "bitreader"
+name = "bitstream-io"
-version = "0.3.6"
+version = "1.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d84ea71c85d1fe98fe67a9b9988b1695bc24c0b0d3bfb18d4c510f44b4b09941"
+checksum = "97d524fdb78bf6dc6d2dc4c02043e4b4962ede0a17ae3e13f0ed211a7eda5897"
-dependencies = [
- "cfg-if",
-]
 
 [[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",

Cargo.toml

@@ -4,7 +4,7 @@ default-members = ["."]
 
 [package]
 name = "retina"
-version = "0.4.2"
+version = "0.4.3"
 authors = ["Scott Lamb <slamb@slamb.org>"]
 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"

fuzz/Cargo.lock

@@ -21,13 +21,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "904dfeac50f3cdaba28fc6f57fdcddb75f49ed61346676a78c4ffe55877802fd"
 
 [[package]]
-name = "bitreader"
+name = "bitstream-io"
-version = "0.3.4"
+version = "1.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9178181a7d44239c6c8eaafa8688558a2ab5fa04b8855381f2681e9591fb941b"
+checksum = "97d524fdb78bf6dc6d2dc4c02043e4b4962ede0a17ae3e13f0ed211a7eda5897"
-dependencies = [
- "cfg-if",
-]
 
 [[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",

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,

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<ChannelConfig>; 8] = [
 impl AudioSpecificConfig {
     /// Parses from raw bytes.
     fn parse(raw: &[u8]) -> Result<Self, String> {
-        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::<u8>(5)
             .map_err(|e| format!("unable to read audio_object_type: {}", e))?
         {
             31 => {
                 32 + r
-                    .read_u8(6)
+                    .read::<u8>(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::<u8>(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::<u32>(24)
                 .map_err(|e| format!("unable to read sampling_frequency ext: {}", e))?,
             0x10..=0xff => unreachable!(),
         };
         let channels = {
             let c = r
-                .read_u8(4)
+                .read::<u8>(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::<u8>(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::<u8>(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]

src/codec/h264.rs

@@ -677,16 +677,18 @@ impl InternalParameters {
     }
 
     fn parse_sps_and_pps(sps_nal: &[u8], pps_nal: &[u8]) -> Result<InternalParameters, String> {
-        let sps_rbsp = h264_reader::rbsp::decode_nal(&sps_nal[1..]);
+        let sps_rbsp = h264_reader::rbsp::decode_nal(sps_nal).map_err(|_| "bad sps")?;
-        if sps_rbsp.len() < 4 {
+        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)
+        let sps = h264_reader::nal::sps::SeqParameterSet::from_bits(
-            .map_err(|e| format!("Bad SPS: {:?}", e))?;
+            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?)

src/testutil.rs

@@ -1,8 +1,23 @@
-// Copyright (C) 2021 Scott Lamb <slamb@slamb.org>
+// Copyright (C) 2022 Scott Lamb <slamb@slamb.org>
 // 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<Bytes> {
     let (msg, len) = rtsp_types::Message::parse(raw).unwrap();
     assert_eq!(len, raw.len());