From e3f9225d1cf6634ff2dfee1ec63a3769a27402ea Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Wed, 16 Aug 2023 01:11:40 +0200 Subject: [PATCH] ndk/media/image_reader: Don't assume ownership of `NativeWindow` (#418) When implementing proper ownership `acquire()` and `release()` semantics for `NativeWindow` in https://github.com/rust-mobile/ndk/pull/207, I thought I had checked all existing calls to `NativeWindow::from_ptr()` to make sure that they return a pointer that we get ownership over, and have to clean up ourselves. This turns out to [not be the case for `AImageReader_getWindow()`]: The ANativeWindow is managed by this image reader. Do NOT call ANativeWindow_release on it. Instead, use AImageReader_delete. And can be trivially reproduced by creating an `ImageReader` and calling `.get_window()`. Dropping the `NativeWindow` is fine but subsequently dropping the `ImageReader` results in a NULL-pointer SEGFAULT. For now calling `clone_from_ptr()` is enough to first acquire an extra reference on the pointer so that ownership remains balanced, but in the future we'd like to investigate a [new non-owned `NativeWindow` type similar to `HardwareBuffer`]. As of writing the following calls to `from_ptr()` remain, that are all confirmed to transfer ownership and require cleanup via `_release()`: - `ASurfaceTexture_acquireANativeWindow()`; - `AMediaCodec_createInputSurface()`; - `AMediaCodec_createPersistentInputSurface()`; - `ANativeWindow_fromSurface()`. [not be the case for `AImageReader_getWindow()`]: https://developer.android.com/ndk/reference/group/media#aimagereader_getwindow [new non-owned `NativeWindow` type similar to `HardwareBuffer`]: https://github.com/rust-mobile/ndk/issues/309 --- ndk/src/media/image_reader.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ndk/src/media/image_reader.rs b/ndk/src/media/image_reader.rs index 590c9782..dd464f08 100644 --- a/ndk/src/media/image_reader.rs +++ b/ndk/src/media/image_reader.rs @@ -174,10 +174,13 @@ impl ImageReader { MediaError::from_status(status) } + /// Get a [`NativeWindow`] that can be used to produce [`Image`]s for this [`ImageReader`]. + /// + /// pub fn get_window(&self) -> Result { unsafe { let ptr = construct_never_null(|res| ffi::AImageReader_getWindow(self.as_ptr(), res))?; - Ok(NativeWindow::from_ptr(ptr)) + Ok(NativeWindow::clone_from_ptr(ptr)) } }