Store framerate internally as Fraction, expose via property#75
Open
nickswalker wants to merge 2 commits into
Open
Store framerate internally as Fraction, expose via property#75nickswalker wants to merge 2 commits into
Fraction, expose via property#75nickswalker wants to merge 2 commits into
Conversation
Previously the framerate setter normalized all inputs to a rounded string (e.g. Fraction(24000, 1001) → "23.98"), so tc.framerate always returned str. The setter now stores a canonical Fraction and the property returns it directly. NTSC rates are canonicalized to their exact fraction (24000/1001, 30000/1001, etc.) regardless of how they were specified. The "frames" alias (1 fps) and "ms" alias (1000 fps) are still accepted as input. As a consequence, frames_to_tc's drop-frame period calculations are switched from float-based rounding to exact integer arithmetic using _int_framerate, which is more correct and avoids precision sensitivity to the exact float representation of the framerate. Co-authored-by: cubicibo <55701024+cubicibo@users.noreply.github.com>
nickswalker
force-pushed
the
fraction-framerate
branch
from
198d555 to
97331b2
Compare
Converting to Fraction is about 2x slower than the original code path. Loading from the cache is modestly faster, and should handle the vast majority of usage
nickswalker
force-pushed
the
fraction-framerate
branch
from
97331b2 to
688e9d4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changeset makes
tc.frameratereturn aFractioninstead of astr. The string representation was the source of the problem fixed with #74 (becausefloat("47.95") != float(48000/1001). WithFractionstorage that class of bug disappears. This was the core of @cubicibo's changes in #69All existing input forms still work. NTSC string aliases (
"23.976","23.98","29.97", etc.) canonicalize to the same exact fraction regardless of how they were specified.Breaking change
Any code comparing
tc.framerateto a string will break:float(tc.framerate)andtc.framerate == other.frameratecontinue to work.Performance
Fraction("29.97")costs ~1.5 µs andfloat(Fraction(...))another ~1.6 µs, enough to double construction time for common string inputs. A module-level lookup table of pre-computedFractionobjects for all standard rates eliminates this:masterTimecode("24", ...)Timecode("29.97", ...)Timecode(Fraction(30000,1001), ...)Times medians of 20k runs, Apple M series chip