diff mbox series

[FFmpeg-devel] configure: libvmaf requires pthreads

Message ID 20201112165657.13350-1-timo@rothenpieler.org
State New
Headers show
Series [FFmpeg-devel] configure: libvmaf requires pthreads
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Timo Rothenpieler Nov. 12, 2020, 4:56 p.m. UTC
Technically, libvmaf itself does not, but our filter does, and there is
no other sensible way to prevent a build with --enable-libvmaf from
succeeding while not actually enabling the filter.
---
 configure | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Limin Wang Nov. 13, 2020, 1:05 a.m. UTC | #1
On Thu, Nov 12, 2020 at 05:56:57PM +0100, Timo Rothenpieler wrote:
> Technically, libvmaf itself does not, but our filter does, and there is
> no other sensible way to prevent a build with --enable-libvmaf from
> succeeding while not actually enabling the filter.

If it's private filter, I think you it's better to add the pthread depends for your filter 
only, search for libvmaf_filter_deps

> ---
>  configure | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 51e43fbf66..21b8cae0c3 100755
> --- a/configure
> +++ b/configure
> @@ -6425,7 +6425,8 @@ enabled libtwolame        && require libtwolame twolame.h twolame_init -ltwolame
>  enabled libuavs3d         && require_pkg_config libuavs3d "uavs3d >= 1.1.41" uavs3d.h uavs3d_decode
>  enabled libv4l2           && require_pkg_config libv4l2 libv4l2 libv4l2.h v4l2_ioctl
>  enabled libvidstab        && require_pkg_config libvidstab "vidstab >= 0.98" vid.stab/libvidstab.h vsMotionDetectInit
> -enabled libvmaf           && require_pkg_config libvmaf "libvmaf >= 1.5.2" libvmaf.h compute_vmaf
> +enabled libvmaf           && require_pkg_config libvmaf "libvmaf >= 1.5.2" libvmaf.h compute_vmaf &&
> +                             { enabled pthreads || die "ERROR: libvmaf requires pthreads"; }
>  enabled libvo_amrwbenc    && require libvo_amrwbenc vo-amrwbenc/enc_if.h E_IF_init -lvo-amrwbenc
>  enabled libvorbis         && require_pkg_config libvorbis vorbis vorbis/codec.h vorbis_info_init &&
>                               require_pkg_config libvorbisenc vorbisenc vorbis/vorbisenc.h vorbis_encode_init
> -- 
> 2.25.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Timo Rothenpieler Nov. 13, 2020, 1:14 a.m. UTC | #2
On 13.11.2020 02:05, lance.lmwang@gmail.com wrote:
> On Thu, Nov 12, 2020 at 05:56:57PM +0100, Timo Rothenpieler wrote:
>> Technically, libvmaf itself does not, but our filter does, and there is
>> no other sensible way to prevent a build with --enable-libvmaf from
>> succeeding while not actually enabling the filter.
> 
> If it's private filter, I think you it's better to add the pthread depends for your filter
> only, search for libvmaf_filter_deps

The filter already depends on pthreads correctly.
This is about the issue where you can produce a build that is configured 
with --enable-libvmaf, but which does not have the libvmaf filter.

Happens for example on Win32, where w32threads are used by default.
Limin Wang Nov. 13, 2020, 1:23 a.m. UTC | #3
On Fri, Nov 13, 2020 at 02:14:05AM +0100, Timo Rothenpieler wrote:
> On 13.11.2020 02:05, lance.lmwang@gmail.com wrote:
> > On Thu, Nov 12, 2020 at 05:56:57PM +0100, Timo Rothenpieler wrote:
> > > Technically, libvmaf itself does not, but our filter does, and there is
> > > no other sensible way to prevent a build with --enable-libvmaf from
> > > succeeding while not actually enabling the filter.
> > 
> > If it's private filter, I think you it's better to add the pthread depends for your filter
> > only, search for libvmaf_filter_deps
> 
> The filter already depends on pthreads correctly.
> This is about the issue where you can produce a build that is configured
> with --enable-libvmaf, but which does not have the libvmaf filter.
> 
> Happens for example on Win32, where w32threads are used by default.

