rust-mobile / ndk

Rust bindings to the Android NDK
Apache License 2.0
1.11k stars 110 forks source link

`MediaCodec`/`ImageReader` callbacks not `Send` #454

Closed Tazdevil971 closed 7 months ago

Tazdevil971 commented 9 months ago

Both MediaCodec and ImageReader use callbacks, such as set_async_notify_callback, but none of the functions are marked as Send.

The documentation for MediaCodec::set_async_notify_callback specifically states that the callbacks will be invoked on an NDK internal thread, so Send should be mandatory.

For ImageReader::set_image_listener and ImageReader::set_buffer_removed_listener the documentation doesn't say anything, but from the looks of it they must run from an internal thread as well.

And after this we should see if making MediaCodec and ImageReader at least Send would make sense, as they are already internally shared between threads.

EDIT

While looking more thoroughly at the API, it may be necessary to also make Image Send, as the current recommended workflow for ImageReader is to acquire the image in the image_listener callback, and then dispatch it to another thread for processing.

Keeping Image !Send while also making the callback Send basically means that all image processing must be done in the callback, which is not ideal.

MarijnS95 commented 9 months ago

Relevant documentation (always useful to link): https://developer.android.com/ndk/reference/group/media#amediacodec_setasyncnotifycallback

Note that it's not just "an NDK internal thread", it is specifically:

All callbacks are fired on one NDK internal thread.

Emphasis mine. So no more than one thread, hence no need for Sync on borrowed items. Note that the AMediaExtractor bindings state that it can be called from any thread, so https://github.com/rust-mobile/ndk/pull/453 carries Send + Sync straight from the get-go.

Should we file an upstream documentation issue for the other two methods?

CC @spencercw