+114 −5
Open

Calculate num_frames if missing from metadata #732

Show file tree
Hide file tree
Changes from all commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86905c1
Calculate num_frames, add mock tests
Dan-FloresJun 18, 2025
fdf1a1d
Add comment when num_frames is none
Dan-FloresJun 23, 2025
3075975
Use common frames comparison function
Dan-FloresJun 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Failed to load files.
Original file line numberDiff line numberDiff line change
Expand Up@@ -123,14 +123,22 @@ def end_stream_seconds(self) -> Optional[float]:

@property
def num_frames(self) -> Optional[int]:
"""Number of frames in the stream. This corresponds to
``num_frames_from_content`` if a :term:`scan` was made, otherwise it
corresponds to ``num_frames_from_header``.
"""Number of frames in the stream (int or None).
This corresponds to ``num_frames_from_content`` if a :term:`scan` was made,
otherwise it corresponds to ``num_frames_from_header``. If it is None,
the number of frames is calculated from the duration and the average fps.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the doc string, we should also say what we do if num_frames is missing from all of the metadata.

if self.num_frames_from_content is not None:
return self.num_frames_from_content
else:
elif self.num_frames_from_header is not None:
return self.num_frames_from_header
elif (
self.average_fps_from_header is not None
and self.duration_seconds_from_header is not None
):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went down an interesting rabbit-hole of asking "Should we instead use the average_fps property?" And the answer is no! :) If self.average_fps_from_header is None, then self.average_fps actually calls this property, self.num_frames and we'd (I think) hit infinite recursion. I do wonder if there is a way for us to be more systematic about this, where we could more centrally define the logic of under what circumstances we use which metadata.

return int(self.average_fps_from_header * self.duration_seconds_from_header)
else:
return None

@property
def average_fps(self) -> Optional[float]:
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,6 +6,8 @@

import contextlib
import gc
import json
from unittest.mock import

import numpy
import pytest
Expand DownExpand Up@@ -738,6 +740,77 @@ def test_get_frames_in_range(self, stream_index, device, seek_mode):
empty_frames.duration_seconds, NASA_VIDEO.empty_duration_seconds
)

@pytest.mark.parametrize("device", cpu_and_cuda())
@pytest.mark.parametrize("seek_mode", ("exact", "approximate"))
@("torchcodec._core._metadata._get_stream_json_metadata")
def test_get_frames_with_missing_num_frames_metadata(
self, mock_get_stream_json_metadata, device, seek_mode
):
# Create a mock stream_dict to test that initializing VideoDecoder without
# num_frames_from_header and num_frames_from_content calculates num_frames
# using the average_fps and duration_seconds metadata.
mock_stream_dict = {
"averageFpsFromHeader": 29.97003,
"beginStreamSecondsFromContent": 0.0,
"beginStreamSecondsFromHeader": 0.0,
"bitRate": 128783.0,
"codec": "h264",
"durationSecondsFromHeader": 13.013,
"endStreamSecondsFromContent": 13.013,
"width": 480,
"height": 270,
"mediaType": "video",
"numFramesFromHeader": None,
"numFramesFromContent": None,
}
# Set the return value of the mock to be the mock_stream_dict
mock_get_stream_json_metadata.return_value = json.dumps(mock_stream_dict)

decoder = VideoDecoder(
NASA_VIDEO.path,
stream_index=3,
device=device,
seek_mode=seek_mode,
)

assert decoder.metadata.num_frames_from_header is None
assert decoder.metadata.num_frames_from_content is None
assert decoder.metadata.duration_seconds is not None
assert decoder.metadata.average_fps is not None
assert decoder.metadata.num_frames == int(
decoder.metadata.duration_seconds * decoder.metadata.average_fps
)
assert len(decoder) == 390

# Test get_frames_in_range
Copy link
Contributor

@scotts scotts Jun 23, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment here that we're really just testing the logic in the Python VideoDecoder class when we do the decoding. We do checks that involve num_frames at the Python layer, and that logic will use the mocked metadata. But the C++ layer does not use that mocked metadata, and will instead use what it read from the video file itself.

We should also create an issue about adding a video file that has the number of frames missing in its metadata so we can actually test the C++ logic as well. This test would then use that video file, and not need any mocking.

ref_frames9 = NASA_VIDEO.get_frame_data_by_range(
start=9, stop=10, stream_index=3
).to(device)
frames9 = decoder.get_frames_in_range(start=9, stop=10)
assert_frames_equal(ref_frames9, frames9.data)

# Test get_frame_at
ref_frame9 = NASA_VIDEO.get_frame_data_by_index(9, stream_index=3).to(device)
frame9 = decoder.get_frame_at(9)
assert_frames_equal(ref_frame9, frame9.data)

# Test get_frames_at
indices = [0, 1, 25, 35]
ref_frames = [
NASA_VIDEO.get_frame_data_by_index(i, stream_index=3).to(device)
for i in indices
]
frames = decoder.get_frames_at(indices)
for ref, frame in zip(ref_frames, frames.data):
assert_frames_equal(ref, frame)

# Test get_frames_played_in_range to get all frames
assert decoder.metadata.end_stream_seconds is not None
all_frames = decoder.get_frames_played_in_range(
decoder.metadata.begin_stream_seconds, decoder.metadata.end_stream_seconds
)
assert_frames_equal(all_frames.data, decoder[:])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a simple assert len(decoder) == CORRECT_VALUE?


@pytest.mark.parametrize("dimension_order", ["NCHW", "NHWC"])
@pytest.mark.parametrize(
"frame_getter",
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -119,7 +119,7 @@ def test_get_metadata_audio_file(metadata_getter):

@pytest.mark.parametrize(
"num_frames_from_header, num_frames_from_content, expected_num_frames",
[(None, 10, 10), (10, None, 10), (None, None, None)],
[(10, 20, 20), (None, 10, 10), (10, None, 10)],
)
def test_num_frames_fallback(
num_frames_from_header, num_frames_from_content, expected_num_frames
Expand All@@ -143,6 +143,34 @@ def test_num_frames_fallback(
assert metadata.num_frames == expected_num_frames


@pytest.mark.parametrize(
"average_fps_from_header, duration_seconds_from_header, expected_num_frames",
[(60, 10, 600), (60, None, None), (None, 10, None), (None, None, None)],
)
def test_calculate_num_frames_using_fps_and_duration(
average_fps_from_header, duration_seconds_from_header, expected_num_frames
):
"""Check that if num_frames_from_content and num_frames_from_header are missing,
`.num_frames` is calculated using average_fps_from_header and duration_seconds_from_header
"""
metadata = VideoStreamMetadata(
duration_seconds_from_header=duration_seconds_from_header,
bit_rate=123,
num_frames_from_header=None, # None to test calculating num_frames
num_frames_from_content=None, # None to test calculating num_frames
begin_stream_seconds_from_header=0,
begin_stream_seconds_from_content=0,
end_stream_seconds_from_content=4,
codec="whatever",
width=123,
height=321,
average_fps_from_header=average_fps_from_header,
stream_index=0,
)

assert metadata.num_frames == expected_num_frames


def test_repr():
# Test for calls to print(), str(), etc. Useful to make sure we don't forget
# to add additional @properties to __repr__
Expand Down
Loading