From c96c9b5eef646f2cf1e5201eec3aa7a59dcc85de Mon Sep 17 00:00:00 2001 From: Tatsuyuki Ishi Date: Thu, 22 Sep 2022 20:17:32 +0900 Subject: [PATCH] virtio: video: Enrich GuestResource with more metadata. This adds: - A buffer size field for each plane - Width, height, format field for each image This is for introducing helpers to convert video GuestResources into AvFrames. In practice, these metadata were readily available either from the protocol or through rubataga RPCs, but they have not been stored in the GuestResource struct. With these change these fields would be available for usage in constructing AvFrames. A tricky topic is the source of the metadata. Right now, neither the virtio_video_params struct nor virtio-gpu backend metadata alone can provide all the metadata we need. See code comment in GuestResource::from_virtio_object_entry and go/crosvm-enc-ffmpeg for future work discussions. BUG=b:239897269 TEST=cargo test --features "video-decoder,ffmpeg" -p ffmpeg -p devices Change-Id: Icb47274d2dd379a10bdf878ed91ffbc238685c6d Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3911110 Reviewed-by: Alexandre Courbot --- .../virtio/video/decoder/backend/ffmpeg.rs | 9 +- devices/src/virtio/video/decoder/mod.rs | 9 +- devices/src/virtio/video/encoder/mod.rs | 6 +- devices/src/virtio/video/format.rs | 1 + devices/src/virtio/video/resource.rs | 89 +++++++++++++------ 5 files changed, 82 insertions(+), 32 deletions(-) diff --git a/devices/src/virtio/video/decoder/backend/ffmpeg.rs b/devices/src/virtio/video/decoder/backend/ffmpeg.rs index 4db5b30be3..1d62cb1100 100644 --- a/devices/src/virtio/video/decoder/backend/ffmpeg.rs +++ b/devices/src/virtio/video/decoder/backend/ffmpeg.rs @@ -990,8 +990,10 @@ mod tests { } )); + let out_format = Format::NV12; + session - .set_output_parameters(NUM_OUTPUT_BUFFERS, Format::NV12) + .set_output_parameters(NUM_OUTPUT_BUFFERS, out_format) .unwrap(); // Pass the buffers we will decode into. @@ -1005,12 +1007,17 @@ mod tests { FramePlane { offset: 0, stride: H264_STREAM_WIDTH as usize, + size: (H264_STREAM_WIDTH * H264_STREAM_HEIGHT) as usize, }, FramePlane { offset: (H264_STREAM_WIDTH * H264_STREAM_HEIGHT) as usize, stride: H264_STREAM_WIDTH as usize, + size: (H264_STREAM_WIDTH * H264_STREAM_HEIGHT) as usize, }, ], + width: H264_STREAM_WIDTH as _, + height: H264_STREAM_HEIGHT as _, + format: out_format, }, ) .unwrap(); diff --git a/devices/src/virtio/video/decoder/mod.rs b/devices/src/virtio/video/decoder/mod.rs index dc1a66e689..3da3890773 100644 --- a/devices/src/virtio/video/decoder/mod.rs +++ b/devices/src/virtio/video/decoder/mod.rs @@ -526,9 +526,9 @@ impl Decoder { }; // Now try to resolve our resource. - let (resource_type, plane_formats) = match queue_type { - QueueType::Input => (ctx.in_params.resource_type, &ctx.in_params.plane_formats), - QueueType::Output => (ctx.out_params.resource_type, &ctx.out_params.plane_formats), + let (resource_type, params) = match queue_type { + QueueType::Input => (ctx.in_params.resource_type, &ctx.in_params), + QueueType::Output => (ctx.out_params.resource_type, &ctx.out_params), }; let resource = match resource_type { @@ -543,6 +543,7 @@ impl Decoder { // exactly one element. unsafe { entries.get(0).unwrap().object }, &self.resource_bridge, + params, ) .map_err(|_| VideoError::InvalidArgument)? } @@ -555,7 +556,7 @@ impl Decoder { ) }, &self.mem, - plane_formats, + params, ) .map_err(|_| VideoError::InvalidArgument)?, }; diff --git a/devices/src/virtio/video/encoder/mod.rs b/devices/src/virtio/video/encoder/mod.rs index 1bc86ca51e..273a3cba54 100644 --- a/devices/src/virtio/video/encoder/mod.rs +++ b/devices/src/virtio/video/encoder/mod.rs @@ -672,6 +672,7 @@ impl EncoderDevice { // exactly one element. unsafe { entries.get(0).unwrap().object }, &self.resource_bridge, + &stream.src_params, ) .map_err(|_| VideoError::InvalidArgument)? } @@ -684,7 +685,7 @@ impl EncoderDevice { ) }, &self.mem, - &stream.src_params.plane_formats, + &stream.src_params, ) .map_err(|_| VideoError::InvalidArgument)?, }; @@ -719,6 +720,7 @@ impl EncoderDevice { // exactly one element. unsafe { entries.get(0).unwrap().object }, &self.resource_bridge, + &stream.dst_params, ) .map_err(|_| VideoError::InvalidArgument)? } @@ -731,7 +733,7 @@ impl EncoderDevice { ) }, &self.mem, - &stream.dst_params.plane_formats, + &stream.dst_params, ) .map_err(|_| VideoError::InvalidArgument)?, }; diff --git a/devices/src/virtio/video/format.rs b/devices/src/virtio/video/format.rs index 9c4eb59c0b..7371517ec6 100644 --- a/devices/src/virtio/video/format.rs +++ b/devices/src/virtio/video/format.rs @@ -306,4 +306,5 @@ pub struct Rect { pub struct FramePlane { pub offset: usize, pub stride: usize, + pub size: usize, } diff --git a/devices/src/virtio/video/resource.rs b/devices/src/virtio/video/resource.rs index 6d3028b3d8..906c5b8509 100644 --- a/devices/src/virtio/video/resource.rs +++ b/devices/src/virtio/video/resource.rs @@ -23,8 +23,9 @@ use crate::virtio::resource_bridge; use crate::virtio::resource_bridge::ResourceBridgeError; use crate::virtio::resource_bridge::ResourceInfo; use crate::virtio::resource_bridge::ResourceRequest; +use crate::virtio::video::format::Format; use crate::virtio::video::format::FramePlane; -use crate::virtio::video::format::PlaneFormat; +use crate::virtio::video::params::Params; use crate::virtio::video::protocol::virtio_video_mem_entry; use crate::virtio::video::protocol::virtio_video_object_entry; @@ -178,6 +179,9 @@ pub struct GuestResource { pub handle: GuestResourceHandle, /// Layout of color planes, if the resource will receive frames. pub planes: Vec, + pub width: u32, + pub height: u32, + pub format: Format, } #[derive(Debug, ThisError)] @@ -207,11 +211,14 @@ impl GuestResource { /// resource. /// /// Convert `mem_entry` into the guest memory resource it represents and resolve it through - /// `mem`. `planes_format` describes the format of the individual planes for the buffer. + /// `mem`. + /// Width, height and format is set from `params`. + /// + /// Panics if `params.format` is `None`. pub fn from_virtio_guest_mem_entry( mem_entries: &[virtio_video_mem_entry], mem: &GuestMemory, - planes_format: &[PlaneFormat], + params: &Params, ) -> Result { let region_desc = match mem_entries.first() { None => return Err(GuestMemResourceCreationError::NoEntriesProvided), @@ -245,9 +252,15 @@ impl GuestResource { }) .collect(); + let handle = GuestResourceHandle::GuestPages(GuestMemHandle { + desc: region_desc, + mem_areas, + }); + // The plane information can be computed from the currently set format. let mut buffer_offset = 0; - let planes = planes_format + let planes = params + .plane_formats .iter() .map(|p| { let plane_offset = buffer_offset; @@ -256,16 +269,17 @@ impl GuestResource { FramePlane { offset: plane_offset as usize, stride: p.stride as usize, + size: p.plane_size as usize, } }) .collect(); Ok(GuestResource { - handle: GuestResourceHandle::GuestPages(GuestMemHandle { - desc: region_desc, - mem_areas, - }), + handle, planes, + width: params.frame_width, + height: params.frame_height, + format: params.format.unwrap(), }) } @@ -277,6 +291,7 @@ impl GuestResource { pub fn from_virtio_object_entry( object: virtio_video_object_entry, res_bridge: &base::Tube, + params: &Params, ) -> Result { // We trust that the caller has chosen the correct object type. let uuid = u128::from_be_bytes(object.uuid); @@ -296,24 +311,45 @@ impl GuestResource { Err(e) => return Err(ObjectResourceCreationError::ResourceBridgeFailure(e)), }; + let handle = GuestResourceHandle::VirtioObject(VirtioObjectHandle { + // Safe because `buffer_info.file` is a valid file descriptor and we are stealing + // it. + desc: unsafe { + SafeDescriptor::from_raw_descriptor(buffer_info.handle.into_raw_descriptor()) + }, + modifier: buffer_info.modifier, + }); + + // TODO(ishitatsuyuki): Right now, there are two sources of metadata: through the + // virtio_video_params fields, or through the buffer metadata provided + // by the VirtioObject backend. + // Unfortunately neither is sufficient. The virtio_video_params struct + // lacks the plane offset, while some virtio-gpu backend doesn't + // have information about the plane size, or in some cases even the + // overall frame width and height. + // We will mix-and-match metadata from the more reliable data source + // below; ideally this should be fixed to use single source of truth. + let planes = params + .plane_formats + .iter() + .zip(&buffer_info.planes) + .map(|(param, buffer)| FramePlane { + // When the virtio object backend was implemented, the buffer and stride was sourced + // from the object backend's metadata (`buffer`). To lean on the safe side, we'll + // keep using data from `buffer`, even in case of stride it's also provided by + // `param`. + offset: buffer.offset as usize, + stride: buffer.stride as usize, + size: param.plane_size as usize, + }) + .collect(); + Ok(GuestResource { - handle: GuestResourceHandle::VirtioObject(VirtioObjectHandle { - // Safe because `buffer_info.file` is a valid file descriptor and we are stealing - // it. - desc: unsafe { - SafeDescriptor::from_raw_descriptor(buffer_info.handle.into_raw_descriptor()) - }, - modifier: buffer_info.modifier, - }), - planes: buffer_info - .planes - .iter() - .take_while(|p| p.offset != 0 || p.stride != 0) - .map(|p| FramePlane { - offset: p.offset as usize, - stride: p.stride as usize, - }) - .collect(), + handle, + planes, + width: params.frame_width, + height: params.frame_height, + format: params.format.unwrap(), }) } @@ -322,6 +358,9 @@ impl GuestResource { Ok(Self { handle: self.handle.try_clone()?, planes: self.planes.clone(), + width: self.width, + height: self.height, + format: self.format, }) } }