Pull true fps from input videos and use it when making new videos#63
Pull true fps from input videos and use it when making new videos#63Dominic-DallOsto merged 5 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the hardcoded 30 fps in video creation, reads the actual frame rate from input videos, and applies it when generating new 2D/3D videos.
- Un-hardcoded
fpsin_make_video,make_pose2d_video, andmake_pose3d_videoby adding afpsparameter with adefault_fpsfallback. - Introduced
get_fps()incore.pyto probe input videos for their average frame rate. - Updated the CLI to pass
core.fpswhen invoking video-generation functions.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| df3d/video.py | Added default_fps, propagated fps through APIs, and removed hardcoded fps = 30. |
| df3d/core.py | Added get_fps() to extract frame rates via ffprobe and store in self.fps. |
| df3d/cli.py | Updated CLI calls to pass the extracted core.fps to the video functions. |
Comments suppressed due to low confidence (3)
df3d/video.py:18
- [nitpick] Per PEP 8, module-level constants should be uppercase. Consider renaming
default_fpstoDEFAULT_FPS.
default_fps = 30
df3d/video.py:21
- The new
fpsparameter should be documented in the function docstring (e.g.,fps: frames per second to use for the output video).
def make_pose2d_video(plot_2d, num_images, input_folder,
df3d/core.py:416
- Add unit tests for
get_fpscovering scenarios: no videos, consistent FPS, inconsistent FPS, and denominator=0 to ensure accurate behavior.
def get_fps(self):
| # self.cidread2cid, self.cid2cidread = read_camera_order(self.output_folder) | ||
| return np.array(camera_ordering) | ||
|
|
||
| def get_fps(self): |
There was a problem hiding this comment.
Wrap the subprocess.check_output call in a try/except to handle situations where ffprobe fails, and log a warning rather than letting the exception crash the run.
There was a problem hiding this comment.
I agree with Copilot here - if this goes wrong, it can be very hard to debug because ffprobe runs in a different process. This can be especially chaotic when someone runs df3d on a cluster or in the background because different STDOUT/STDERR streams from different processes can be redirected to the same file but with different buffering configurations. Potentially this can make the location of the error message in the log very confusing.
| rates.append(subprocess.check_output(cmd, text=True)) | ||
| if len(rates) == 0: | ||
| return None | ||
| if any(rate != rates[0] for rate in rates): |
There was a problem hiding this comment.
When returning None due to inconsistent frame rates, consider logging a warning so users know why the default FPS fallback is used.
| if any(rate != rates[0] for rate in rates): | |
| if any(rate != rates[0] for rate in rates): | |
| logger.warning("Inconsistent frame rates detected across videos. Falling back to default FPS (None).") |
|
Well, this revealed the fascinating fact that the sample videos provided in
|
| # self.cidread2cid, self.cid2cidread = read_camera_order(self.output_folder) | ||
| return np.array(camera_ordering) | ||
|
|
||
| def get_fps(self): |
There was a problem hiding this comment.
I agree with Copilot here - if this goes wrong, it can be very hard to debug because ffprobe runs in a different process. This can be especially chaotic when someone runs df3d on a cluster or in the background because different STDOUT/STDERR streams from different processes can be redirected to the same file but with different buffering configurations. Potentially this can make the location of the error message in the log very confusing.
df3d/core.py
Outdated
| if any(rate != rates[0] for rate in rates): | ||
| return None | ||
| # All videos returned the same rate, so we return that | ||
| numerator, denominator = map(int, rates[0].split('/')) |
There was a problem hiding this comment.
What's the default output if no FPS information is saved in the video metadata? In other words, will this line fail because "/" is not detected?
Dominic-DallOsto
left a comment
Dominic-DallOsto
left a comment
There was a problem hiding this comment.
What's the default output if no FPS information is saved in the video metadata? In other words, will this line fail because "/" is not detected?
I think rates[0].split('/') will give a list with an empty string, and int will throw an error when trying to parse it.
Dominic-DallOsto
left a comment
Dominic-DallOsto
left a comment
There was a problem hiding this comment.
Well, this revealed the fascinating fact that the sample videos provided in
DeepFly3D/tests/data/reference/*mp4all have their framerate set to a blazing fast 0.2 fps. I... suppose we want to change that? Presumably the true fps of that recording was either 100 or 30, but perhaps it's desirable for the sample output videos to be played back slower than real speed to make it easier for users to inspect the results. This means we could:
- Set the sample video's framerate to like 5 so that the output videos have that framerate
- Set the sample video's framerate to 100, which is most likely the true recording speed, and then add an
--output-fpsflag todf3d-clito allow users to override the input video framerate and specify what fps they want their output videos to be made at.
I think the CLI param would be nice to have, yeah. Then maybe the second option makes the most sense
|
Thanks for the comments! The only small change in behavior I made to the When the tests complete, if this looks good to you please feel free to merge the PR, otherwise I will in a day or two. |
|
The test failure looks like a pretty minor issue: It looks like there are tiny pixel value differences between the video frames that the test function generates and the video frames that the test function believes it should generate. @Dominic-DallOsto do you know what this could be about? I pushed new versions of |
Dominic-DallOsto
left a comment
Dominic-DallOsto
left a comment
There was a problem hiding this comment.
I updated the test data output video framerates using this approach and I think it works all good now
Dominic-DallOsto
left a comment
Dominic-DallOsto
left a comment
There was a problem hiding this comment.
Yeah, maybe the video data just got mangled / reencoded somehow when updating the fps last time. Looks all good now so I'll merge it.
|
That's strange... In d960d82 I had df3d re-generate those videos. So I would expect that the test suite, which also re-generates the videos from input images plus df3d predictions, would produce the same ones. I'm confused as to how the old 0.2fps videos with modified framerates matches the output of the test suite but the output of my local df3d run doesn't. Any ideas? I feel like we're in a bad spot if users and the test suite are generating slightly different outputs! Maybe it just comes down to something small like a different version of a library that I have installed vs the test suite installs? If you run In [3]: vid1, fps1 = npimage.load_video('video_pose3d.mp4', return_framerate=True)
Loading video: 93%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████▎ | 14/15 [00:00<00:00, 19.47it/s]
In [4]: vid1.shape
Out[4]: (15, 3332, 5000, 3)
In [5]: fps1
Out[5]: 5.294117647058823
In [6]: vid2, fps2 = npimage.load_video('video_pose3d_me.mp4', return_framerate=True)
Loading video: 93%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████▎ | 14/15 [00:00<00:00, 19.37it/s]
In [7]: vid2.shape
Out[7]: (15, 3332, 5000, 3)
In [8]: fps2
Out[8]: 5.0
In [10]: (vid1.astype(np.int16) - vid2).max()
Out[10]: np.int16(15)
In [11]: (vid1.astype(np.int16) - vid2).min()
Out[11]: np.int16(-21)Also I see the fps of the current video_3d.mp4 in the repo as 5.294. If we can get a user-df3d-generated video to have the correct values I suggest we push it since it'd have the correct fps. |
Dominic-DallOsto
left a comment
Dominic-DallOsto
left a comment
There was a problem hiding this comment.
I get the same framerates as you - not sure why the ffmpeg edited one is mangled, but for me the two videos are identifcal.
I'll make a PR to update the videos.
In case you want to check - I have these library versions:
numpy 1.26.4
matplotlib 3.9.1
matplotlib-inline 0.1.7
opencv-python 4.10.0.84
opencv-python-headless 4.10.0.84
df3d/video.pypreviously had a hardcoded linefps=30in_make_video(). I un-harcoded this by making it a keyword argument of_make_video(), so now other functions can specify what fps they want when calling_make_video().core.fpswhich is read from the input video files during__init__. (Note that I'm using ffprobe to request theavg_frame_rate, wherer_frame_rateis another possible field that could have been pulled instead - these are supposed to be the same for constant framerate videos, but might be slightly different for variable framerate videos.)--video-2dor--video-3dflags,core.fpsis used to produce a video at the same framerate that the input videos play at.In summary, this eliminates the discrepancy between the output videos previously always being made at 30 fps regardless of the input video framerate.