FFMPEG.jl icon indicating copy to clipboard operation
FFMPEG.jl copied to clipboard

Published 20 hours ago • verified publisher icon JuliaIO

Removal of `have_avcodec()` etc

Open IanButterworth opened this issue 5 years ago • 5 comments

What was the rationale for removing have_avcodec() etc. in https://github.com/JuliaIO/FFMPEG.jl/commit/a124e581f97196171d4064976a7dde8ceb4a2dcc#diff-25d902c24283ab8cfbac54dfa101ad31 ?

Is it supposed to be provided somewhere else now? It seems like it's breaking VideoIO

IanButterworth avatar Feb 24 '20 22:02 IanButterworth

@giordano - any reason? I believe the JLL provides it by default, so we don't really need them anymore?

asinghvi17 avatar Feb 24 '20 22:02 asinghvi17

No, they aren't... those were part of this package ;)

SimonDanisch avatar Feb 24 '20 22:02 SimonDanisch

I think this error is evidence that we need to be more certain that changes to FFMPEG.jl don't break downstream packages like VideoIO, Makie, Plots, before merging PRs.

But.. to be honest, it was rightly released as a breaking change version (especially given it's pre v1.0) so it's actually VideoIO's fault that this broke it (my fault given I think I wrote that compat line...)

Maybe we just need more eyes on a PR before it's merged here. (also there were some commits directly to master recently, which we shouldn't do)

I've corrected VideoIO's compat here, just testing https://github.com/JuliaIO/VideoIO.jl/pull/223

IanButterworth avatar Feb 24 '20 22:02 IanButterworth

What was the rationale for removing have_avcodec()

I removed those functions in #19 because I think they would all always return true with the JLL package. If you want to bring them back -- for example in case someone use Overrides.toml to not use the JLL libraries/executable -- feel free to do so.

I think this error is evidence that we need to be more certain that changes to FFMPEG.jl don't break downstream packages like VideoIO, Makie, Plots, before merging PRs.

I had opened the PR as draft and never marked it as ready to review, I'm not sure why it was merged at all :slightly_smiling_face:

giordano avatar Feb 24 '20 22:02 giordano

Removing them does actually totally make sense.

I've merged & tagged a release of VideoIO pre JLL that limits FFMPEG to 0.2

Also have this PR which allows in FFMPEG 0.3 with JLL and limits to julia 1.3+ https://github.com/JuliaIO/VideoIO.jl/pull/224 (that will likely be a minor version bump given the restriction on julia version)

We can wait to release JLL-based VideoIO until it's nice and tested

IanButterworth avatar Feb 24 '20 23:02 IanButterworth