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

Add audiofile.has_video() #153

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Add audiofile.has_video() #153

merged 6 commits into from
Jul 26, 2024

Conversation

hagenw
Copy link
Member

@hagenw hagenw commented Jul 25, 2024

Add audiofile.has_video() to check if a given media file contains video.

In audbcards we need to inspect media files if they contain video or not. One approach is to use mediainfo for this. As audiofile relies already on mediainfo, I thought it would be a good fit to integrate that function here.

image

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.0%. Comparing base (a8a130b) to head (5d83477).

Additional details and impacted files
Files Coverage Δ
audiofile/__init__.py 100.0% <100.0%> (ø)
audiofile/core/info.py 100.0% <100.0%> (ø)

@schruefer
Copy link
Member

Hey, thanks for the update! I think it's a great extension.

My only question is how we should handle it if a file does not exist.

If we have an non-existing ["wav", "flac", "mp3", "ogg"] file we get an result

>>> has_video('no_file.wav')
False

whereas it raises an Error for all other datatypes.

>>> has_video('no_file.mp4')
  File "<stdin>", line 1, in <module>
  File "/home/usr/audiofile/audiofile/core/info.py", line 213, in has_video
    video_format = run(cmd)
  File "/home/usr/audiofile/audiofile/core/utils.py", line 86, in run
    out = subprocess.check_output(shell_command, stderr=subprocess.STDOUT)
  File "/usr/lib/python3.8/subprocess.py", line 415, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/usr/lib/python3.8/subprocess.py", line 516, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['mediainfo', '--Inform=Video;%Format%', 'no_file.mp4']' returned non-zero exit status 1.

I am also not getting the binary_missing_error, but also "mediainfo cannot be found. Please make sure it is installed" is a little bit none saying to me.

Do you think it makes sense to include an os.path.isfile(file) at the beginning?

@hagenw
Copy link
Member Author

hagenw commented Jul 26, 2024

Thanks for testing this out.
I figured out, that we also have a mismatch between the reported error and the documented error for other functions as well and I created #154.

For audiofile.has_video(), I decided to stay with the current approach for wav, mp3, ogg, flac files, as the functions should be as fast as possible and checking if a file exists takes time. I updated the docstring accordingly:

image

and added a test for it.

Now we get:

>>> audiofile.has_video('no_file.wav')
False
>>> audiofile.has_video('no_file.mp4')
...
RuntimeError: 'no_file.mp4' does not exist.

Regarding your other comment. When mediainfo is missing I get a meaningful error message:

$ sudo mv /usr/bin/mediainfo /usr/bin/mediainfo.bak
$ touch empty.mp4
$ python -c "import audiofile; audiofile.has_video('empty.mp4')"
...
FileNotFoundError: mediainfo cannot be found.
Please make sure it is installed.
For further instructions visit: https://audeering.github.io/audiofile/installation.html

@schruefer
Copy link
Member

Thanks for the updates!

Regarding your other comment. When mediainfo is missing I get a meaningful error message:

Yes, this is fine. I meant, that it is no meaningful error message if the file is missing.

Look, good to me.

@hagenw hagenw merged commit 027e21b into main Jul 26, 2024
34 checks passed
@hagenw hagenw deleted the has-video branch July 26, 2024 14:13
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.

2 participants