ndk: Fix HardwareBuffer leak for AndroidBitmap and clarify handli…

…ng in docs (#296)

According to [`AndroidBitmap_getHardwareBuffer`]:

    outBuffer   On success, is set to a pointer to the AHardwareBuffer
                associated with bitmap. This acquires a reference on the
                buffer, and the client must call AHardwareBuffer_release
                when finished with it.

By returning the `outBuffer` wrapped in `HardwareBuffer` we represent a
weak instead of strong reference, and don't `_release` it on `Drop`.  If
we instead wrap it in a `HardwareBufferRef` we get the desired `release`
semantics on `Drop` and prevent resource leaks for the user.

Note that a similar API on [`AImage_getHardwareBuffer`] does **not**
aquire a strong reference, and should remain returning a weak
`HardwareBuffer`.

This feat isn't very well described in the docs, so we now echo what was
only stated on `HardwareBufferRef` across the `from_ptr()`/`from_jni()`
implementations.

Finally, just like `ForeignLooper` and `NativeWindow` this
`acquire`+`release` kind of type can implement `Clone` to allow the user
to take advantage of internal refcounting instead of having to re-wrap
it in `Rc` or `Arc`.

[`AndroidBitmap_getHardwareBuffer`]: https://developer.android.com/ndk/reference/group/bitmap#androidbitmap_gethardwarebuffer
[`AImage_getHardwareBuffer`]: https://developer.android.com/ndk/reference/group/media#aimage_gethardwarebuffer

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)

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<HardwareBuffer> {
+    pub fn get_hardware_buffer(&self) -> BitmapResult<HardwareBufferRef> {
         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))
         }
     }
 }

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<ffi::AHardwareBuffer>) -> Self {
         Self { inner: ptr }
     }
@@ -125,16 +129,21 @@ 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)))
         }
     }
 
     /// Create a [`HardwareBuffer`] from JNI pointers
     ///
     /// # 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<ffi::AHardwareBuffer>) -> 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()
     }
 }
 

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<HardwareBuffer> {
         unsafe {