From 56fde0d71bb822b85b77adda5c6158c09b86e899 Mon Sep 17 00:00:00 2001
From: Scott Lamb <slamb@slamb.org>
Date: Wed, 11 May 2022 10:36:11 -0700
Subject: [PATCH] remove `Bytes` from public codec API

Part of #47

The main benefit is that `VideoFrame::into_data` can cheaply return a
`Vec<u8>` that the caller can mutate. In particular, they could convert
H.264 data from Annex B to AVC form or skip non-VCL NALs. Neither of
these transformations add bytes so they're both possible without
allocation.

Audio and message frames still use `Bytes` internally, as that allows
them to avoid copying when the frame is wholly contained in a packet.
I think this is much more common than for video. I didn't add an
`AudioFrame::into_data` or `VideoFrame::into_data`.
---
 examples/client/mp4.rs | 21 ++++-----------------
 src/codec/h264.rs      |  1 -
 src/codec/mod.rs       | 25 ++++++++-----------------
 3 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/examples/client/mp4.rs b/examples/client/mp4.rs
index 5506e19..b6f9067 100644
--- a/examples/client/mp4.rs
+++ b/examples/client/mp4.rs
@@ -93,17 +93,6 @@ macro_rules! write_box {
     }};
 }
 
-async fn write_all_buf<W: AsyncWrite + Unpin, B: Buf>(
-    writer: &mut W,
-    buf: &mut B,
-) -> Result<(), Error> {
-    // TODO: this doesn't use vectored I/O. Annoying.
-    while buf.has_remaining() {
-        writer.write_buf(buf).await?;
-    }
-    Ok(())
-}
-
 /// Writes `.mp4` data to a sink.
 /// See module-level documentation for details.
 pub struct Mp4Writer<W: AsyncWrite + AsyncSeek + Send + Unpin> {
@@ -262,7 +251,7 @@ impl<W: AsyncWrite + AsyncSeek + Send + Unpin> Mp4Writer<W> {
         });
         buf.extend_from_slice(&b"\0\0\0\0mdat"[..]);
         let mdat_start = u32::try_from(buf.len())?;
-        write_all_buf(&mut inner, &mut buf).await?;
+        inner.write_all(&buf).await?;
         Ok(Mp4Writer {
             inner,
             video_params: Vec::new(),
@@ -311,7 +300,7 @@ impl<W: AsyncWrite + AsyncSeek + Send + Unpin> Mp4Writer<W> {
                 self.write_audio_trak(&mut buf, self.audio_params.as_ref().unwrap())?;
             }
         });
-        write_all_buf(&mut self.inner, &mut buf.freeze()).await?;
+        self.inner.write_all(&buf).await?;
         self.inner
             .seek(SeekFrom::Start(u64::from(self.mdat_start - 8)))
             .await?;
@@ -594,8 +583,7 @@ impl<W: AsyncWrite + AsyncSeek + Send + Unpin> Mp4Writer<W> {
             self.video_sync_sample_nums
                 .push(u32::try_from(self.video_trak.samples)?);
         }
-        let mut data = frame.into_data();
-        write_all_buf(&mut self.inner, &mut data).await?;
+        self.inner.write_all(frame.data()).await?;
         Ok(())
     }
 
@@ -618,8 +606,7 @@ impl<W: AsyncWrite + AsyncSeek + Send + Unpin> Mp4Writer<W> {
             .mdat_pos
             .checked_add(size)
             .ok_or_else(|| anyhow!("mdat_pos overflow"))?;
-        let mut data = frame.into_data();
-        write_all_buf(&mut self.inner, &mut data).await?;
+        self.inner.write_all(frame.data()).await?;
         Ok(())
     }
 }
diff --git a/src/codec/h264.rs b/src/codec/h264.rs
index d30d0d6..c5af1ed 100644
--- a/src/codec/h264.rs
+++ b/src/codec/h264.rs
@@ -492,7 +492,6 @@ impl Depacketizer {
             piece_idx = next_piece_idx;
         }
         debug_assert_eq!(retained_len, data.len());
-        let data = Bytes::from(data);
         self.nals.clear();
         self.pieces.clear();
 
diff --git a/src/codec/mod.rs b/src/codec/mod.rs
index f911c00..d33a3e6 100644
--- a/src/codec/mod.rs
+++ b/src/codec/mod.rs
@@ -9,11 +9,12 @@
 
 use std::num::{NonZeroU16, NonZeroU32};
 
+use bytes::Bytes;
+
 use crate::rtp::ReceivedPacket;
 use crate::ConnectionContext;
 use crate::Error;
 use crate::StreamContext;
-use bytes::Bytes;
 
 pub(crate) mod aac;
 pub(crate) mod g723;
@@ -112,7 +113,7 @@ impl VideoParameters {
 
     /// The codec-specific "extra data" to feed to eg ffmpeg to decode the video frames.
     /// *   H.264: an AvcDecoderConfig.
-    pub fn extra_data(&self) -> &Bytes {
+    pub fn extra_data(&self) -> &[u8] {
         &self.extra_data
     }
 }
@@ -226,14 +227,9 @@ impl AudioFrame {
     }
 
     #[inline]
-    pub fn data(&self) -> &Bytes {
+    pub fn data(&self) -> &[u8] {
         &self.data
     }
-
-    #[inline]
-    pub fn into_data(self) -> Bytes {
-        self.data
-    }
 }
 
 impl std::fmt::Debug for AudioFrame {
@@ -301,14 +297,9 @@ impl MessageFrame {
     }
 
     #[inline]
-    pub fn data(&self) -> &Bytes {
+    pub fn data(&self) -> &[u8] {
         &self.data
     }
-
-    #[inline]
-    pub fn into_data(self) -> Bytes {
-        self.data
-    }
 }
 
 /// A single video frame (aka video sample or video access unit).
@@ -330,7 +321,7 @@ pub struct VideoFrame {
     stream_id: usize,
     is_random_access_point: bool,
     is_disposable: bool,
-    data: bytes::Bytes,
+    data: Vec<u8>,
 }
 
 impl VideoFrame {
@@ -400,12 +391,12 @@ impl VideoFrame {
     /// In the future, a configuration parameter may allow the caller to request Annex B encoding
     /// instead.  See [#44](https://github.com/scottlamb/retina/issues/44).
     #[inline]
-    pub fn data(&self) -> &Bytes {
+    pub fn data(&self) -> &[u8] {
         &self.data
     }
 
     #[inline]
-    pub fn into_data(self) -> Bytes {
+    pub fn into_data(self) -> Vec<u8> {
         self.data
     }
 }