Skip to content
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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

jminor
Copy link
Collaborator

@jminor jminor commented Nov 8, 2022

This PR improves OTIO's to_timecode, is_valid_timecode_rate, and nearest_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 and nearest_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.

image

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.

  • Avid Media Composer's Calculator says: 1084319 frames = 05:01:11:29 @ 60 fps
  • OpenTimelineIO says: 1084319 frames = 05:01:11:59 @ 60 fps
  • https://robwomack.com/timecode-calculator/ says: 1084319 frames = 05:01:30:03 @ 60 fps

Test code: otio.opentime.from_frames(1084319, 60).to_timecode()

@jminor jminor marked this pull request as ready for review November 8, 2022 22:43
@meshula
Copy link
Collaborator

meshula commented Nov 8, 2022

To summarize, MC and OTIO agree disagree, but time-code-calculator significantly differs agrees with OTIO. ? Maybe we need to ping Avid. the calculator's author...

@markreidvfx
Copy link
Contributor

I would like to suggest RationalTime::from_timecode explicitly take the nominal fps as a argument along with the frame rate

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. 
You might for example be using 24 frame timecode at playback rate of 30 fps, which isn't possible with the current method.

@markreidvfx
Copy link
Contributor

I tried that online tc calculator.
Its pretty finicky but you might have had it set to 59.97fps DF , I get
1084319 frames = 05:01:11:59 @ 60 fps.
1084319 frames = 05:01:11:59 @ 59.97 fps. Non Drop Frame (should be the same as 60)
and
1084319 frames = 05:01;30;03 @ 59.97 fps Drop Frame

That rate should really be 59.94, not 59.97, but oh well.
I haven't figure out how to get 59.94 DF TC working on my avid yet to verify if that calculation is correct.

@jminor
Copy link
Collaborator Author

jminor commented Nov 9, 2022

MC and OTIO are off by 30 frames from each other.

@markreidvfx
Copy link
Contributor

markreidvfx commented Nov 9, 2022

The TC display in MC is might be showing 30fps TC where it duplicates every step. My MC cannot display 60 TC without using a burn in.
image

To make things a little more odd, the 60fps TC burnin only seems to works when working at 59.94fps. I am still using 2018, maybe there is better support in a newer version.

@markreidvfx
Copy link
Contributor

ok, in MC general settings there is option to change the TC display format from 30fps to 60 fps
image

image

Still haven't found a 60fps drop frame option.

@jminor
Copy link
Collaborator Author

jminor commented Nov 9, 2022

Here's the Media Composer features I used to check against:

  • The "=" menu in the bottom left corner of the timeline window has "Show Track" which lets me turn on multiple timecode rulers along the timeline:
    image

  • The Timecode Window right-click menu lets me choose these:
    image

  • The Calculator Window lets you convert between "Total Frame Count" and various rates. This is where I got 1084319 frames = 05:01:11:29 @ 60 fps
    image

@jminor
Copy link
Collaborator Author

jminor commented Nov 9, 2022

@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?

a = otio.opentime.from_timecode(timecode_str, 24, 30)
b = otio.opentime.from_timecode(timecode_str, 24, 24).rescaled_to(30)

and the other direction:

a = otio.opentime.from_frames(frame_number, 30).rescaled_to(24).to_timecode()
b = otio.opentime.from_frames(frame_number, 30).to_timecode(rate=24)

@markreidvfx
Copy link
Contributor

No, those would produce different answers. maybe using 23.97 is a bit more clear.
It would be the equivalent of this

frame = otio.opentime.from_timecode(timecode_str, 24).value
a = otio.opentime.from_frames(frame, 23.97)
b = otio.opentime.from_timecode(timecode_str, 24).rescaled_to(23.97)

a.to_seconds() != b.to_seconds()

Timecode is converted to a frame number first.
Then that frame number is converted to a time at current frame rate.

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

frames_to_seconds(timecode_to_frames(timecode_str, timecode_fps), frame_rate)

and the other direction:

frames_to_timecode(seconds_to_frames(seconds, frame_rate), timecode_fps, drop=False)

@markreidvfx
Copy link
Contributor

what is the the project format/edit rate?
image

some formats can't access certain timecodes.

@jminor
Copy link
Collaborator Author

jminor commented Nov 10, 2022

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?

@jminor
Copy link
Collaborator Author

jminor commented Nov 10, 2022

This PR overlaps or conflicts with this older PR: #1180

@meshula
Copy link
Collaborator

meshula commented Nov 11, 2022

@jminor I closed that one in favor of this one, since this one rectifies the treatments of documented SMPTE rates with the code. @splidje NLEs have various "tricks" for dealing with high frame rates, let's carry on the discussion of what to do with the high frame rates here.

@markreidvfx
Copy link
Contributor

markreidvfx commented Nov 22, 2022

I've verified those calculations in Resolve, it actually supports 59.94 TC with drop frame.

1084319 frames = 05:01:11:59 @ 59.94 fps without drop frame
1084319 frames = 05:01;30;03 @ 59.94 fps with Drop Frame

You can set the tc format when you create a timeline or in your project settings
60fp_tc

You can toggle between frames and tc by right clicking on the timecode in the viewer
frame_to_tc

image

@markreidvfx
Copy link
Contributor

markreidvfx commented Nov 22, 2022

Resolve also additionally supports a 16, 18, 47.952 and 48 TC format.
The 47.952 appears to just be 48, like 23.976 is just 24

image

@meshula
Copy link
Collaborator

meshula commented Nov 22, 2022

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.

@markreidvfx
Copy link
Contributor

markreidvfx commented Nov 22, 2022

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.mp4

This mode is somewhat described in the spec intro, page 5

