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 CI, fix or workaround build errors pre-FFmpeg 6.0 #187

Merged
merged 8 commits into from
Dec 7, 2024

Conversation

projectgus
Copy link
Contributor

I got a bit carried away here, hopefully it's of some use. :)

  1. Expand CI to run on FFmpeg 6.1 with Ubuntu 24.04, as well as the exsiting FFmpeg 4.4 on Ubuntu 22.04.
  2. Fix build errors against FFmpeg 4.4:
  • Restore the pre-struct ChannelLayout support that was replaced in Update for ffmpeg 6.0 #173, implementation is selected based on feature flags.
  • Add feature guards on some ChannelLayout functions so they work pre-6.0, disable some entirely where it looks like more work to support.
  • Added a lot of workarounds for const-ification of pointer arguments in FFmpeg 5.0. Not sure if this is the best way around this, but seemed like an OK approach.
  • Wrapped the chapter id in a type as it changed i32 -> i64 in 5.0
  • Additional workarounds for things added or changed >=5.0

All in separate commits.

I was able to run the transcode-x264 example under Ubuntu 22.04 (FFmpeg 4.4) but I have not done any additional testing. It's likely many other things are broken on earlier FFmpeg, but building is a first step to fixing. 😁

@projectgus
Copy link
Contributor Author

Couple more offers of help, if they're desirable:

  • I'd be happy to expand the CI to also run some of the examples against cached test files (i.e. Big Buck Bunny) and at least confirm they run without crashing.
  • I'd also be happy to submit a PR to remove compatibility for FFMPEG <4.0, as I'm guessing it probably also doesn't work

- Restore the channel layout implementation from before bdb9f76

- Some channel layout functions are kept, some newer ones currently
  disabled on older FFMPEG as need more work to implement.
The ChapterId argument changed from i32 to i64 in FFMPEG 5.0,
so expose this with a ChapterId alias.
A number of pointers were made 'const' in FFMPEG 5.0.

This is a fairly clunky way to handle these, maybe there's a better way?
Before the nb_inputs/nb_outputs fields, the array is NULL-terminated
and need to call a function to get the number of entries.
- time_base field missing
- duration field available as pkt_duration I think
  (the newer versions deprecate pkt_duration for duration)..
- ffmpeg 4.4 on Ubuntu 22.04 (Jammy)
- ffmpeg 6.1 on Ubuntu 24.04 (Noble)
@projectgus
Copy link
Contributor Author

Forgot this repo uses tabs not spaces. Have fixed the most obvious indentation fails, happy to go back and tabify the rest if it looks like this PR is going to be of use. 😁

@kornelski kornelski merged commit 9e1086a into meh:master Dec 7, 2024
2 checks passed
@kornelski
Copy link
Collaborator

Thanks for the PR!

BTW, Rust doesn't care whether you use *const or *mut, and C mostly doesn't care either. Unlike Rust's references, raw pointers don't really control mutability, and for just purpose of using an ABI, you can cast the pointers however you need.

@projectgus
Copy link
Contributor Author

Thanks for the PR!

No worries! Would it be of any use if I submit PRs for the other stuff i mentioned, and/or to use rustfmt?

Happy to do a bit of janitorial work but also don't want to be a pain. :)

BTW, Rust doesn't care whether you use *const or *mut, and C mostly doesn't care either. Unlike Rust's references, raw pointers don't really control mutability, and for just purpose of using an ABI, you can cast the pointers however you need.

Right, thanks for pointing this out. I'm very familiar with how this works in C but less confident with unsafe Rust. I was worried that a clippy lint might pop up saying "you don't need the cast_mut() here". But not only does this not seem to be a thing, I realise now that even if it was then it'd be a lot simpler to have a conditional cfg_attr() to ignore the lint or warning...

I'll submit another PR to shrink some of these back down to a plain .cast_mut() or similar.

@projectgus projectgus deleted the ci_fixes branch December 8, 2024 03:38
@kornelski
Copy link
Collaborator

When you're changing things, it's best to follow what https://lib.rs/crates/ffmpeg-next is doing to make it easier to copy patches and avoid redundant work.

Unfortunately all ffmpeg wrappers for Rust are cursed and have very unresponsive maintainers (I can only merge stuff here, but can't release it).

@projectgus
Copy link
Contributor Author

projectgus commented Dec 9, 2024

When you're changing things, it's best to follow what https://lib.rs/crates/ffmpeg-next is doing to make it easier to copy patches and avoid redundant work.

Unfortunately all ffmpeg wrappers for Rust are cursed and have very unresponsive maintainers (I can only merge stuff here, but can't release it).

Thanks for the pointers. I had missed that the zmwangx fork has been quite active of late, maybe I should switch focus to there as well (while noting it's also in maintenance mode).

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