Skip to content

Fix to_systemtime sub-second precision for non-standard NTSC rates#74

Open
nickswalker wants to merge 1 commit into
eoyilmaz:mainfrom
nickswalker:fix-systemtime-subframe
Open

Fix to_systemtime sub-second precision for non-standard NTSC rates#74
nickswalker wants to merge 1 commit into
eoyilmaz:mainfrom
nickswalker:fix-systemtime-subframe

Conversation

@nickswalker

Copy link
Copy Markdown

to_systemtime computes the sub-second portion of the timestamp by dividing the frame count within the current second by the framerate:

hh, mm, ss, ff = self.frames_to_tc(self.frames + 1, skip_rollover=True)
framerate = float(self.framerate) if self._ntsc_framerate else self._int_framerate
ms = ff / framerate

For NTSC rates, float(self.framerate) is used, but self.framerate stores whatever string was passed in, or a float rounded to 2 decimal places. For a standard rate like 29.97 that's fine. For a non-standard NTSC multiple like 48000/1001 (≈47.952 fps), the setter stores "47.95", so the divisor is 47.95 instead of the correct integer framerate 48:

tc = Timecode(Fraction(48000, 1001), frames=24)
tc.to_systemtime()  # "00:00:00.501" — wrong, should be "00:00:00.500"
#                        ^^^
# 24 / 47.95 = 0.50052 → rounds to 501 ms
# 24 / 48    = 0.5     → rounds to 500 ms

All to_systemtime tests assert on whole-second boundaries, where ff = 0 and therefore ms = 0 / anything = 0, so the divisor never mattered. The rounding error for standard rates (29.97, 59.94, 119.88) is also small enough (1ms per frame) that it happens to survive the 3-decimal string format for the values tested.

System time is defined in the integer frame grid. The divisor should always be _int_framerate:

ms = ff / self._int_framerate

@cubicibo fixed this as part of #69. Parting it out for easier review.

System time is defined in the integer frame grid, so the sub-second
calculation should always divide by _int_framerate. Using
float(framerate) was imprecise for non-standard NTSC rates stored as
rounded strings (e.g. "47.95" for 48000/1001), producing wrong
millisecond values mid-second.

Co-authored-by: cubicibo <55701024+cubicibo@users.noreply.github.com>
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.

1 participant