Skip to content

feat(rldb): h264 video-backed images + role-in-key annotations + span selection - #541

Open
ElmoPA wants to merge 1 commit into
elmo/ttt-layerfrom
rldb/video-and-annotations
Open

feat(rldb): h264 video-backed images + role-in-key annotations + span selection#541
ElmoPA wants to merge 1 commit into
elmo/ttt-layerfrom
rldb/video-and-annotations

Conversation

@ElmoPA

@ElmoPA ElmoPA commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Video image storage (video_codec.py, _common.decode_video_span, zarr_writer,
zarr_dataset_multi): store frames as one mp4 per group of frames_per_chunk
instead of per-frame JPEG. Measured on real fold episodes: 44.8 -> 20.2 KB/frame
and 8.33 -> 1.68 ms/frame decode, and h264 also beats JPEG at EQUAL bytes
(+2.4..2.9 dB PSNR at 4-8 KB), so it is strictly better coding rather than a
quality-for-size trade. crf15/gop30/yuv420p; decode speed is flat across CRF, so
compressing harder would buy only disk.

Layout: chunk-indexed, NOT one blob per episode -- a span read would otherwise
pull the whole episode (~68 MB) and we read ~13 spans/episode. Frame f lives in
chunk f//fpc at offset f%fpc; chunks start on a keyframe (keyint_min=g,
sc_threshold=0) so each decodes independently. Readers dispatch on
_features[key]["dtype"] ("jpeg" vs "h264"), so existing episodes are untouched
and both formats coexist. decode_video_span returns the same (T,3,H,W) float
array as decode_jpeg_window, so pack_collate/TargetBuilder need no change;
equivalence-gated against the JPEG path on the same episode (38.6-39.1 dB,
including a chunk-boundary span and the partial tail chunk) and stride parity
matches decode_jpeg_window_strided.

annotation_processing.py: port of the role-in-key-name scheme -- role lives in
the zarr key NAME (annotations_task/annotations_subtask), entries are plain
{text,start_idx,end_idx} spans. _load_annotations is now per-key (the previous
single-slot cache aliased across keys) and an absent key degrades to [] rather
than raising.

zarr_dataset_packed: annotation_key + span_indices, so a run can read a
structural segmentation key (fold_segments) and select individual spans. Note
fold_segments deliberately sits OUTSIDE the annotations* glob -- those spans
define training windows, not language, and must not reach the batch as a text
role.

fold_span_transforms: RH_WRIST_MODE=none -> 126-dim keypoints-only (the wrist
pose is redundant with the keypoints). ypr(138)/pos(132) unchanged; default
still pos.

ElmoPA commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude Code Review

Review of PR #541

Summary

Substantial feature PR adding h264-video-backed image storage (chunked mp4s), role-in-key annotation processing, and span-based selection to the rldb data path. The primary changes look well-thought-out (chunked layout with keyframe-aligned decoding, dispatch on _features[key]["dtype"], JPEG parity verified). However, the diff also bundles several unrelated changes that warrant separate scrutiny and raise correctness concerns.

Key concerns

1. Unrelated changes bundled into one PR

The description advertises h264 + annotations + span selection, but the diff also includes:

  • A wholesale rewrite of embodiment identifiers (aria_*/mecka_*/scale_*human_*), collapsing four embodiments into one.
  • Mandatory intrinsics in zarr.attrs, with ZarrWriter.create_and_write raising on absence.
  • Aria EE/wrist orientation "fix" (T_ROT_CAM change) that mutates the meaning of already-stored data.
  • A hard removal of the fallback in PI.control_mode — now raises instead of defaulting.
  • HPT text-prompt modality plumbing (Qwen stem).
  • A new eval_latent.py (~1000 lines).

Each of these is a training-affecting change on its own. Bundling them makes the PR very hard to bisect if training regressions appear, and violates the "don't break training" priority. Strongly recommend splitting into at least: (a) h264 + annotations + spans (the advertised change), (b) embodiment collapse + mandatory intrinsics, (c) Aria orientation fix, (d) HPT text prompts, (e) eval_latent.

2. Backward-compat break: embodiment collapse is a hard-crash migration

The README explicitly acknowledges that already-uploaded episodes with embodiment="aria_bimanual" will KeyError: 'ARIA_BIMANUAL' at load time with no alias fallback. This is a serious break:

  • All contributors must re-download.
  • Any in-flight training run resuming from checkpoints referencing old norm-stat keys (which are keyed by embodiment id) will silently mismatch — old id 5 (aria_bimanual) now means eva_left_arm under the new table. Please verify norm-stat and checkpoint compatibility explicitly, and add an aliasing shim in get_embodiment_id ("aria_bimanual" → "human_bimanual") at minimum for a deprecation window. A hard KeyError with no migration path is hostile to any collaborator with cached data or running experiments.

