rust-mobile / ndk

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

ndk/image_reader: Special-case return statuses in `Image`-acquire functions #457

Closed MarijnS95 closed 4 months ago

MarijnS95 commented 8 months ago

Both async and non-async acquire_next/latest_image() functions will return ImgreaderNoBufferAvailable when the producer has not provided a buffer that is either ready for consumption or that can be blocked on (either inside a non-async method, or by returning the accompanying fence file descriptor). But only the non-_async() functions were marked as if this is a common case by returning an Option<>, seemingly out of the assumption that the _async() functions can always give you an image (if the MaxImagesAcquired limit is not reached) but with a file-descriptor sync fence to wait on. This is not the case as the producer needs to submit a buffer together with a sync fence on the producer-end first.

Hence the current API signatures create the false assumption that only non-async functions can "not have a buffer available at all", when the exact same is true for _async() functions, in order to provide an image buffer with a fence that is signalled when it is ready for reading.

Instead of special-casing this error in the _async() functions, special-case both NoBufferAvailable and MaxImagesAcquired in a new enum AcquireResult and let it be returned by both non-async and async functions.

MarijnS95 commented 8 months ago

Instead of special-casing this error in the _async() functions, make all functions return a flat Result and remove the Option, while documenting that this is a common error for the caller to handle gracefully.

Still up for debate, we could also wrap the _async() functions in an extra Option since this is quite a common situation to deal with. We should document that ImgreaderNoBufferAvailable is translated to None in that case though.

MarijnS95 commented 7 months ago

Yeah, if we special-case it (which indeed seems the better idea to not confuse users with things that aren't errors and are annoying to match on), let's do that.

I think we should also remove the non-error codes from MediaError in that case?

MarijnS95 commented 4 months ago

While working on this it really hit me that _only acquire_next_image_async()_ is marked unsafe, but acquire_latest_image_async() is not. Safety docs seem to indicate that's because Image can only be used after the returned OwnedFd (if it's not None) must be awaited before safe use is possible.

MarijnS95 commented 4 months ago

Quickly tested, this still works as expected.