media_codec: Add support for asynchronous notification callbacks

[spencercw]

Note that unlike the audio data callback, I do not provide a reference to the codec as a callback parameter. Happy to be overruled on this, but I find the notion of constructing a new MediaCodec which wraps an ffi::AMediaCodec already owned by another MediaCodec somewhere else to be a bit questionable. If you really need access to the MediaCodec within the callback than you can use a Mutex or something, but the reality is that you can't actually do very much within the callback. For example, if you try to release the output buffer in the 'output available' callback then the whole thing just deadlocks.

Typical usage might look something like this (sans proper error handling):

let (input_tx, input_rx) = channel(32);
let (output_tx, output_rx) = channel(32);
codec
    .set_async_notify_callback(Some(AsyncNotifyCallback {
        on_input_available: Some(Box::new(move |index| {
            input_tx.blocking_send(index).unwrap();
        })),
        on_output_available: Some(Box::new(move |index, _info| {
            output_tx.blocking_send(index).unwrap();
        })),
        on_format_changed: None,
        on_error: Some(Box::new(move |error, action_code, detail| panic!()))
    })).unwrap();

Elsewhere...

select! {
    buffer_index = input_rx.recv() => {
        let buffer_index = buffer_index.unwrap();
        let input_buffer = codec.input_buffer(buffer_index).unwrap();
        // Fill buffer...
        codec.queue_input_buffer_by_index(buffer_index, 0, buffer_len, 0, 0).unwrap();
    }
    buffer_index = output_rx.recv() => {
        codec.release_output_buffer_by_index(buffer_index.unwrap(), true).unwrap();
    }
}

[MarijnS95]

Happy to be overruled on this, but I find the notion of constructing a new MediaCodec which wraps an ffi::AMediaCodec already owned by another MediaCodec somewhere else to be a bit questionable.

It is not only questionable, this should break on Drop unless AMediaCodec is internally refcounted and we can increase the refcount like we do for other Android objects in a custom Clone implementation.

Perhaps you can work something out to give a &MediaCodec back? Though this does rely on some weak/strong handle to a Mutex/Arc indeed, so that the callback can't technically get to a MediaCodec if it happens to be fired after the object has already been destroyed and deallocated. On the other hand Android might define this API to not call any callbacks after it was destroyed, meaning some "working around the borrow checker" might've been adequate.

In the past I've been wanting to have some unsafe fn borrrow_from_ptr<'a>(x: NonNull<...>) -> &'a Self but we can't really borrow it "from somewhere" nor cast a pointer to a borrow of said pointer.

[MarijnS95]

@zarik5 you might want to see this!

[MarijnS95]

Let's not have the PR link in the commit title (as if this was a final merge-commit) please :)

[spencercw]

On the other hand Android might define this API to not call any callbacks after it was destroyed, meaning some "working around the borrow checker" might've been adequate.

Looking at the Android source code, when an AMediaCodec is deleted, it stops the looper used for asynchronous callbacks, and this appears to block while waiting for the looper thread to exit. Naturally this is not documented, but I don't think I would be too bothered about just assuming the AMediaCodec is still valid when we get called. It gives us an AMediaCodec pointer after all, so it would be a bit weird for that pointer to be potentially invalid.

Maybe we can pass a reference to the callback, but.. why? Even something as simple as AMediaCodec_getName deadlocks when invoked from a callback. As far as I can tell there is nothing useful you can do with the codec handle. It's just a footgun, luring you into doing something which is going to break your program.

Let's not have the PR link in the commit title (as if this was a final merge-commit) please :)

I was just copying the convention from previous commits. Is the PR number added to the commit automatically or something?

[MarijnS95]

@spencercw oh I may have skimmed your question too quickly then and assumed the deadlocks were caused by wrapping things in a Mutex on the Rust side for lack of "temporarily borrowing" (e.g. preventing Drop to be called) a MediaCodec based on the ffi::AMediaCodec given to you. If you say that Android givind you that pointer is only a misleading black hole, let's not use that pointer for now.

(The only use I can think of then is typically using the same callback function for multiple objects, and being able to distinguish which of the codec objects just called the callback... But you can also use the userdata pointer for that, as already done by storing extra data in Box<dyn FnMut> closures)

Let's not have the PR link in the commit title (as if this was a final merge-commit) please :)

I was just copying the convention from previous commits. Is the PR number added to the commit automatically or something?

It'll be added automatically on squash merge by GitHub indeed 🙂 (and I think we'd have it twice otherwise, if I'm careless while clicking merge).

[MarijnS95]

@spencercw thanks for #412, it's merged now. Can you rebase this and perhaps look at / mark-as-resolved the above discussion items?

[spencercw]

Rebased on master. I think that's all comments addressed now.

[MarijnS95]

Looking good already, thanks for doing so many drive-by cleanups and improvements to the media_codec.rs API too!

Probably my main concern is not having doc-comments on any of these, but (for functions that were already here before your PR) that should be tackled in a separate PR.

Let me know if you're up for that or if I should tackle this, as I am quite pedantic towards tailoring the upstream docs to Rust 😉

[MarijnS95]

Looking good! The only remaining comments are to update the line-ranges in the links, then this is good to merge (and I know I've been asking a lot, thanks for bearing with me...).

[spencercw]

No worries. It's good to get it all polished up before merging :)

[MarijnS95]

Really nitty, but want to make the same remark that previous callbacks are also unregistered when this returns an error?

[spencercw]

Not sure how I feel about documenting implementation behaviour that is not mentioned in the upstream documentation. I guess it's not the worst thing since we rely on that behaviour within this function anyway, just feels a bit icky..

[MarijnS95]

Then let's not, and let's have our Rust docs as incomplete as the NDK ones 👌

[zmerp]

That's a bit sad to be honest but 🤷‍♂️

[MarijnS95]

@zarik5 what's sad? The fact that AOSP unregisters previously-successfully-registered callbacks on error, or the fact that we're not going to document it?

[zmerp]

The documentation bit.