Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ndk/native_window: Use release/acquire fns for Drop and Clone #207

Merged
merged 1 commit into from
Apr 19, 2022
Merged

ndk/native_window: Use release/acquire fns for Drop and Clone #207

merged 1 commit into from
Apr 19, 2022

Conversation

MarijnS95
Copy link
Member

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 MarijnS95 mentioned this pull request Dec 6, 2021
Closed
@MarijnS95
Copy link
Member Author

Note that I haven't tested this PR locally yet. Curious what will happen when we release the latest reference with ANativeWindow_release in on_window_destroyed, or if we give up all references that way before Android destroys our window.

Much like ALooper, should we also call _acquire in fn ptr()?

@MarijnS95
Copy link
Member Author

MarijnS95 commented Apr 8, 2022 •
edited
Loading

Much like ALooper, should we also call _acquire in fn ptr()?

Finally got around to testing this, and we must indeed increment the refcount if we ever wish to also decrement it (ie. the added drop() implementation here). Turns out we can "keep using" the window after finishing the activity even though it doesn't show anymore, so it's not too bad that users can now do this. We already had derive(Clone) anyway.

@zarik5 This should also addess the NativeWindow leaking from AMediaCodec_createInputSurface. Note that you're taking this window by value in set_input_surface/set_output_surface which will require the caller to clone the window while resulting in a drop() soon afterwards. I don't think that's intended?

@MarijnS95 MarijnS95 force-pushed the native-window-acquire branch 2 times, most recently from 9ec2568 to 48b0aab Compare April 8, 2022 12:58
MarijnS95
MarijnS95 commented Apr 8, 2022
View reviewed changes
ndk/src/native_window.rs
///
/// # Safety
/// `ptr` must be a valid pointer to an Android [`ffi::ANativeWindow`].
pub unsafe fn clone_from_ptr(ptr: NonNull<ffi::ANativeWindow>) -> Self {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zarik5
Copy link
Contributor

zarik5 commented Apr 8, 2022

I actually have never checked about leaked NativeWindow, thanks for the work.

@MarijnS95 MarijnS95 requested a review from dvc94ch April 19, 2022 14:59
@MarijnS95
Copy link
Member Author

@dvc94ch Re-requesting your review because I changed this PR since your approval.

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 19, 2022

still looks good

@MarijnS95
ndk/native_window: Use release/acquire fns for Drop and Clone
d8f8932 
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 MarijnS95 merged commit f24606c into master Apr 19, 2022
@MarijnS95 MarijnS95 deleted the native-window-acquire branch April 19, 2022 17:25
@MarijnS95 MarijnS95 added the type: api Design and usability label Jun 10, 2022
@MarijnS95 MarijnS95 mentioned this pull request Jun 12, 2022
Merged
@MarijnS95 MarijnS95 mentioned this pull request Jul 5, 2022
Merged
MarijnS95 added a commit that referenced this pull request Aug 11, 2023
@MarijnS95
ndk/media/image_reader: Don't assume ownership of NativeWindow
ac0caf6 
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
@MarijnS95 MarijnS95 mentioned this pull request Aug 11, 2023
Merged
MarijnS95 added a commit that referenced this pull request Aug 15, 2023
@MarijnS95
ndk/media/image_reader: Don't assume ownership of NativeWindow (#418)
e3f9225 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvc94ch dvc94ch dvc94ch approved these changes

Assignees
No one assigned
Labels
type: api Design and usability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarijnS95 @zarik5 @dvc94ch