Calculate num_frames if missing from metadata #732

Dan-Flores · 2025-06-18T21:10:59Z

This PR enables the instantiation of a VideoDecoder without certain metadata to resolve issue #727.

Without num_frames_from_content or num_frames_from_header in the metadata, the VideoDecoder will calculate the number of frames using average_fps_from_header and duration_seconds_from_header.
Without duration_seconds_from_header, or end_stream_seconds_from_content and begin_stream_seconds_from_content from a scan, the VideoDecoder will calculate the duration using num_frames_from_header and average_fps_from_header.

Tests in test_decoders.py

The return value of _get_stream_json_metadata is mocked to ensure VideoDecoder is instantiated without num_frames_from_content or num_frames_from_header.
Python logic evaluating the metadata in VideoDecoder instantiation and get_frames_in_range are tested.

Tests in test_metadata.py

test_calculate_num_frames_using_fps_and_duration instantiates VideoStreamMetadata without num_frames_from_content or num_frames_from_header, and checks that various values of average_fps and duration_seconds result in the correct number of frames.
In test_num_frames_fallback, the test where both num_frames_from_content and num_frames_from_header are None is no longer valid. A similar test case is added in test_calculate_num_frames_using_fps_and_duration.
test_duration_seconds_fallback validates that calculating metadata using a scan is prioritized over header metadata.
test_calculate_duration_seconds_using_fps_and_num_frames tests the fallback implemented in this PR.

scotts · 2025-06-20T14:16:06Z

src/torchcodec/_core/_metadata.py

        """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``.
        """


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

scotts · 2025-06-20T14:35:03Z

src/torchcodec/_core/_metadata.py

+        elif (
+            self.average_fps_from_header is not None
+            and self.duration_seconds_from_header is not None
+        ):


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.

scotts · 2025-06-20T14:50:27Z

test/test_decoders.py

+        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[:])


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

scotts

I had initially approved, but it looks like the tests are failing on Mac - let's make sure we're using the right tolerances there.

scotts · 2025-06-20T14:56:08Z

Thank you! Also, this is only half of #727. We also want to be able to handle if num_frames is present but duration is not. This half, though, is definitely the more valuable side, as core operations in the Python VideoDecoder depend on num_frames.

scotts · 2025-06-23T16:54:16Z

test/test_decoders.py

+        )
+        assert len(decoder) == 390
+
+        # Test get_frames_in_range


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.

Good point! I'll remove some unnecessary python calls here, and add a comment indicating that the python logic is the only aspect being tested.

It looks like issue #710 covers this subject.

scotts · 2025-06-23T16:56:43Z

Thanks, @Dan-Flores! I looked into the failures, and they're all known (#740, #734, #724).

Dan-Flores · 2025-06-23T21:00:31Z

I've updated this PR to complete the other half of #727, and updated the description as well.

NicolasHug

Thanks for the PR, and great work on the tests/mocking!

NicolasHug · 2025-06-24T12:14:30Z

test/test_metadata.py

