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

ffmpeg version query #4254

Merged
merged 5 commits into from
Aug 6, 2021
Merged

ffmpeg version query #4254

merged 5 commits into from
Aug 6, 2021

Conversation

vmoens
Copy link
Contributor

@vmoens vmoens commented Aug 5, 2021

Closes #4214

setup.py Outdated
stdout=subprocess.PIPE).stdout.decode('utf-8')
ffmpeg_version = ffmpeg_version.split('version')[-1].split()[0]
ffmpeg_version_str = str(subprocess.check_output(["ffmpeg", "-version"]))
ffmpeg_version = re.split(r"ffmpeg\ version\ |\.|\ |-", ffmpeg_version_str)[1:3]
Copy link
Member

@NicolasHug NicolasHug Aug 5, 2021

Choose a reason for hiding this comment

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

Thanks @vmoens !

Do we need to escape the spaces? i.e. could this just be ffmpeg version |\.| |-?

Also, let's add a small description for future-us, something like

# this splits on both dots and spaces as the output format differs across versions / platforms

Copy link
Member

Choose a reason for hiding this comment

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

@vmoens do we still need to espace the spaces? Other than that, LGTM :)

Copy link
Contributor Author

@vmoens vmoens Aug 6, 2021

Choose a reason for hiding this comment

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

I thought i corrected that, but you're right there are a couple remaining

@vmoens vmoens changed the title WIP Ffmpeg version query ffmpeg version query Aug 5, 2021
@vmoens vmoens marked this pull request as ready for review August 5, 2021 09:54
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

LGTM modulo minor escaping question above

@vmoens vmoens merged commit f88985b into pytorch:master Aug 6, 2021
@github-actions
Copy link

github-actions bot commented Aug 6, 2021

Hey @vmoens!

You merged this PR, but no labels were added.

facebook-github-bot pushed a commit that referenced this pull request Aug 20, 2021
Summary:
* string manipulation for ffmpeg version retrieval

* ffmpeg version 4.2 control

* commenting solution

* removing all escapes for spaces

Reviewed By: NicolasHug

Differential Revision: D30417202

fbshipit-source-id: 1e685d810eae1d1887b06586c8142e87ee4a1b4b

Co-authored-by: Vincent Moens <vmoens@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Torchvision Python Setup Script Does not handle FFMPEG Version Properly
3 participants