rust-mobile
media_codec: Fix `dequeue_output_buffer` treating some events as error
The MediaCodec wrapper had some issues with handling some non-error results for dequeue_output_buffer. This PR fixes it.
The PR has been tested for the decoder path in soon-to-be-in-production code. I did not experience any unhandleable error originating inside the ndk calls.
Here I used unreachable!() to avoid adding an unused variant in MediaCodecOutputResult and I would like some feedback for a better solution.
Here I used
unreachable!()to avoid adding an unused variant inMediaCodecOutputResultand I would like some feedback for a better solution.
Can you call from_status() unconditionally and match on the Ok(()), Err(ffi::AMEDIACODEC_INFO_TRY_AGAIN_LATER) and Err(_)?
@MarijnS95 The problem is that result is not used in a regular way. AMediaCodec_dequeueOutputBuffer documentation says that 0 or positive values are the buffer index, negative values are error or other non-fatal events. I cannot wrap the result with from_status, first I need to check if the value is a buffer index. But then the Ok(()) value still remains redundant and should never be hit.
I've replaced .map(|()| unreachable!()) with the more idiomatic Err([...].unwrap_err())
@MarijnS95 ping, I'm using my branch in production and it works, I think this PR can be merged.
@zarik5 This patch simply fails to explain why the caller can't match on the Err results from these functions, it seems there's nothing wrong with the current implementation hence this PR isn't "fixing" anything?
@MarijnS95 I see this is a matter of convention. I think AMEDIACODEC_INFO_* events should be not considered as error, in fact there is “INFO” in the name. Furthermore AMEDIACODEC_INFO_* event are not “universal” media_status_t errors, the meaning of these events represented by low magnitude negative integers seems specific to the AMediaCodec_dequeueOutputBuffer call.
Any documentation we can quote that states that these functions return both AMEDIACODEC_INFO_* or any of the AMEDIA_* error values?
Regardless, I'd rather see anything that doesn't return an OutputBuffer to be treated as an Err. Perhaps your MediaCodecOutputResult can instead wrap NdkMediaError? Maybe give input buffers the same treatment. Not sure what the best way forward is here, except that the current implementation looks messy and inconsistent.
Nevertheless, don't forget to document this problem and API change in the changelog.
@MarijnS95 I looked in the source code and it's not completely clear what to do either https://android.googlesource.com/platform/frameworks/av/+/master/media/ndk/NdkMediaCodec.cpp#693 . I tried another opinionated solution.
In any case I fixed also a "real" bug, the AMEDIACODEC_ERROR_INSUFFICIENT_RESOURCE and AMEDIACODEC_ERROR_RECLAIMED error were recognized as a buffer index. I changed the check from "res >= 0" to "0 <= res < 1000"
In any case I fixed also a "real" bug, the
AMEDIACODEC_ERROR_INSUFFICIENT_RESOURCEandAMEDIACODEC_ERROR_RECLAIMEDerror were recognized as a buffer index. I changed the check from "res >= 0" to "0 <= res < 1000"
That's absolutely horrible, is there any documentation stating the highest buffer index being used? Or is it just these two loose positive values?
If you can't find anything I'll ping some NDK contacts to see what's up here.
@MarijnS95 I didn't find any information about a "minimum error code integer" or "maximum buffer index". The "1000" cut point was completely arbitrary on my side.
AMediaCodec_dequeueInputBuffer can indeed return AMEDIACODEC_INFO_TRY_AGAIN_LATER. I fixed it.
@MarijnS95 I didn't find any information about a "minimum error code integer" or "maximum buffer index". The "1000" cut point was completely arbitrary on my side.
Anything we can do about this? Otherwise I'd recommend purely parsing the two positive error codes and considering any other positive number as a valid buffer index.
https://github.com/rust-windowing/android-ndk-rs/blob/25e647abe800d08453b74c3a1fa90528ff97844c/ndk-sys/src/ffi_aarch64.rs#L14563-L14568
Otherwise can you inquire about this over at https://github.com/android/ndk?
Anything we can do about this? Otherwise I'd recommend purely parsing the two positive error codes and considering any other positive number as a valid buffer index.
Not a good idea to me, it might break in a new API level.
Otherwise can you inquire about this over at https://github.com/android/ndk?
Sure.
Not a good idea to me, it might break in a new API level.
It is already broken by design unless there's a maximum buffer index.
Otherwise can you inquire about this over at https://github.com/android/ndk?
Here you go: https://github.com/android/ndk/discussions/1768
This is blocked on filing an upstream bug report to figure out what to do with the broken return codes.
Looks like you bumped the issue today @zarik5, unfortunate to see that upstream hasn't provided any clarification thus far :(
Given no feedback upstream, should we merge this PR with my assumption of 1000 as an absolute value cutting point? I think It's reasonable, because how C APIs normally evolve. Usually error codes get added with increasing magnitude, maybe sometimes skipping tens or hundreds to reserve slots to more neatly group errors by scope, but almost never going under the lowest initial error value (in absolute terms). As for testing, I've been using this PR in production for a while now.
In its current form this PR proposes a breaking change, merging it would mean forcing the release of ndk 0.8.0 and the entire ecosystem propagation mess with it whenever we want to release an ndk update for this or other means.
Rather we accept that this API was fundamentally broken and Deal With It™ until the next sensible opportunity to correct it. Users can currently work around this by manually matching these "error-like" values out of index anyway.
One thing you could do is return these codes in Err(NdkMediaError::UnknownResult(...)) which would not break the API, though we cannot extend MediaErrorResult as it has never been marked #[non_exhaustive] making it equally inconvenient for users to figure out what they have to do. However, I'd like to sanity-check that with a second opinion from someone else who uses the media codec API to prevent this whole "oh we discovered the API doesn't represent important thing X" situation within a month after release again.
Correct me if I'm wrong, but if a certain API is completely broken, it's ok to fix it with a patch release (-.-.X) even if the API surface changes in a breaking way (it's a bug fix). I imagine noone in the wild is doing what you propose right now, instead if they are interested in the Mediacodec API they are probably using my PR.
If it goes paired with a yank of the broken release, within a day of releasing, maybe? At this point you can't assume no-one is using MediaCodec, and you can't stop their project from compiling by publishing a semver-breaking release without bumping semver.
I'm not gonna debate further. I'll will continue using my fork for production, I hope this PR will land sometimes. Maybe add it to a 0.8 tag as a reminder?
Nothing to debate, circumventing semver is simply not done.
Proposing another alternative: new unbroken functions with a different name, and #[deprecate] the broken ones (without duplicating all code)?
Proposing another alternative: new unbroken functions with a different name, and #[deprecate] the broken ones (without duplicating all code)?
No reason messing with it IMO. Let's merge this PR once you are ready to release v0.8.