Improve handling of rates when converting to timecode strings (#1477)

* Deal with "close" timecode rates via a heuristic, instead of misleading entries in valid_timecode_rates table.
* Renamed "valid_timecode" functions to "smpte_timecode" for clarity (old names are deprecated, but still work).
* Both `is_smpte_timecode_rate` and `nearest_smpte_timecode_rate` now adhere to ST 12-1:2014 - SMPTE Standard - Time and Control Code.

Signed-off-by: Joshua Minor <[email protected]>

Author: jminor

src/opentime/errorStatus.cpp

@@ -13,9 +13,9 @@ ErrorStatus::outcome_to_string(Outcome o)
         case OK:
             return std::string();
         case INVALID_TIMECODE_RATE:
-            return "invalid timecode rate";
+            return "SMPTE timecode does not support this rate";
         case INVALID_TIMECODE_STRING:
-            return "string is not a valid timecode string";
+            return "string is not a SMPTE timecode string";
         case TIMECODE_RATE_MISMATCH:
             return "timecode specifies a frame higher than its rate";
         case INVALID_TIME_STRING:

src/opentime/rationalTime.cpp

@@ -13,75 +13,68 @@ namespace opentime { namespace OPENTIME_VERSION {
 
 RationalTime RationalTime::_invalid_time{ 0, RationalTime::_invalid_rate };
 
-static constexpr std::array<double, 4> dropframe_timecode_rates{ {
-    // 23.976,
-    // 23.98,
-    // 23.97,
-    // 24000.0/1001.0,
-    29.97,
+static constexpr std::array<double, 2> dropframe_timecode_rates{ {
     30000.0 / 1001.0,
-    59.94,
     60000.0 / 1001.0,
 } };
 
+// See the official source of these numbers here:
+// ST 12-1:2014 - SMPTE Standard - Time and Control Code
+// https://ieeexplore.ieee.org/document/7291029
+//
 static constexpr std::array<double, 11> smpte_timecode_rates{
-    { 1.0,
-      12.0,
-      24000.0 / 1001.0,
+    { 24000.0 / 1001.0,
       24.0,
       25.0,
       30000.0 / 1001.0,
       30.0,
+      48000.0 / 1001.0,
       48.0,
       50.0,
       60000.0 / 1001.0,
-      60.0 }
-};
-
-static constexpr std::array<double, 16> valid_timecode_rates{
-    { 1.0,
-      12.0,
-      23.97,
-      23.976,
-      23.98,
-      24000.0 / 1001.0,
-      24.0,
-      25.0,
-      29.97,
-      30000.0 / 1001.0,
-      30.0,
-      48.0,
-      50.0,
-      59.94,
-      60000.0 / 1001.0,
-      60.0 }
+      60.0
+    }
 };
 
+// deprecated in favor of `is_smpte_timecode_rate`
 bool
 RationalTime::is_valid_timecode_rate(double fps)
 {
-    auto b = valid_timecode_rates.begin(), e = valid_timecode_rates.end();
+    return is_smpte_timecode_rate(fps);
+}
+
+bool
+RationalTime::is_smpte_timecode_rate(double fps)
+{
+    auto b = smpte_timecode_rates.begin(), e = smpte_timecode_rates.end();
     return std::find(b, e, fps) != e;
 }
 
+// deprecated in favor of `is_smpte_timecode_rate`
 double
 RationalTime::nearest_valid_timecode_rate(double rate)
+{
+    return nearest_smpte_timecode_rate(rate);
+}
+
+double
+RationalTime::nearest_smpte_timecode_rate(double rate)
 {
     double nearest_rate = 0;
     double min_diff     = std::numeric_limits<double>::max();
-    for (auto valid_rate: smpte_timecode_rates)
+    for (auto smpte_rate: smpte_timecode_rates)
     {
-        if (valid_rate == rate)
+        if (smpte_rate == rate)
         {
             return rate;
         }
-        auto diff = std::abs(rate - valid_rate);
+        auto diff = std::abs(rate - smpte_rate);
         if (diff >= min_diff)
         {
             continue;
         }
         min_diff     = diff;
-        nearest_rate = valid_rate;
+        nearest_rate = smpte_rate;
     }
     return nearest_rate;
 }
@@ -200,7 +193,7 @@ RationalTime::from_timecode(
     double             rate,
     ErrorStatus*       error_status)
 {
-    if (!RationalTime::is_valid_timecode_rate(rate))
+    if (!RationalTime::is_smpte_timecode_rate(rate))
     {
         if (error_status)
         {
@@ -331,7 +324,7 @@ RationalTime::from_time_string(
     double             rate,
     ErrorStatus*       error_status)
 {
-    if (!RationalTime::is_valid_timecode_rate(rate))
+    if (!RationalTime::is_smpte_timecode_rate(rate))
     {
         set_error(
             time_string,
@@ -460,7 +453,12 @@ RationalTime::to_timecode(
         return std::string();
     }
 
-    if (!is_valid_timecode_rate(rate))
+    // It is common practice to use truncated or rounded values
+    // like 29.97 instead of exact SMPTE rates like 30000/1001
+    // so as a convenience we will snap the rate to the nearest
+    // SMPTE rate if it is close enough.
+    double nearest_smpte_rate = nearest_smpte_timecode_rate(rate);
+    if (abs(nearest_smpte_rate - rate) > 0.1)
     {
         if (error_status)
         {
@@ -469,6 +467,9 @@ RationalTime::to_timecode(
         return std::string();
     }
 
+    // Let's assume this is the rate instead of the given rate.
+    rate = nearest_smpte_rate;
+
     bool rate_is_dropframe = is_dropframe_rate(rate);
     if (drop_frame == IsDropFrameRate::ForceYes and not rate_is_dropframe)
     {
@@ -504,11 +505,11 @@ RationalTime::to_timecode(
     }
     else
     {
-        if ((rate == 29.97) or (rate == 30000 / 1001.0))
+        if (rate == 30000 / 1001.0)
         {
             dropframes = 2;
         }
-        else if (rate == 59.94)
+        else if (rate == 60000 / 1001.0)
         {
             dropframes = 4;
         }
@@ -582,7 +583,7 @@ RationalTime::to_nearest_timecode(
     {
         *error_status = ErrorStatus();
 
-        double nearest_rate = nearest_valid_timecode_rate(rate);
+        double nearest_rate = nearest_smpte_timecode_rate(rate);
 
         return to_timecode(nearest_rate, drop_frame, error_status);
     }

src/opentime/rationalTime.h

@@ -171,13 +171,20 @@ class RationalTime
                                    start_time._rate };
     }
 
-    /// @brief Returns true if the rate is valid for use with timecode.
+    /// @brief Returns true is the rate is supported by SMPTE timecode.
+    [[deprecated("Use is_smpte_timecode_rate() instead")]]
     static bool is_valid_timecode_rate(double rate);
 
-    /// @brief Returns the first valid timecode rate that has the least
-    /// difference from rate.
+    /// @brief Returns true is the rate is supported by SMPTE timecode.
+    static bool is_smpte_timecode_rate(double rate);
+
+    /// @brief Returns the SMPTE timecode rate nearest to the given rate.
+    [[deprecated("Use nearest_smpte_timecode_rate() instead")]]
     static double nearest_valid_timecode_rate(double rate);
 
+    /// @brief Returns the SMPTE timecode rate nearest to the given rate.
+    static double nearest_smpte_timecode_rate(double rate);
+
     /// @brief Convert a frame number and rate into a time.
     static constexpr RationalTime
     from_frames(double frame, double rate) noexcept

src/py-opentimelineio/opentime-bindings/opentime_rationalTime.cpp

@@ -102,14 +102,22 @@ Compute the duration of samples from first to last (including last). This is not
 
 For example, the duration of a clip from frame 10 to frame 15 is 6 frames. Result will be in the rate of start_time.
 )docstring")
-        .def_static("is_valid_timecode_rate", &RationalTime::is_valid_timecode_rate, "rate"_a, "Returns true if the rate is valid for use with timecode.")
+        .def_static("is_valid_timecode_rate", &RationalTime::is_valid_timecode_rate, "rate"_a,
+            "Deprecated. Please use `is_smpte_timecode_rate` instead. This function will be removed in a future release.")
+        .def_static("is_smpte_timecode_rate", &RationalTime::is_smpte_timecode_rate, "rate"_a,
+            "Returns true if the rate is valid for use with SMPTE timecode.")
         .def_static("nearest_valid_timecode_rate", &RationalTime::nearest_valid_timecode_rate, "rate"_a,
-            "Returns the first valid timecode rate that has the least difference from the given value.")
-        .def_static("from_frames", &RationalTime::from_frames, "frame"_a, "rate"_a, "Turn a frame number and rate into a :class:`~RationalTime` object.")
+            "Deprecated. Please use `nearest_smpte_timecode_rate` instead. This function will be removed in a future release.")
+        .def_static("nearest_smpte_timecode_rate", &RationalTime::nearest_smpte_timecode_rate, "rate"_a,
+            "Returns the first SMPTE timecode rate that has the least difference from the given value.")
+        .def_static("from_frames", &RationalTime::from_frames, "frame"_a, "rate"_a,
+            "Turn a frame number and rate into a :class:`~RationalTime` object.")
         .def_static("from_seconds", static_cast<RationalTime (*)(double, double)> (&RationalTime::from_seconds), "seconds"_a, "rate"_a)
         .def_static("from_seconds", static_cast<RationalTime (*)(double)> (&RationalTime::from_seconds), "seconds"_a)
-        .def("to_frames", (int (RationalTime::*)() const) &RationalTime::to_frames, "Returns the frame number based on the current rate.")
-        .def("to_frames", (int (RationalTime::*)(double) const) &RationalTime::to_frames, "rate"_a, "Returns the frame number based on the given rate.")
+        .def("to_frames", (int (RationalTime::*)() const) &RationalTime::to_frames,
+            "Returns the frame number based on the current rate.")
+        .def("to_frames", (int (RationalTime::*)(double) const) &RationalTime::to_frames, "rate"_a,
+            "Returns the frame number based on the given rate.")
         .def("to_seconds", &RationalTime::to_seconds)
         .def("to_timecode", [](RationalTime rt, double rate, std::optional<bool> drop_frame) {
                 return rt.to_timecode(