[format][python] Persist keyframe indexes for efficient random video frame reads - #9831
XiaoHongbo-Hope wants to merge 8 commits into
Conversation
d8fffbd to
12e689e
Compare
0db216c to
405600d
Compare
a8b3b9f to
9f115c2
Compare
| descriptor = frame.keyframe_index_descriptor | ||
| if descriptor is None: | ||
| return b'' | ||
| mapping = Blob.from_descriptor(blob.uri_reader, descriptor).to_data() |
There was a problem hiding this comment.
[P1] Avoid materializing unbounded keyframe indexes
This materializes the entire compressed index before the chunked validator runs, and _keyframe_indexes retains every block until the writer closes. BlobRef.to_data() issues a single read for the descriptor length, while rolling is checked only after add_element; therefore a valid or crafted large index can allocate beyond the worker heap despite the 64 KiB decompression chunks. The Java path has the same behavior in VideoFormatWriter.java. Please validate from a bounded stream into spillable storage, or enforce explicit per-index and cumulative limits, instead of materializing unbounded bytes.
|
The format work is careful, and the bounded-validation issue from my earlier review appears addressed. However, this PR still does not deliver the claimed random-read benefit to a Paimon user: |
Thanks, I did the read related change in the previous commit of this PR, but removed later. I will add back. |
101fc42 to
81ee157
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
The payload-relative sparse-index layout can be retained. These two issues affect indexed-read correctness and read amplification.
| container.seek( | ||
| anchor_pts, backward=False, any_frame=False, stream=stream) |
There was a problem hiding this comment.
[P1] Seek backward to the persisted keyframe anchor
backward=False can skip the indexed keyframe, but the loop below requires an exact match for anchor_pts. With PyAV 18.1.0, a valid 150-frame fragmented H.264 MP4 (movflags=frag_keyframe+empty_moov+default_base_moof) is automatically indexed successfully, yet all 150 indexed reads fail: seeking to the first anchor at PTS 1024 starts at the next GOP, PTS 8704. Ordinary HEVC MP4 also fails for several frame ranges. The same forward seek skips the anchor on the complete file, so fetching more bytes or retrying earlier anchors does not fix it; the unindexed reader returns the correct frames.
Please seek backward and decode until the stored anchor is found, and add real fMP4/HEVC regression coverage. Changing only this flag to backward=True made all 750 frame comparisons pass across closed/open-GOP H.264, fragmented H.264, and HEVC, without changing the stored fields.
| ranges.append(( | ||
| offset, header_size if box_type == b"mdat" else size)) |
There was a problem hiding this comment.
[P2] Exclude padding bodies from initialization ranges
Treating every non-mdat box as initialization data includes the entire body of free/skip boxes. These ranges are fetched for every uncached indexed read, so MP4 files with reserved space can retain almost full-file read amplification despite the sparse index. I reproduced this with a valid MP4 remuxed using FFmpeg's -moov_size 16777216: reading frame 75 requests 16,779,069 bytes out of a 16,785,845-byte file. Keeping only the padding box headers reduces that request to 4,329 bytes, and all 150 decoded frames still match full decoding.
Please preserve the headers needed for box traversal while excluding free/skip bodies from the persisted initialization ranges. Other boxes should be handled according to their decoding requirements rather than dropped indiscriminately.
JingsongLi
left a comment
There was a problem hiding this comment.
The new producer/consumer path addresses my earlier end-to-end concern: the writer now generates seek indexes and PaimonLeRobotDataset consumes them. I verified the public import -> persisted video -> two-worker DataLoader test on this head, together with the format/index tests (20 passed). There are two remaining issues: automatically indexed MPEG-4 Part 2 videos can fail reads that the scan path handles correctly, and the documented local path/Blob.from_local inputs silently bypass index generation. Details and reproduction conditions are in the inline comments.
| raise ValueError( | ||
| "Cannot decode video frames from persisted seek index." | ||
| ) from last_error |
There was a problem hiding this comment.
[P2] Keep valid videos on a readable path when sparse seeking is unsupported
Automatic index generation accepts MPEG-4 Part 2 MP4s, but these retries do not always decode them. With PyAV 18.1.0, I encoded 60 frames using mpeg4, GOP 12 and 2 B frames, then ingested them through add_video(BlobDescriptor(uri, 0, length), rows). The writer persisted a valid 140-byte index, but cold reads of frames 36, 48 and 59 through VideoFrameCollator with the dataset decoder all reach this exception. The identical persisted payload and frame indices, with the index omitted, match full PyAV decoding exactly. Thus successful ingestion can now make a valid video unreadable through default/PyAV reads. Please retain the codec initialization data needed for these seeks, or decline index generation for unsupported cases so they keep the scan path, and add this regression case.
| mapping = VideoKeyframeIndex.inspect( | ||
| stream, frame.payload_descriptor.length).serialize() | ||
| except (ImportError, OSError, ValueError, EOFError): | ||
| return b'' |
There was a problem hiding this comment.
[P2] Resolve unknown source lengths before inspecting the video
The documented add_video(path, rows) and add_video(Blob.from_local(path), rows) inputs carry BlobDescriptor.length == -1. Passing that sentinel to inspect makes _iso_bmff_metadata_ranges skip its box loop and raise; this handler silently stores an empty index. Copying the payload later discovers its length but never retries indexing. I verified the public API with the same supported H.264 MP4: path and Blob.from_local inputs produced 60 unindexed frame rows, while an explicit descriptor with the actual length produced a 133-byte index on every row. Please resolve the effective source length before inspection and cover both documented local-file inputs, so ordinary ingestion receives the advertised random-read benefit.
81ee157 to
639907b
Compare
|
My concrete recommendation is to keep the current storage layout and replace the zero-filled sparse temporary file with a seekable file-like reader backed by range reads. Use the persisted index to prefetch likely ranges; when the decoder requests uncached bytes, fetch the actual bytes from the video payload. This makes decoding correctness independent of the fixed extra-GOP and two-keyframe retry heuristics. Also apply the two inline fixes: exclude I do not see a demonstrated need to add DTS or GOP-length fields, or to redesign the on-disk layout. |
|
Please add a reproducible end-to-end benchmark for the cold random-read claim. The current tests verify decoding correctness and that selected payload ranges cover fewer bytes than the full video. The byte-count assertion measures unique payload coverage; it excludes index reads and does not count repeated transfers. There is no indexed/unindexed latency or request-count comparison. Use representative long videos in object storage, the same decoder, and identical random target frames with and without the persisted index. Start cold runs with fresh decoder/cache state and report:
This would quantify both the read benefit and the cost shifted to ingestion. |
639907b to
0a3a923
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
The current head now has end-to-end value: normal video ingestion generates the persisted seek index, and PaimonLeRobotDataset consumes it through the public read path. The earlier missing producer/consumer path, unbounded index buffering, unknown local-source length, padding prefetch, and forward-seek issues are addressed in this revision.
I revalidated the format and reader behavior on cf35bae: Java VideoFrameDescriptorTest 4/4, VideoFileFormatTest 13/13, and BlobTableTest 49 passed/1 skipped; Python index/format tests 25/25 plus 16 subtests passed with PyAV 15.1.0. I also repeated the prior MPEG-4 Part 2 case with fresh decoders for individual frames 0, 1, 11, 12, 24, 36, 48, and 59: all matched full-file decoding. On a 195,793-byte video those reads transferred 44,946–88,889 bytes each, including repeated range reads. The PR's public import-to-DataLoader test covers the integrated path in CI; my local environment lacks Torch, so I did not rerun that specific test locally. I did not independently reproduce the object-store benchmark.
I found no additional code blocker in this review. The production gate is still red: Python 3.10–3.13 CI jobs fail and Flink 1 Common was cancelled. The Python 3.10 log shows 36 failures in native_commit_test.py (an unchanged file in this PR), so these may be a branch/base issue, but the failures and cancelled job need a green rerun or documented resolution before merge.
| source = _RangeBackedVideo( | ||
| self._video_length(), self._read_video_ranges) | ||
| try: | ||
| source.prefetch(_merge_video_ranges( |
There was a problem hiding this comment.
[P1] Bound batch range prefetch by bytes
This prefetches the metadata plus every GOP window for all missing indices in one call, and _RangeBackedVideo._ensure keeps every returned body in _segments until the whole batch finishes. A shuffled batch that selects frames from many distant GOPs in one large, valid video can therefore materialize the sum of those windows, potentially approaching the whole video; multiple DataLoader workers multiply the peak. The 16/64 MiB limits only bound index blocks, not video ranges. Please decode groups in byte-bounded chunks or evict range segments instead of retaining the full batch working set.
| ordinal = physicalVideoLengths.size(); | ||
| physicalVideoLengths.add(length); | ||
| Blob keyframeIndex = VideoFrameDescriptor.keyframeIndexBlob(blob); | ||
| byte[] mapping = keyframeIndex == null ? new byte[0] : keyframeIndex.toData(); |
There was a problem hiding this comment.
[P2] Apply the configured NULL policy to the index fetch
payloadWriter.write(element) honors blob-write-null-on-missing-file and blob-write-null-on-fetch-failure, but this independent toData() fetch bypasses both policies and the fetch metrics. If the payload range succeeds and the index range then returns 404/416, is truncated, or fails transiently, the writer aborts even when NULL fallback is enabled; the payload bytes have already been written by that point. Please fetch and validate the index through the same policy before writing the payload, append NULL when the configured fallback applies, and cover payload-success/index-failure for both options.
| "Corrupt video file: negative keyframe index length." | ||
| ) | ||
|
|
||
| keyframe_index_size = sum(keyframe_index_lengths) |
There was a problem hiding this comment.
[P2] Enforce the documented index-size limits while reading
The format specification limits each keyframe-index block to 16 MiB and all blocks in one file to 64 MiB, but this reader only rejects negative lengths or totals outside the file. VideoFrameCollator later passes the declared length to read_file_range and materializes the entire block before VideoKeyframeIndex.deserialize can validate it. A corrupt or forged .video file can therefore force an arbitrarily large range read and allocation. Please reject per-block and cumulative lengths above the documented limits in VideoFileMeta before exposing these descriptors, and mirror the check in the Java reader.
| stream.seek(position) | ||
| mapping = VideoKeyframeIndex.inspect( | ||
| stream, payload_length).serialize() | ||
| except (ImportError, OSError, ValueError, EOFError): |
There was a problem hiding this comment.
[P2] Include PyAV FFmpeg errors in the scan fallback
The documented behavior is that videos which cannot be indexed use the scan fallback, but this handler does not cover every av.error.FFmpegError. For example, decoder-not-found and other FFmpeg failures may inherit LookupError or only FFmpegError, so they escape this tuple and abort ingestion instead of returning an empty index. The reader in this PR already adds av.error.FFmpegError dynamically in _video_decoder_fallback_errors; please apply the same exception family here and add a real PyAV regression test.
Purpose
Avoid full-video probing and scan amplification for cold random frame reads. PyPaimon now generates a seek index while ingesting supported MP4 videos.
PaimonLeRobotDatasetuses it to prefetch video metadata and requested GOP ranges, then serves any uncached decoder reads through Range GET.Design
.video (in file order) |-- Complete video payloads A, B, ... |-- NEW: Video seek index blocks A, B, ... | |-- Video metadata ranges: (offset, length) pairs | `-- Compressed keyframe entries: (frame ordinal, PTS, packet byte position) |-- Video payload lengths |-- NEW: Video seek index block lengths |-- Consecutive-row counts / video references / starting frame ordinals `-- Footer |-- Five index byte lengths: 5 x 4 bytes |-- Magic: 4 bytes `-- Format version: 1 byteEach video has one block-length entry; zero selects the existing scan fallback. All seek-index offsets are relative to the first byte of that encoded video.
moovGOP index and in-GOP frame index are derived from the requested ordinal and preceding keyframe, so the format stores entries only for keyframes. PTS keeps VFR exact, and packet position enables Range GET. Stream identity and time base remain in the encoded video.
Read and write paths
mdat,free, andskipbodies. Unsupported videos and environments without PyAV retain the scan fallback.add,add_video,add_videos,replace_video, rolling, and compaction. Conflicting indexes for one payload fail the write.PaimonLeRobotDatasetloads the index, prefetches initialization metadata and a bounded GOP window throughFileIO.read_ranges_coalesced, and gives PyAV a seekable range-backed stream. Uncached decoder reads fetch actual payload bytes; no zero-filled temporary file or fixed retry heuristic is used.Current open-source Lance comparison
The current open-source LeRobot-Lance stores each source MP4 and its seek metadata as one row in
videos.lance:moov_offset,moov_sizekf_indiceskf_positionsPaimon stores the metadata beside its payload in the same
.videofile and compresses the keyframe entries.Benchmark
CPU-only, single-node
ecs.g8i.xlarge(4 vCPU, 16 GiB) against same-region OSS internal endpoints. Each of 10 samples used a fresh decoder, HTTP session, and client byte cache; OSS service-side cache was not controlled. Both videos contained 20,000 frames.Index generation took 17.3–22.8 seconds per video. The 59.7–60.6 KiB indexes were 0.043–0.060% of video size. All decoded frame hashes matched. The paired run used
7fa244ccf1; an indexed rerun onf3bb9da803measured 154–196 ms for one frame and 801–848 ms for batch 32 with unchanged hashes and GET counts.Validation
load_from_lerobot->.video->PaimonLeRobotDataset-> two-workerDataLoader; decoded B-frame MP4 output matched expected pixels. A middle-frame read from a 120-frame, multi-GOP video transferred fewer total bytes than the complete payloads, including repeated range requests and index reads.moov; indexed output matched full PyAV decoding. The MPEG-4 test omits media prefetch to verify on-demand reads.Blob.from_localingestion both generated indexes; largefree/skipbodies were excluded from initialization ranges.git diff --checkpassed.