diff --git a/ndk/CHANGELOG.md b/ndk/CHANGELOG.md index ed19a4c9..769c3fc9 100644 --- a/ndk/CHANGELOG.md +++ b/ndk/CHANGELOG.md @@ -16,6 +16,7 @@ - **Breaking:** input_queue: `get_event()` now returns a `Result` with `std::io::Error`; `InputQueueError` has been removed. (#292) - **Breaking:** input_queue: `has_events()` now returns a `bool` directly without being wrapped in `Result`. (#294) - **Breaking:** hardware_buffer: `HardwareBufferError` has been removed and replaced with `std::io::Error` in return types. (#295) +- Fixed `HardwareBuffer` leak on buffers returned from `AndroidBitmap::get_hardware_buffer()`. (#296) - **Breaking:** Update `jni` crate (used in public API) from `0.18` to `0.19`. (#300) # 0.6.0 (2022-01-05) diff --git a/ndk/src/bitmap.rs b/ndk/src/bitmap.rs index b2de892b..fc95c7d8 100644 --- a/ndk/src/bitmap.rs +++ b/ndk/src/bitmap.rs @@ -11,7 +11,7 @@ use num_enum::{IntoPrimitive, TryFromPrimitive}; use std::{convert::TryInto, mem::MaybeUninit, ptr::NonNull}; #[cfg(feature = "hardware_buffer")] -use crate::hardware_buffer::HardwareBuffer; +use crate::hardware_buffer::HardwareBufferRef; #[repr(i32)] #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -101,8 +101,11 @@ impl AndroidBitmap { BitmapError::from_status(status) } + /// Retrieve the native object associated with a `HARDWARE` [`AndroidBitmap`]. + /// + /// Client must not modify it while an [`AndroidBitmap`] is wrapping it. #[cfg(all(feature = "hardware_buffer", feature = "api-level-30"))] - pub fn get_hardware_buffer(&self) -> BitmapResult { + pub fn get_hardware_buffer(&self) -> BitmapResult { unsafe { let result = construct(|res| ffi::AndroidBitmap_getHardwareBuffer(self.env, self.inner, res))?; @@ -111,7 +114,7 @@ impl AndroidBitmap { } else { NonNull::new_unchecked(result) }; - Ok(HardwareBuffer::from_ptr(non_null)) + Ok(HardwareBufferRef::from_ptr(non_null)) } } } diff --git a/ndk/src/hardware_buffer.rs b/ndk/src/hardware_buffer.rs index 63571b2c..a9b46811 100644 --- a/ndk/src/hardware_buffer.rs +++ b/ndk/src/hardware_buffer.rs @@ -103,11 +103,15 @@ pub struct HardwareBuffer { } impl HardwareBuffer { - /// Create a `HardwareBuffer` from a native pointer + /// Create an _unowned_ [`HardwareBuffer`] from a native pointer + /// + /// To wrap a strong reference (that is `release`d on [`Drop`]), call + /// [`HardwareBufferRef::from_ptr()`] instead. /// /// # Safety - /// By calling this function, you assert that it is a valid pointer to - /// an NDK [`ffi::AHardwareBuffer`]. + /// By calling this function, you assert that it is a valid pointer to an NDK + /// [`ffi::AHardwareBuffer`] that is kept alive externally, or retrieve a strong reference + /// using [`HardwareBuffer::acquire()`]. pub unsafe fn from_ptr(ptr: NonNull) -> Self { Self { inner: ptr } } @@ -125,9 +129,7 @@ impl HardwareBuffer { unsafe { let ptr = construct(|res| ffi::AHardwareBuffer_allocate(&desc.into_native(), res))?; - Ok(HardwareBufferRef { - inner: Self::from_ptr(NonNull::new_unchecked(ptr)), - }) + Ok(HardwareBufferRef::from_ptr(NonNull::new_unchecked(ptr))) } } @@ -135,6 +137,13 @@ impl HardwareBuffer { /// /// # Safety /// By calling this function, you assert that these are valid pointers to JNI objects. + /// + /// This method does not acquire any additional reference to the AHardwareBuffer that is + /// returned. To keep the [`HardwareBuffer`] alive after the [Java `HardwareBuffer`] object + /// is closed, explicitly or by the garbage collector, be sure to retrieve a strong reference + /// using [`HardwareBuffer::acquire()`]. + /// + /// [Java `HardwareBuffer`]: https://developer.android.com/reference/android/hardware/HardwareBuffer pub unsafe fn from_jni(env: *mut JNIEnv, hardware_buffer: jobject) -> Self { let ptr = ffi::AHardwareBuffer_fromHardwareBuffer(env, hardware_buffer); @@ -273,23 +282,41 @@ impl HardwareBuffer { status_to_io_result(status, ()) } + /// Acquire a reference on the given [`HardwareBuffer`] object. + /// + /// This prevents the object from being deleted until the last strong reference, represented + /// by [`HardwareBufferRef`], is [`drop()`]ped. pub fn acquire(&self) -> HardwareBufferRef { unsafe { ffi::AHardwareBuffer_acquire(self.as_ptr()); - } - HardwareBufferRef { - inner: HardwareBuffer { inner: self.inner }, + HardwareBufferRef::from_ptr(self.inner) } } } -/// A [`HardwareBuffer`] with an owned reference, the reference is released when dropped. +/// A [`HardwareBuffer`] with an owned reference, that is released when dropped. /// It behaves much like a strong [`std::rc::Rc`] reference. #[derive(Debug)] pub struct HardwareBufferRef { inner: HardwareBuffer, } +impl HardwareBufferRef { + /// Create an _owned_ [`HardwareBuffer`] from a native pointer + /// + /// To wrap a weak reference (that is **not** `release`d on [`Drop`]), call + /// [`HardwareBuffer::from_ptr()`] instead. + /// + /// # Safety + /// By calling this function, you assert that it is a valid pointer to an NDK + /// [`ffi::AHardwareBuffer`]. + pub unsafe fn from_ptr(ptr: NonNull) -> Self { + Self { + inner: HardwareBuffer { inner: ptr }, + } + } +} + impl Deref for HardwareBufferRef { type Target = HardwareBuffer; @@ -300,9 +327,13 @@ impl Deref for HardwareBufferRef { impl Drop for HardwareBufferRef { fn drop(&mut self) { - unsafe { - ffi::AHardwareBuffer_release(self.inner.as_ptr()); - } + unsafe { ffi::AHardwareBuffer_release(self.inner.as_ptr()) } + } +} + +impl Clone for HardwareBufferRef { + fn clone(&self) -> Self { + self.acquire() } } diff --git a/ndk/src/media/image_reader.rs b/ndk/src/media/image_reader.rs index 97ab8bc9..2e8e1401 100644 --- a/ndk/src/media/image_reader.rs +++ b/ndk/src/media/image_reader.rs @@ -322,6 +322,20 @@ impl Image { construct(|res| unsafe { ffi::AImage_getNumberOfPlanes(self.as_ptr(), res) }) } + /// Get the hardware buffer handle of the input image intended for GPU and/or hardware access. + /// + /// Note that no reference on the returned [`HardwareBuffer`] handle is acquired automatically. + /// Once the [`Image`] or the parent [`ImageReader`] is deleted, the [`HardwareBuffer`] handle + /// from previous [`Image::get_hardware_buffer()`] becomes invalid. + /// + /// If the caller ever needs to hold on a reference to the [`HardwareBuffer`] handle after the + /// [`Image`] or the parent [`ImageReader`] is deleted, it must call + /// [`HardwareBuffer::acquire()`] to acquire an extra reference, and [`drop()`] it when + /// finished using it in order to properly deallocate the underlying memory managed by + /// [`HardwareBuffer`]. If the caller has acquired an extra reference on a [`HardwareBuffer`] + /// returned from this function, it must also register a listener using + /// [`ImageReader::set_buffer_removed_listener()`] to be notified when the buffer is no longer + /// used by [`ImageReader`]. #[cfg(feature = "hardware_buffer")] pub fn get_hardware_buffer(&self) -> Result { unsafe {