rav1e icon indicating copy to clipboard operation
rav1e copied to clipboard

"unstable" feature is redundant

Open rzumer opened this issue 4 years ago • 2 comments

Opening this issue for discussion. As far as I can tell, the unstable feature is not supposed to do anything on its own, but is meant to indicate that one or more unstable feature(s) was/were enabled at build time. However it is useless because there is nothing forcing a user to set it correctly.

There is one extra function defined if it is on, but that seems like an error, since channel-api, the only unstable feature, can be enabled without it. If the intent was to block compilation by adding a function required by the channel API in unstable, that's confusing and poor practice.

It does not seem to be pulled with other unstable features automatically, and it does not affect the version string or anything to indicate that the build includes unstable features.

I have two potential solutions in mind:

  • remove it
  • enforce its use in build.rs by panicking if an unstable feature is enabled without it

rzumer avatar Sep 16 '20 20:09 rzumer

The channel-api feature right now it is silently omitted if unstable is not passed along. So it works as intended in preventing the user from using it without being aware it is unstable.

A build.rs check (or a compile-time check) would be a good idea so who doesn't read the instruction can be told to do so.

lu-zero avatar Sep 17 '20 05:09 lu-zero

Thanks for clarifying.

A secondary issue is that unstable is presented as just a compile-time guard, but in practice it also defines one function (not sure if it changes encoder behavior on its own). Based on its description it seems like it should not change code in the encoder. I propose making the function it defines part of another feature. Even if it is minor, it is unintuitive from a user point of view for the mandatory meta-feature to also pull in unrelated changes.

rzumer avatar Sep 17 '20 06:09 rzumer