RationalTime rate shouldn't allow to set 0 as value

Tilix4 commented 2 years ago

Bug Report

Incorrect Functionality and General Questions

Describe the issue here.

To Reproduce

Operating System: Any
Python version: Python3.9
OpenTimelineIO 0.14.0


from opentimelineio.opentime import RationalTime, TimeRange
from opentimelineio.schema import Clip, Track, Timeline

time = RationalTime(0.0, 25)

tl = Timeline()
track = Track("Track 1")
clip = Clip("movie.mkv")
clip.source_range = TimeRange(
    RationalTime(30, 0),  # Must raise an error
    RationalTime(10, 0),  # Must raise an error
)
track.append(clip)
tl.tracks.append(track)

print(clip.source_range.duration.is_invalid_time())
# >> True

print(tl.duration())
# >> RationalTime(inf, 1)
# 'inf' is very tough to handle

Expected Behavior

Setting 0 as RationalTime.rate value must raise a ValueError("Rate must be positive."). I'm not sure a rate could be negative though, unless it means the movie must be played reversed by the software.

meshula commented 2 years ago

Thanks for the report!

Note to the implementor of a fix: OpenTime itself doesn't throw exceptions from the foundational types of TimeRange and RationalTime. The expected implementation would be implemented on the assignment to source_range within Clip.

Currently, source_range is stored as optional<TimeRange>. Checked values in OpenTimelineIO itself either throw, or set an ErrorStatus. Due to our use of initializers in construction, an ErrorStatus parameter would make no sense, therefore the choices are to silently set the optional to empty, or to throw. Silently setting empty seems like a foot gun, because it's 99% sure that a quick TD script is not going to check the result, suggesting that it should throw on invalid values.

std::optional is a very primitive class lacking predicated assignment, so the logical way forward is to create a new checked_optional template class that takes a templated predication function, which throws. We could then typedef a checked_optional_TimeRange as

template <typename T>
class checked_optional {
     template <auto check(const T&) -> bool> checked_optional(const T& v) {
            if (!check(v)) throw ValueError("Invalid checked assignment");
            value = v; valid = true;
     }

     T value;
     bool valid = false;
};
typedef checked_optional<TimeRange, [](const TimeRange& tr) -> bool { 
          return !tr.start_time.is_invalid() && !tr.duration.is_invalid(); } 
checked_optional_TimeRange;

Note that I would emphatically not use std::optional here as the value type, but that said, enough of std::optional would need to replicated to support the current implementation of OTIO. This mostly means implementing the pattern-of-5 constructor/assignment suite. The pybind11 shim would need some massaging as well, but I expect this to be easy. The other language bindings are not auto-magic like pybind11, but updating would be straight forward.

I'm of course open to other suggestions, but I think this is the most robust way to handle it.

jminor commented 2 years ago

Another option is to do more rigorous validation at other times, like right before and after function calls that are expected to have completely valid timelines. Specifically, adapters.write_to_file and possibly read_from_file could validate the timeline including every time range. By doing this validation only at moments when the whole timeline is being processed, we can check for both small and large consistency issues, and ensure that momentarily invalid states are okay during the construction/modification of a timeline.

rogernelson commented 2 years ago

Due to our use of initializers in construction, an ErrorStatus parameter would make no sense, therefore the choices are to silently set the optional to empty, or to throw. Silently setting empty seems like a foot gun, because it's 99% sure that a quick TD script is not going to check the result, suggesting that it should throw on invalid values.

What about using nodiscard with ErrorStatus:

struct [[nodiscard]] ErrorStatus

Then the signature of set_source_status could be changed to

ErrorStatus set_source_range(optional<TimeRange> const& source_range);

Resulting in a compilation warning if any callers ignored the return value. Of course, this implies that anywhere ErrorStatus is returned, it would need to be handled, but that's not necessarily bad...

meshula commented 2 years ago

The [[nodiscard]] tag is worth looking at for an overall API scrub, now that you mention it.

The reason I was thinking of the checked_optional pattern is that throughout the python bindings we have this pattern in dozens of places:

        .def_property("available_range", &MediaReference::available_range, &MediaReference::set_available_range)

the most common method we use for setting in python is not the set_ functions, but rather the auto-magic property bindings. By introducing a new checked_optional type, we would get 100% coverage for property assignment via pybind11 automagic.

My current feeling is that the three proposals we've got so far (checked_optional, rigorous validation, [[nodiscard]], are all highly complementary and would all contribute to library quality.

jminor commented 1 year ago

AcademySoftwareFoundation / OpenTimelineIO