media: ffmpeg: Convert SwConverter to a AvFrame-based API.

We were already accepting AvFrame for swscale inputs, but for the output
a different SwConverterTarget trait was being used.

The old interface had a few issues:
- The use of memory mapping traits, putting a dependency on crosvm/base
- Insufficient metadata, not being able to represent custom padding on
  the stride or non-contiguous plane layouts
- Inconsistency between input and output types

In this patch, we:
1. Introduce a AvFrame-based API to replace the old SwConverter API.
2. Add a TryAsAvFrameExt extension trait inside devices/ to convert
   virtio handles to AvFrames.
   (This is placed inside devices to avoid ffmpeg depending on base.)
3. Add a new wrapper type, MemoryMappingAvBufferSource, that wraps a
   MemoryMappingArena and implements AvBufferSource for it. This was
   technically not sound, but it's a low-risk attack vector that we
   implicitly allowed before. The patch doesn't change the behavior; it
   just makes this assumption more explicit.

BUG=b:239897269
TEST=cargo test --features "video-decoder,ffmpeg" -p devices -p ffmpeg

Change-Id: Id15e19b7d2452d1bc722a65f941c2c9150876205
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3889245
Commit-Queue: Tatsuyuki Ishi <ishitatsuyuki@google.com>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>

Cargo.lock

