Fix parsing of time_strings lacking leading zeroes (#1297)

meshula · web-flow · commit 78941ddf12a1 · 2022-09-14T18:12:57.000-07:00
Fixes #1293 fixes parsing of time strings without leading zeroes. enforces that a negative sign can only appear in the left most position implementation does not allocate memory or copy strings implementation does not allow exponential notation and other things that std does allow but are inappropriate for time strings compatible with strings produced by ffprobe associated tests adds C based tests corresponding to the existing Python based tests.
diff --git a/src/opentime/rationalTime.cpp b/src/opentime/rationalTime.cpp
@@ -94,6 +94,103 @@ is_dropframe_rate(double rate)
     return std::find(b, e, rate) != e;
 }
 
+static bool
+parseFloat(char const* pCurr, char const* pEnd, bool allow_negative, double* result)
+{
+    if (pCurr >= pEnd || !pCurr)
+    {
+        *result = 0.0;
+        return false;
+    }
+
+    double ret = 0.0;
+    double sign = 1.0;
+
+    if (*pCurr == '+')
+    {
+        ++pCurr;
+    }
+    else if (*pCurr == '-') 
+    {
+        if (!allow_negative) 
+        {
+            *result = 0.0;
+            return false;
+        }
+        sign = -1.0;
+        ++pCurr;
+    }
+
+    // get integer part
+    //
+    // Note that uint64_t is used because overflow is well defined for
+    // unsigned integers, but it is undefined behavior for signed integers,
+    // and floating point values are couched in the specification with
+    // the caveat that an implementation may be IEEE-754 compliant, or only
+    // partially compliant.
+    //
+    uint64_t uintPart = 0;
+    while (pCurr < pEnd) 
+    {
+        char c = *pCurr;
+        if (c < '0' || c > '9')
+        {
+            break;
+        }
+        uint64_t accumulated = uintPart * 10 + c - '0';
+        if (accumulated < uintPart)
+        {
+            // if there are too many digits, resulting in an overflow, fail
+            *result = 0.0;
+            return false;
+        }
+        uintPart = accumulated;
+        ++pCurr;
+    }
+
+    ret = static_cast<double>(uintPart);
+    if (uintPart != static_cast<uint64_t>(ret))
+    {
+        // if the double cannot be casted precisely back to uint64_t, fail
+        // A double has 15 digits of precision, but a uint64_t can encode more.
+        *result = 0.0;
+        return false;
+    }
+
+    // check for end of string or delimiter
+    if (pCurr == pEnd || *pCurr == '\0') 
+    {
+        *result = sign * ret;
+        return true;
+    }
+
+    // if the next character is not a decimal point, the string is malformed.
+    if (*pCurr != '.') 
+    {
+        *result = 0.0; // zero consistent with earlier error condition
+        return false;
+    }
+
+    ++pCurr; // skip decimal
+
+    double position_scale = 0.1;
+    while (pCurr < pEnd) 
+    {
+        char c = *pCurr;
+        if (c < '0' || c > '9')
+        {
+            break;
+        }
+        ret = ret + static_cast<double>(c - '0') * position_scale;
+        ++pCurr;
+        position_scale *= 0.1;
+    }
+
+    *result = sign * ret;
+    return true;
+}
+
+
 RationalTime
 RationalTime::from_timecode(
     std::string const& timecode, double rate, ErrorStatus* error_status)
@@ -205,55 +302,109 @@ RationalTime::from_timecode(
     return RationalTime{ double(value), rate };
 }
 
