Skip to content

Add const to input_fmt #335

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

Merged
merged 1 commit into from
Aug 5, 2022
Merged

Conversation

jordemort
Copy link
Contributor

Hey folks, ran into a compile error building this against ffmpeg 5.0 - the compiler really wanted the input_fmt to be const.

@naushir
Copy link
Collaborator

naushir commented Aug 5, 2022

This is a bit of an annoying one. Our current ffmpeg build v4.3 requires a non-const value to avformat_open_input:

int avformat_open_input(AVFormatContext **ps, const char *url, ff_const59 AVInputFormat *fmt, AVDictionary **options);

so this change will break existing builds. But as you noted, for ffmpeg v5.0 and later this will have to change to a const.

Unfortunately, I don't think we can take this change as-is yet as we are not ready to move to ffmpeg v5.0 in our releases just yet.

@naushir
Copy link
Collaborator

naushir commented Aug 5, 2022

You can see the compile error from the CI build here.

@@ -129,7 +129,7 @@ void LibAvEncoder::initVideoCodec(VideoOptions const *options, StreamInfo const

void LibAvEncoder::initAudioInCodec(VideoOptions const *options, StreamInfo const &info)
{
AVInputFormat *input_fmt = (AVInputFormat *)av_find_input_format("pulse");
const AVInputFormat *input_fmt = (AVInputFormat *)av_find_input_format("pulse");

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps things would work for all versions if you changed const to ff_const59?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not - see avformat.h:

    /**
     * The ff_const59 define is not part of the public API and will
     * be removed without further warning.
     */

Copy link
Collaborator

Choose a reason for hiding this comment

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

You may be able to use the FF_API_AVIOFORMAT define, It's not clear what exactly that is meant to be used for, but might work for our problem here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds like FF_API_AVIOFORMAT is also private, re: https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg81099.html

Could you add a comment stating that, much like the FF_API_ defines, this ff_const59 define is not part of the public API and will eventually disappear without warning?

It seems like LIBAVUTIL_VERSION_MAJOR is intended for public consumption tho, so I've given that a shot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good. Once the checks have passed, I'll merge this in. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's correct but it passes your CI and also builds with 5.0 over here :)

@naushir
Copy link
Collaborator

naushir commented Aug 5, 2022

Actually, would you be able to re-submit this as a single patch instead of 2 commits? Would be tidier in the history.

The ffmpeg API changed in 5.0 to make a bunch of additional things
const. This patch attempts to make things compile cleanly againt both
ffmpeg 5.0 and earlier versions.
@jordemort
Copy link
Contributor Author

Actually, would you be able to re-submit this as a single patch instead of 2 commits? Would be tidier in the history.

Squashed down to one commit, I tried to write a better commit message for you too

@naushir
Copy link
Collaborator

naushir commented Aug 5, 2022

Great, I'll merge this as soon as the tests have completed.

@naushir naushir merged commit 40acce3 into raspberrypi:main Aug 5, 2022
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