JingsongLi commented on code in PR #10511:
URL: https://github.com/apache/paimon/pull/10511#discussion_r4248538240
##########
paimon-python/pypaimon/multimodal/window_dataset.py:
##########
@@ -642,15 +704,18 @@ def _require_tag_snapshot(table, tag_name, snapshot_id):
"built from snapshot %s." % (tag_name, resolved, snapshot_id))
-def _reject_video_columns(table, columns):
- video_columns = [
- name for name in columns if name in table.options.video_frame_fields()
- ]
- if video_columns:
- raise ValueError(
- "ContiguousWindowDataset does not support video frame columns %s "
- "yet: a window read would drop their frame metadata, such as "
- "frame_index." % video_columns)
+def _open_default_video_decoder(stream, backend=None):
+ # Keep the codec implementation shared with PaimonLeRobotDataset while the
+ # generic collator owns grouping, range reads, and process-local caching.
+ from pypaimon.multimodal.lerobot.dataset import _open_video_decoder
+ return _open_video_decoder(stream, backend=backend)
+
+
+def _decode_video_frames(decoder, frame_indices, unused_rows):
+ get_frames_at = getattr(decoder, "get_frames_at", None)
+ if get_frames_at is not None:
+ return get_frames_at(indices=frame_indices).data
Review Comment:
[P2] Give each decoded frame independent storage
`get_frames_at(...).data` is a batched Tensor, so the collator assigns views
of its storage to individual rows. `_sample()` then calls `copy.deepcopy()`
separately for each requested frame (lines 303–305), which copies the entire
backing batch storage for every view. With no `column_transforms`, a real PyAV
read of 32 three-frame windows (34 decoded rows) returned 96 independent
storages of 4,896 bytes each, although each 8×6 RGB frame contains only 144
bytes: 470,016 retained bytes instead of 13,824 (34×). `torch.stack` releases
those oversized copies after each transform, but still incurs the full batch
copies. This substantially increases memory and copying costs for
video-training batches. Please clone each frame before passing it to the
collator, or use `Tensor.clone()` instead of `deepcopy` in `_sample()`, and add
a regression check for the returned frames' storage size. Per-frame cloning
reduced this reproduction to 1× storage with identical frame values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
