From 3bd43885cf7758cc2742a1bda5999bc682cec96a Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 25 Dec 2023 12:10:30 +0100 Subject: [PATCH] ndk/image_reader: Special-case return statuses in `Image`-acquire functions Both async and non-async `acquire_next/latest_image()` functions will return `ImgreaderNoBufferAvailable` when the producer has not provided a buffer that is either ready for consumption or that can be blocked on (either inside a non-async method, or by returning the accompanying fence file descriptor). But only the non-`_async()` functions were marked as if this is a common case by returning an `Option<>`, seemingly out of the assumption that the `_async()` functions can _always_ give you an image (if the `MaxImagesAcquired` limit is not reached) but with a file-descriptor sync fence to wait on. This is not the case as the producer needs to submit a buffer together with a sync fence on the producer-end first. Hence the current API signatures create the false assumption that only non-async functions can "not have a buffer available at all", when the exact same is true for `_async()` functions, in order to provide an image buffer with a fence that is signalled when it is ready for reading. Instead of special-casing this error in the `_async()` functions, special-case both `NoBufferAvailable` and `MaxImagesAcquired` in a new `enum AcquireResult` and let it be returned by both non-async and async functions. --- ndk/CHANGELOG.md | 2 + ndk/src/media/image_reader.rs | 118 +++++++++++++++++++++++----------- ndk/src/media_error.rs | 9 +-- 3 files changed, 86 insertions(+), 43 deletions(-) diff --git a/ndk/CHANGELOG.md b/ndk/CHANGELOG.md index 1a3b4b19..1afb504f 100644 --- a/ndk/CHANGELOG.md +++ b/ndk/CHANGELOG.md @@ -15,6 +15,8 @@ - Drop previous `Box`ed callbacks _after_ registering new ones, instead of before. (#455) - input_queue: Add `from_java()` constructor, available since API level 33. (#456) - event: Add `from_java()` constructors to `KeyEvent` and `MotionEvent`, available since API level 31. (#456) +- **Breaking:** image_reader: Special-case return statuses in `Image`-acquire functions. (#457) +- **Breaking:** image_reader: Mark `ImageReader::acquire_latest_image_async()` `unsafe` to match the safety requirements on `ImageReader::acquire_next_image_async()`. (#457) - event: Implement `SourceClass` `bitflag` and provide `Source::class()` getter. (#458) - Ensure all `bitflags` implementations consider all (including unknown) bits in negation and `all()`. (#458) - **Breaking:** Mark all enums as `non_exhaustive` and fix `repr` types. (#459) diff --git a/ndk/src/media/image_reader.rs b/ndk/src/media/image_reader.rs index b1fd2006..d4143776 100644 --- a/ndk/src/media/image_reader.rs +++ b/ndk/src/media/image_reader.rs @@ -54,6 +54,62 @@ pub type ImageListener = Box; #[cfg(feature = "api-level-26")] pub type BufferRemovedListener = Box; +/// Result returned by: +/// - [`ImageReader::acquire_next_image()`]` +/// - [`ImageReader::acquire_next_image_async()`]` +/// - [`ImageReader::acquire_latest_image()`]` +/// - [`ImageReader::acquire_latest_image_async()`]` +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum AcquireResult { + /// Returned if there is no buffers currently available in the reader queue. + #[doc(alias = "AMEDIA_IMGREADER_NO_BUFFER_AVAILABLE")] + NoBufferAvailable, + /// Returned if the number of concurrently acquired images has reached the limit. + #[doc(alias = "AMEDIA_IMGREADER_MAX_IMAGES_ACQUIRED")] + MaxImagesAcquired, + + /// Returned if an [`Image`] (optionally with fence) was successfully acquired. + Image(T), +} + +impl AcquireResult { + fn map(self, f: impl FnOnce(T) -> U) -> AcquireResult { + match self { + AcquireResult::Image(img) => AcquireResult::Image(f(img)), + AcquireResult::NoBufferAvailable => AcquireResult::NoBufferAvailable, + AcquireResult::MaxImagesAcquired => AcquireResult::MaxImagesAcquired, + } + } +} + +impl AcquireResult { + /// Inlined version of [`construct_never_null()`] with IMGREADER-specific result mapping. + fn construct_never_null( + with_ptr: impl FnOnce(*mut *mut ffi::AImage) -> ffi::media_status_t, + ) -> Result { + let mut result = MaybeUninit::uninit(); + let status = with_ptr(result.as_mut_ptr()); + match status { + ffi::media_status_t::AMEDIA_IMGREADER_NO_BUFFER_AVAILABLE => { + Ok(Self::NoBufferAvailable) + } + ffi::media_status_t::AMEDIA_IMGREADER_MAX_IMAGES_ACQUIRED => { + Ok(Self::MaxImagesAcquired) + } + status => MediaError::from_status(status).map(|()| { + let result = unsafe { result.assume_init() }; + Self::Image(Image { + inner: if cfg!(debug_assertions) { + NonNull::new(result).expect("result should never be null") + } else { + unsafe { NonNull::new_unchecked(result) } + }, + }) + }), + } + } +} + /// A native [`AImageReader *`] /// /// [`AImageReader *`]: https://developer.android.com/ndk/reference/group/media#aimagereader @@ -218,16 +274,10 @@ impl ImageReader { } #[doc(alias = "AImageReader_acquireNextImage")] - pub fn acquire_next_image(&self) -> Result> { - let res = construct_never_null(|res| unsafe { + pub fn acquire_next_image(&self) -> Result> { + AcquireResult::construct_never_null(|res| unsafe { ffi::AImageReader_acquireNextImage(self.as_ptr(), res) - }); - - match res { - Ok(inner) => Ok(Some(Image { inner })), - Err(MediaError::ImgreaderNoBufferAvailable) => Ok(None), - Err(e) => Err(e), - } + }) } /// Acquire the next [`Image`] from the image reader's queue asynchronously. @@ -239,31 +289,26 @@ impl ImageReader { /// #[cfg(feature = "api-level-26")] #[doc(alias = "AImageReader_acquireNextImageAsync")] - pub unsafe fn acquire_next_image_async(&self) -> Result<(Image, Option)> { + pub unsafe fn acquire_next_image_async( + &self, + ) -> Result)>> { let mut fence = MaybeUninit::uninit(); - let inner = construct_never_null(|res| { + AcquireResult::construct_never_null(|res| { ffi::AImageReader_acquireNextImageAsync(self.as_ptr(), res, fence.as_mut_ptr()) - })?; - - let image = Image { inner }; - - Ok(match fence.assume_init() { - -1 => (image, None), - fence => (image, Some(unsafe { OwnedFd::from_raw_fd(fence) })), + }) + .map(|result| { + result.map(|image| match fence.assume_init() { + -1 => (image, None), + fence => (image, Some(unsafe { OwnedFd::from_raw_fd(fence) })), + }) }) } #[doc(alias = "AImageReader_acquireLatestImage")] - pub fn acquire_latest_image(&self) -> Result> { - let res = construct_never_null(|res| unsafe { + pub fn acquire_latest_image(&self) -> Result> { + AcquireResult::construct_never_null(|res| unsafe { ffi::AImageReader_acquireLatestImage(self.as_ptr(), res) - }); - - if let Err(MediaError::ImgreaderNoBufferAvailable) = res { - return Ok(None); - } - - Ok(Some(Image { inner: res? })) + }) } /// Acquire the latest [`Image`] from the image reader's queue asynchronously, dropping older images. @@ -275,17 +320,18 @@ impl ImageReader { /// #[cfg(feature = "api-level-26")] #[doc(alias = "AImageReader_acquireLatestImageAsync")] - pub fn acquire_latest_image_async(&self) -> Result<(Image, Option)> { + pub unsafe fn acquire_latest_image_async( + &self, + ) -> Result)>> { let mut fence = MaybeUninit::uninit(); - let inner = construct_never_null(|res| unsafe { + AcquireResult::construct_never_null(|res| { ffi::AImageReader_acquireLatestImageAsync(self.as_ptr(), res, fence.as_mut_ptr()) - })?; - - let image = Image { inner }; - - Ok(match unsafe { fence.assume_init() } { - -1 => (image, None), - fence => (image, Some(unsafe { OwnedFd::from_raw_fd(fence) })), + }) + .map(|result| { + result.map(|image| match fence.assume_init() { + -1 => (image, None), + fence => (image, Some(unsafe { OwnedFd::from_raw_fd(fence) })), + }) }) } } diff --git a/ndk/src/media_error.rs b/ndk/src/media_error.rs index 261b1065..99fb0d5f 100644 --- a/ndk/src/media_error.rs +++ b/ndk/src/media_error.rs @@ -62,10 +62,6 @@ pub enum MediaError { DrmLicenseExpired = ffi::media_status_t::AMEDIA_DRM_LICENSE_EXPIRED.0, #[doc(alias = "AMEDIA_IMGREADER_ERROR_BASE")] ImgreaderErrorBase = ffi::media_status_t::AMEDIA_IMGREADER_ERROR_BASE.0, - #[doc(alias = "AMEDIA_IMGREADER_NO_BUFFER_AVAILABLE")] - ImgreaderNoBufferAvailable = ffi::media_status_t::AMEDIA_IMGREADER_NO_BUFFER_AVAILABLE.0, - #[doc(alias = "AMEDIA_IMGREADER_MAX_IMAGES_ACQUIRED")] - ImgreaderMaxImagesAcquired = ffi::media_status_t::AMEDIA_IMGREADER_MAX_IMAGES_ACQUIRED.0, #[doc(alias = "AMEDIA_IMGREADER_CANNOT_LOCK_IMAGE")] ImgreaderCannotLockImage = ffi::media_status_t::AMEDIA_IMGREADER_CANNOT_LOCK_IMAGE.0, #[doc(alias = "AMEDIA_IMGREADER_CANNOT_UNLOCK_IMAGE")] @@ -136,10 +132,9 @@ pub(crate) fn construct_never_null( with_ptr: impl FnOnce(*mut *mut T) -> ffi::media_status_t, ) -> Result> { let result = construct(with_ptr)?; - let non_null = if cfg!(debug_assertions) { + Ok(if cfg!(debug_assertions) { NonNull::new(result).expect("result should never be null") } else { unsafe { NonNull::new_unchecked(result) } - }; - Ok(non_null) + }) }