[WIP] init Add support for Config::validate

[Klaven]

Added support for Config::validate in capi issue: #1659

I have a few questions as I was making this, figured I would make a PR and ask them :)

1. where should this functions that I made get called :) I made them figured they should be used.

fn rav1e_config_validate(&mut self) -> bool feels like it should be fn rav1e_config_validate(&self) -> Result<(), InvalidConfig> This would then require whatever calls rav1e_config_validate to set last_err

3. per the issue Add support for Config::validate() in the CAPI #1659 Remove the early validation in the setter and update the documentation accordingly. What setter are you talking about? and what documentation? I looked at the files in /doc but was not sure any fit the bill for the capi.

4. I made these impl but could have just as easily made them functions. what is your preference?

Other then that, how does this look as a start? I have never really done unsafe rust, and need to work on a setup to test out what I have done. but it seems to build at this point :) As I don't know quite where the methods are to be called from i'm not sure what their scope should be.

[coveralls]

Coverage decreased (-0.09%) to 71.013% when pulling bb0d25f on Klaven:master into 45585ee on xiph:master.

[dwbuiten]

Apologies, as the biggest C API user, and part designer... I had not seen #1659 yet.

I disagree with the entire premise of the issue - and I will reproduce this there as a well, but:

There's absolutely no reason, as far as I can tell, that Config::validate should be exposed as a separate API function. Unlike the Rust API, whuch is stuct based and requires a separate call to valdate it, all options set in the C API must be done through setter functions, and this is where Config::validate should be called, and error returned, not as a new C API. It's not idiomatic in the C API, and I am not aware of a compelling reason that it should be a separate C API call. I am keen to be informed if there is one... (mirroring an API from a different language is not a compelling reason to be non-idiomatic).

Again apologies, this is not you're doing, and we (and I!) appreciate the submission!

I can, however, answer some of your questions still!

1. I have a patch to add librav1e (the C API lib) support to FFmpeg, which hasn't been merged upstream yet because we are waiting for rav1e to release a crate (and thus commit to semver on breaking changes). You can find the latest version here. We (Vimeo) use it from our own codebase. If there's not an open bug for a good C API example in the repo, I suppose there should be one.

2. N/A due to above Opinion™.

3. C API documentation is generated from the comments above public C functions in capi.rs during build of the C library and header, and placed in the resulting rav1e.h.

4. N/A

Again, thanks for the contribution! I think we still need some changes, just not a new API - that is, calling the valiation function from within the setter C API call...

[Klaven]

Ok cool, I guess then we can just close this PR? or do you have some way you want me to take and update it? @dwbuiten

[dwbuiten]

@Klaven It can probably be reworked to call Config::validate() from within the setter function(s).

[Klaven]

I'll get something pushed out and see if its more on the lines of what you where thinking and maybe you could then give me a few more pointers on what I could do to make it solid! thanks for the input!

[Klaven]

@dwbuiten I updated it with a few calls. Let me know what you think. I'm not sure if I should have extended out the Invalid Config enum like that... but I did so that I could set a useful "last_err" on all the set calls. If this is not wanted, I will change it however you would like! please just let me know if this is in the right direction or not!

[dwbuiten]

Sorry for the slow response.

I don't really have an opinion on extending the config error enum - maybe @tdaede or @lu-zero have opinions.

Two notes:

1. Your patch seems to be missing a call to validate inside the setter functions.
2. Your git history isn't clean - we don't put various old WIP commits in our history, and we require each commit to be atomic, with a clean commit message (not 'fix x' or 'apply review changes'.

Other than that, it's getting there!

[Klaven]

@dwbuiten I plan on squashing no problem. I understand that. most of the time I leave the commits when changes are asked so the reviewer gets to she what changed after they requested a change. I take this to mean you are ready for a squash and will do that tonight.