+
+@pytest.mark.parametrize(
+    "duration_seconds_from_header, begin_stream_seconds_from_content, end_stream_seconds_from_content, expected_duration_seconds",
+    [(60, 5, 20, 15), (60, 1, None, 60), (None, 0, 10, 10)],


Nit for completeness, I think we can also test the case when only begin_stream_seconds_from_content is None?

Suggested change

[(60, 5, 20, 15), (60, 1, None, 60), (None, 0, 10, 10)],

[(60, 5, 20, 15), (60, 1, None, 60), (60, None, 1, 60), (None, 0, 10, 10)],

NicolasHug · 2025-06-24T12:16:23Z

test/test_metadata.py

+
+@pytest.mark.parametrize(
+    "num_frames_from_header, average_fps_from_header, expected_duration_seconds",
+    [(100, 10, 10), (100, None, None), (None, None, None)],


Similarly here we may want to test when only num_frames_from_header is missing:

Suggested change

[(100, 10, 10), (100, None, None), (None, None, None)],

[(100, 10, 10), (100, None, None), (None, 10, None), (None, None, None)],

NicolasHug · 2025-06-24T12:17:14Z

src/torchcodec/_core/_metadata.py

    def duration_seconds(self) -> Optional[float]:
        """Duration of the stream in seconds. We try to calculate the duration
        from the actual frames if a :term:`scan` was performed. Otherwise we
        fall back to ``duration_seconds_from_header``.


Similarly to what was done for the num_frames properties, we can slightly update the docstring above to describe the new fall-back

src/torchcodec/_core/_metadata.py

scotts

All the new stuff looks good too, good to merge!

The new tests introduced in ffac96c (meta-pytorch#732) are failing since d6ce570 (meta-pytorch#737) because d6ce570 was merged by rebase.

facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jun 18, 2025

scotts reviewed Jun 20, 2025

View reviewed changes

scotts approved these changes Jun 20, 2025

View reviewed changes

scotts requested changes Jun 20, 2025

View reviewed changes

Dan-Flores force-pushed the calculate_missing_num_frames branch from 8d60d07 to 3075975 Compare June 23, 2025 15:01

scotts reviewed Jun 23, 2025

View reviewed changes

scotts approved these changes Jun 23, 2025

View reviewed changes

Dan-Flores marked this pull request as draft June 23, 2025 20:16

Dan-Flores marked this pull request as ready for review June 23, 2025 21:00

NicolasHug approved these changes Jun 24, 2025

View reviewed changes

Dan-Flores added 7 commits June 24, 2025 06:47

Calculate num_frames, add mock tests

2c7722f

Add comment when num_frames is none

8167eb2

Use common frames comparison function

0ae588f

Remove unnecessary get_frames functions, add comment

e71ab06

fallback to calculate duration using frames and fps

38aa16b

Test duration_seconds metadata

d6b2ef0

Added test cases, updated docstring

e94fc04

Dan-Flores force-pushed the calculate_missing_num_frames branch from 63613c7 to e94fc04 Compare June 24, 2025 14:33

scotts reviewed Jun 24, 2025

View reviewed changes

src/torchcodec/_core/_metadata.py Outdated Show resolved Hide resolved

scotts reviewed Jun 24, 2025

View reviewed changes

src/torchcodec/_core/_metadata.py Outdated Show resolved Hide resolved

knits

3b0d4ed

scotts approved these changes Jun 25, 2025

View reviewed changes

Dan-Flores merged commit ffac96c into meta-pytorch:main Jun 25, 2025
42 of 44 checks passed

carandraug mentioned this pull request Jun 25, 2025

test/: fix failing tests due to missing argument (fixup d6ce5703) #741

Merged

	[(60, 5, 20, 15), (60, 1, None, 60), (None, 0, 10, 10)],
	[(60, 5, 20, 15), (60, 1, None, 60), (60, None, 1, 60), (None, 0, 10, 10)],

	[(100, 10, 10), (100, None, None), (None, None, None)],
	[(100, 10, 10), (100, None, None), (None, 10, None), (None, None, None)],

Calculate num_frames if missing from metadata #732

Calculate num_frames if missing from metadata #732

Uh oh!

Conversation

Dan-Flores commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests in test_decoders.py

Tests in test_metadata.py

Uh oh!

scotts Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

scotts Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

scotts Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

scotts left a comment

Choose a reason for hiding this comment

Uh oh!

scotts commented Jun 20, 2025

Uh oh!

scotts Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dan-Flores Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

scotts commented Jun 23, 2025

Uh oh!

Dan-Flores commented Jun 23, 2025

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

NicolasHug Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

NicolasHug Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

NicolasHug Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

scotts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Dan-Flores commented Jun 18, 2025 •

edited

Loading

scotts Jun 23, 2025 •

edited

Loading