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

Upgrade to ffmpeg 6.1 #1252

Merged
merged 2 commits into from
Feb 3, 2024
Merged

Upgrade to ffmpeg 6.1 #1252

merged 2 commits into from
Feb 3, 2024

Conversation

WyattBlue
Copy link
Member

No description provided.

@WyattBlue WyattBlue force-pushed the ffmpeg-6.1 branch 3 times, most recently from 5bec1be to 5cd3ac4 Compare December 27, 2023 07:24
@jlaine
Copy link
Member

jlaine commented Dec 27, 2023

This PR does not do what it says: it is also dropping all tests against FFmpeg 5.0, why?

@jlaine
Copy link
Member

jlaine commented Dec 27, 2023

Please do NOT merge until you have answered my question.

@WyattBlue
Copy link
Member Author

it is also dropping all tests against FFmpeg 5.0, why?

  1. Actions Minutes are not unlimited
  2. We don't need to test every minor release. ffmpeg is good at following semvar and we already should be relying on obscure implementation details
  3. ffmpeg 7.0 is coming out soon, and I assume when that happens, our support for the 5.x series will be dropped

@WyattBlue
Copy link
Member Author

This PR does not do what it says

This is an odd comment. This commit tests with 6.1 and will ship the ffmpeg 6.1 binaries instead of the 6.0 binaries when it's released on PyPI.
6.1 does set average_rate to 25/1 instead of null on empty streams now when it didn't before so I removed that assert. This might not be the best way to handle it, but I wanted to make sure nothing else was breaking.

@jlaine
Copy link
Member

jlaine commented Jan 7, 2024

This PR does not do what it says

This is an odd comment.

My point is that PRs and commit messages need to actually describe what they are doing: this one adds tests against FFmpeg 6.1 (good), but also drops tests against FFmpeg 5.0 - which we say we still support (not good). Either we still support FFmpeg 5.0, in which case we keep testing against it or we officially abandon support FFmpeg 5.0.

I'm not adverse to dropping FFmpeg 5.0 but this needs to be reflected both in the commit message and the documentation (docs/overview/installation.rst:).

FFmpeg minor versions can also introduce new APIs or deprecate old ones: I think this is exactly what happens with FFmpeg 5.1, I noticed we're getting compilation warnings specifically in FFmpeg 5.1 and up ‘channel_layout’ is deprecated.. but the replacement API doesn't exist in FFmpeg 5.0. So if we fix this deprecation warning we will be breaking FFmpeg 5.0 compatibility, but CI won't tell us that because we have stopped testing against 5.0!

@WyattBlue
Copy link
Member Author

Okay, I've restored all ffmpeg 5.0 and 5.1 tests.

@WyattBlue WyattBlue merged commit 365ddeb into main Feb 3, 2024
19 checks passed
@WyattBlue WyattBlue deleted the ffmpeg-6.1 branch February 3, 2024 18:50
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