3. Aria EE/wrist orientation change silently invalidates cached episodes

Changing T_ROT_CAM for the left hand means every previously-processed Aria episode has wrong EE orientation under the new convention, but the zarr looks structurally identical (same shape, same dtype, same key). There is no version bump in zarr.attrs and no way for the loader to tell "pre-fix" from "post-fix" data. This is a data-integrity landmine. Suggest adding a processing_version or aria_orientation_convention field in zarr.attrs so downstream code can refuse to mix old and new.

4. PI.control_mode fallback removal

Removing the "aria" in emb_name fallback and raising on unknown embodiment is defensible, but combined with the embodiment rename in the same PR, any existing yaml with control_mode: {aria_bimanual: ...} now silently fails to match human_bimanual and raises. Please confirm all model yamls in hydra_configs/ are updated, and grep for any lab-side configs that could break.

5. h264 chunk layout — a couple things to verify

The chunked scheme (chunk = f // frames_per_chunk, keyframe at chunk start via keyint_min=g, sc_threshold=0) is sound in principle. Concerns:

  • Partial tail chunk: verified equivalence-gated per the description — good. But please confirm the tail chunk's frames_per_chunk metadata is written correctly, otherwise f % fpc at read time will index off-the-end.
  • Span crossing chunk boundary: does decode_video_span open both chunks and concatenate? The description mentions "chunk-boundary span" was tested — please confirm this is exercised by a unit test in CI, not just an ad-hoc measurement.
  • crf=15 is quite conservative; fine as a default, but please confirm the norm-stat computations were re-run on h264-decoded frames or verified pixel-equivalent to JPEG at training precision. A 2-3 dB PSNR advantage is not zero error.

6. _load_annotations per-key fix

The old single-slot cache aliasing bug across keys is a real correctness fix — good catch. Verify: are there any existing training runs whose annotation_key config relied (accidentally) on the aliased cache returning the wrong key's data? Unlikely, but worth grepping hydra_configs/ for anything reading multiple annotation keys.

7. fold_segments sitting outside annotations* glob

This is a subtle but correct design decision (structural segmentation ≠ language). Please add a comment/docstring in annotation_processing.py explaining why fold_segments is deliberately excluded, so a future contributor doesn't "fix" it by broadening the glob.

8. Mandatory intrinsics breaks existing writers

ZarrWriter.create_and_write now raises if intrinsics is missing or not a non-empty dict. This will break any lab's data-conversion script that hasn't been updated. Is there an inventory of which conversion scripts (aria_process/, mecka_process/, scale_process/, eva_process/) have been updated in this PR? The README says "Mecka and Scale will be asked to add intrinsics" — meaning this PR


Reviewed by Claude · Review workflow

… selection

Video image storage (video_codec.py, _common.decode_video_span, zarr_writer,
zarr_dataset_multi): store frames as one mp4 per group of frames_per_chunk
instead of per-frame JPEG. Measured on real fold episodes: 44.8 -> 20.2 KB/frame
and 8.33 -> 1.68 ms/frame decode, and h264 also beats JPEG at EQUAL bytes
(+2.4..2.9 dB PSNR at 4-8 KB), so it is strictly better coding rather than a
quality-for-size trade. crf15/gop30/yuv420p; decode speed is flat across CRF, so
compressing harder would buy only disk.

Layout: chunk-indexed, NOT one blob per episode -- a span read would otherwise
pull the whole episode (~68 MB) and we read ~13 spans/episode. Frame f lives in
chunk f//fpc at offset f%fpc; chunks start on a keyframe (keyint_min=g,
sc_threshold=0) so each decodes independently. Readers dispatch on
_features[key]["dtype"] ("jpeg" vs "h264"), so existing episodes are untouched
and both formats coexist. decode_video_span returns the same (T,3,H,W) float
array as decode_jpeg_window, so pack_collate/TargetBuilder need no change;
equivalence-gated against the JPEG path on the same episode (38.6-39.1 dB,
including a chunk-boundary span and the partial tail chunk) and stride parity
matches decode_jpeg_window_strided.

annotation_processing.py: port of the role-in-key-name scheme -- role lives in
the zarr key NAME (annotations_task/annotations_subtask), entries are plain
{text,start_idx,end_idx} spans. _load_annotations is now per-key (the previous
single-slot cache aliased across keys) and an absent key degrades to [] rather
than raising.

zarr_dataset_packed: annotation_key + span_indices, so a run can read a
structural segmentation key (fold_segments) and select individual spans. Note
fold_segments deliberately sits OUTSIDE the annotations* glob -- those spans
define training windows, not language, and must not reach the batch as a text
role.

fold_span_transforms: RH_WRIST_MODE=none -> 126-dim keypoints-only (the wrist
pose is redundant with the keypoints). ypr(138)/pos(132) unchanged; default
still pos.
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