ndk/native_window: Use release/acquire fns for Drop and Clone#207
ndk/native_window: Use release/acquire fns for Drop and Clone#207
release/acquire fns for Drop and Clone#207Conversation
2d3df9f to
e4027c7
Compare
|
Note that I haven't tested this PR locally yet. Curious what will happen when we release the latest reference with Much like |
e4027c7 to
eee6ef8
Compare
Finally got around to testing this, and we must indeed increment the refcount if we ever wish to also decrement it (ie. the added @zarik5 This should also addess the |
9ec2568 to
48b0aab
Compare
| /// | ||
| /// # Safety | ||
| /// `ptr` must be a valid pointer to an Android [`ffi::ANativeWindow`]. | ||
| pub unsafe fn clone_from_ptr(ptr: NonNull<ffi::ANativeWindow>) -> Self { |
There was a problem hiding this comment.
Naming suggestions welcome here. There are some cases (fn from_ptr()) where Android gives us a pointer that we own, ie. we should call ANativeWindow_release when done (and not call ANativeWindow_acquire when constructing our owned wrapper). There are also cases - on_window_created + on_window_destroyed - where Android does its own refcounting/lifetime management. If we wish to participate we should also increment the refcount before even thinking about releasing it again.
|
I actually have never checked about leaked NativeWindow, thanks for the work. |
|
@dvc94ch Re-requesting your review because I changed this PR since your approval. |
|
still looks good |
48b0aab to
37285c3
Compare
Much like `ALooper` and `AHardwareBuffer` the reference counter of `ANativeWindow` must be properly incremented on `Clone`, in turn allowing us to also `_release` the window as soon as it is dropped.
37285c3 to
d8f8932
Compare
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
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
Much like ALooper and AHardwareBuffer the reference counter of ANativeWindow must be properly incremented on
Clone, in turn allowing us to also_releasethe window as soon as it is dropped.