so it doesn't work with  --enable-pthreads also?

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Timo Rothenpieler Nov. 13, 2020, 12:20 p.m. UTC | #4
On 13.11.2020 02:23, lance.lmwang@gmail.com wrote:
> On Fri, Nov 13, 2020 at 02:14:05AM +0100, Timo Rothenpieler wrote:
>> On 13.11.2020 02:05, lance.lmwang@gmail.com wrote:
>>> On Thu, Nov 12, 2020 at 05:56:57PM +0100, Timo Rothenpieler wrote:
>>>> Technically, libvmaf itself does not, but our filter does, and there is
>>>> no other sensible way to prevent a build with --enable-libvmaf from
>>>> succeeding while not actually enabling the filter.
>>>
>>> If it's private filter, I think you it's better to add the pthread depends for your filter
>>> only, search for libvmaf_filter_deps
>>
>> The filter already depends on pthreads correctly.
>> This is about the issue where you can produce a build that is configured
>> with --enable-libvmaf, but which does not have the libvmaf filter.
>>
>> Happens for example on Win32, where w32threads are used by default.
> 
> so it doesn't work with  --enable-pthreads also?

Of course it does, but there is no indication for that to the user 
whatsoever.
And given that there is only that one consumer of --enable-libvmaf, the 
libvmaf filter, which also depends on pthreads, adding an error like 
this seems appropriate to inform users about the pthread requirement.
Limin Wang Nov. 13, 2020, 2:27 p.m. UTC | #5
On Fri, Nov 13, 2020 at 01:20:57PM +0100, Timo Rothenpieler wrote:
> On 13.11.2020 02:23, lance.lmwang@gmail.com wrote:
> > On Fri, Nov 13, 2020 at 02:14:05AM +0100, Timo Rothenpieler wrote:
> > > On 13.11.2020 02:05, lance.lmwang@gmail.com wrote:
> > > > On Thu, Nov 12, 2020 at 05:56:57PM +0100, Timo Rothenpieler wrote:
> > > > > Technically, libvmaf itself does not, but our filter does, and there is
> > > > > no other sensible way to prevent a build with --enable-libvmaf from
> > > > > succeeding while not actually enabling the filter.
> > > > 
> > > > If it's private filter, I think you it's better to add the pthread depends for your filter
> > > > only, search for libvmaf_filter_deps
> > > 
> > > The filter already depends on pthreads correctly.
> > > This is about the issue where you can produce a build that is configured
> > > with --enable-libvmaf, but which does not have the libvmaf filter.
> > > 
> > > Happens for example on Win32, where w32threads are used by default.
> > 
> > so it doesn't work with  --enable-pthreads also?
> 
> Of course it does, but there is no indication for that to the user
> whatsoever.
> And given that there is only that one consumer of --enable-libvmaf, the
> libvmaf filter, which also depends on pthreads, adding an error like this
> seems appropriate to inform users about the pthread requirement.

