f2e-spec icon indicating copy to clipboard operation
f2e-spec copied to clipboard

Fix compatibility with ffmpeg 8

Open lfos opened this issue 3 months ago • 14 comments

lfos avatar Sep 25 '25 18:09 lfos

The avcodec_close() function, previously used to close an AVCodecContext in FFmpeg, was deprecated and subsequently removed from the FFmpeg API. This removal occurred with commit 0d48da2db around February 9, 2024.

The functionality provided by avcodec_close() is now handled automatically by avcodec_free_context(). When you allocate an AVCodecContext using avcodec_alloc_context3(), you should free it using avcodec_free_context() when it's no longer needed. This function now handles the necessary cleanup and closing of the codec context.

lfos avatar Sep 25 '25 18:09 lfos

Thanks for the quick review!

This was deprecated just in ffmpeg7, and we support several older versions. Accordingly, this should be dependent on the ffmpeg version currently being used.

It looks like it has actually been unnecessary for a long time; see, e.g., the commit message of 2ef6dab. Let me know if your understanding is different.

I would also like to include ffmpeg8 support for the macos bundler in here. I can do it if necessary, but the code in the bundler for this is nor particularly complex and you can probably fix that yourself as well, check

https://github.com/performous/performous/blob/d5278e3264e87b4a0d2815e1b67f0fd672e9305c/osx-utils/macos-bundler.py#L95-L119

Sure, just added some changes to the commit; PTAL and let me know if anything else is needed.

lfos avatar Sep 25 '25 20:09 lfos

Thanks for the quick review!

This was deprecated just in ffmpeg7, and we support several older versions. Accordingly, this should be dependent on the ffmpeg version currently being used.

It looks like it has actually been unnecessary for a long time; see, e.g., the commit message of 2ef6dab. Let me know if your understanding is different.

I would also like to include ffmpeg8 support for the macos bundler in here. I can do it if necessary, but the code in the bundler for this is nor particularly complex and you can probably fix that yourself as well, check https://github.com/performous/performous/blob/d5278e3264e87b4a0d2815e1b67f0fd672e9305c/osx-utils/macos-bundler.py#L95-L119

Sure, just added some changes to the commit; PTAL and let me know if anything else is needed.

That commit is ancient, but it appears it hadn't been merged until recently. If you look at avcodec.h even in ffmpeg 6.0, you will notice that is missing.

Lord-Kamina avatar Sep 26 '25 17:09 Lord-Kamina

That commit is ancient, but it appears it hadn't been merged until recently. If you look at avcodec.h even in ffmpeg 6.0, you will notice that is missing.

It was merged in 2016 and appears in the link you provided: https://github.com/FFmpeg/FFmpeg/blob/release/6.0/libavcodec/avcodec.h#L2457-L2460

lfos avatar Sep 26 '25 18:09 lfos

That commit is ancient, but it appears it hadn't been merged until recently. If you look at avcodec.h even in ffmpeg 6.0, you will notice that is missing.

It was merged in 2016 and appears in the link you provided: https://github.com/FFmpeg/FFmpeg/blob/release/6.0/libavcodec/avcodec.h#L2457-L2460

Here's where it appears in ffmpeg 3.1: https://github.com/FFmpeg/FFmpeg/blob/release/3.1/libavcodec/avcodec.h#L4285-L4288

lfos avatar Sep 26 '25 18:09 lfos

That commit is ancient, but it appears it hadn't been merged until recently. If you look at avcodec.h even in ffmpeg 6.0, you will notice that is missing.

It was merged in 2016 and appears in the link you provided: FFmpeg/FFmpeg@release/6.0/libavcodec/avcodec.h#L2457-L2460

Here's where it appears in ffmpeg 3.1: FFmpeg/FFmpeg@release/3.1/libavcodec/avcodec.h#L4285-L4288

Brainfart. You're right then. You may nuke that line.

Lord-Kamina avatar Sep 26 '25 18:09 Lord-Kamina

@Lord-Kamina - If you need anything else from me here, please let me know!

lfos avatar Sep 27 '25 14:09 lfos

Nope. I was waiting for the CI to finish yesterday because I want to hijack the PR a bit with some CI fixes so hopefully we don't go around building gcc from source on the regular 🥲

Lord-Kamina avatar Sep 27 '25 14:09 Lord-Kamina

av_stream_get_side_data have to be replaced too with ffmpeg-8.0, no?

PPN-SD avatar Nov 04 '25 04:11 PPN-SD

