Skip to content

Commit

Permalink
ndk/image_reader: Special-case return statuses in Image-acquire fun…
Browse files Browse the repository at this point in the history
…ctions (#457)

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.
  • Loading branch information
MarijnS95 authored Apr 26, 2024
1 parent ce07460 commit 9ba82ab
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 43 deletions.
2 changes: 2 additions & 0 deletions ndk/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
118 changes: 82 additions & 36 deletions ndk/src/media/image_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,62 @@ pub type ImageListener = Box<dyn FnMut(&ImageReader) + Send>;
#[cfg(feature = "api-level-26")]
pub type BufferRemovedListener = Box<dyn FnMut(&ImageReader, &HardwareBuffer) + Send>;

/// 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<T> {
/// 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<T> AcquireResult<T> {
fn map<U>(self, f: impl FnOnce(T) -> U) -> AcquireResult<U> {
match self {
AcquireResult::Image(img) => AcquireResult::Image(f(img)),
AcquireResult::NoBufferAvailable => AcquireResult::NoBufferAvailable,
AcquireResult::MaxImagesAcquired => AcquireResult::MaxImagesAcquired,
}
}
}

impl AcquireResult<Image> {
/// 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<Self> {
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
Expand Down Expand Up @@ -218,16 +274,10 @@ impl ImageReader {
}

#[doc(alias = "AImageReader_acquireNextImage")]
pub fn acquire_next_image(&self) -> Result<Option<Image>> {
let res = construct_never_null(|res| unsafe {
pub fn acquire_next_image(&self) -> Result<AcquireResult<Image>> {
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.
Expand All @@ -239,31 +289,26 @@ impl ImageReader {
/// <https://developer.android.com/ndk/reference/group/media#aimagereader_acquirenextimageasync>
#[cfg(feature = "api-level-26")]
#[doc(alias = "AImageReader_acquireNextImageAsync")]
pub unsafe fn acquire_next_image_async(&self) -> Result<(Image, Option<OwnedFd>)> {
pub unsafe fn acquire_next_image_async(
&self,
) -> Result<AcquireResult<(Image, Option<OwnedFd>)>> {
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<Option<Image>> {
let res = construct_never_null(|res| unsafe {
pub fn acquire_latest_image(&self) -> Result<AcquireResult<Image>> {
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.
Expand All @@ -275,17 +320,18 @@ impl ImageReader {
/// <https://developer.android.com/ndk/reference/group/media#aimagereader_acquirelatestimageasync>
#[cfg(feature = "api-level-26")]
#[doc(alias = "AImageReader_acquireLatestImageAsync")]
pub fn acquire_latest_image_async(&self) -> Result<(Image, Option<OwnedFd>)> {
pub unsafe fn acquire_latest_image_async(
&self,
) -> Result<AcquireResult<(Image, Option<OwnedFd>)>> {
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) })),
})
})
}
}
Expand Down
9 changes: 2 additions & 7 deletions ndk/src/media_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -136,10 +132,9 @@ pub(crate) fn construct_never_null<T>(
with_ptr: impl FnOnce(*mut *mut T) -> ffi::media_status_t,
) -> Result<NonNull<T>> {
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)
})
}

0 comments on commit 9ba82ab

Please sign in to comment.