Generalize README benchmark #327
Conversation
facebook-github-bot
added
the
CLA Signed
| video_count += 1 | ||
|
|
||
| wait(futures) | ||
| for f in futures: | ||
| assert f.result() |
There was a problem hiding this comment.
This ensures that if we encounter an error while generating a video, we stop.
| shutil.rmtree(videos_path) | ||
| ffmpeg_path = "ffmpeg" | ||
| videos_path = "/tmp/torchcodec_benchmarking_videos" | ||
| shutil.rmtree(videos_path, ignore_errors=True) |
There was a problem hiding this comment.
We don't want to fail if the tmp directory does not exist already.
| @@ -56,7 +56,7 @@ def main() -> None: | |||
| ffmpeg_path, | |||
| videos_path, | |||
| ) | |||
| video_paths = glob.glob(f"{videos_path}/*.mp4") | |||
| video_files = glob.glob(f"{videos_path}/*.mp4") | |||
There was a problem hiding this comment.
I was confused by how close the old variable names were to each other.
There was a problem hiding this comment.
See comment above about rename
| videos_path = "/tmp/videos" | ||
| shutil.rmtree(videos_path) | ||
| ffmpeg_path = "ffmpeg" | ||
| videos_path = "/tmp/torchcodec_benchmarking_videos" |
There was a problem hiding this comment.
Maybe this should be renamed to videos_dir_path
I like path over files because it tells the user it's a full path and not the filename only
There was a problem hiding this comment.
Yeah, good point, I'll make one use dir and the other file.
| @@ -56,7 +56,7 @@ def main() -> None: | |||
| ffmpeg_path, | |||
| videos_path, | |||
| ) | |||
| video_paths = glob.glob(f"{videos_path}/*.mp4") | |||
| video_files = glob.glob(f"{videos_path}/*.mp4") | |||
There was a problem hiding this comment.
See comment above about rename
I could not run the benchmarks at first; these are the changes I had to make to get them to run fresh.