ndk/native_window: Use release/acquire fns for Drop and Clone
#207
Conversation
MarijnS95
force-pushed
the
native-window-acquire
branch
from
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 |
MarijnS95
force-pushed
the
native-window-acquire
branch
from
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 |
MarijnS95
force-pushed
the
native-window-acquire
branch
2 times, most recently
from
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 |
MarijnS95
force-pushed
the
native-window-acquire
branch
from
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.
MarijnS95
force-pushed
the
native-window-acquire
branch
from
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.