av_stream_get_side_data have to be replaced too with ffmpeg-8.0, no?

Correct. I missed it because it was added more recently and wasn't in the latest release (which I had to patch in order to rebuild it for Arch Linux).

@Lord-Kamina - What's the status of this PR? Would you be interested in me adding the missing change? Has the CI issue been resolved in the meantime?

lfos avatar Nov 13 '25 14:11 lfos

Would you be interested in me adding the missing change?

As it stands, the PR does not resolve the compatibility with ffmpeg-8. Maybe @kt-- still has recent commits in mind to move forward on this point?

PPN-SD avatar Nov 13 '25 15:11 PPN-SD

Would you be interested in me adding the missing change?

As it stands, the PR does not resolve the compatibility with ffmpeg-8. Maybe @kt-- still has recent commits in mind to move forward on this point?

TL;DR I made a MR/PR: https://github.com/performous/performous/pull/1094

EDIT: After looking into it a bit ...

So av_stream_get_side_data() is deprecated in FFMPEG8, and needs to be replaced with av_packet_side_data_get(). Seems a relatively small patch. Conveniently, VLC has made that patch too: https://code.videolan.org/videolan/vlc/-/merge_requests/6658/diffs so there's a good example of what-to-do.

EDIT2: Here's a provisional patch. Only tested a little. Let me know if you want it added as a separate MR/PR

bool FFmpeg::readReplayGain(const AVStream *stream)
{
    bool gainFound = false;
    m_replayGainDecibels = 0.0;  // 0.0 indicates not defined
    m_replayGainFactor   = 1.0;

    // Only use Replay Gain if the option for normalisation is enabled
    if (stream != nullptr && config["audio/normalize_songs"].b() == true) {
#if (LIBAVFORMAT_VERSION_MAJOR) < 58
        int replay_gain_size;
        const AVReplayGain *replay_gain = (AVReplayGain *)av_stream_get_side_data(stream, AV_PKT_DATA_REPLAYGAIN, &replay_gain_size);
#else
        // av_packet_side_data_get() appears first in VERSION 58
        size_t replay_gain_size = 0;
        const AVReplayGain *replay_gain = nullptr;
        const AVCodecParameters *cp = stream->codecpar;
        const AVPacketSideData *psd = av_packet_side_data_get(cp->coded_side_data, cp->nb_coded_side_data, AV_PKT_DATA_REPLAYGAIN);
        if (psd) {
            replay_gain_size = psd->size;
            replay_gain = static_cast<const AVReplayGain*>(static_cast<const void *>(psd->data)); // double-cast ... is there a better way?
        }
#endif
        if (replay_gain_size > 0 && replay_gain != nullptr) {
            m_replayGainDecibels = static_cast<double>(replay_gain->track_gain);
            m_replayGainDecibels /= 100000.0;   // convert from microbels to decibels
            m_replayGainFactor = calculateLinearGain(m_replayGainDecibels);
            SpdLogger::debug(LogSystem::FFMPEG, "REPLAY Audio Gain is [{0:.2f}] dB, Linear Gain is [{1:.2f}]", m_replayGainDecibels, m_replayGainFactor);
            gainFound = true;
        }
        else {
            SpdLogger::debug(LogSystem::FFMPEG, "No REPLAY Audio Gain in file");
        }
    }
    return gainFound;
}

Scrolling through the Performous "record list", the debug log is confusing. I can see it reading the REPLAY_GAIN tag OK - so this means it is working. But then a few lines later in the log, it says "No REPLAY Audio Gain in file". I think this might be the silence clip, but I really don't know.

EDIT3: ~After adding some extra logging, it seems that in "record list screen" the first time we open the file, we do read the tags OK. But then the same file is opened again (not sure why) the gain is not found. In "performing" mode, the tags are always read OK. I'm not sure what's going on, but I'm reasonably certain this has always "worked" like this, and it is not caused by the change.~

EDIT4: PBKAC issue. It's correctly not finding the audio gain in the video file, because it's separate. I didn't notice the .m4a Vs .mp4 extension in the log. :facepalm:

SO: Patch is Good-to-Go.

kt-- avatar Nov 13 '25 21:11 kt--

Sorry about me disappearing. I wanted to take the opportunity to do some work on the CI but am being blocked by a PR upstream at macports. Likely won't wait for it to be merged but I honestly haven't had time/energy to work on the other bits that need fixing. Hope to do it soonish though.

Lord-Kamina avatar Nov 14 '25 02:11 Lord-Kamina