+static void
+set_error(std::string const& time_string,
+          ErrorStatus::Outcome code,
+          ErrorStatus* err)
+{
+    if (err) {
+        *err = ErrorStatus(
+            code,
+            string_printf(
+                "Error: '%s' - %s",
+                time_string.c_str(),
+                ErrorStatus::outcome_to_string(code).c_str()));
+    }
+}
+
 RationalTime
 RationalTime::from_time_string(
     std::string const& time_string, double rate, ErrorStatus* error_status)
 {
-    if (!RationalTime::is_valid_timecode_rate(rate))
+    if (!RationalTime::is_valid_timecode_rate(rate)) 
     {
-        if (error_status)
-        {
-            *error_status = ErrorStatus(ErrorStatus::INVALID_TIMECODE_RATE);
-        }
+        set_error(time_string, ErrorStatus::INVALID_TIMECODE_RATE, error_status);
         return RationalTime::_invalid_time;
     }
 
-    std::vector<std::string> fields(3, std::string());
-
-    // split the fields
-    int last_pos = 0;
-
-    for (int i = 0; i < 2; i++)
+    const char* start = time_string.data();
+    const char* end = start + time_string.length();
+    char* current = const_cast<char*>(end);
+    char* parse_end = current;
+    char* prev_parse_end = current;
+
+    double power[3] = {
+        1.0,   // seconds
+        60.0,  // minutes
+        3600.0 // hours
+    };
+
+    double accumulator = 0.0;
+    int radix = 0;
+    while (start <= current) 
     {
-        fields[i] = time_string.substr(last_pos, 2);
-        last_pos  = last_pos + 3;
-    }
-
-    fields[2] = time_string.substr(last_pos, time_string.length());
-
-    double hours, minutes, seconds;
+        if (*current == ':') 
+        {
+            parse_end = current + 1;
+            char c = *parse_end;
+            if (c != '\0' && c != ':') 
+            {
+                if (c< '0' || c > '9') 
+                {
+                    set_error(time_string, ErrorStatus::INVALID_TIME_STRING, error_status);
+                    return RationalTime::_invalid_time;
+                }
+                double val = 0.0;
+                if (!parseFloat(parse_end, prev_parse_end + 1, false, &val))
+                {
+                    set_error(time_string, ErrorStatus::INVALID_TIME_STRING, error_status);
+                    return RationalTime::_invalid_time;
+                }
+                prev_parse_end = nullptr;
+                if (radix < 2 && val >= 60.0) 
+                {
+                    set_error(time_string, ErrorStatus::INVALID_TIME_STRING, error_status);
+                    return RationalTime::_invalid_time;
+                }
+                accumulator += val * power[radix];
+            }
+            ++radix;
+            if (radix == sizeof(power) / sizeof(power[0])) 
+            {
+                set_error(time_string, ErrorStatus::INVALID_TIME_STRING, error_status);
+                return RationalTime::_invalid_time;
+            }
+        }
+        else if (current < prev_parse_end &&
+                 (*current < '0' || *current > '9') &&
+                 *current != '.') 
+        {
+            set_error(time_string, ErrorStatus::INVALID_TIME_STRING, error_status);
+            return RationalTime::_invalid_time;
+        }
 
-    try
-    {
-        hours   = std::stod(fields[0]);
-        minutes = std::stod(fields[1]);
-        seconds = std::stod(fields[2]);
-    }
-    catch (std::exception const& e)
-    {
-        if (error_status)
+        if (start == current) 
         {
-            *error_status = ErrorStatus(
-                ErrorStatus::INVALID_TIME_STRING,
-                string_printf(
-                    "Input time string '%s' is an invalid time string",
-                    time_string.c_str()));
+            if (prev_parse_end) 
+            {
+                double val = 0.0;
+                if (!parseFloat(start, prev_parse_end + 1, true, &val))
+                {
+                    set_error(time_string, ErrorStatus::INVALID_TIME_STRING, error_status);
+                    return RationalTime::_invalid_time;
+                 }
+                accumulator += val * power[radix];
+            }
+            break;
+        }
+        --current;
+        if (!prev_parse_end) 
+        {
+            prev_parse_end = current;
         }
-        return RationalTime::_invalid_time;
     }
 
-    return from_seconds(seconds + minutes * 60 + hours * 60 * 60)
-        .rescaled_to(rate);
+    return from_seconds(accumulator).rescaled_to(rate);
 }
 
 std::string
diff --git a/src/opentime/rationalTime.h b/src/opentime/rationalTime.h
@@ -127,6 +127,11 @@ class RationalTime
         std::string const& timecode,
         double             rate,
         ErrorStatus*       error_status = nullptr);
+
+    // parse a string in the form
+    // hours:minutes:seconds
+    // which may have a leading negative sign. seconds may have up to
+    // microsecond precision.
     static RationalTime from_time_string(
         std::string const& time_string,
         double             rate,
@@ -154,9 +159,13 @@ class RationalTime
         return to_timecode(_rate, IsDropFrameRate::InferFromRate, error_status);
     }
 
+    // produce a string in the form
+    // hours:minutes:seconds
+    // which may have a leading negative sign. seconds may have up to
+    // microsecond precision.
     std::string to_time_string() const;
 
