Skip to content

Commit

Permalink
ndk/media/image_reader: Don't assume ownership of NativeWindow
Browse files Browse the repository at this point in the history
When implementing proper ownership `acquire()` and `release()` semantics
for `NativeWindow` in #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`]: #309
  • Loading branch information
MarijnS95 committed Aug 11, 2023
1 parent a3c34f7 commit ac0caf6
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion ndk/src/media/image_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,13 @@ impl ImageReader {
MediaError::from_status(status)
}

/// Get a [`NativeWindow`] that can be used to produce [`Image`]s for this [`ImageReader`].
///
/// <https://developer.android.com/ndk/reference/group/media#aimagereader_getwindow>
pub fn get_window(&self) -> Result<NativeWindow> {
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))
}
}

Expand Down

0 comments on commit ac0caf6

Please sign in to comment.