-
Notifications
You must be signed in to change notification settings - Fork 50
Update to FFMPEG_jll v6.1.1. Minor bump to v1.2.0 #418
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
Conversation
Co-authored-by: Mosè Giordano <[email protected]>
|
This segfault does not look good 😅 |
|
For the benefit of future (and present) readers, the segmentation fault @theogf refers to is In particular, it's happening to Line 208 in 0398aa6
ffmpeg, so it shouldn't be too complicated to craft a reproducible example to report upstream for someone motivated to push for FFMPEG v6, otherwise we'd have to yank that version, if it's broken (and will also be stuck forever to v4 until someone else does that anyway)
|
|
Actually, I was looking for |
|
@theogf bump |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #418 +/- ##
==========================================
+ Coverage 78.50% 78.72% +0.21%
==========================================
Files 10 10
Lines 1247 1274 +27
==========================================
+ Hits 979 1003 +24
- Misses 268 271 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I am not sure what you mean here. Is the idea to just make sure that |
Yes, but you don't do that manually, but run the script in https://github.com/JuliaIO/VideoIO.jl/tree/42b25a21c0ec524d75bfead8209a0bfd61ca91b5/gen |
|
So I generated I located the corresponding C code here But I don't know how to translate this to Julia? (and add it to |
|
That's a macro, it doesn't really have a correspondence in julia since it's a source-code transformation, not part of the ABI of the library. I don't know what's going on there. |
Ah I think it's just some constructor for |
|
@Gnimuc do you have a clue how to deal with this? Lines 22098 to 22104 in a9384d9
AV_CHANNEL_LAYOUT_MASK has not been expanded.
|
unfortunately, Clang.jl doesn't support such "convoluted" macros. Honestly, I don't even know how to manually translate this function-like macro into Julia... if there is no direct use, it's safe to ignore all of the |
Good point, it looks like all the |
|
Just a small update, adding the right regex worked, but now there are some deprecated functions I need to find replacement for, namely |
|
Hopefully it should work now 😅 |
| time_base == 0 && error("No time base") | ||
| target_pts = seconds_to_timestamp(seconds, time_base) | ||
| frame_rate = convert(Rational, av_stream_get_r_frame_rate(stream)) | ||
| frame_rate = convert(Rational, stream.r_frame_rate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing so I can only assume that this is not the solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the correct change based on https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/
And the test failures could be rounding assumptions. Not looked deeper into it
Encoding with frame rate 29.5: Test Failed at /home/runner/work/VideoIO.jl/VideoIO.jl/test/writing.jl:196
Expression: parse(Float64, measured_dur_str[1]) == target_dur
Evaluated: 3.389831 == 3.39
Encoding with frame rate 29.5: Test Failed at /home/runner/work/VideoIO.jl/VideoIO.jl/test/writing.jl:214
Expression: parse(Float64, measured_dur_str[1]) == target_dur
Evaluated: 3.389831 == 3.39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're a bit far from rounding error? Maybe it's a difference of 1 frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is just correct
julia> 100 / 29.5
3.389830508474576
julia> 99 / 29.5
3.3559322033898304
julia> 101 / 29.5
3.4237288135593222
Perhaps ffmpeg -v error -show_entries format=duration -of default=noprint_wrappers=1:nokey=1 just returns more precision on newer versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a change to the full precision
|
It'd be great to get this figured out |
| FFMPEG = "0.3, 0.4" | ||
| FFMPEG_jll = "4.1" | ||
| FFMPEG = "0.3, 0.4, 1" | ||
| FFMPEG_jll = "6.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to directly depend on both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a93e069 to
f7f18a8
Compare
|
Sorry that this stalled so much. I've merged but not released the FFMPEG.jl PR https://github.com/JuliaIO/FFMPEG.jl We also can unskip an ARM test 🎉 I guess this can be a minor bump to v1.2.0, in case anyone wants to stay on the old FFMPEG. @giordano I think we can move forward, do you agree? |
giordano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move forward, do you agree?
Yes.
|
FFMPEG.jl v0.4.3 registered JuliaRegistries/General#135435 |
As suggested in JuliaIO/FFMPEG.jl#59 (comment), this should test if VideoIO runs correctly with the new version o FFMPEG