Draft: Fix a time string parsing bug

[jchen9]

With ffprobe version

ffprobe version 3.4.1-tessus Copyright (c) 2007-2017 the FFmpeg developers built with Apple LLVM version 8.0.0 (clang-800.0.42.1)

It prints media duration string like 0:12:04.829792. Its hour field has only one digit. If you put the string through from_time_string() to to_time_string(), you would get a different string out at the end.

Python 3.10.4 | packaged by conda-forge | (main, Mar 24 2022, 17:45:10) [Clang 12.0.1 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import opentimelineio as otio
>>> s = "0:12:04.829792"
>>> t = otio.opentime.from_time_string(s, 24.0)
>>> t
otio.opentime.RationalTime(value=2995.92, rate=24)
>>> otio.opentime.to_timecode(t, 24.0)
'00:02:04:19'
>>> otio.opentime.to_time_string(t)
'00:02:04.829792'
>>> 

The fix should be detect the colon separator and determine the field string length accordingly.

[meshula]

Could I ask, in addition to the python test, could you throw this into tests/test_opentime.cpp so we get C++ coverage as well by running make test

    tests.add_test("test to_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));
    });

[meshula]

A thing I did for the older PR was to be robust against a varying number of colons. ie. being able to parse 1:23 to mean one second and 23 frames, or 1:2:3 to mean 1 minute, 2 seconds, 3 frames.

the bit in the middle of the parse was simple and looked like

    std::vector<int> fields;
    int days = 0;
    int hours = 0;
    int minutes = 0;
    int seconds = 0;
    int frames = 0;

    std::string residual = timecode;

    try {
        while (residual.length() > 0) {
            size_t colon_pos = residual.rfind(':');
            std::string part = residual.substr(colon_pos + 1);
            fields.push_back(std::stoi(part));
            residual = residual.substr(0, colon_pos);
            if (colon_pos == std::string::npos)
                break;
        }
        
        auto it = fields.begin();
        if (it != fields.end()) frames = *it++;
        if (it != fields.end()) seconds = *it++;
        if (it != fields.end()) minutes = *it++;
        if (it != fields.end()) hours = *it++;
        if (it != fields.end()) days = *it;
    }
    catch(const std::exception& e) {
        *error_status = ErrorStatus(ErrorStatus::INVALID_TIMECODE_STRING,
                                    string_printf("Input timecode '%s' is an invalid timecode, exception: %s",
                                                  timecode.c_str(), e.what()));
        return RationalTime::_invalid_time;
    }

with the rest of the calculation following in the obvious manner. Maybe we could do that here as well.

it also validated that the frames field was legal:

    const int nominal_fps = static_cast<int>(std::ceil(rate));
    if (frames >= nominal_fps) {
        *error_status = ErrorStatus(ErrorStatus::TIMECODE_RATE_MISMATCH,
                                    string_printf("Frame rate mismatch.  Timecode '%s' has "
                                                  "frames beyond %f", timecode.c_str(),
                                                  nominal_fps - 1));
        return RationalTime::_invalid_time;
    }

[jchen9]

FWIW, here's how ffprobe constructs the time string: https://github.com/FFmpeg/FFmpeg/blob/83e1a1de8833845224948e5d002355c03dd117d5/fftools/ffprobe.c#L412

[meshula]

Ah, good reference, the ffprobe time string is h:m:seconds with no frames.

[codecov-commenter]

Codecov Report

Merging #1293 (8bcbbc0) into main (fb0b86d) will decrease coverage by 0.02%. The diff coverage is 68.42%.

@@            Coverage Diff             @@
##             main    #1293      +/-   ##
==========================================
- Coverage   86.14%   86.12%   -0.03%     
==========================================
  Files         196      196              
  Lines       19813    19831      +18     
  Branches     2314     2314              
==========================================
+ Hits        17068    17079      +11     
- Misses       2181     2188       +7     
  Partials      564      564              

Flag	Coverage Δ
py-unittests	86.12% <68.42%> (-0.03%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/opentime/rationalTime.cpp	82.60% <60.00%> (-3.00%)	:arrow_down:
tests/test_opentime.py	99.73% <100.00%> (+<0.01%)	:arrow_up:
src/opentime/errorStatus.h	87.50% <0.00%> (-12.50%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fb0b86d...8bcbbc0. Read the comment docs.

[jchen9]

Closing this in favor of @meshula 's change in https://github.com/AcademySoftwareFoundation/OpenTimelineIO/pull/1297