pytorch / torchcodec

PyTorch video decoding
BSD 3-Clause "New" or "Revised" License
77 stars 9 forks source link

Expose `getFramesAtIndices` and `getFramesDisplayedByTimestamps` as public Python methods of VideoDecoder #293

Closed NicolasHug closed 1 week ago

NicolasHug commented 1 week ago

This is about exposing the 2 C++ methods from https://github.com/pytorch/torchcodec/pull/280 and https://github.com/pytorch/torchcodec/pull/282 into the Python class.

It feels natural to want these methods to be called get_frames_at() and get_frames_displayed_at(), but these names are already occupied by our "range" function. We may then need to rename these existing "range" functions (see diff).

scotts commented 1 week ago

I was puzzling over this myself, and I like this solution. I had considered keeping all current APIs as-is, and adding get_frames_at_indices() and get_frames_displayed_at_timestamps(), which is clunkier. This is cleaner.

ahmadsharif1 commented 1 week ago

This is not my first preference, but you can merge it.

Reason is that "displayed" is not the critical difference b/w the two methods. Frames are always displayed -- whether at an index or timestamp.

I would have picked get_frame_at_index and get_frame_at_time, get_frames_at_index_range, get_frames_at_time_range, etc.

I like names that you can read from the callsite to tell what parameters they would need and what they are doing and other people may disagree with that.

NicolasHug commented 1 week ago

Thank for the feedback. I'll follow-up with separate PRs.