Timebase optionality

[kornelski]

LGTM.

Have you considered changing the Rational itself to disallow zero value? There's some redundancy between None and Rational::ZERO.

It could use NonZeroI32 internally and return Option<Self> from new.

[tilpner]

@kornelski Unfortunately, I don't think the zero value (or values, 0/1 is just the only sensible variation)) are universally different from other rationals in ffmpeg. Rather, it is their usage (time base, frame rate, aspect ratio) for which a zero-valued rational doesn't make sense and is abused to indicate not set/absence.

In fact, this is slightly lossy already, in that

(*foo.as_mut_ptr()).time_base = Rational(0, 2).into();
foo.set_time_base(foo.time_base());
assert_eq!((*foo.as_ptr()).time_base.den, 2);

would fail, because time_base maps the (denormal?) zero 0/2 to None, which is mapped back to 0/1 in set_time_base. I do feel weird about that, but it's probably a bug (that we would be obscuring) in ffmpeg or user code for 0/n where n != 1 (including 0/0) to occur in time_base/frame_rate fields.

I did originally have non_0_1 instead of non_zero, which did check for the exact denominator, but then we would be passing through denormal zeroes, and the function name was horrible.

[kornelski]

So if I understand correctly, there can be contexts where someone would want to use Rational 0/1 on purpose, and its use as a sentinel value is API-specific.