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

Unify lifetime handling for NativeWindow and HardwareBuffer #309

Open
MarijnS95 opened this issue Jul 2, 2022 · 1 comment
Open

Unify lifetime handling for NativeWindow and HardwareBuffer #309

MarijnS95 opened this issue Jul 2, 2022 · 1 comment
Labels
difficulty: average Likely as difficult as most tasks here priority: low Nice to have status: needs investigation Issue must be confirmed and researched type: enhancement Wouldn't this be the coolest?

Comments

@MarijnS95
Copy link
Member

MarijnS95 commented Jul 2, 2022 •
edited
Loading

See #296 (comment), there are multiple ways we can approach this.

Something based on borrows (Like how String + &str work together) has my preference, but we can't give any sensible lifetime to for example what fn native_window() returns (which we currently have to forcibly _acquire to make it valid). Alas, we'll probably remove those static getters anyway in light of recent suggestions and to support multi-Activity, so this specific case wouldn't affect us anymore. At which point a borrow-based approach might as well work out :)
@MarijnS95 MarijnS95 added type: enhancement Wouldn't this be the coolest? difficulty: average Likely as difficult as most tasks here priority: low Nice to have status: needs investigation Issue must be confirmed and researched labels Jul 2, 2022
@MarijnS95 MarijnS95 mentioned this issue Jul 5, 2022
Merged
MarijnS95 added a commit that referenced this issue 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 issue Aug 11, 2023
Merged
MarijnS95 added a commit that referenced this issue 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
@MarijnS95 MarijnS95 mentioned this issue Jul 29, 2024
Open
@MarijnS95
Copy link
Member Author

Looking into this again, handing out &'_ Type would be preferred (i.e. to match Rust's &str vs String deisgn), but we need storage for Type in this case which is impossible. Hence HardwareBufferRef solves this by simply owning a HardwareBuffer and implementing Drop for itself.

We could possibly improve this API by adding a lifetime to HardwareBuffer, which HardwareBufferRef hides by setting it to 'static internally because it controls the lifetime. That would allow us to derive Clone, Copy on the non-refcounted HardwareBuffer, as long as Deref for HardwareBufferRef returns a target of (&'a self) -> &'a HardwareBuffer<'a> and not 'static. On the other hand the current implementation (without lifetime on HardwareBuffer) already captures a very similar semantic when &'_ HardwareBuffer exists, except that HardwareBuffer::from_ptr() gives you an owned and lifetime-less HardwareBuffer.

@MarijnS95 MarijnS95 mentioned this issue Aug 10, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty: average Likely as difficult as most tasks here priority: low Nice to have status: needs investigation Issue must be confirmed and researched type: enhancement Wouldn't this be the coolest?
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@MarijnS95