From 2bbc847754093bbdbd683a79624798366e3786f2 Mon Sep 17 00:00:00 2001 From: Scott Lamb Date: Sat, 30 Apr 2022 12:08:01 -0700 Subject: [PATCH] replace new_parameters with has_new_parameters As described in #47 --- examples/client/mp4.rs | 4 ++-- src/client/mod.rs | 20 ++++++++++++++------ src/codec/h264.rs | 24 +++++++++--------------- src/codec/mod.rs | 20 ++++++++++---------- 4 files changed, 35 insertions(+), 33 deletions(-) diff --git a/examples/client/mp4.rs b/examples/client/mp4.rs index 56f94af..615c277 100644 --- a/examples/client/mp4.rs +++ b/examples/client/mp4.rs @@ -550,9 +550,9 @@ impl Mp4Writer { &frame.timestamp(), frame.data().remaining(), ); - let sample_description_index = if let (Some(i), None) = ( + let sample_description_index = if let (Some(i), true) = ( self.cur_video_params_sample_description_index, - frame.new_parameters.as_ref(), + frame.has_new_parameters(), ) { // Use the most recent sample description index for most frames, without having to // scan through self.video_sample_index. diff --git a/src/client/mod.rs b/src/client/mod.rs index 5705b22..7ed72e6 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -780,13 +780,12 @@ impl Stream { /// /// # With [`Demuxed`] /// - /// When using [`Demuxed`]'s frame-by-frame `futures::Stream` impl: + /// When using [`Demuxed`]'s frame-by-frame `futures::Stream` impl, `parameters` reflects + /// all parameters as of returned frames that have been returned from from `poll_next` via + /// `Poll::Ready`. /// - /// * After `poll_next` returns `Ready`, `parameters` reflects all parameter changes as of - /// returned frame. - /// * After `poll_next` returns `Pending`, currently the parameters may or may not reflect - /// changes sent as part of the *next* frame that `poll_next` will return. (It's likely - /// that an upcoming Retina release will guarantee not.) + /// It's guaranteed to *not* reflect any parameter changes in the upcoming frame, even after + /// a `Poll::Pending` return. /// /// If there is no packet loss, parameters are generally available after the first frame is /// returned. In the case of H.264, [RFC 6184 section @@ -2357,7 +2356,16 @@ impl futures::Stream for Demuxed { description, }) })?; + + // Note we're committed now to calling `pull` and returning + // `Ready` if it has a frame. This is because the + // `Stream::parameters` contract guarantees that changes in + // upcoming frames are *not* reflected. It's implemented by a + // call to `Depacketizer::pull`, which doesn't make a like + // guarantee about the state between `push` and `pull`. So we + // can't let our callers observe that state. } + match depacketizer.pull(conn_ctx, stream_ctx) { Ok(Some(item)) => { self.state = DemuxedState::Pulling(stream_id); diff --git a/src/codec/h264.rs b/src/codec/h264.rs index 1cca4e3..d30d0d6 100644 --- a/src/codec/h264.rs +++ b/src/codec/h264.rs @@ -496,31 +496,27 @@ impl Depacketizer { self.nals.clear(); self.pieces.clear(); - let new_parameters = match ( + let has_new_parameters = match ( new_sps.as_deref(), new_pps.as_deref(), self.parameters.as_ref(), ) { (Some(sps_nal), Some(pps_nal), _) => { // TODO: could map this to a RtpPacketError more accurately. - let ip = InternalParameters::parse_sps_and_pps(sps_nal, pps_nal)?; - let p = ip.generic_parameters.clone(); - self.parameters = Some(ip); - Some(Box::new(p)) + self.parameters = Some(InternalParameters::parse_sps_and_pps(sps_nal, pps_nal)?); + true } (Some(_), None, Some(old_ip)) | (None, Some(_), Some(old_ip)) => { let sps_nal = new_sps.as_deref().unwrap_or(&old_ip.sps_nal); let pps_nal = new_pps.as_deref().unwrap_or(&old_ip.pps_nal); // TODO: as above, could map this to a RtpPacketError more accurately. - let ip = InternalParameters::parse_sps_and_pps(sps_nal, pps_nal)?; - let p = ip.generic_parameters.clone(); - self.parameters = Some(ip); - Some(Box::new(p)) + self.parameters = Some(InternalParameters::parse_sps_and_pps(sps_nal, pps_nal)?); + true } - _ => None, + _ => false, }; Ok(VideoFrame { - new_parameters, + has_new_parameters, loss: au.loss, start_ctx: au.start_ctx, end_ctx: au.end_ctx, @@ -1429,9 +1425,7 @@ mod tests { }; // After pull, new_parameters and parameters() both reflect the change. - assert!(frame.new_parameters.is_some()); - let p = frame.new_parameters.unwrap(); - assert_eq!(p.pixel_dimensions(), (640, 480)); + assert!(frame.has_new_parameters); match d.parameters() { Some(crate::codec::Parameters::Video(v)) => { assert_eq!(v.pixel_dimensions(), (640, 480)); @@ -1538,7 +1532,7 @@ mod tests { Some(CodecItem::VideoFrame(frame)) => frame, _ => panic!(), }; - assert!(frame.new_parameters.is_some()); + assert!(frame.has_new_parameters); assert!(d.parameters().is_some()); } } diff --git a/src/codec/mod.rs b/src/codec/mod.rs index 35d5827..0a5b4c3 100644 --- a/src/codec/mod.rs +++ b/src/codec/mod.rs @@ -324,16 +324,8 @@ pub struct VideoFrame { start_ctx: crate::PacketContext, end_ctx: crate::PacketContext, - /// New video parameters. - /// - /// Rarely populated and large, so boxed to reduce bloat. - // - /// To obtain the current parameters for the stream regardless of whether this frame set new - /// parameters, see [`crate::client::Stream::parameters`]. - pub new_parameters: Option>, - + has_new_parameters: bool, loss: u16, - timestamp: crate::Timestamp, stream_id: usize, is_random_access_point: bool, @@ -347,6 +339,14 @@ impl VideoFrame { self.stream_id } + /// Returns true if this frame set new video parameters. + /// + /// The parameters can be obtained via [`crate::client::Stream::parameters`]. + #[inline] + pub fn has_new_parameters(&self) -> bool { + self.has_new_parameters + } + /// Returns the number of lost RTP packets before this video frame. See /// [`crate::rtp::ReceivedPacket::loss`]. /// @@ -418,7 +418,7 @@ impl std::fmt::Debug for VideoFrame { .field("start_ctx", &self.start_ctx) .field("end_ctx", &self.end_ctx) .field("loss", &self.loss) - .field("new_parameters", &self.new_parameters) + .field("has_new_parameters", &self.has_new_parameters) .field("is_random_access_point", &self.is_random_access_point) .field("is_disposable", &self.is_disposable) .field("data", &crate::hex::LimitedHex::new(&self.data, 64))