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

[MarijnS95]

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]

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.

[rib]

Hmm, right, yeah I suppose my initial instinct was that an Option or some status enum could make sense if it doesn't conceptually feel like it's strictly an error condition or just a valid outcome that needs to be considered.

The documented return values are:

AMEDIA_OK if the method call succeeds.
AMEDIA_ERROR_INVALID_PARAMETER if reader or image is NULL.
AMEDIA_IMGREADER_MAX_IMAGES_ACQUIRED if the number of concurrently acquired images has reached the limit.
AMEDIA_IMGREADER_NO_BUFFER_AVAILABLE if there is no buffers currently available in the reader queue.
AMEDIA_ERROR_UNKNOWN if the method fails for some other reasons.

and it feels notable that neither AMEDIA_IMGREADER_MAX_IMAGES_ACQUIRED or AMEDIA_IMGREADER_NO_BUFFER_AVAILABLE have an 'ERROR' infix.

I wonder if these should have an enum for the _OK, AMEDIA_IMGREADER_MAX_IMAGES_ACQUIRED and AMEDIA_IMGREADER_NO_BUFFER_AVAILABLE outcomes?

[MarijnS95]

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]

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]

I'm quite certain (but please back me up on this) that only the 4 functions touched here can return this error code. I don't want users to go and try to match on these MediaError constants going forward if there are now special AcquireResult variants. If it turns out there are functions that do return this code (which gets turned into __Unknown), that's an error on our side that we should rectify.

For the lock errors below, I think they're more-or-less true errors (user tries to lock an image that's not been created for CPU access) and don't need a more obvious handling than these functions.

I'm a bit skeptical how that fares for the acquire functions. After all the first revision of this PR removed special-casing entirely, so that users looking for NoBufferAvailable had to match the Err themselves. And in a way they can "prevent this invalid(?) state": for example an ImageReader bound to an EGL Context/Surface will not return an Image (not even with a fence) until SwapBuffers() has been called.

[rib]

I'm quite certain (but please back me up on this) that only the 4 functions touched here can return this error code.

yeah, that's also how it looks to me

[rib]

This all seems good to me

[MarijnS95]

Quickly tested, this still works as expected.