feedback: allow stack or heap allocated FciBuilders

This commit makes it possible to avoid heap allocation when a
`TransportFeedbackBuilder` or `PayloadFeedbackBuilder` doesn't escape the
lifetime of its `FciBuilder`.

src/compound.rs

@@ -308,8 +308,8 @@ pub enum PacketBuilder<'a> {
     Rr(crate::receiver::ReceiverReportBuilder),
     Sdes(crate::sdes::SdesBuilder<'a>),
     Sr(crate::sender::SenderReportBuilder),
-    TransportFeedback(crate::feedback::TransportFeedbackBuilder),
-    PayloadFeedback(crate::feedback::PayloadFeedbackBuilder),
+    TransportFeedback(crate::feedback::TransportFeedbackBuilder<'a>),
+    PayloadFeedback(crate::feedback::PayloadFeedbackBuilder<'a>),
     Unknown(UnknownBuilder<'a>),
 }
 
@@ -493,12 +493,12 @@ impl_try_from!(
 );
 impl_try_from!(
     crate::feedback::TransportFeedback<'a>,
-    crate::feedback::TransportFeedbackBuilder,
+    crate::feedback::TransportFeedbackBuilder<'a>,
     TransportFeedback
 );
 impl_try_from!(
     crate::feedback::PayloadFeedback<'a>,
-    crate::feedback::PayloadFeedbackBuilder,
+    crate::feedback::PayloadFeedbackBuilder<'a>,
     PayloadFeedback
 );
 

src/feedback/fir.rs

@@ -150,7 +150,7 @@ mod tests {
         let mut data = [0; REQ_LEN];
         let fir = {
             let fci = Fir::builder().add_ssrc(0xfedcba98, 0x30);
-            PayloadFeedback::builder(fci)
+            PayloadFeedback::builder_owned(fci)
                 .sender_ssrc(0x98765432)
                 .media_ssrc(0)
         };

src/feedback/mod.rs

@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+use std::borrow::Borrow;
+
 use crate::{
     prelude::*,
     utils::{pad_to_4bytes, parser, writer},
@@ -77,12 +79,33 @@ impl<'a> RtcpPacketParser<'a> for TransportFeedback<'a> {
 }
 
 impl<'a> TransportFeedback<'a> {
-    pub fn builder(fci: impl FciBuilder<'static> + 'static) -> TransportFeedbackBuilder {
+    /// Constructs a [`TransportFeedbackBuilder`] which refers to the provided [`FciBuilder`].
+    ///
+    /// Use [`TransportFeedback::builder_owned`] to return the [`TransportFeedbackBuilder`]
+    /// from a function.
+    pub fn builder(fci: &'a dyn FciBuilder<'a>) -> TransportFeedbackBuilder<'a> {
+        TransportFeedbackBuilder {
+            padding: 0,
+            sender_ssrc: 0,
+            media_ssrc: 0,
+            fci: FciBuilderWrapper::Borrowed(fci),
+        }
+    }
+
+    /// Constructs a [`TransportFeedbackBuilder`] which owns the provided [`FciBuilder`].
+    ///
+    /// This allows returning it from a function.
+    ///
+    /// **Warning:** this causes the [`FciBuilder`] to be heap-allocated.
+    /// Use [`TransportFeedback::builder`] when possible.
+    pub fn builder_owned(
+        fci: impl FciBuilder<'static> + 'static,
+    ) -> TransportFeedbackBuilder<'static> {
         TransportFeedbackBuilder {
             padding: 0,
             sender_ssrc: 0,
             media_ssrc: 0,
-            fci: Box::new(fci),
+            fci: FciBuilderWrapper::Owned(Box::new(fci)),
         }
     }
 
@@ -112,14 +135,14 @@ impl<'a> TransportFeedback<'a> {
 /// TransportFeedback packet builder
 #[derive(Debug)]
 #[must_use = "The builder must be built to be used"]
-pub struct TransportFeedbackBuilder {
+pub struct TransportFeedbackBuilder<'a> {
     padding: u8,
     sender_ssrc: u32,
     media_ssrc: u32,
-    fci: Box<dyn FciBuilder<'static>>,
+    fci: FciBuilderWrapper<'a>,
 }
 
-impl TransportFeedbackBuilder {
+impl<'a> TransportFeedbackBuilder<'a> {
     pub fn sender_ssrc(mut self, sender_ssrc: u32) -> Self {
         self.sender_ssrc = sender_ssrc;
         self
@@ -169,7 +192,7 @@ fn fb_write_into<T: RtcpPacket>(
     end
 }
 
-impl RtcpPacketWriter for TransportFeedbackBuilder {
+impl<'a> RtcpPacketWriter for TransportFeedbackBuilder<'a> {
     /// Calculates the size required to write this TransportFeedback packet.
     ///
     /// Returns an error if:
@@ -241,12 +264,32 @@ impl<'a> RtcpPacketParser<'a> for PayloadFeedback<'a> {
 }
 
 impl<'a> PayloadFeedback<'a> {
-    pub fn builder(fci: impl FciBuilder<'static> + 'static) -> PayloadFeedbackBuilder {
+    /// Constructs a [`PayloadFeedbackBuilder`] which refers to the provided [`FciBuilder`].
+    ///
+    /// Use [`PayloadFeedback::builder_owned`] to return the [`PayloadFeedbackBuilder`] from a function.
+    pub fn builder(fci: &'a dyn FciBuilder<'a>) -> PayloadFeedbackBuilder<'a> {
+        PayloadFeedbackBuilder {
+            padding: 0,
+            sender_ssrc: 0,
+            media_ssrc: 0,
+            fci: FciBuilderWrapper::Borrowed(fci),
+        }
+    }
+
+    /// Constructs a [`PayloadFeedbackBuilder`] which owns the provided [`FciBuilder`].
+    ///
+    /// This allows returning it from a function.
+    ///
+    /// **Warning:** this causes the [`FciBuilder`] to be heap-allocated.
+    /// Use [`PayloadFeedback::builder`] when possible.
+    pub fn builder_owned(
+        fci: impl FciBuilder<'static> + 'static,
+    ) -> PayloadFeedbackBuilder<'static> {
         PayloadFeedbackBuilder {
             padding: 0,
             sender_ssrc: 0,
             media_ssrc: 0,
-            fci: Box::new(fci),
+            fci: FciBuilderWrapper::Owned(Box::new(fci)),
         }
     }
 
@@ -276,14 +319,14 @@ impl<'a> PayloadFeedback<'a> {
 /// TransportFeedback packet builder
 #[derive(Debug)]
 #[must_use = "The builder must be built to be used"]
-pub struct PayloadFeedbackBuilder {
+pub struct PayloadFeedbackBuilder<'a> {
     padding: u8,
     sender_ssrc: u32,
     media_ssrc: u32,
-    fci: Box<dyn FciBuilder<'static>>,
+    fci: FciBuilderWrapper<'a>,
 }
 
-impl PayloadFeedbackBuilder {
+impl<'a> PayloadFeedbackBuilder<'a> {
     pub fn sender_ssrc(mut self, sender_ssrc: u32) -> Self {
         self.sender_ssrc = sender_ssrc;
         self
@@ -301,7 +344,7 @@ impl PayloadFeedbackBuilder {
     }
 }
 
-impl RtcpPacketWriter for PayloadFeedbackBuilder {
+impl<'a> RtcpPacketWriter for PayloadFeedbackBuilder<'a> {
     /// Calculates the size required to write this PayloadFeedback packet.
     ///
     /// Returns an error if:
@@ -357,6 +400,52 @@ pub trait FciParser<'a>: Sized {
     fn parse(data: &'a [u8]) -> Result<Self, RtcpParseError>;
 }
 
+/// Stack or heap allocated [`FciBuilder`] wrapper.
+///
+/// This wrapper allows borrowing or owning an [`FciBuilder`] without
+/// propagating the concrete type of the [`FciBuilder`] to types such as
+/// [`PayloadFeedbackBuilder`] or [`TransportFeedback`]. This is needed for
+/// these types to be included in collections such as [`crate::CompoundBuilder`].
+///
+/// `Cow` from `std` would not be suitable here because it would require
+/// declaring the `B` type parameter as an implementer of `ToOwned`,
+/// which is not object-safe.
+#[derive(Debug)]
+enum FciBuilderWrapper<'a> {
+    Borrowed(&'a dyn FciBuilder<'a>),
+    // Note: using `'a` and not `'static` here to help with the implementations
+    // of `deref()` and `as_ref()`. This enum is private and the `Owned` variant
+    // can only be constructed from `PayloadFeedbackBuilder::builder_owned` &
+    // `TransportFeedback::builder_owned` which constrain the `FciBuilder` to be `'static`.
+    Owned(Box<dyn FciBuilder<'a>>),
+}
+
+impl<'a, T: FciBuilder<'a>> From<&'a T> for FciBuilderWrapper<'a> {
+    fn from(value: &'a T) -> Self {
+        FciBuilderWrapper::Borrowed(value)
+    }
+}
+
+impl<'a> std::convert::AsRef<dyn FciBuilder<'a> + 'a> for FciBuilderWrapper<'a> {
+    fn as_ref(&self) -> &(dyn FciBuilder<'a> + 'a) {
+        match self {
+            FciBuilderWrapper::Borrowed(this) => *this,
+            FciBuilderWrapper::Owned(this) => this.borrow(),
+        }
+    }
+}
+
+impl<'a> std::ops::Deref for FciBuilderWrapper<'a> {
+    type Target = dyn FciBuilder<'a> + 'a;
+
+    fn deref(&self) -> &(dyn FciBuilder<'a> + 'a) {
+        match self {
+            FciBuilderWrapper::Borrowed(this) => *this,
+            FciBuilderWrapper::Owned(this) => this.borrow(),
+        }
+    }
+}
+
 pub trait FciBuilder<'a>: RtcpPacketWriter {
     /// The format field value to place in the RTCP header
     fn format(&self) -> u8;

src/feedback/nack.rs

@@ -221,7 +221,7 @@ mod tests {
             for i in (0..=n - 1).step_by(m as usize) {
                 fci = fci.add_rtp_sequence(start + i);
             }
-            TransportFeedback::builder(fci)
+            TransportFeedback::builder_owned(fci)
                 .sender_ssrc(0x98765432)
                 .media_ssrc(0x10fedcba)
         };
@@ -269,4 +269,33 @@ mod tests {
     fn nack_build_parse_12_2_timestamps() {
         nack_build_parse_n_m_timestamps(0x1234, 12, 2, &[0x12, 0x34, 0x02, 0b1010_1010]);
     }
+
+    #[test]
+    fn nack_build_ref_2_consecutive_timestamps() {
+        let n = 2;
+        let m = 1;
+        let fci = &[0x12, 0x34, 0x00, 0x01];
+
+        let r = (n + 1) % m;
+        let req_len = TransportFeedback::MIN_PACKET_LEN + ((n - r + 16) / 17 * 4) as usize;
+        let mut data = vec![0; req_len];
+        let mut expected = vec![0; req_len];
+        const TEMPLATE: [u8; 12] = [
+            0x81, 0xcd, 0x00, 0x00, 0x98, 0x76, 0x54, 0x32, 0x10, 0xfe, 0xdc, 0xba,
+        ];
+        expected[0..12].copy_from_slice(&TEMPLATE);
+        expected[3] = (req_len / 4 - 1) as u8;
+        expected[12..12 + fci.len()].copy_from_slice(fci);
+        let mut fci = Nack::builder();
+        for i in (0..=n - 1).step_by(m as usize) {
+            fci = fci.add_rtp_sequence(0x1234 + i);
+        }
+        let nack = TransportFeedback::builder(&fci)
+            .sender_ssrc(0x98765432)
+            .media_ssrc(0x10fedcba);
+        assert_eq!(nack.calculate_size().unwrap(), req_len);
+        let len = nack.write_into(&mut data).unwrap();
+        assert_eq!(len, req_len);
+        assert_eq!(data, expected);
+    }
 }

src/feedback/pli.rs

@@ -68,7 +68,7 @@ mod tests {
         let mut data = [0; REQ_LEN];
         let pli = {
             let fci = Pli::builder();
-            PayloadFeedback::builder(fci)
+            PayloadFeedback::builder_owned(fci)
                 .sender_ssrc(0x98765432)
                 .media_ssrc(0x10fedcba)
         };
@@ -87,6 +87,23 @@ mod tests {
         let _pli = fb.parse_fci::<Pli>().unwrap();
     }
 
+    #[test]
+    fn pli_build_ref() {
+        const REQ_LEN: usize = PayloadFeedback::MIN_PACKET_LEN;
+        let mut data = [0; REQ_LEN];
+        let fci = Pli::builder();
+        let pli = PayloadFeedback::builder(&fci)
+            .sender_ssrc(0x98765432)
+            .media_ssrc(0x10fedcba);
+        assert_eq!(pli.calculate_size().unwrap(), REQ_LEN);
+        let len = pli.write_into(&mut data).unwrap();
+        assert_eq!(len, REQ_LEN);
+        assert_eq!(
+            data,
+            [0x81, 0xce, 0x00, 0x02, 0x98, 0x76, 0x54, 0x32, 0x10, 0xfe, 0xdc, 0xba,]
+        );
+    }
+
     #[test]
     fn pli_parse_wrong_packet() {
         let fb = TransportFeedback::parse(&[
@@ -103,12 +120,10 @@ mod tests {
     fn pli_build_wrong_packet_type() {
         const REQ_LEN: usize = TransportFeedback::MIN_PACKET_LEN;
         let mut data = [0; REQ_LEN];
-        let pli = {
-            let fci = Pli::builder();
-            TransportFeedback::builder(fci)
-                .sender_ssrc(0x98765432)
-                .media_ssrc(0x10fedcba)
-        };
+        let fci = Pli::builder();
+        let pli = TransportFeedback::builder(&fci)
+            .sender_ssrc(0x98765432)
+            .media_ssrc(0x10fedcba);
         assert!(matches!(
             pli.calculate_size(),
             Err(RtcpWriteError::FciWrongFeedbackPacketType)

src/feedback/rpsi.rs

@@ -151,8 +151,8 @@ mod tests {
             let data = &[0xf0];
             let fci = Rpsi::builder()
                 .payload_type(96)
-                .native_data_owned(data.as_ref(), 4);
-            PayloadFeedback::builder(fci)
+                .native_data(data.as_ref(), 4);
+            PayloadFeedback::builder_owned(fci)
                 .sender_ssrc(0x98765432)
                 .media_ssrc(0x10fedcba)
         };
@@ -175,4 +175,35 @@ mod tests {
         assert_eq!(rpsi.payload_type(), 96);
         assert_eq!(rpsi.bit_string(), ([0xf0].as_ref(), 4));
     }
+
+    #[test]
+    fn rpsi_build_parse_ref() {
+        const REQ_LEN: usize = PayloadFeedback::MIN_PACKET_LEN + 4;
+        let mut data = [0; REQ_LEN];
+        let data_fci = &[0xf0];
+        let fci = Rpsi::builder()
+            .payload_type(96)
+            .native_data(data_fci.as_ref(), 4);
+        let rpsi = PayloadFeedback::builder(&fci)
+            .sender_ssrc(0x98765432)
+            .media_ssrc(0x10fedcba);
+        assert_eq!(rpsi.calculate_size().unwrap(), REQ_LEN);
+        let len = rpsi.write_into(&mut data).unwrap();
+        assert_eq!(len, REQ_LEN);
+        assert_eq!(
+            data,
+            [
+                0x83, 0xce, 0x00, 0x03, 0x98, 0x76, 0x54, 0x32, 0x10, 0xfe, 0xdc, 0xba, 0x0c, 0x60,
+                0xf0, 0x00
+            ]
+        );
+
+        let fb = PayloadFeedback::parse(&data).unwrap();
+
+        assert_eq!(fb.sender_ssrc(), 0x98765432);
+        assert_eq!(fb.media_ssrc(), 0x10fedcba);
+        let rpsi = fb.parse_fci::<Rpsi>().unwrap();
+        assert_eq!(rpsi.payload_type(), 96);
+        assert_eq!(rpsi.bit_string(), ([0xf0].as_ref(), 4));
+    }
 }

src/feedback/sli.rs

@@ -181,7 +181,7 @@ mod tests {
         let mut data = [0; REQ_LEN];
         let sli = {
             let fci = Sli::builder().add_lost_macroblock(0x1234, 0x0987, 0x25);
-            PayloadFeedback::builder(fci)
+            PayloadFeedback::builder_owned(fci)
                 .sender_ssrc(0x98765432)
                 .media_ssrc(0x10fedcba)
         };
@@ -212,4 +212,24 @@ mod tests {
         );
         assert_eq!(mb_iter.next(), None);
     }
+
+    #[test]
+    fn sli_build_ref() {
+        const REQ_LEN: usize = PayloadFeedback::MIN_PACKET_LEN + 4;
+        let mut data = [0; REQ_LEN];
+        let fci = Sli::builder().add_lost_macroblock(0x1234, 0x0987, 0x25);
+        let sli = PayloadFeedback::builder(&fci)
+            .sender_ssrc(0x98765432)
+            .media_ssrc(0x10fedcba);
+        assert_eq!(sli.calculate_size().unwrap(), REQ_LEN);
+        let len = sli.write_into(&mut data).unwrap();
+        assert_eq!(len, REQ_LEN);
+        assert_eq!(
+            data,
+            [
+                0x82, 0xce, 0x00, 0x03, 0x98, 0x76, 0x54, 0x32, 0x10, 0xfe, 0xdc, 0xba, 0x91, 0xa2,
+                0x61, 0xe5
+            ]
+        );
+    }
 }