@@ -712,7 +712,6 @@ name = "ffmpeg"
 version = "0.1.0"
 dependencies = [
  "anyhow",
- "base",
  "libc",
  "pkg-config",
  "thiserror",

devices/src/virtio/video/decoder/backend/ffmpeg.rs

@@ -33,6 +33,9 @@ use base::MemoryMappingArena;
 use thiserror::Error as ThisError;
 
 use crate::virtio::video::decoder::backend::*;
+use crate::virtio::video::ffmpeg::GuestResourceToAvFrameError;
+use crate::virtio::video::ffmpeg::MemoryMappingAvBufferSource;
+use crate::virtio::video::ffmpeg::TryAsAvFrameExt;
 use crate::virtio::video::format::FormatDesc;
 use crate::virtio::video::format::FormatRange;
 use crate::virtio::video::format::FrameFormat;
@@ -131,6 +134,8 @@ pub struct FfmpegDecoderSession {
 enum TrySendFrameError {
     #[error("error while converting frame: {0}")]
     CannotConvertFrame(#[from] ConversionError),
+    #[error("error while constructing AvFrame: {0}")]
+    IntoAvFrame(#[from] GuestResourceToAvFrameError),
     #[error("error while sending picture ready event: {0}")]
     BrokenPipe(#[from] base::Error),
 }
@@ -386,7 +391,10 @@ impl FfmpegDecoderSession {
         };
 
         // Convert the frame into the target buffer and emit the picture ready event.
-        format_converter.convert(&avframe, target_buffer)?;
+        format_converter.convert(
+            &avframe,
+            &mut target_buffer.try_as_av_frame(MemoryMappingAvBufferSource::from)?,
+        )?;
         self.event_queue.queue_event(picture_ready_event)?;
 
         Ok(true)

devices/src/virtio/video/ffmpeg.rs

@@ -2,9 +2,129 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+use std::error::Error;
+
+use thiserror::Error as ThisError;
+
+use base::MappedRegion;
+use base::MemoryMappingArena;
+use base::MmapError;
+use ffmpeg::avcodec::AvBuffer;
+use ffmpeg::avcodec::AvBufferSource;
+use ffmpeg::avcodec::AvError;
+use ffmpeg::avcodec::AvFrame;
+use ffmpeg::avcodec::AvFrameError;
 use ffmpeg::avcodec::AvPixelFormat;
+use ffmpeg::avcodec::Dimensions;
+use ffmpeg::avcodec::PlaneDescriptor;
 
 use crate::virtio::video::format::Format;
+use crate::virtio::video::resource::BufferHandle;
+use crate::virtio::video::resource::GuestResource;
+
+/// A simple wrapper that turns a [`MemoryMappingArena`] into an [`AvBufferSource`].
+///
+/// **Note:** Only use this if you can reasonably assume the mapped memory won't be written to from
+/// another thread or the VM! The reason [`AvBufferSource`] is not directly implemented on
+/// [`MemoryMappingArena`] is exactly due to this unsafety: If the guest or someone else writes to
+/// the buffer FFmpeg is currently reading from or writing to, undefined behavior might occur. This
+/// struct acts as an opt-in mechanism for this potential unsafety.
+pub struct MemoryMappingAvBufferSource(MemoryMappingArena);
+
+impl From<MemoryMappingArena> for MemoryMappingAvBufferSource {
+    fn from(inner: MemoryMappingArena) -> Self {
+        Self(inner)
+    }
+}
+impl AvBufferSource for MemoryMappingAvBufferSource {
+    fn as_ptr(&self) -> *const u8 {
+        self.0.as_ptr() as _
+    }
+
+    fn len(&self) -> usize {
+        self.0.size()
+    }
+}
+
+pub trait TryAsAvFrameExt {
+    type BufferSource;
+    type Error: Error;
+
+    /// Try to create an [`AvFrame`] from `self`.
+    ///
+    /// `wrapper` is a constructor for `T` that wraps `Self::BufferSource` and implement the
+    /// `AvBufferSource` functionality. This can be used to provide a custom destructor
+    /// implementation: for example, sending a virtio message signaling the buffer source is no
+    /// longer used and it can be recycled by the guest.
+    ///
+    /// Note that `Self::BufferSource` might not always implement `AvBufferSource`. For instance,
+    /// it's not guaranteed that a `MemoryMappingArena` created from `GuestResource` will be always
+    /// immutable, and the backend need to explicit acknowledge the potential unsoundness by
+    /// wrapping it in [`MemoryMappingAvBufferSource`].
+    fn try_as_av_frame<T: AvBufferSource + 'static>(
+        &self,
+        wrapper: impl FnOnce(Self::BufferSource) -> T,
+    ) -> Result<AvFrame, Self::Error>;
+}
+
+#[derive(Debug, ThisError)]
+pub enum GuestResourceToAvFrameError {
+    #[error("error while creating the AvFrame: {0}")]
+    AvFrameError(#[from] AvFrameError),
+    #[error("cannot mmap guest resource: {0}")]
+    MappingResource(#[from] MmapError),
+    #[error("virtio resource format is not convertible to AvPixelFormat")]
+    InvalidFormat,
+    #[error("out of memory while allocating AVBuffer")]
+    CannotAllocateAvBuffer,
+}
+
+impl From<AvError> for GuestResourceToAvFrameError {
+    fn from(e: AvError) -> Self {
+        GuestResourceToAvFrameError::AvFrameError(AvFrameError::AvError(e))
+    }
+}
+
+impl TryAsAvFrameExt for GuestResource {
+    type BufferSource = MemoryMappingArena;
+    type Error = GuestResourceToAvFrameError;
+
+    fn try_as_av_frame<T: AvBufferSource + 'static>(
+        &self,
+        wrapper: impl FnOnce(MemoryMappingArena) -> T,
+    ) -> Result<AvFrame, Self::Error> {
+        let mut builder = AvFrame::builder()?;
+        builder.set_dimensions(Dimensions {
+            width: self.width,
+            height: self.height,
+        })?;
+        let format = self
+            .format
+            .try_into()
+            .map_err(|_| GuestResourceToAvFrameError::InvalidFormat)?;
+        builder.set_format(format)?;
+        let planes = &self.planes;
+        // We have at least one plane, so we can unwrap below.
+        let len = format.plane_sizes(planes.iter().map(|p| p.stride as _), self.height)?;
+        let start = planes.iter().map(|p| p.offset).min().unwrap();
+        let end = planes
+            .iter()
+            .enumerate()
+            .map(|(i, p)| p.offset + len[i])
+            .max()
+            .unwrap();
+        let mapping = self.handle.get_mapping(start, end - start)?;
+        Ok(builder.build_owned(
+            [AvBuffer::new(wrapper(mapping))
+                .ok_or(GuestResourceToAvFrameError::CannotAllocateAvBuffer)?],
+            planes.iter().map(|p| PlaneDescriptor {
+                buffer_index: 0,
+                offset: p.offset - start,
+                stride: p.stride,
+            }),
+        )?)
+    }
+}
 
 /// The error returned by `AvPixelFormat::try_from` when there's no applicable format.
 // The empty field prevents constructing this and allows extending it in the future.

devices/src/virtio/video/utils.rs

@@ -12,8 +12,6 @@ use base::Event;
 use sync::Mutex;
 use thiserror::Error as ThisError;
 
-#[cfg(feature = "ffmpeg")]
-use crate::virtio::video::resource::BufferHandle;
 use crate::virtio::video::resource::GuestResource;
 
 /// Manages a pollable queue of events to be sent to the decoder or encoder.
@@ -132,25 +130,6 @@ impl<T> AsRawDescriptor for SyncEventQueue<T> {
     }
 }
 
-#[cfg(feature = "ffmpeg")]
-impl ffmpeg::swscale::SwConverterTarget for GuestResource {
-    fn stride(&self) -> Option<usize> {
-        self.planes.get(0).map(|p| p.stride)
-    }
-
-    fn num_planes(&self) -> usize {
-        self.planes.len()
-    }
-
-    fn get_mapping(
-        &mut self,
-        plane: usize,
-        required_size: usize,
-    ) -> Result<base::MemoryMappingArena, base::MmapError> {
-        self.handle.get_mapping(plane, required_size)
-    }
-}
-
 /// Queue of all the output buffers provided by crosvm.
 pub struct OutputQueue {
     // Max number of output buffers that can be imported into this queue.

media/ffmpeg/Cargo.toml

@@ -6,7 +6,6 @@ edition = "2021"
 
 [dependencies]
 anyhow = "*"
-base = { path = "../../base" }
 libc = "*"
 thiserror = "*"
 

media/ffmpeg/src/avcodec.rs

@@ -71,7 +71,7 @@ pub enum AvCodecOpenError {
 }
 
 /// Dimensions of a frame, used in AvCodecContext and AvFrame.
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
 pub struct Dimensions {
     pub width: u32,
     pub height: u32,

media/ffmpeg/src/swscale.rs

@@ -6,44 +6,33 @@
 //! `libswscale`. It is designed to concentrate all calls to unsafe methods in one place, while
 //! providing a higher-level interface for converting decoded frames from one format to another.
 
-use base::MappedRegion;
-use base::MemoryMappingArena;
-use base::MmapError;
 use thiserror::Error as ThisError;
 
+use crate::avcodec::AvError;
 use crate::avcodec::AvFrame;
 use crate::ffi;
 
-const MAX_FFMPEG_PLANES: usize = 4;
-
-pub trait SwConverterTarget {
-    fn stride(&self) -> Option<usize>;
-    fn num_planes(&self) -> usize;
-    fn get_mapping(
-        &mut self,
-        plane: usize,
-        required_size: usize,
-    ) -> Result<MemoryMappingArena, MmapError>;
-}
-
 /// A struct able to copy a decoded `AvFrame` into an `OutputBuffer`'s memory, converting the pixel
 /// format if needed.
 pub struct SwConverter {
     sws_context: *mut ffi::SwsContext,
+    src_pix_format: ffi::AVPixelFormat,
     dst_pix_format: ffi::AVPixelFormat,
 }
 
 #[derive(Debug, ThisError)]
 pub enum ConversionError {
-    #[error("cannot map destination buffer: {0}")]
-    CannotMapBuffer(base::MmapError),
-    #[error("resource has no plane description, at least 1 is required")]
-    NoPlanesInResource,
-    #[error(
-        "resource has more planes ({0}) than ffmpeg can handle ({})",
-        MAX_FFMPEG_PLANES
-    )]
-    TooManyPlanesInTarget(usize),
+    #[error("AvFrame's format {frame} does not match converter {converter} configuration")]
+    FormatMismatch {
+        frame: ffi::AVPixelFormat,
+        converter: ffi::AVPixelFormat,
+    },
+    #[error("source AvFrame's dimension does not match destination's")]
+    DimensionMismatch,
+    #[error("destination AvFrame needs to be refcounted with refcount=1")]
+    NotWritable,
+    #[error("error during conversion with libswscale: {0}")]
+    AvError(#[from] AvError),
 }
 
 impl Drop for SwConverter {
@@ -86,66 +75,56 @@ impl SwConverter {
 
         Ok(Self {
             sws_context,
+            src_pix_format,
             dst_pix_format,
         })
     }
 
     /// Copy `src` into `dst` while converting its pixel format according to the parameters the
     /// frame converter was created with.
-    pub fn convert<T: SwConverterTarget>(
-        &mut self,
-        src: &AvFrame,
-        dst: &mut T,
-    ) -> Result<(), ConversionError> {
-        let stride = dst.stride().ok_or(ConversionError::NoPlanesInResource)? as i32;
-
-        let required_mapping_size =
-            // Safe because this function does not take any pointer, it just performs a size
-            // calculation.
-            unsafe { ffi::av_image_get_buffer_size(self.dst_pix_format, stride, src.height, 1) }
-                as usize;
-
-        // Map the destination resource so the conversion function can write into it.
-        let mapping = dst
-            .get_mapping(0, required_mapping_size)
-            .map_err(ConversionError::CannotMapBuffer)?;
-
-        let mut dst_stride = [0i32; MAX_FFMPEG_PLANES];
-        let mut dst_data = [std::ptr::null_mut(); MAX_FFMPEG_PLANES];
-        if dst.num_planes() > MAX_FFMPEG_PLANES {
-            return Err(ConversionError::TooManyPlanesInTarget(dst.num_planes()));
+    ///
+    /// `dst` must be a [writable] frame with the same dimensions as `src` and the same format as
+    /// `dst_pix_format` passed to the constructor.
+    ///
+    /// Note that empty `dst` is not currently allowed as this function does not handle allocation.
+    ///
+    /// [writable]: AvFrame::is_writable
+    pub fn convert(&mut self, src: &AvFrame, dst: &mut AvFrame) -> Result<(), ConversionError> {
+        if src.format != self.src_pix_format {
+            return Err(ConversionError::FormatMismatch {
+                frame: src.format,
+                converter: self.src_pix_format,
+            });
         }
 
-        // crosvm currently only supports single-buffer frames, so we just need to compute the
-        // destination address of each plane according to the destination format.
-        // Safe because `dst_data` and `dst_stride` are of the expected size for this function and
-        // the mapping is large enough to cover the whole frame.
-        unsafe {
-            ffi::av_image_fill_arrays(
-                dst_data.as_mut_ptr(),
-                dst_stride.as_mut_ptr(),
-                mapping.as_ptr(),
-                self.dst_pix_format,
-                stride,
-                src.height,
-                1,
-            )
-        };
+        if dst.format != self.dst_pix_format {
+            return Err(ConversionError::FormatMismatch {
+                frame: dst.format,
+                converter: self.dst_pix_format,
+            });
+        }
+
+        if src.dimensions() != dst.dimensions() {
+            return Err(ConversionError::DimensionMismatch);
+        }
+
+        if !dst.is_writable() {
+            return Err(ConversionError::NotWritable);
+        }
 
         // Safe because `sws_context`, `src_ref.data` and `dst_data` are all valid pointers, and
         // we made sure the sizes provided are within the bounds of the buffers.
-        unsafe {
+        AvError::result(unsafe {
             ffi::sws_scale(
                 self.sws_context,
                 src.data.as_ptr() as *const *const u8,
                 src.linesize.as_ptr(),
                 0,
                 src.height,
-                dst_data.as_ptr(),
-                dst_stride.as_ptr(),
-            );
-        }
-
-        Ok(())
+                dst.data.as_ptr(),
+                dst.linesize.as_ptr(),
+            )
+        })
+        .map_err(Into::into)
     }
 }