Fix subtask annotation on multi-episode-per-file (LeRobot v3) videos: clip-relative frame timestamps

[Binh Pham (pham-tuan-binh)]

Behavior

When a VideoFile is decoded with a clip window (from_timestamp_s /
to_timestamp_s), iter_encoded_frames returns timestamps relative to the
whole video file, not to the clip.

This is what test_iter_frames_respects_clip_bounds asserts today: a clip
starting at from_timestamp_s=0.2 yields frame timestamps [0.2, 0.4, 0.6]
instead of [0.0, 0.2, 0.4].

def test_iter_frames_respects_clip_bounds(tmp_path) -> None:
    path = tmp_path / "video.mp4"
    _write_video(path, num_frames=5, fps=5)
    video = mdr.video.VideoFile(
        DataFile.resolve(path),
        from_timestamp_s=0.2,
        to_timestamp_s=0.7,
    )

    frames = asyncio.run(_collect_frames(video))

    assert [frame.index for frame in frames] == [0, 1, 2]
    assert [frame.timestamp_s for frame in frames] == [0.2, 0.4, 0.6]

Impact

A LeRobot v3 dataset bundles multiple episodes into one video file, so each
episode is a clip with from_timestamp > 0. With the current behavior, decoding
an episode returns video-wide timestamps, not episode timestamps.

robotics.subtask_annotation relies on episode-relative time, so on v3 datasets
it produces segments on the video clock (which don't match the episode's
timestamp column) — dropping segments for every episode except the first one
in each video file.

Fix

Rebase yielded timestamps to the clip start in iter_encoded_frames (subtract
clip_from once). _frame_timestamp_s stays absolute. No-op for unclipped videos
(clip_from == 0), and brings VideoFile in line with the in-memory sources.

Updated test_iter_frames_respects_clip_bounds to expect [0.0, 0.2, 0.4].

Question

Is this behavior expected? That a segmented video would return absolute timestamp instead of relative timestamps?

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[gemini-code-assist]

Code Review

This pull request updates the video decoding logic to report frame timestamps relative to the clip start, adjusting the test assertions accordingly. The reviewer pointed out that while timestamp_s is rebased, pts remains absolute, which introduces an inconsistency. Additionally, they noted that a frame slightly before clip_from could yield a negative timestamp, and suggested clamping both timestamp_s and pts to be non-negative to prevent this.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Tomás Duarte (tomas-duarte)]

Hello Binh Pham (@pham-tuan-binh), thank you a lot for the bug report and the PR!

This looks good, it's a pretty straightforward change.
Besides the small comment I left on the diff, I'm just not 100% sure on subtracting clip_from instead of the first frame's time. As is, you can run into potentially weird timing issues if the first frame you get doesn't exactly match the clip timestamp; i.e. you might not get 0.0 on the first frame. I'm also thinking we should perhaps sync dts as well, otherwise stuff might break in unexpected ways?
On another note, rebasing our wrapper timestamp_s, then rebasing pts might be unnecessary, as _frame_timestamp_s is essentially how the frame.time property is computed anyway. Then again, this was already there.

Tomorrow, I will take a more thorough look at what to do, and will look at how libavfilter implements the trim and setpts filters we know from ffmpeg.
If we wanted, though, we could also merge this sooner and do an improvement pass on the video internals later, I guess.

[Tomás Duarte (tomas-duarte)]

I ended up tweaking and merging this with #230, as I could not push a new commit onto this PR branch, even though it's supposedly editable...
Anyway, thank you so much for the bug report and fix suggestion again! Let us know if you have any other issues :)