Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

removed unnecessary ffmpeg requirement - fixes issue #482 #506

Closed
wants to merge 1 commit into from
Closed

removed unnecessary ffmpeg requirement - fixes issue #482 #506

wants to merge 1 commit into from

Conversation

PixelOmen
Copy link

Description

This pull request removes a redundant ffmpeg depedency.

Related Issue

Fixes #482

Changes

  • removed ffmpeg from requirements.txt
  • removed ffmpeg from environment.yml

Details

The ffmpeg package includes the following modules:

  • audio
  • images
  • stream
  • video

None of these modules are called upon in the DeOldify code. It also includes a blank __init__.py that is overridden by the ffmpeg-python dependency referenced in issue #482.

The DeOldify code uses the following ffmpeg calls that are all resolved with the ffmpeg-python dependecy.

import ffmpeg
probe = ffmpeg.probe(str(path))
except ffmpeg.Error as e:
process = (
    ffmpeg
        .input(str(source_path))
        .output(str(bwframe_path_template), format='image2', vcodec='mjpeg', **{'q:v':'0'})
        .global_args('-hide_banner')
        .global_args('-nostats')
        .global_args('-loglevel', 'error')
)


It also includes other direct system calls which don't rely on either dependency:

os.system(
    'ffmpeg -y -i "'
    + str(source_path)
    + '" -vn -acodec copy "'
    + str(audio_file)
    + '"'
    + ' -hide_banner'
    + ' -nostats'
    + ' -loglevel error'
)

Note:

Both ffmpeg dependencies rely on the ffmpeg binary being globally available via the PATH env variable.

Testing

Tested with the ColorizeVideo_gen.pth model and the following code using only the ffmpeg-python dependency:

from deoldify import device
from deoldify.device_id import DeviceId
#choices:  CPU, GPU0...GPU7
device.set(device=DeviceId.GPU0)

from deoldify.visualize import *
plt.style.use('dark_background')
import warnings
warnings.filterwarnings("ignore", category=UserWarning, message=".*?Your .*? set is empty.*?")

colorizer = get_video_colorizer()
colorizer.colorize_from_file_name("mytestfile.mp4")

bwframes, colorframes, and result were all rendered successfully.

@snynisada
Copy link

snynisada commented Jun 29, 2024 via email

@jantic
Copy link
Owner

jantic commented Jul 27, 2024

I appreciate your work here! This is a tricky issue unfortunately. From ffmpeg-python's github readme:

Note: ffmpeg-python makes no attempt to download/install FFmpeg, as ffmpeg-python is merely a pure-Python wrapper - whereas FFmpeg installation is platform-dependent/environment-specific, and is thus the responsibility of the user, as described below.

Not sure what exactly the environment was that you were testing on, but I have to guess that ffmpeg was already installed somewhere because ffmpeg--python doesn't actually do this for you. I'm not a fan of that design choice personally but anyway- that's why I have a separate explicit ffmpeg install in there.

So unfortunately I can't merge this pull request in. And it's a particular shame because you did such a great job with putting this together!

@jantic jantic closed this Jul 27, 2024
@PixelOmen
Copy link
Author

PixelOmen commented Jul 27, 2024 •
edited
Loading

@jantic

Thanks. You're absolute right, but neither does ffmpeg as far as I can tell, at least in my environment. Unless I'm mistaken, it seems like it's the user's responsibility to make the ffmpeg binary available in the PATH regardless of which library you use, as I mentioned in the note:

"Both ffmpeg dependencies rely on the ffmpeg binary being globally available via the PATH env variable."

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jantic jantic Awaiting requested review from jantic jantic is a code owner

@alexandrevicenzi alexandrevicenzi Awaiting requested review from alexandrevicenzi alexandrevicenzi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dependencies in requirements.txt have module conflicts.
3 participants
@PixelOmen @snynisada @jantic