from_timecode doesn't treat rate properly

[meshula]

@darbyjohnston reports

I was debugging an issue in my code and noticed something interesting about RationalTime::from_timecode(), however I'm not an expert in timecode so I don't know if this is expected behavior?
If I compare the result of RationalTime::from_timecode("01:00:00:00", 24.0) and RationalTime::from_timecode("01:00:00:00", 23.976), they both have the same value, 86400.0, but with the respective 24.0 and 23.976 rates. I guess I was expecting the second value to be 86,313.6 (23.976 * 3600.0)?
It looks like this is caused by RationalTime::from_timecode() running std::ceil() on the rate before calculating the result.

@meshula analyzes

I notice test_opentime.py, and the cpp, lack tests for equality between rates. For example, a test comparing two identical timecodes at different rates, converted to seconds (by dividing out the rate), should return the same result.
nominal_fps is just supposed to be used to check if the integer frame count in the time code string is greater than the number of frames that can possibly be valid indicated in the string.
The conversion to frames at the end of the function should be using rate, not nominal_fps.

Fix for this issue should include unit tests at the cpp level, optionally reported in the py bindings.

[markreidvfx]

when I've implemented timecode to seconds in the past, I've always found it less confusing to explicitly pass the nominal_fps like timecode_to_seconds(timecode_str, nominal_fps, frame_rate)
or split it up
frames_to_seconds(timecode_to_frames(timecode_str, nominal_fps), frame_rate)

[meshula]

When OpenTime implements time travel, I'm going back and explaining, emphatically, to the NTSC that their clever yet beknighted idea of wodging colorburst information between audio and the black and white in the transmission is going to screw up the computation of time, until the end of time. The last analog broadcast in North America was switched off in 2009, but we are still stuck with the analog hack for black and white/color compatibility!

[darbyjohnston]

The conversion to frames at the end of the function should be using rate, not nominal_fps.

Should a similar change also be made in RationalTime::to_timecode() so the values round trip? Also what about this test in test_opentime.py:

def test_timecode_23976_fps(self):
        # This should behave exactly like 24 fps
        timecode = "00:00:01:00"
        t = otio.opentime.RationalTime(value=24, rate=23.976)
        self.assertEqual(t, otio.opentime.from_timecode(timecode, 23.976))

Is this test irrelevant if 24 and 23.976 fps behave differently, or should the comment be removed and the test values updated to work?

[meshula]

When is one second not a second? When NTSC has had a go.

A more meaningful test to replace that one would be to generate rational times for "00:00:01:00" at 24 and 23.976, and then assert a meaningful equivalence due to imprecision from the NTSC rate.

[jminor]

Related: support for NTSC rates in timecode conversion was improved in this PR: #1477

[meshula]

I think it's addressed, thanks.