Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MP4 video output in save_video_as_grid_and_mp4 #408

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

FlyingFathead
Copy link

Summary:

This PR addresses a critical issue in the save_video_as_grid_and_mp4 function when running video_sampling.py, where video files intended to be saved as MP4 were being mishandled, leading to crashes when writing to MP4 files. The function has been updated to correctly leverage imageio and ffmpeg for MP4 video creation.

Changes:

  1. FFmpeg Integration:

    • Added a system-level check for FFmpeg availability using shutil.which("ffmpeg"). If FFmpeg is not installed, a clear runtime error is raised, prompting the user to install it.
    • Incorporated a check to ensure that the imageio-ffmpeg package is properly installed and functioning in order for the MP4 video encoding to proceed without issues.
  2. Video Encoding with Imageio:

    • Corrected the writer format in save_video_as_grid_and_mp4 by ensuring the use of the ffmpeg format and libx264 codec for MP4 output.
    • Implemented a confirmation message to notify the user that the video has been successfully saved to the specified path.
  3. Dependency Update:

    • Updated requirements.txt to include imageio[ffmpeg]==2.26.1, ensuring that all required video processing dependencies are installed when setting up the environment.

How to Test:

  • Ensure FFmpeg is installed on your system and accessible via your system’s PATH.
  • Run the application with the updated code. If FFmpeg is not available, a runtime error will prompt you to install it.
  • After video sampling, check if the resulting MP4 is correctly created in the specified folder and playable.

@FlyingFathead
Copy link
Author

Fixed the check method on imageio[ffmpeg] avaialbility; previous wasn't applicable after all. Feel free to either include or exclude the checking inside the function, but at the very least the imageio[ffmpeg] pip package should be in the requirements, since it hadn't been included all in any of the install scripts.

Copy link

@RahulVadisetty91 RahulVadisetty91 left a comment

Choose a reason for hiding this comment

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

image

The variable device is declared globally and then re-declared inside the function initial_model_load. Redundant use of the device assignment might cause confusion. Consider using the global device variable consistently.

@xijiu9
Copy link

xijiu9 commented Oct 8, 2024

For scripts/sampling/simple_video_sample.py vscode also can not view the mp4 file. after downgrade to imageio[ffmpeg]==2.26.1 it works fine.

@FlyingFathead
Copy link
Author

FlyingFathead commented Oct 13, 2024

@RahulVadisetty91 @xijiu9 I've addressed your comments:

  • Refactored the device variable to use the global declaration consistently.
  • The version I proposed uses imageio[ffmpeg] 2.26.1 as suggested.

Feel free to chime in if you think it needs something more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants