Skip to content
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

ffmpeg/4.4: Upgrade and properly handle component versioning #7563

Merged
merged 11 commits into from
Oct 8, 2021

Conversation

jpilet
Copy link
Contributor

@jpilet jpilet commented Oct 6, 2021

This merge request upgrades ffmpeg/4.4 and adds support for component versioning.
ffmpeg is composed of several libraries. The release page shows versions of all libraries the ffmpeg package contain.

This MR proposes to propagate those versions to the corresponding conan components, so that consumers that check for components version work without modification. I came to this when working on installing gstreamer with conan (I will open a serparate MR for this).


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

Properly version components.
@CLAassistant
Copy link

CLAassistant commented Oct 6, 2021

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@ghost
Copy link

ghost commented Oct 6, 2021

I detected other pull requests that are modifying ffmpeg/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@ghost ghost mentioned this pull request Oct 6, 2021
4 tasks
@jpilet
Copy link
Contributor Author

jpilet commented Oct 6, 2021

I am now reading versions from the source code. Nothing strange added in conandata.yml.

@conan-center-bot

This comment has been minimized.

@SSE4
Copy link
Contributor

SSE4 commented Oct 6, 2021

yes, this is useful. ffmpeg libraries might have different versions:

/usr/lib64/libavcodec.so.56
/usr/lib64/libavcodec.so.56.60.100
/usr/lib64/libavdevice.so.56
/usr/lib64/libavdevice.so.56.4.100
/usr/lib64/libavfilter.so.5
/usr/lib64/libavfilter.so.5.40.101
/usr/lib64/libavformat.so.56
/usr/lib64/libavformat.so.56.40.101
/usr/lib64/libavresample.so.2
/usr/lib64/libavresample.so.2.1.0
/usr/lib64/libavutil.so.54
/usr/lib64/libavutil.so.54.31.100
/usr/lib64/libpostproc.so.53
/usr/lib64/libpostproc.so.53.3.100
/usr/lib64/libswresample.so.1
/usr/lib64/libswresample.so.1.2.101
/usr/lib64/libswscale.so.3
/usr/lib64/libswscale.so.3.1.101

for the Mac failure, you need to add the following code below the line 529 (

self.cpp_info.components["avcodec"].frameworks.append("AudioToolbox")
):

self.cpp_info.components["avdevice"].frameworks.append("AudioToolbox")

@jpilet
Copy link
Contributor Author

jpilet commented Oct 6, 2021

Thanks for the tip! Let's see how it goes with your fix.

@SSE4
Copy link
Contributor

SSE4 commented Oct 6, 2021

Thanks for the tip! Let's see how it goes with your fix.

okay, it seems like I was wrong. the functions are from CoreAudio framework, not from the AudioToolbox: https://developer.apple.com/documentation/coreaudio/1422524-audioobjectgetpropertydata?language=objc
sorry for the inconvinience

recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@MartinDelille
Copy link
Contributor

Just for curiosity: what is the goal of filling this version information? I don't find documentation about component version.

@SSE4
Copy link
Contributor

SSE4 commented Oct 6, 2021

Just for curiosity: what is the goal of filling this version information? I don't find documentation about component version.

one of the scenarios is to support pkg-config version expressions, like:

pkg-config --libs "libavcodec >= 56"

I think meson build files in GStreamer might be using it under the hood: https://gitlab.freedesktop.org/gstreamer/gst-libav/-/blob/master/meson.build#L19

Co-authored-by: Martin Delille <martin@delille.org>
@jpilet
Copy link
Contributor Author

jpilet commented Oct 7, 2021

I think meson build files in GStreamer might be using it under the hood: https://gitlab.freedesktop.org/gstreamer/gst-libav/-/blob/master/meson.build#L19

this is exactly my use case :-)

@conan-center-bot

This comment has been minimized.

@jpilet
Copy link
Contributor Author

jpilet commented Oct 7, 2021

OK, I checked, there's no option fore coreaudio. But it seems linked with AudioToolbox.
So I add the framework only if audiotoolbox is on. Let's see if it works like this.

https://github.com/FFmpeg/FFmpeg/blob/master/configure#L3437

@SSE4
Copy link
Contributor

SSE4 commented Oct 7, 2021

OK, I checked, there's no option fore coreaudio. But it seems linked with AudioToolbox. So I add the framework only if audiotoolbox is on. Let's see if it works like this.

https://github.com/FFmpeg/FFmpeg/blob/master/configure#L3437

yes, looks good!

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 requested a review from uilianries October 7, 2021 14:31
SSE4
SSE4 previously approved these changes Oct 7, 2021
MartinDelille
MartinDelille previously approved these changes Oct 7, 2021
recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
Comment on lines +364 to +365
for line in version_file:
match = re.search(pattern, line)
Copy link
Contributor

Choose a reason for hiding this comment

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

How big are the files? I am wondering if package_info runs more then once there might be an annoying performance issue...

See #4319

Copy link
Contributor

Choose a reason for hiding this comment

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

these are relatively small text files, about 50 lines:
https://github.com/FFmpeg/FFmpeg/blob/master/libavdevice/version.h

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 do not know conan well enough. I do not know what is best. Here are the solutions I have in mind:

  1. execute this code at package build time and create a file containing the versions.
    in package_info(), I could read this file
  2. I could simply cache the version, so that if package_info() is called multiple times for the same instance, the files are read just once.
  3. do both of these.
  4. keep it like this. Reading 766 lines from 9 files (~27kb total) in package_info() is OK.

@prince-chrismc @SSE4 what solution seems best?

Copy link
Contributor

Choose a reason for hiding this comment

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

keep it simple, no need for caching or creating an extra file for now :)

recipes/ffmpeg/all/conanfile.py Outdated Show resolved Hide resolved
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@jpilet jpilet dismissed stale reviews from MartinDelille and SSE4 via e6fe796 October 8, 2021 09:32
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@conan-center-bot

This comment has been minimized.

@MartinDelille
Copy link
Contributor

I had another question: why ffmpeg 4.4 doesn't have three digit in its versioning scheme (like 4.2.1 and 4.3.2) ?

@jpilet
Copy link
Contributor Author

jpilet commented Oct 8, 2021

I do not know. Maybe they think 4.4 looks better than 4.4.0?

@SSE4
Copy link
Contributor

SSE4 commented Oct 8, 2021

I had another question: why ffmpeg 4.4 doesn't have three digit in its versioning scheme (like 4.2.1 and 4.3.2) ?

it was always the case. they had 4.3, but not 4.3.0. and then added 4.3.1 and 4.3.2.

@conan-center-bot
Copy link
Collaborator

All green in build 10 (e6dbff200420479665a84e60865d105c49fb4e63):

  • ffmpeg/4.4@:
    All packages built successfully! (All logs)

  • ffmpeg/4.3.2@:
    All packages built successfully! (All logs)

  • ffmpeg/4.2.1@:
    All packages built successfully! (All logs)

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 7961155 into conan-io:master Oct 8, 2021
@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 22, 2022

@czoido @uilianries @SSE4

This trick doesn't work with new generator PkgConfigDeps. version attribute in cpp_info components is not documented, is it expected to be supported in conan v2? Should we use pkg_config_custom_content property instead (but I guess it would lead to 2 Version: x.x.x lines)?

conan-io/conan#10629

@jpilet jpilet deleted the ffmpeg-4.4 branch February 24, 2022 06:32
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.

8 participants