ndk: Fix HardwareBuffer leak for AndroidBitmap and clarify handling in docs

[MarijnS95]

Depends on #294, #295

According to AndroidBitmap_getHardwareBuffer:

outBuffer   On success, is set to a pointer to the AHardwareBuffer
            associated with bitmap. This acquires a reference on the
            buffer, and the client must call AHardwareBuffer_release
            when finished with it.

By returning the outBuffer wrapped in HardwareBuffer we represent a weak instead of strong reference, and don't _release it on Drop. If we instead wrap it in a HardwareBufferRef we get the desired release semantics on Drop and prevent resource leaks for the user.

Note that a similar API on AImage_getHardwareBuffer does not aquire a strong reference, and should remain returning a weak HardwareBuffer.

This feat isn't very well described in the docs, so we now echo what was only stated on HardwareBufferRef across the from_ptr()/from_jni() implementations.

Finally, just like ForeignLooper and NativeWindow this acquire+release kind of type can implement Clone to allow the user to take advantage of internal refcounting instead of having to re-wrap it in Rc or Arc.

[MarijnS95]

@JasperDeSutter I've recently implemented this differently for NativeWindow in #207 by always making sure the NativeWindow is acquired, hence always release it on Drop. That way it can't accidentally become invalid either, since there exist no checks here to see whether the object is still alive.

What do you think, is it advantageous to have an ever-so-slightly-faster API that doesn't call acquire, or should we remove HardwareBufferRef entirely? IMO seems advantageous to have a somewhat consistent API around this.

For the cases where we have to call acquire (clone_from_ptr() in #207) to construct a valid NativeWindow I wish I could have returned (through transmuting) a &NativeWindow instead. That immediately makes this clear through the type system, puts an implicit borrow on the source type, and obviates the acquire operation through a .clone() when the user wishes to store an owned version of it.

But I didn't manage to write such a transmute since NativeWindow is already a pointer reference inside, &NativeWindow would be a double indirection and not type compatible in any way. (I've seen a crate doing this, but they were newtyping the owned private ffi object internally [struct NativeWindow(ffi::ANativeWindow)] together with ?Sized if I'm not mistaken, so that &NativeWindow would be binary compatible with a raw pointer to the ffi type - and then have a separate owned type around it that derefs to &NativeWindow).

What do you think?

[JasperDeSutter]

Nice catch about the bitmap api, some of these gotchas are easy to miss in the documentation.

The current design which is based on camera/video usage where frames are transmitted frequently and usually has a need to be well-optimised. You wouldn't normally lock the buffer if you can synchronously process or memcpy it before the system callback returns. However I didn't do any profiling to see if this actually matters at all so it is premature optimization.

I agree with just removing the *Ref system if it leads to a cleaner API, let's not delve into more unsafe than is needed with the transmutes. Having the reference (&) explicitly in the type system is very clear for users, but the extra work (or risk?) doesn't seem worth it. Note that std library types like Path and PathBuf also don't always do this.
Edit: nvm about that last one, I forgot Path is unsized 😄

[MarijnS95]

Nice catch about the bitmap api, some of these gotchas are easy to miss in the documentation.

I wouldn't be surprised if they added that doc bit later, certain functions (onWindowCreated anyone?) don't document this at all (last I looked at them).

Lets get this PR in with all its documentation improvements and a leak-fix for Bitmap, and we can make a note to revisit this API later. In hindsight I don't think we need lots of unsafe transmuting if we have a HardwareBuffer(ffi::AHardwareBuffer) (that we make unsized to prevent the user owning it), hand out & to it and perhaps a custom .clone() function that hands out a HardwareBufferRef which is basically a NonNull<HardwareBuffer> then, performing the _acquire and _release. That's easy to replicate for NativeWindow too, execpt that a '_ lifetime on native_window() isn't very descriptive 😬

EDIT: In that sense it's perhaps still worth unifying both APIs, but we'll make sure that NativeWindow still calls _acquire to not run into accidents with onWindowDestroyed, because we can't associate a proper lifetime to a borrow that we could return in an acquire-less codepath.