media/media_codec: Replace construct_never_null and infallible unwrap() pattern with something more sensible
#248
Conversation
…rap()` pattern with something more sensible
These functions return a pointer directly instead of an error code with
the pointer as argument, making them unsuitable for use with
`construct_never_null`. Because there is no error code, returning a
manual success value still requires an infallible `unwrap()` call to
unpack the `Result`.
Replace this with a new helper function that does not unnecessarily wrap
the resulting pointer in a `Result` when there is no result code to base
this off, and match the signature to a function that returns the pointer
directly.
At the same time reduce the size of some `unsafe {}` blocks to wrap just
what is unsafe.
|
@zarik5 I trust you have checked all these calls wrapped in |
|
@MarijnS95 Well no, I didn't check all functions. In general the MediaCodec API requires the user to know what they're doing, because if you call some functions in the wrong context it will probably lead to a crash. For example it's invalid to call |
|
@zarik5 Meaning that none or most of these should not call In that case these should never be substituted with |
If you want the caller to uphold "undocumented" (I don't see it anywhere in |
|
For when the function returns a pointer this makes sense. But specifically for |
I'm fine with this. |
Would it crash in the native function or just correctly return a
Note again that this should only be necessary if we can't do anything else about it. Ie. if the function just returns |
The the thing is I don't know. I'm afraid it might crash inside the native function. I tested only the decoder path, with the proper usage, and at the moment I put that code pause to work on other things. I might try to do more testing, but it will be still be limited. This is more a problem with the (loack of) official documentation. |
Yeah indeed. Let me know if you get to testing, otherwise I might try at some point. We can still merge this PR even though there's not much sense if we end up replacing all I do like to get this done before the next release though, so that these are no breaking changes after the initial API for |
|
Merged this PR for its initial cleanup, @zarik5 one of us should really see about fallibility so that we can fix up the API before the next release. |
|
Unfortunately I can't allocate more time on this. The only help i can provide is this code, which has been tested to some degree but not fully. I started working (for a job) on other fronts. |
|
As such I'm not sure if we should back out of this change entirely, or mark the module private until safety concerns are resolved. Alternatively one could preemptively change the necessary interfaces to return
FWIW I think we're also fine removing |
The Android documentation does not comment on when functions could possibly return `null`, and while discussion in [#248] finds these unlikely enough to be communicated back to the user there is no 100% guarantee and the resulting pointers should as such always be checked (either at all, or also when `cfg!(debug_assertions)` happens to be disabled). Note that this `NonNull::new(..).unwrap()` pattern is found in many other places in the `ndk`. [#248]: #248
|
@zarik5 Perhaps if you do manage to find time at some point, would you mind porting that code to a minimal example for inclusion in |
|
A "possibly-unsafe" feature is ugly but I don't have any nicer alternative. Removing the whole module would be a loss since it would work fine if used correctly. I found this crate which wraps the mediacodec API, which seems to have much more extensive checking. Maybe a collaboration is possible? Currently it's not nice to use with ndk (the crate in this repo) since it reimplements all wrappers for pointers like ANativeWindow etc. |
|
I'll open an issue on their repo |
|
@zarik5 Good call. Sharing their |
…256) The Android documentation does not comment on when functions could possibly return `null`, and while discussion in [#248] finds these unlikely enough to be communicated back to the user there is no 100% guarantee and the resulting pointers should as such always be checked (either at all, or also when `cfg!(debug_assertions)` happens to be disabled). Note that this `NonNull::new(..).unwrap()` pattern is found in many other places in the `ndk`. [#248]: #248
Back when I refactored these in #248 and #256 the closure argument should have been removed entirely as it is only called immediately without any arguments, so its return value could have been passed instead. At that point the function becomes simple enough to warrant removal entirely, inlining the unwrap and even allowing a more natural string literal error message in `.expect()`.
Back when I refactored these in #248 and #256 the closure argument should have been removed entirely as it is only called immediately without any arguments, so its return value could have been passed instead. At that point the function becomes simple enough to warrant removal entirely, inlining the unwrap and even allowing a more natural string literal error message in `.expect()`.
These functions return a pointer directly instead of an error code with the pointer as argument, making them unsuitable for use with
construct_never_null. Because there is no error code, returning a manual success value still requires an infallibleunwrap()call to unpack theResult.Replace this with a new helper function that does not unnecessarily wrap the resulting pointer in a
Resultwhen there is no result code to base this off, and match the signature to a function that returns the pointer directly.At the same time reduce the size of some
unsafe {}blocks to wrap just what is unsafe.