Safe mediacodec bindings

[zmerp]

Add MediaCodec and MediaFormat bindings with most of their methods. Crypto and DRM support is not implemented.

Testing
Only the decoder code path is tested. sample

Note: This PR needs to be rebased after #213 is merged

[MarijnS95]

For starters, there are a lot of unnecessary as _ casts and typed initializations. Please either be explicit about a cast or remove it altogether.

Adding #![warn(trivial_casts)] at the top of this file (hopefully we can do that repo-wide at some point) will help you track these down.

[zmerp]

@MarijnS95 Ok, I should have addressed all your suggestions. I used:

cargo clippy -p ndk --target armv7-linux-androideabi --features api-level-30 --features media
cargo clippy -p ndk --target aarch64-linux-android --features api-level-30 --features media

[MarijnS95]

Fwiw feel free to leave the warning at the top of the file for the meantime. I'll work out to combine that with #219.

[MarijnS95]

cargo clippy -p ndk --target armv7-linux-androideabi --features api-level-30 --features media
cargo clippy -p ndk --target aarch64-linux-android --features api-level-30 --features media

You may also use x86_64/i686 as those have a different pointer type for c_char (u8), afaik armv7 also has i8 like aarch64?

[zmerp]

You may also use x86_64/i686 as those have a different pointer type for c_char (u8), afaik armv7 also has i8 like aarch64?

Ok, tried and no extra warnings or errors.

[zmerp]

There are still some warnings because of unused imports when using api-level-24, not only from my code

[MarijnS95]

There are still some warnings because of unused imports when using api-level-24, not only from my code

That is something we should perhaps build-test in the CI and fix? Mind opening a separate PR for that (or issue if nontrivial)?

[zmerp]

Can this PR be merged? The warnings can be addressed in a second moment.

[dvc94ch]

LGTM, thanks @zarik5

[MarijnS95]

I wasn't actually okay with this yet, but that's what I get for being on holiday. Doc comments with double instead of triple slashes, get_ prefixes, changelog in the past tense and no reference to this PR, inconsistent use statements that are unused when certain features are not enabled, enum that doesn't derive common traits like Eq but instead has a strange matches! (why aren't the flags modeled directly with a #[repr(u32)] enum?), and that's just the stylistic nits.

[dvc94ch]

Sometimes things are good enough. But agree that it would be better with the changes you mentioned.

[MarijnS95]

If crates are going to be maintained (or at least PRs merged) with a "good is good enough" mentality I'll have to step down as a maintainer, as I can't be cleaning up behind peoples back to uphold an acceptable feeling of responsibility towards said project.

That is, unless there's clear agreement and documentation that a PR is dragging on too long (in terms of discussions, not silence), and is best merged in preparation for followup changes to land through separate PRs.

[zmerp]

I'm sorry @MarijnS95, I realize that things were rushed. I tried picking up the style around but I did a poor job. The wrong doc comments were the most glaring mistake for me. I think it would really help a PR checklist (not necessarily in this repo, you could link to a good one in another repo or website).

[MarijnS95]

I don't think a checklist for standard style errors really exists or make sense; it should all be caught by (locally runnable) CI tools like clippy. In the grand scheme of things I'd like to fail on many more default-allow (rustc or clippy) lint warnings, but it takes a ginormous effort to get this project to pass stricter lints initially 😶

We could however use a checklist for obvious things like requiring a changelog entry if a PR modifies public-facing API/behaviour.

[MarijnS95]

In any case, my post above links at least twice to https://rust-lang.github.io/api-guidelines/ - that's a good starting place if you're looking for more guidance.