Show the length that was asked for, at a rate that is not a whole number - #47
Merged
Merged
Conversation
A regression I introduced, caught by CI on an unrelated branch: a piece set to twenty seconds came back reading 00:00:19:29. The frame fix before this replaced the whole of the timecode, seconds included, with a frame count divided by the nominal rate. That is what non-drop timecode does and it is right for a rate somebody chose. It is wrong for this one. The rate here is measured off whatever clip was loaded, so it lands on 29.95 as readily as on 30, and at 29.95 exactly twenty seconds is 599 frames, which divided by thirty is nineteen. Reproduced before touching anything: fps 30 00:00:20:00 fps 29.97 00:00:19:29 fps 29.95 00:00:19:29 what CI saw fps 29.9 00:00:19:28 So the seconds are the seconds again. The frame count is still what the frames field is worked out from -- as the difference between this moment and the start of its second, in whole frames, where the subtraction that started all this cannot lose one. The 29.97 test that asked for slot counting is replaced. It was mine, and it pinned a broadcast convention this app does not follow: what somebody typing a length needs is to be shown the length they typed, at every rate. Three tests now say so -- twenty seconds and ninety seconds read back at nine different rates, the frames field never runs past the rate it is counted in, and the second ticks over exactly on the second. Checked both ways. Putting the fraction-first arithmetic back fails four tests, and putting this regression back fails two. 270 unit tests, 88 browser tests, build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ux1kydvUkLRoMbHp82ofDi
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
A regression I introduced in #39, caught by CI on an unrelated branch: a piece set to twenty seconds came back reading
00:00:19:29.What I got wrong
The frame fix replaced the whole of the timecode — seconds included — with a frame count divided by the nominal rate. That's what non-drop timecode does, and it's right for a rate somebody chose. It's wrong for this one.
The rate here is measured off whatever clip was loaded, so it lands on 29.95 as readily as on 30. At 29.95, exactly twenty seconds is 599 frames, and 599 ÷ 30 is nineteen.
Reproduced before touching anything:
00:00:20:0000:00:19:2900:00:19:29— what CI saw00:00:19:28The fix
The seconds are the seconds again. The frame count is still what the frames field is worked out from — as the difference between this moment and the start of its second, in whole frames, where the subtraction that caused the original bug can't lose one.
So both hold now:
timecode(2.3, 30)is still00:00:02:09, andtimecode(20, 29.95)is00:00:20:00.The test I had to replace
The 29.97 slot-counting test was mine, from #39, and it pinned a broadcast convention this app doesn't follow. What somebody typing a length needs is to be shown the length they typed, at every rate. Three tests say so now:
Checked both ways
270 unit tests, 88 browser tests, build clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Ux1kydvUkLRoMbHp82ofDi
Generated by Claude Code