But libvmaf library itself does not depend on pthread, so if libvmaf filter
is used, it'll depend pthread by libvmaf_filter_deps. I'm not sure why the
deps doesn't working as expected as I can't test win32 system.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Nov. 13, 2020, 2:34 p.m. UTC | #6
On 11/13/2020 11:27 AM, lance.lmwang@gmail.com wrote:
> On Fri, Nov 13, 2020 at 01:20:57PM +0100, Timo Rothenpieler wrote:
>> On 13.11.2020 02:23, lance.lmwang@gmail.com wrote:
>>> On Fri, Nov 13, 2020 at 02:14:05AM +0100, Timo Rothenpieler wrote:
>>>> On 13.11.2020 02:05, lance.lmwang@gmail.com wrote:
>>>>> On Thu, Nov 12, 2020 at 05:56:57PM +0100, Timo Rothenpieler wrote:
>>>>>> Technically, libvmaf itself does not, but our filter does, and there is
>>>>>> no other sensible way to prevent a build with --enable-libvmaf from
>>>>>> succeeding while not actually enabling the filter.
>>>>>
>>>>> If it's private filter, I think you it's better to add the pthread depends for your filter
>>>>> only, search for libvmaf_filter_deps
>>>>
>>>> The filter already depends on pthreads correctly.
>>>> This is about the issue where you can produce a build that is configured
>>>> with --enable-libvmaf, but which does not have the libvmaf filter.
>>>>
>>>> Happens for example on Win32, where w32threads are used by default.
>>>
>>> so it doesn't work with  --enable-pthreads also?
>>
>> Of course it does, but there is no indication for that to the user
>> whatsoever.
>> And given that there is only that one consumer of --enable-libvmaf, the
>> libvmaf filter, which also depends on pthreads, adding an error like this
>> seems appropriate to inform users about the pthread requirement.
> 
> But libvmaf library itself does not depend on pthread, so if libvmaf filter
> is used, it'll depend pthread by libvmaf_filter_deps. I'm not sure why the
> deps doesn't working as expected as I can't test win32 system.

The point here is that, if a user configures with --enable-libvmaf but 
doesn't have pthreads, the library will be enabled but the filter will 
not. This results in a libavfilter binary that links to libvmaf for no 
reason, potentially bloating it if it was linked statically.

Since we have no other module using libvmaf, we can simply make libvmaf 
itself dependent on pthreads. This way --enable-libvmaf will fail if 
pthreads is not present, which is better than succeeding but ultimately 
not compiling the filter as the user clearly wanted by passing the above 
option.

> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Nicolas George Nov. 13, 2020, 2:40 p.m. UTC | #7
James Almer (12020-11-13):
> This results in a libavfilter binary that links to libvmaf for no reason,
> potentially bloating it if it was linked statically.

That is not how static linking works. Static linking takes in the
library exactly the object files required by the binary and other
object files, nothing more. We could have -lqt -lwebkit2gtk on the link
command, the linker would just observe that nothing in them is needed
and ignore them.