Progressive video systems with frame rates above 30 frames per second are described in this document, documenting what have become “de facto” implementations. Since the frame rate of these 50 and 60 frames-per-second progressive systems exceeds the frame count capacity of the time address, counting is done on frame pairs, which results in an edit resolution of two frames using traditional linear time code.

There are some further details in 12.1

@markreidvfx
Copy link
Contributor

markreidvfx commented Nov 22, 2022

I've also verified those timecode values in Unreal Engine

60 fps NDF
image

59.94 fps DF

image

@jminor
Copy link
Collaborator Author

jminor commented Nov 29, 2022

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

@meshula
Copy link
Collaborator

meshula commented Nov 30, 2022

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.

@meshula
Copy link
Collaborator

meshula commented Apr 26, 2023

I ran the tests in my own repo, the python unit tests are what failed, the tests themselves may need correction.

======================================================================
FAIL: test_invalid_rate_to_timecode_functions (test_opentime.TestTime.test_invalid_rate_to_timecode_functions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/OpenTimelineIO/OpenTimelineIO/tests/test_opentime.py", line 400, in test_invalid_rate_to_timecode_functions
    with self.assertRaises(ValueError):
AssertionError: ValueError not raised

======================================================================
FAIL: test_timecode_infer_drop_frame (test_opentime.TestTime.test_timecode_infer_drop_frame)
---------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/OpenTimelineIO/OpenTimelineIO/tests/test_opentime.py", line 347, in test_timecode_infer_drop_frame
    self.assertEqual(t.to_timecode(rate, drop_frame=None), timecode)
AssertionError: '05:01:30;03' != '05:01:11;59'
- 05:01:30;03
+ 05:01:11;59

@jminor
Copy link
Collaborator Author

jminor commented May 11, 2023

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 otio.opentime.from_frames(1084319, 60).to_timecode() ?

@jminor jminor force-pushed the fix_timecode branch 2 times, most recently from 6114436 to 2b290f5 Compare May 11, 2023 00:10
@meshula
Copy link
Collaborator

meshula commented May 11, 2023

otio.opentime.from_frames(1084319, 60).to_timecode() # no dropframe rate requested

referencing @markreidvfx's reported values from Resolve and Unreal

1084319 frames = 05:01:11:59 @ 59.94 fps without drop frame
1084319 frames = 05:01:30;03 @ 59.94 fps with 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.

@jminor jminor force-pushed the fix_timecode branch 2 times, most recently from 5a637fd to 59750d0 Compare April 24, 2024 07:14
@jminor
Copy link
Collaborator Author

jminor commented Apr 24, 2024

Okay, I did these three things:

  • Rebased on main
  • Updated the test to use the correct timecode value (note I edited the comment above to correct the corrected value)
  • Renamed both "valid_timecode" functions to "smpte_timecode" and deprecated the older functions.

Two questions:

  • Is there a better way to tell pybind11 that those older functions are deprecated?
  • Now there are failures in some adapter tests, where they should be using nearest_smpte_timecode_rate. These are probably legit issues that should be fixed. Should we fix them in this PR, or after the impending adapter split out?

@jminor
Copy link
Collaborator Author

jminor commented Apr 24, 2024

Hmm... that's odd. When I run the tests locally only the contrib ALE tests fail, but in the CI it fails earlier on test_invalid_rate_to_timecode_functions. I'll look further later in the week.

@meshula
Copy link
Collaborator

meshula commented Apr 25, 2024

@jminor Did 28a8f50 fix the issue

When I run the tests locally only the contrib ALE tests fail, but in the CI it fails earlier on test_invalid_rate_to_timecode_functions. I'll look further later in the week.

? The commit seems to be after the comment :)

@jminor
Copy link
Collaborator Author

jminor commented Apr 25, 2024

I fixed the ALE rate problem in that commit, but this test still fails:

FAIL: test_invalid_rate_to_timecode_functions (test_opentime.TestTime)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/OpenTimelineIO/OpenTimelineIO/tests/test_opentime.py", line 424, in test_invalid_rate_to_timecode_functions
    with self.assertRaises(ValueError):
AssertionError: ValueError not raised

Let me try one other experiment...

@jminor
Copy link
Collaborator Author

jminor commented Apr 25, 2024

So I wondered if maybe a different exception was thrown? My experiment dc0051c shows that indeed no exception is thrown by to_timecode when given a bogus rate. However, when I run this locally, ValueError is thrown as desired.

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.

@meshula
Copy link
Collaborator

meshula commented Apr 26, 2024

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?

@jminor
Copy link
Collaborator Author

jminor commented Apr 26, 2024

Progress... I was able to temporarily turn off fail-fast in the GitHub Action workflow to get the full matrix of CI jobs to run. Now we can see clearly which ones succeed and which fail.

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...

image

@meshula
Copy link
Collaborator

meshula commented Apr 26, 2024

@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.

jminor and others added 16 commits June 16, 2024 23:34
…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]>
Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
…ll matrix of CI results"

This reverts commit 8a945f7.
Signed-off-by: Joshua Minor <[email protected]>
Signed-off-by: Joshua Minor <[email protected]>
@meshula
Copy link
Collaborator

meshula commented Jun 18, 2024

@jminor This is looking good to me, aside from this: did you mean to add the ALE adaptor code to the commit?

@jminor
Copy link
Collaborator Author

jminor commented Jun 18, 2024

The ALE adapter fix was written before the adapters were moved, so those files are left in here just since I rebased this change. I can re-do those fixes in the new ALE adapter repo after this lands.

In the meantime, I was able to use the tmate GitHub Action to get a reverse shell on one of the runners in order to diagnose the Ubuntu-specific test failure. From poking at that in Python I can repro the issue, but I think I'll need to write a C++ test for to_timecode in order to find the true source of the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants