[FFmpeg-devel] [PATCH]Do not require pkg-config for libx265

[Clément Bœsch]

On Mon, May 26, 2014 at 11:46:43AM +0200, Carl Eugen Hoyos wrote:
> On Monday 26 May 2014 11:02:32 am Clément Bœsch wrote:
> > On Mon, May 26, 2014 at 10:42:47AM +0200, Carl Eugen Hoyos wrote:
> >
> > > As HEVC gets more common, this is starting to hit users.
> 
> > x265 doesn't require -lstdc++ unless static.
> 
> I think I don't understand...
> (We add it for example for libgme although it is not "required".)
> 

If it is required additionally to pkg-config then that's probably because
of a missing entry in the .pc, the upstream should be notified about that.
Maybe I'm missing something obvious here though.

> > And if you have want a static build you actually need -lm and 
> > -lrt as well. At least on my system/configuration. 
> 
> True, new patch attached.
> 

Except that's probably specific to my system and x265 build.

> > It may also differs as x265 continue to be developed.
> 
> Yes, and if I remember correctly, we have to write patches 
> anyway when they update.
> 

Well not if you rely on pkg-config, that's the point: the .pc file is
supposed to contain the dependencies that can vary accross versions,
systems and configurations.

> >     [~]☭ pkg-config --libs x265
> >     -lx265
> >     [~]☭ pkg-config --static --libs x265
> >     -lx265 -lstdc++ -lm -lrt
> >
> > Not relying on pkg-config will break this.
> >
> > So, what's the problem with pkg-config requirement?
> 
> To the best of my knowledge, it only works if you know 
> the right incantation (which FFmpeg's configure cannot 
> tell you).
> 

The right incantation? What do you mean?

> > It's 100kb tool available on every system.
> 
> Available? Possibly
> Installed? No
> 

Then we just need a message such as "--enable-libx265 was enabled but
pkg-config is not available for the library detection. You need to install
it in order to get libx265."

> > I think what you want instead is a configure script that warn 
> > for the non presence of pkg-config when an external library 
> > is enabled. We have such warning IIRC, but way too late.
> 
> The warning cannot help users, they don't (imo cannot) know 
> which environment variable they have to set.

They don't have to set anything, we just require them to install
pkg-config and pkg-config will deal with the dependencies for them. If
you are afraid of lacking visibility on what happens during detection, we
could increase the verbosity of the pkg config checks. Like...
"[libfoobar detection] pkg-config --libs "x265 < 0.9" returned "Requested
'x265 < 0.9' but version of x265 is 1.0" and compilation test returned:
...
libfoobar could not be detected." or the flags returned by the tool, etc

> --extra-cflags and --extra-ldflags (which are usually not 
> required for the default case) are much more straight-forward 
> (and we can document them).

Just like we don't have a --path to configure PATH, I don't see why we
should require users to go down to the compilation and link flags instead
of configuring the PKG_CONFIG_PATH. We can document that as well.

> 
> Imo, none of the problems that you see can be solved by 
> pkg-config, some of them are caused by it. I honestly don't 
> understand why we make the user's life more difficult than 
> necessary.

So well, it's all about:

  PKG_CONFIG_PATH=/usr/local/lib/pkgconfig ./configure --enable-foobar

vs:

  ./configure --extra-cflags='-I/usr/local/include' --extra-ldflags='-L/usr/local/lib'

I'm not sure how "more difficult" the first one is over the second one.

Also, for static builds, the second one is not enough, you have to
hardcode all the extra libraries that libfoobar needs. The user don't know
them. They can of course use pkg-config to figure it out, but our
configure could do that for them. If there is a bug in these flags, it's a
hell for the user to get a clue about where is the issue.

And since the second solution is not relying on pkg-config, we probably
have the hardcoded links flags wrong (or useless to the best) in various
situations.

With pkg-config we move the responsibility out of the build script: if
it's broken, then it's probably that the upstream project did set the flag
wrongly in the .pc file.

> 
> Carl Eugen

> diff --git a/Changelog b/Changelog
> index 3d416c4..d52f4ea 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -26,6 +26,7 @@ version <next>:
>  - native Opus decoder
>  - display matrix export and rotation api
>  - WebVTT encoder
> +- Do not require pkg-config for x265
>  
>  
>  version 2.2:
> diff --git a/configure b/configure
> index fae2f77..6f326ee 100755
> --- a/configure
> +++ b/configure
> @@ -4695,7 +4695,7 @@ enabled libwebp           && require_pkg_config libwebp webp/encode.h WebPGetEnc
>  enabled libx264           && require libx264 x264.h x264_encoder_encode -lx264 &&
>                               { check_cpp_condition x264.h "X264_BUILD >= 118" ||
>                                 die "ERROR: libx264 must be installed and version must be >= 0.118."; }
> -enabled libx265           && require_pkg_config x265 x265.h x265_encoder_encode &&
> +enabled libx265           && require libx265 x265.h x265_encoder_encode -lx265 -lstdc++ -lm -lrt &&

So are you sure librt is always a requirement?

>                               { check_cpp_condition x265.h "X265_BUILD >= 13" ||
>                                 die "ERROR: libx265 version must be >= 13."; }
>  enabled libxavs           && require libxavs xavs.h xavs_encoder_encode -lxavs


-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140526/6d4d7b0d/attachment.asc>