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 ffmpeg_reader not selecting a default stream #2114

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bzczb
Copy link
Contributor

@bzczb bzczb commented Feb 14, 2024

fix #869 #899
ffmpeg, at least the version that's installed automatically with moviepy, sometimes comes up with no default stream for a given input file. This PR does nothing more than set a default stream automatically when none is specified, which is the cause of the missing "video_fps" issue.

Unfortunately I fixed this bug a long time ago in a working branch, and can't find the file that raised this problem in the first place. I think it was a ".wmv" or ".asf".

@coveralls
Copy link

coveralls commented Feb 14, 2024

Coverage Status

coverage: 81.835% (-0.07%) from 81.904%
when pulling 338335b on bzczb:i869-default-stream
into bc8d1a8 on Zulko:master.

@keikoro
Copy link
Collaborator

keikoro commented Mar 5, 2024

@bzczb Thanks for the submission – I don't have time to review its actual contents, but saw it didn't pass the formatting checks. Could you update the formatting?

@bzczb
Copy link
Contributor Author

bzczb commented Mar 9, 2024

@keikoro formatting fixed. Can't get pre-commit setup in CONTRIBUTING.md to work any which way. 3.6 venv fails when doing pip install -e, 3.12 venv fails because it's not 3.6.

@OsaAjani
Copy link
Collaborator

I would need a few tests added to show the proper working of this, the line 645 by itself make me doubt this work perfectly as intended.

@bzczb
Copy link
Contributor Author

bzczb commented Jan 10, 2025

I need to remake this PR to match latest master.

The line looks right to me, the intent is to select a default at the end instead of the middle, so the global_data variable has to be removed and new logic added.

@OsaAjani
Copy link
Collaborator

Oh, I missed the rewrite of return on parsing methods. I let you check what need to be updated for the new master and add some tests showing an example of this in work. Thanks a lot for your work !

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.

KeyError: 'video_fps'
4 participants