New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve handling of rates when converting to timecode strings #1477
base: main
Are you sure you want to change the base?
Conversation
To summarize, MC and OTIO |
I would like to suggest like what I proposed here: #1452 (comment) RationalTime
RationalTime::from_timecode(std::string const& timecode,
uint32_t timecode_fps,
double rate,
ErrorStatus* error_status) This would eliminate the need to do all the guessing from the playback frame rate. |
I tried that online tc calculator. That rate should really be 59.94, not 59.97, but oh well. |
MC and OTIO are off by 30 frames from each other. |
Here's the Media Composer features I used to check against: |
@markreidvfx can you explain more about using 24 frame timecode with 30 fps playback? That's not a scenario I've seen, so I'm confused about how it works. With your proposed change, would these produce the same answer?
and the other direction:
|
No, those would produce different answers. maybe using 23.97 is a bit more clear.
Timecode is converted to a frame number first. Maybe adding another arg isn't the correct answer. The big thing I'm trying to get across is that timecode should be treated as frame number encoding and is not always reliable to be a timestamp. It should really be called framecode in my opinion. :p This is why I normally use a pattern like this
and the other direction:
|
Aha! That explanation makes total sense now. The conversion to a frame count vs a timestamp really makes it clear - thanks @markreidvfx ! I'll check my MC project settings tomorrow. I'd love to compare this to Resolve and/or Premiere as well. Are there other trusted timecode conversion tools or software libraries out there to compare to? |
This PR overlaps or conflicts with this older PR: #1180 |
Thanks for doing the extra checks! So I think the theory is that MC is displaying time code at 30, even for a selected rate of 60 (as opposed to MC displaying a value that is 30 frames off of our calculation). A check for that would be to knock off 5 frames at 30, and check if MC then shows 24 instead of 29, or if it shows 19. 24 would demonstrate it is displaying a 30 rate, instead of 60; whereas 19 would show that the calculation is off by 30. |
I'm quite sure @jminor had his avid set in 30 TC mode for 60. Its the default setting. The timecode ticks every 2 frames and the top dot of the last colon blinks on and off to tell you if your on an even or odd frame. timecode.mp4This mode is somewhat described in the spec intro, page 5
There are some further details in 12.1 |
Sorry for the delay in getting back to this. I should hopefully have time to revisit this in a couple of weeks. In the meantime, I happened across this which is interesting to compare/contrast with OTIO's approach: https://github.com/orchetect/TimecodeKit |
Had a look at the link you provided, quite an interesting read ~ TimeCodeKit's major difference, I'd say, is that it introduces a strongly typed TimeCode object from which a string can be generated, and which functions as a mathematical object. It hasn't got a stronger ability than otio::RationalTime to represent a point in time, or a stronger ability to do math than otio::RationalTime, and would probably resist computations we are interested in, such as linear timewarps. Another point of contrast, philosophically, is the view suggested by @markreidvfx earlier in the thread, that the timecode string should be considered, in a way, a "rendered" interpretation of a time, as opposed to a ground truth representation of time. TimeCodeKit does some interesting things that we don't support, at all. If you decrement time below zero, it will wrap around to 24 hours. We generate negative values. We could possibly do something similar in genarating time code strings. At the moment, negative time values are rejected. If the are negative and greater than -24 hours, we could wrap around. I think it would be worth working through the math of TimeCodeKit to determine whether our conversion algorithm matches. Also, it would be worth working through the unit tests, and perhaps adding one to one corresponding unit tests to our own code. |
I ran the tests in my own repo, the python unit tests are what failed, the tests themselves may need correction.
|
Yes, @meshula that failing test is the one I mentioned in the initial post at the very top of this thread. My question is/was: what is the correct return value for |
6114436
to
2b290f5
Compare
referencing @markreidvfx's reported values from Resolve and Unreal 1084319 frames = 05:01:11:59 @ 59.94 fps without drop frame suggest that answer should be 05:01:11:59 I've gone through this thread a bunch of times in the last hour, and I feel we can be confident about that. |
Re-reading the thread, it looks like I need to do these things:
For the method rename, do we need to deprecate the old names & support both for a release? |
The use of the "valid_xx" functions have come up in quite a few discussions and issues. |
@apetrynet I'm not following the comment "The use of the "valid_xx" functions have come up in quite a few discussions and issues." are you saying that supports merging, or that there some other work? @jminor I concur with @apetrynet on the new/old+deprecation strategy, Thanks for looking at this, folks :D |
@meshula |
…ng entries in valid_timecode_rates table. Signed-off-by: Joshua Minor <[email protected]>
…014 - SMPTE Standard - Time and Control Code. Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
Okay, I did these three things:
Two questions:
|
Signed-off-by: Joshua Minor <[email protected]>
Hmm... that's odd. When I run the tests locally only the contrib ALE tests fail, but in the CI it fails earlier on |
Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
I fixed the ALE rate problem in that commit, but this test still fails:
Let me try one other experiment... |
Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
So I wondered if maybe a different exception was thrown? My experiment dc0051c shows that indeed no exception is thrown by I don't have an Ubuntu VM handy to try to repro this. I am using Python version (3.11) on my Mac. I'll see if I can tell the CI to run the Mac version of this to see if the failure is OS dependent. If that doesn't help, maybe I can add some print statements in the C++ side of this function? I think the unit test system eats that output? Any other debugging suggestions here are welcome. |
Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
…x of CI results Signed-off-by: Joshua Minor <[email protected]>
On the C++ side if the test framework eats the output, you can instead use an ofstream, and send your prints to say /var/tmp/test_log.txt or some such, and review the log after the fact? |
…ll matrix of CI results" This reverts commit 8a945f7. Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
Progress... I was able to temporarily turn off The test fails on Ubuntu and Win-Mingw, but succeeds on macOS and Windows. It does not depend on Python version. (See screenshot below) Let me make an Ubuntu VM to see if I can repro in there... |
@jminor At this point, it may come down to compiler settings, such as fast-math being enabled only on ubuntu. (fast-math, aka "non-reproducible sometimes broken math") should not be set at all for otio. |
This PR improves OTIO's
to_timecode
,is_valid_timecode_rate
, andnearest_valid_timecode_rate
functions so they handle rate values which are close, but not exactly matching the correct rates.Specifically the previous strategy of keeping a table of commonly mis-typed non-integer rates (e.g. 29.97, 29.976, 23.98, etc.) is replaced by a heuristic which matches rates to the closest correct rate within some tolerance.
Rates checked by
is_valid_timecode_rate
andnearest_valid_timecode_rate
now match each other, and adhere to ST 12-1:2014 - SMPTE Standard - Time and Control Code (which is now freely available 👏🎉)We also spot-checked some comparisons between Avid Media Composer and OTIO to make sure we got the drop frames in the right spot - that was already correct before this PR.
Note: there is 1 test failing which needs to be addressed, and my updates to the other tests should be scrutinized to make sure the changes are good.
For the failing test... something doesn't match up.
Test code: otio.opentime.from_frames(1084319, 60).to_timecode()