Skip to content

Conversation

@theogf
Copy link
Contributor

@theogf theogf commented Feb 17, 2024

As suggested in JuliaIO/FFMPEG.jl#59 (comment), this should test if VideoIO runs correctly with the new version o FFMPEG

@theogf
Copy link
Contributor Author

theogf commented Feb 17, 2024

This segfault does not look good 😅

@giordano
Copy link
Member

For the benefit of future (and present) readers, the segmentation fault @theogf refers to is

[6222] signal (11.2): Segmentation fault: 11
in expression starting at /Users/runner/work/VideoIO.jl/VideoIO.jl/src/VideoIO.jl:203
av_freep at /Users/runner/.julia/artifacts/112745bebf2bab1be3313a35fda52d857c5b4ab4/lib/libavutil.58.29.100.dylib (unknown line)
avcodec_parameters_from_context at /Users/runner/.julia/artifacts/112745bebf2bab1be3313a35fda52d857c5b4ab4/lib/libavcodec.60.31.102.dylib (unknown line)

In particular, it's happening to

VideoIO.save(fname, imgstack)
My loose understanding is that VideoIO use only CLI 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)

@giordano
Copy link
Member

Actually, I was looking for ccall in src/, but I just realised that there are thousands of ccalls in https://github.com/JuliaIO/VideoIO.jl/blob/0398aa6be81a1e637132a39136ebad94afd2f196/lib/libffmpeg.jl, I guess you'd need to update the generated wrappers, too. Side note, this means that VideoIO pretty much needs to specify FFMPEG_jll explicitly.

@giordano
Copy link
Member

@theogf bump

@codecov
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.72%. Comparing base (54ece49) to head (feceb08).

Files with missing lines Patch % Lines
src/avio.jl 50.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@theogf
Copy link
Contributor Author

theogf commented Feb 26, 2024

Actually, I was looking for ccall in src/, but I just realised that there are thousands of ccalls in https://github.com/JuliaIO/VideoIO.jl/blob/0398aa6be81a1e637132a39136ebad94afd2f196/lib/libffmpeg.jl, I guess you'd need to update the generated wrappers, too. Side note, this means that VideoIO pretty much needs to specify FFMPEG_jll explicitly.

I am not sure what you mean here.
One wrapper is for example

function avcodec_decode_subtitle2(avctx, sub, got_sub_ptr, avpkt)
    ccall((:avcodec_decode_subtitle2, libavcodec), Cint, (Ptr{AVCodecContext}, Ptr{AVSubtitle}, Ptr{Cint}, Ptr{AVPacket}), avctx, sub, got_sub_ptr, avpkt)
end

Is the idea to just make sure that libavcodec directly comes from FFMPEG_jll? Or by update you mean to make sure that all wrappers are compatible with 6.1?

@giordano
Copy link
Member

Or by update you mean to make sure that all wrappers are compatible with 6.1?

Yes, but you don't do that manually, but run the script in https://github.com/JuliaIO/VideoIO.jl/tree/42b25a21c0ec524d75bfead8209a0bfd61ca91b5/gen

@theogf
Copy link
Contributor Author

theogf commented Feb 26, 2024

So I generated libffmpeg.jl again, (also restricting the dependency to FFMPEG_jll). However I get the error

Failed to precompile VideoIO [d6d074c3-1acf-5d4c-9a43-ef38773959a2] to "/home/theo/.julia/compiled/v1.10/VideoIO/jl_nKnqkE".
ERROR: LoadError: UndefVarError: `AV_CHANNEL_LAYOUT_MASK` not defined

I located the corresponding C code here
https://github.com/FFmpeg/FFmpeg/blob/84e669167de9394e0de1bee0244f48ffac6f3826/libavutil/channel_layout.h#L378

But I don't know how to translate this to Julia? (and add it to prologue.jl)

@giordano
Copy link
Member

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.

@theogf
Copy link
Contributor Author

theogf commented Feb 26, 2024

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 AVChannelLayout which is defined just above.

@giordano
Copy link
Member

@Gnimuc do you have a clue how to deal with this? Clang.jl correctly defined the macros

VideoIO.jl/lib/libffmpeg.jl

Lines 22098 to 22104 in a9384d9

const AV_CHANNEL_LAYOUT_MONO = AV_CHANNEL_LAYOUT_MASK(1, AV_CH_LAYOUT_MONO)
const AV_CHANNEL_LAYOUT_STEREO = AV_CHANNEL_LAYOUT_MASK(2, AV_CH_LAYOUT_STEREO)
const AV_CHANNEL_LAYOUT_2POINT1 = AV_CHANNEL_LAYOUT_MASK(3, AV_CH_LAYOUT_2POINT1)
const AV_CHANNEL_LAYOUT_2_1 = AV_CHANNEL_LAYOUT_MASK(3, AV_CH_LAYOUT_2_1)
etc., but the macro AV_CHANNEL_LAYOUT_MASK has not been expanded.

@Gnimuc
Copy link

Gnimuc commented Feb 27, 2024

AV_CHANNEL_LAYOUT_MASK(nb, m) \
    { /* .order */ AV_CHANNEL_ORDER_NATIVE, \
      /* .nb_channels */  (nb), \
      /* .u.mask */ { m }, \
      /* .opaque */ NULL }

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 AV_CHANNEL* macro identifiers via the .toml file.

@giordano
Copy link
Member

if there is no direct use, it's safe to ignore all of the AV_CHANNEL* macro identifiers via the .toml file.

Good point, it looks like all the AV_CHANNEL_LAYOUT_* macros are only defined in the resulting file, but not used. Thanks!

@theogf
Copy link
Contributor Author

theogf commented Feb 29, 2024

Just a small update, adding the right regex worked, but now there are some deprecated functions I need to find replacement for, namely av_stream_get_r_frame_rate

@theogf
Copy link
Contributor Author

theogf commented Feb 29, 2024

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)
Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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

@IanButterworth
Copy link
Member

It'd be great to get this figured out

Comment on lines 20 to 21
FFMPEG = "0.3, 0.4"
FFMPEG_jll = "4.1"
FFMPEG = "0.3, 0.4, 1"
FFMPEG_jll = "6.1"
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IanButterworth
Copy link
Member

Sorry that this stalled so much.

I've merged but not released the FFMPEG.jl PR https://github.com/JuliaIO/FFMPEG.jl
and switched to master here, and things are looking good.

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?

@IanButterworth IanButterworth changed the title Test new version FFMPEG Update to FFMPEG_jll v6.1.1 Jul 26, 2025
Copy link
Member

@giordano giordano left a 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.

@IanButterworth
Copy link
Member

FFMPEG.jl v0.4.3 registered JuliaRegistries/General#135435

@IanButterworth IanButterworth marked this pull request as ready for review July 26, 2025 15:41
@IanButterworth IanButterworth changed the title Update to FFMPEG_jll v6.1.1 Update to FFMPEG_jll v6.1.1. Minor bump to v1.2.0 Jul 26, 2025
@IanButterworth IanButterworth merged commit 88e6411 into JuliaIO:master Jul 26, 2025
16 checks passed
@theogf theogf deleted the tgf/test-new-FFMPEG branch July 26, 2025 22:56
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.

4 participants