From 83c32ed99fe9e6ece78f8b3568d87ac0aeb67a36 Mon Sep 17 00:00:00 2001
From: Sludge <96552222+SludgePhD@users.noreply.github.com>
Date: Sat, 9 Mar 2024 23:04:50 +0100
Subject: [PATCH] Make `render_picture` unsafe

---
 examples/jpeg-decode.rs |  6 ++++--
 src/context.rs          | 37 ++++++++++++++++++++++++++++---------
 src/jpeg.rs             | 14 ++++++++------
 src/vpp.rs              |  4 +---
 4 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/examples/jpeg-decode.rs b/examples/jpeg-decode.rs
index 36235eb..2ae2da3 100644
--- a/examples/jpeg-decode.rs
+++ b/examples/jpeg-decode.rs
@@ -111,8 +111,10 @@ fn main() -> anyhow::Result<()> {
     let mut pppbuf = Buffer::new_param(&vpp_context, BufferType::ProcPipelineParameter, pppbuf)?;
 
     let mut picture = vpp_context.begin_picture(&mut vpp_surface)?;
-    picture.render_picture(&mut pppbuf)?;
-    unsafe { picture.end_picture()? }
+    unsafe {
+        picture.render_picture(&mut pppbuf)?;
+        picture.end_picture()?;
+    }
 
     drop(pppbuf);
 
diff --git a/src/context.rs b/src/context.rs
index d61bf17..b39f231 100644
--- a/src/context.rs
+++ b/src/context.rs
@@ -40,6 +40,9 @@ impl Context {
     }
 
     /// Begins a libva operation that will render to (or encode from) the given [`Surface`].
+    ///
+    /// Returns an [`InProgressPicture`] that can be used to submit parameter and data buffers to
+    /// libva.
     pub fn begin_picture<'a>(
         &'a mut self,
         target: &'a mut Surface,
@@ -72,6 +75,9 @@ impl Drop for Context {
 }
 
 /// An operation whose submission is still in progress.
+///
+/// Submit parameter and data buffers with [`InProgressPicture::render_picture`] and finish the
+/// operation, kicking off decoding or encoding, by calling [`InProgressPicture::end_picture`].
 pub struct InProgressPicture<'a> {
     d: Arc<DisplayOwner>,
     context: &'a mut Context,
@@ -82,15 +88,28 @@ impl<'a> InProgressPicture<'a> {
     ///
     /// Typically, libva does not document which buffer types are required for any given entry
     /// point, so good luck!
-    pub fn render_picture<T>(&mut self, buffer: &mut Buffer<T>) -> Result<()> {
-        unsafe {
-            check(
-                "vaRenderPicture",
-                self.d
-                    .libva
-                    .vaRenderPicture(self.d.raw, self.context.id, &mut buffer.id(), 1),
-            )
-        }
+    ///
+    /// # Safety
+    ///
+    /// Buffers containing metadata structures must contain a valid value of the particular subtype
+    /// required by the configured [`Profile`][crate::Profile] and
+    /// [`Entrypoint`][crate::Entrypoint].
+    ///
+    /// For example, when using [`Profile::JPEGBaseline`][crate::Profile::JPEGBaseline] and
+    /// [`Entrypoint::VLD`][crate::Entrypoint::VLD], submitting a [`Buffer`] with
+    /// [`BufferType::SliceParameter`][crate::buffer::BufferType::SliceParameter] requires that the
+    /// [`Buffer`] contains a [`jpeg::SliceParameterBuffer`][crate::jpeg::SliceParameterBuffer],
+    /// and submitting only the substructure [`SliceParameterBufferBase`] will cause Undefined
+    /// Behavior.
+    ///
+    /// [`SliceParameterBufferBase`]: crate::SliceParameterBufferBase
+    pub unsafe fn render_picture<T>(&mut self, buffer: &mut Buffer<T>) -> Result<()> {
+        check(
+            "vaRenderPicture",
+            self.d
+                .libva
+                .vaRenderPicture(self.d.raw, self.context.id, &mut buffer.id(), 1),
+        )
     }
 
     /// Finishes submitting buffers, and begins the libva operation (encode, decode, etc.).
diff --git a/src/jpeg.rs b/src/jpeg.rs
index 9a4116f..cd61e9b 100644
--- a/src/jpeg.rs
+++ b/src/jpeg.rs
@@ -594,12 +594,14 @@ impl JpegDecodeSession {
             Buffer::new_data(&self.jpeg_context, BufferType::SliceData, &slice_data)?;
 
         let mut picture = self.jpeg_context.begin_picture(&mut self.jpeg_surface)?;
-        picture.render_picture(&mut buf_dht)?;
-        picture.render_picture(&mut buf_iq)?;
-        picture.render_picture(&mut buf_pp)?;
-        picture.render_picture(&mut buf_slice_param)?;
-        picture.render_picture(&mut buf_slice_data)?;
-        unsafe { picture.end_picture()? }
+        unsafe {
+            picture.render_picture(&mut buf_dht)?;
+            picture.render_picture(&mut buf_iq)?;
+            picture.render_picture(&mut buf_pp)?;
+            picture.render_picture(&mut buf_slice_param)?;
+            picture.render_picture(&mut buf_slice_data)?;
+            picture.end_picture()?;
+        }
 
         Ok(&mut self.jpeg_surface)
     }
diff --git a/src/vpp.rs b/src/vpp.rs
index 4743f25..b5dca35 100644
--- a/src/vpp.rs
+++ b/src/vpp.rs
@@ -682,7 +682,6 @@ mod tests {
     use super::*;
 
     #[test]
-    #[ignore] // FIXME: fails on AMD/Mesa
     fn vpp_copy() {
         run_test(|display| {
             // Surface with test data.
@@ -705,8 +704,8 @@ mod tests {
                 Buffer::new_param(&context, BufferType::ProcPipelineParameter, pppbuf).unwrap();
 
             let mut picture = context.begin_picture(&mut output_surface).unwrap();
-            picture.render_picture(&mut params).unwrap();
             unsafe {
+                picture.render_picture(&mut params).unwrap();
                 picture.end_picture().unwrap();
             }
 
@@ -719,7 +718,6 @@ mod tests {
             .expect("failed to create output image");
 
             output_surface.sync().expect("sync failed");
-            // TODO: the following unwrap fails on AMD/Mesa for seemingly no reason
             output_surface.copy_to_image(&mut output_image).unwrap();
 
             output_surface.sync().unwrap();