By the way, if the objections I got against merging the libraries (I
don't remember exactly from whom) were based on this kind of
misconception, it would be a good idea to revise them.

On the other hand, it makes a difference with dynamic linking: the
binary would be unable to run without the shared library present.

Regards,
Limin Wang Nov. 13, 2020, 2:44 p.m. UTC | #8
On Fri, Nov 13, 2020 at 11:34:09AM -0300, James Almer wrote:
> On 11/13/2020 11:27 AM, lance.lmwang@gmail.com wrote:
> > On Fri, Nov 13, 2020 at 01:20:57PM +0100, Timo Rothenpieler wrote:
> > > On 13.11.2020 02:23, lance.lmwang@gmail.com wrote:
> > > > On Fri, Nov 13, 2020 at 02:14:05AM +0100, Timo Rothenpieler wrote:
> > > > > On 13.11.2020 02:05, lance.lmwang@gmail.com wrote:
> > > > > > On Thu, Nov 12, 2020 at 05:56:57PM +0100, Timo Rothenpieler wrote:
> > > > > > > Technically, libvmaf itself does not, but our filter does, and there is
> > > > > > > no other sensible way to prevent a build with --enable-libvmaf from
> > > > > > > succeeding while not actually enabling the filter.
> > > > > > 
> > > > > > If it's private filter, I think you it's better to add the pthread depends for your filter
> > > > > > only, search for libvmaf_filter_deps
> > > > > 
> > > > > The filter already depends on pthreads correctly.
> > > > > This is about the issue where you can produce a build that is configured
> > > > > with --enable-libvmaf, but which does not have the libvmaf filter.
> > > > > 
> > > > > Happens for example on Win32, where w32threads are used by default.
> > > > 
> > > > so it doesn't work with  --enable-pthreads also?
> > > 
> > > Of course it does, but there is no indication for that to the user
> > > whatsoever.
> > > And given that there is only that one consumer of --enable-libvmaf, the
> > > libvmaf filter, which also depends on pthreads, adding an error like this
> > > seems appropriate to inform users about the pthread requirement.
> > 
> > But libvmaf library itself does not depend on pthread, so if libvmaf filter
> > is used, it'll depend pthread by libvmaf_filter_deps. I'm not sure why the
> > deps doesn't working as expected as I can't test win32 system.
> 
> The point here is that, if a user configures with --enable-libvmaf but
> doesn't have pthreads, the library will be enabled but the filter will not.
> This results in a libavfilter binary that links to libvmaf for no reason,
> potentially bloating it if it was linked statically.

This make senses, thanks for the explanation.

> 
> Since we have no other module using libvmaf, we can simply make libvmaf
> itself dependent on pthreads. This way --enable-libvmaf will fail if
> pthreads is not present, which is better than succeeding but ultimately not
> compiling the filter as the user clearly wanted by passing the above option.
> 
> > 
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Nov. 13, 2020, 4:43 p.m. UTC | #9
On 11/13/2020 11:40 AM, Nicolas George wrote:
> James Almer (12020-11-13):
>> This results in a libavfilter binary that links to libvmaf for no reason,
>> potentially bloating it if it was linked statically.
> 
> That is not how static linking works. Static linking takes in the
> library exactly the object files required by the binary and other
> object files, nothing more. We could have -lqt -lwebkit2gtk on the link
> command, the linker would just observe that nothing in them is needed
> and ignore them.

I thought that was -lto behavior.

> 
> By the way, if the objections I got against merging the libraries (I
> don't remember exactly from whom) were based on this kind of
> misconception, it would be a good idea to revise them.

Don't think they were.

> 
> On the other hand, it makes a difference with dynamic linking: the
> binary would be unable to run without the shared library present.
> 
> Regards,
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Nicolas George Nov. 13, 2020, 4:58 p.m. UTC | #10
James Almer (12020-11-13):
> I thought that was -lto behavior.

No, I am sure of what I am saying about classic static libraries. I do
not know link-time optimizations very well, but I suspect they relate to
optimizing withing a single object file.

> Don't think they were.

I strongly suspect some of them were.

Regards,
diff mbox series

Patch

diff --git a/configure b/configure
index 51e43fbf66..21b8cae0c3 100755
--- a/configure
+++ b/configure
@@ -6425,7 +6425,8 @@  enabled libtwolame        && require libtwolame twolame.h twolame_init -ltwolame
 enabled libuavs3d         && require_pkg_config libuavs3d "uavs3d >= 1.1.41" uavs3d.h uavs3d_decode
 enabled libv4l2           && require_pkg_config libv4l2 libv4l2 libv4l2.h v4l2_ioctl
 enabled libvidstab        && require_pkg_config libvidstab "vidstab >= 0.98" vid.stab/libvidstab.h vsMotionDetectInit
-enabled libvmaf           && require_pkg_config libvmaf "libvmaf >= 1.5.2" libvmaf.h compute_vmaf
+enabled libvmaf           && require_pkg_config libvmaf "libvmaf >= 1.5.2" libvmaf.h compute_vmaf &&
+                             { enabled pthreads || die "ERROR: libvmaf requires pthreads"; }
 enabled libvo_amrwbenc    && require libvo_amrwbenc vo-amrwbenc/enc_if.h E_IF_init -lvo-amrwbenc
 enabled libvorbis         && require_pkg_config libvorbis vorbis vorbis/codec.h vorbis_info_init &&
                              require_pkg_config libvorbisenc vorbisenc vorbis/vorbisenc.h vorbis_encode_init