-   RationalTime const& operator+=(RationalTime other) noexcept
+    RationalTime const& operator+=(RationalTime other) noexcept
     {
         if (_rate < other._rate)
         {
@@ -170,7 +179,7 @@ class RationalTime
         return *this;
     }
 
-   RationalTime const& operator-=(RationalTime other) noexcept
+    RationalTime const& operator-=(RationalTime other) noexcept
     {
         if (_rate < other._rate)
         {
diff --git a/tests/test_opentime.cpp b/tests/test_opentime.cpp
@@ -38,6 +38,75 @@ main(int argc, char** argv)
         assertFalse(t1 != t3);
     });
 
+    tests.add_test("test_from_time_string", [] {
+         std::string time_string = "0:12:04";
+         auto t = otime::RationalTime(24 * (12 * 60 + 4), 24);
+         auto time_obj = otime::RationalTime::from_time_string(time_string, 24);
+         assertTrue(t.almost_equal(time_obj, 0.001));
+     });
+
+    tests.add_test("test_from_time_string24", [] {
+        std::string time_string = "00:00:00.041667";
+        auto t = otime::RationalTime(1, 24);
+        auto time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "00:00:01";
+        t = otime::RationalTime(24, 24);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "00:01:00";
+        t = otime::RationalTime(60 * 24, 24);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "01:00:00";
+        t = otime::RationalTime(60 * 60 * 24, 24);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "24:00:00";
+        t = otime::RationalTime(24 * 60 * 60 * 24, 24);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "23:59:59.92";
+        t = otime::RationalTime((23 * 60 * 60 + 59 * 60 + 59.92) * 24, 24);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+    });
+
+    tests.add_test("test_from_time_string25", [] {
+        std::string time_string = "0:12:04.929792";
+        auto t = otime::RationalTime((12 * 60 + 4.929792) * 25, 25);
+        auto time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "00:00:01";
+        t = otime::RationalTime(25, 25);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "0:1";
+        t = otime::RationalTime(25, 25);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "1";
+        t = otime::RationalTime(25, 25);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "00:01:00";
+        t = otime::RationalTime(60 * 25, 25);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "01:00:00";
+        t = otime::RationalTime(60 * 60 * 25, 25);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "24:00:00";
+        t = otime::RationalTime(24 * 60 * 60 * 25, 25);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+        time_string = "23:59:59.92";
+        t = otime::RationalTime((23 * 60 * 60 + 59 * 60 + 59.92) * 25, 25);
+        time_obj = otime::RationalTime::from_time_string(time_string, 24);
+        assertTrue(t.almost_equal(time_obj, 0.001));
+    });
+
     tests.run(argc, argv);
     return 0;
 }
diff --git a/tests/test_opentime.py b/tests/test_opentime.py
@@ -17,6 +17,11 @@ def test_create(self):
         self.assertIsNotNone(t)
         self.assertEqual(t.value, t_val)
 
+        t_val = -30.2
+        t = otio.opentime.RationalTime(t_val)
+        self.assertIsNotNone(t)
+        self.assertEqual(t.value, t_val)
+
         t = otio.opentime.RationalTime()
         self.assertEqual(t.value, 0)
         self.assertEqual(t.rate, 1.0)
@@ -73,7 +78,6 @@ def test_deepcopy(self):
         self.assertEqual(t2, otio.opentime.RationalTime(18, 24))
 
     def test_base_conversion(self):
-
         # from a number
         t = otio.opentime.RationalTime(10, 24)
         with self.assertRaises(TypeError):
@@ -93,6 +97,14 @@ def test_time_timecode_convert(self):
         t = otio.opentime.from_timecode(timecode, 24)
         self.assertEqual(timecode, otio.opentime.to_timecode(t))
 
+    def test_negative_timecode(self):
+        with self.assertRaises(ValueError):
+            otio.opentime.from_timecode('-01:00:13:13', 24)
+
+    def test_bogus_timecode(self):
+        with self.assertRaises(ValueError):
+            otio.opentime.from_timecode('pink elephants', 13)
+
     def test_time_timecode_convert_bad_rate(self):
         with self.assertRaises(ValueError) as exception_manager:
             otio.opentime.from_timecode('01:00:13:24', 24)

Original file line number	Diff line number	Diff line change
`@@ -127,6 +127,11 @@ class RationalTime`
`127`	`127`	`std::string const& timecode,`
`128`	`128`	`double rate,`
`129`	`129`	`ErrorStatus* error_status = nullptr);`
	`130`	`+`
	`131`	`+ // parse a string in the form`
	`132`	`+ // hours:minutes:seconds`
	`133`	`+ // which may have a leading negative sign. seconds may have up to`
	`134`	`+ // microsecond precision.`
`130`	`135`	`static RationalTime from_time_string(`
`131`	`136`	`std::string const& time_string,`
`132`	`137`	`double rate,`
`@@ -154,9 +159,13 @@ class RationalTime`
`154`	`159`	`return to_timecode(_rate, IsDropFrameRate::InferFromRate, error_status);`
`155`	`160`	`}`
`156`	`161`
	`162`	`+ // produce a string in the form`
	`163`	`+ // hours:minutes:seconds`
	`164`	`+ // which may have a leading negative sign. seconds may have up to`
	`165`	`+ // microsecond precision.`
`157`	`166`	`std::string to_time_string() const;`
`158`	`167`
`159`		`- RationalTime const& operator+=(RationalTime other) noexcept`
	`168`	`+ RationalTime const& operator+=(RationalTime other) noexcept`
`160`	`169`	`{`
`161`	`170`	`if (_rate < other._rate)`
`162`	`171`	`{`
`@@ -170,7 +179,7 @@ class RationalTime`
`170`	`179`	`return *this;`
`171`	`180`	`}`
`172`	`181`
`173`		`- RationalTime const& operator-=(RationalTime other) noexcept`
	`182`	`+ RationalTime const& operator-=(RationalTime other) noexcept`
`174`	`183`	`{`
`175`	`184`	`if (_rate < other._rate)`
`176`	`185`	`{`