fix(lerobot): stop flooring a fractional source fps - #461
Conversation
kstonekuan
left a comment
There was a problem hiding this comment.
Both halves land and both are held, which is the thing I most wanted to see here. They fix different problems: dropping the truncation fixes new conversions, the version bump is what stops resume handing back the episodes already written with a stretched time axis. A PR that did only the first would have looked complete and left every existing delivery broken.
The mutation sweep says neither can be quietly undone:
truncation restored at the call site FAILED test_fractional_fps_reaches_conversion_untruncated
FAILED test_fractional_fps_reaches_the_transcoder_for_the_keyframe_interval
FAILED test_fractional_fps_sets_the_log_times_from_the_declared_rate
version bump reverted, fix kept FAILED test_converter_version_bumped_with_the_label_support
FAILED test_reuse_refuses_an_episode_written_before_the_fps_fix
log time truncated at use instead FAILED test_fractional_fps_sets_the_log_times_from_the_declared_rate
The middle one is the one that matters. Reverting v8 to v7 while keeping the fix turns two tests red, so the bump is pinned behavior rather than a number someone can tidy away next time they read the constant. The third confirms the rate reaches the division rather than being truncated one layer down.
Validated: full gate clean, 1636 passed / 6 skipped.
Two things done well beyond the ask. test_fractional_fps_reaches_the_transcoder_for_the_keyframe_interval asserts 30 and then asserts it differs from the floored 29, so it fails if the two ever coincide rather than passing on a number that happens to be right. And the CONVERTER_VERSION comment explains the bump as a resume-invalidation rather than as a byte change, which is the distinction I raised on the issue: integer-fps corpora produce identical output either way, so a reader comparing content ids would otherwise conclude the bump was unnecessary.
Merging.
Worth noting for #376, which you touched in a test docstring: this closes one specific way gop_seconds could already be false in provenance. It does not close the general case, which is still that the field is stamped as "actually used" and never verified.
Fixes #458
meta/info.jsonis allowed a fractional fps and_parse_dataset_informationstores it asone, but the single call site floored it with
int(source_archive.fps). A corpusdeclaring 29.97 was converted as if it were 29.
The keyframe interval is the primary half.
_transcode_mp4_to_h264computesround(gop_seconds * frames_per_second), which is 30 at the declared rate and 29 at thefloored one, so the file carried a 29 frame GOP while
provenance/v1stampedgop_secondsas1. That field is recorded as what was actually used and nothing checksit, which is what #376 is open about. This was one of the ways it could already be false.
The time axis is the visible half. Log times are
frame_index * 1e9 / frames_per_second,so the whole episode stretches:
Both consumers already took a float, so the fix is the
int()at the call site and theannotation that forced it.
CONVERTER_VERSIONmoves to v8, and the reason is resume rather than bytes._episode_identity_matchescompares dataset, revision, episode index, camera keys,converter version and gop_seconds. It never looks at fps, so an episode delivered before
this fix with the stretched time axis still matches on every field and the resume path
would hand it back as completed work. The bump is what makes those episodes fail identity
and be re-converted. This differs from the bumps in #390 and #403: those moved every
episode's content id, this one changes bytes only for fractional-fps sources, and an
integer-fps corpus produces identical output either way.
Tests, in the order they were asked for:
All four fail on the commit before the fix. The corpus helper now takes an fps so the
end-to-end import runs at 29.97 rather than the integer 30 every existing fixture uses,
which is why this survived: nine fixtures, all at 30, and
int(30.0)is 30.tests/test_lerobot_converter.py tests/test_lerobot_metadata_refusals.py tests/test_lerobot_export.py: 107 passed.ruff check,ruff format --checkandty checkare clean.