Repository navigation
Conversation
e642184 to
bd04b38
Compare
8ae0806 to
0b25bc8
Compare
bd04b38 to
204c215
Compare
0b25bc8 to
07c145d
Compare
204c215 to
f5b66c3
Compare
12ee83c to
ffa99f1
Compare
f5b66c3 to
ca35d13
Compare
ffa99f1 to
4e434b9
Compare
ca35d13 to
64165e7
Compare
RyanPCo
left a comment
RyanPCo
left a comment
There was a problem hiding this comment.
Automated adversarial review (read-only; findings were checked by a second skeptical pass before posting): 4 low, 3 nit.
| return parse_index(pre + moov, (len(pre) + hlen, len(pre) + size)) | ||
| if typ == b"mdat" and not head: | ||
| head = buf[: pos - base] if base == 0 else b"" | ||
| pos += size |
There was a problem hiding this comment.
low (correctness): read_index does not handle a box with size == 0 (runs to EOF), which _boxes does handle. For a non-moov box, pos += 0 loops forever with no further read, so a DataLoader worker hangs on a malformed or streamed MP4 instead of raising. Raise a ValueError when size == 0 or size < hlen, and raise a clear 'no moov before EOF' error when read returns fewer than 8 bytes, instead of letting struct.error escape.
| leaves = LanceStreamingResolver( | ||
| root, video_chunks={}, key_map=KEY_MAP, cameras={} | ||
| ).resolve() | ||
| leaves = LanceStreamingResolver(root, key_map=KEY_MAP, cameras={}).resolve() |
There was a problem hiding this comment.
low (test-quality): Every streaming test here passes cameras={}, so _fetch_range_frames returns early, and the new batched range-fetch path (URL grouping, the num_frames check, merged-range jobs, per-sample decode) has no unit coverage. The only end-to-end check is skipped unless EGOVERSE_TEST_LANCE_ROOT is set. Consider writing a small AV1 MP4 into the fixture catalog with fixtures.videos.write_av1, then asserting that a streaming leaf with a camera key matches the local leaf, including two samples that share a URL.
| def __init__(self, recs, images): | ||
| self.recs, self.images = recs, images | ||
| self.timing = {"take": 0.0, "blobs": 0.0, "decode": 0.0} | ||
| def _read(url: str, a: int, b: int) -> bytes: |
There was a problem hiding this comment.
low (duplication): _read repeats dataset.read_range from this PR line for line, and RangeDS._decoder repeats dataset._mp4 without the LRU bound. Import read_range and _mp4 from egomimic.rldb.lance.dataset so the bench times the same read and index code the loader uses and can't drift from it.
| return parse_index(pre + moov, (len(pre) + hlen, len(pre) + size)) | ||
| if typ == b"mdat" and not head: | ||
| head = buf[: pos - base] if base == 0 else b"" | ||
| pos += size |
There was a problem hiding this comment.
low (correctness): Unlike _boxes, read_index doesn't handle size == 0. A top-level box with size 0 before moov (for example an mdat from a non-seekable muxer) makes pos += size add nothing, and the loop spins forever, hanging the worker. A file with no moov fails with a bare struct.error or an S3 range error. Raise ValueError("moov not found before EOF") when size is 0 or the read comes back short.
| def __init__(self, recs, images): | ||
| self.recs, self.images = recs, images | ||
| self.timing = {"take": 0.0, "blobs": 0.0, "decode": 0.0} | ||
| def _read(url: str, a: int, b: int) -> bytes: |
There was a problem hiding this comment.
nit (simplification): _read duplicates egomimic.rldb.lance.dataset.read_range, which this PR adds (local seek/read, or S3 get_object with an inclusive Range). Import read_range and delete _read so the benchmark measures the same code path the loader uses.
| pos += size | ||
|
|
||
|
|
||
| def ranges(index: Mp4Index, frames, gap: int = 0) -> list[tuple[int, int]]: |
There was a problem hiding this comment.
nit (dead-code): No caller passes gap; the loader and the bench both use 0. Drop the parameter and merge on a <= out[-1][1], or set it from a named constant in _fetch_range_frames if near-adjacent merging is meant for S3.
| Lance writer. As conversions finish it validates them (lance_pilot/commit.py), appends the | ||
| recordings' video chunk rows when --chunk-k is set, then commits them in batches. Reruns | ||
| Lance writer. As conversions finish it validates them (lance_pilot/commit.py), then | ||
| commits them in batches. Reruns |
There was a problem hiding this comment.
nit (docs): Reflow this paragraph. Removing the chunk sentence left a ragged break after "Reruns".
LanceStreamingDataset now reads frames straight from the MP4s: each process reads an MP4's sample table once (one range GET of its moov), then a batch fetches only the packets its frames need (keyframe through frame; adjacent with GOP 2) in one concurrent round of range GETs and decodes them in memory with PyAV. Frames match torchcodec bit for bit. On R2 with 8 workers this ties the K=8 chunk tables on raw reads (277 vs 276 samples/s steady state) and runs 5-15% behind through the full loader, with video stored once and nothing for the committer to build. So video_chunks tables, chunk_videos.py and --chunk-k go. The chunk path had been broken since leaves were keyed by recording_id (row.episode_hash). Claude-Session: https://claude.ai/code/session_01Kt1E8T2x8kdV9akAvbJyVd
4e434b9 to
8313302
Compare
64165e7 to
6278aec
Compare
|
Review findings: status after the fixup pass. The fixes are squashed into this PR's commits, and the whole stack was force-pushed. Fixed
Partial
Invalid
|
LanceStreamingDataset now reads frames straight from the MP4s: each
process reads an MP4's sample table once (one range GET of its moov),
then a batch fetches only the packets its frames need (keyframe through
frame; adjacent with GOP 2) in one concurrent round of range GETs and
decodes them in memory with PyAV. Frames match torchcodec bit for bit.
On R2 with 8 workers this ties the K=8 chunk tables on raw reads (277
vs 276 samples/s steady state) and runs 5-15% behind through the full
loader, with video stored once and nothing for the committer to build.
So video_chunks tables, chunk_videos.py and --chunk-k go. The chunk path
had been broken since leaves were keyed by recording_id (row.episode_hash).
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw