diff mbox series

[FFmpeg-devel] avcodec/libx264: don't define X264_API_IMPORTS when compiling static

Message ID pull.29.ffstaging.FFmpeg.1653000752870.ffmpegagent@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/libx264: don't define X264_API_IMPORTS when compiling static | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

softworkz May 19, 2022, 10:52 p.m. UTC
From: softworkz <softworkz@hotmail.com>

The definition of X264_API_IMPORTS is required for shared linking
(when MSVC is used) but it must not be defined in case of static
builds as is stated in x264.h:

https://code.videolan.org/videolan/x264/-/blob/
bfc87b7a330f75f5c9a21e56081e4b20344f139e/x264.h#L63-67

This commit adds a check for the definition of _LIB which indicates
static linking.

Signed-off-by: softworkz <softworkz@hotmail.com>
---
    avcodec/libx264: don't define X264_API_IMPORTS when compiling static
    
    The definition of X264_API_IMPORTS is required for shared linking (when
    MSVC is used) but it must not be defined in case of static builds as is
    stated in x264.h:
    
    https://code.videolan.org/videolan/x264/-/blob/bfc87b7a330f75f5c9a21e56081e4b20344f139e/x264.h#L63-67

Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-29%2Fsoftworkz%2Fsubmit_x264_api_imports-v1
Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-29/softworkz/submit_x264_api_imports-v1
Pull-Request: https://github.com/ffstaging/FFmpeg/pull/29

 libavcodec/libx264.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 41a558fea06cc0a23b8d2d0dfb03ef6a25cf5100

Comments

Derek Buitenhuis May 20, 2022, 8:49 a.m. UTC | #1
On 5/19/2022 11:52 PM, softworkz wrote:
> This commit adds a check for the definition of _LIB which indicates
> static linking.

Doesn't this indiate that FFmpeg is being compiled statically, and not
necessarily that x264 is? Googling also seems to indicate that this
definition is no longer available on newer MSVC versions.

- Derek
Soft Works May 20, 2022, 9:36 a.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Derek Buitenhuis
> Sent: Friday, May 20, 2022 10:50 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
> X264_API_IMPORTS when compiling static
> 
> On 5/19/2022 11:52 PM, softworkz wrote:
> > This commit adds a check for the definition of _LIB which indicates
> > static linking.
> 

> Googling also seems to indicate that this
> definition is no longer available on newer MSVC versions.

Probably we have read the same article on SO ;-)
But it's not true.

When creating a "Static Library" project in Visual Studio 2019, the 
_LIB macro is defined. When creating a dll project, then it is
not defined (instead there is _USRDLL and _WINDLL)

Though, building ffmpeg with the VS project system is not the 
officially supported way for compiling ffmpeg with MSVC, which is 
performing the build on MSYS2/MinGW from which it is calling the 
cl.exe and link.exe binaries directly.

In that case, no _LIB macro is defined which means that this 
commit doesn't affect the official MSVC build method.


> Doesn't this indicate that FFmpeg is being compiled statically, and not
> necessarily that x264 is? 

Correct. Or to be precise, it indicates that libavcodec is compiled 
statically. 

But the thing is:

  1. At this point we are in the world of the VS project system
     (Only)

  2. When - in this case - you would want to link the ffmpeg libs
     statically but x264 as dll, then you'll need to configure this
     specifically for that. Part of this would be to
     define "X264_API_IMPORTS" - but in the project properties,
     not in this code file.

In other words: this is a narrow-scoped fix that doesn't 
affect default ffmpeg build behavior with MSVC. 

The underlying issue most likely applies to the default MSVC
build method as well, but that's out of my scope because I 
don't use it that way and I can't test that.

Thanks,
softworkz
Timo Rothenpieler May 20, 2022, 10:18 a.m. UTC | #3
On 20/05/2022 00:52, softworkz wrote:
> From: softworkz <softworkz@hotmail.com>
> 
> The definition of X264_API_IMPORTS is required for shared linking
> (when MSVC is used) but it must not be defined in case of static
> builds as is stated in x264.h:

This doesn't seem right. It's about shared or static linking of libx264 
itself, not ffmpeg.
The correct flag should come via pkg-config at configure time.
Soft Works May 20, 2022, 10:39 a.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Timo
> Rothenpieler
> Sent: Friday, May 20, 2022 12:18 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
> X264_API_IMPORTS when compiling static
> 
> On 20/05/2022 00:52, softworkz wrote:
> > From: softworkz <softworkz@hotmail.com>
> >
> > The definition of X264_API_IMPORTS is required for shared linking
> > (when MSVC is used) but it must not be defined in case of static
> > builds as is stated in x264.h:
> 
> This doesn't seem right. It's about shared or static linking of
> libx264
> itself, not ffmpeg.

How about some custom macro like DISABLE_X264_API_IMPORTS that one
can set when desired?

In that case there wouldn't be any logical irritation.


> The correct flag should come via pkg-config at configure time.


There has been a patch which does that, but it didn't go anywhere:

https://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html

That's why I wanted something straight and simple which doesn't 
hurt anybody.



Thanks,
softworkz
Timo Rothenpieler May 20, 2022, 11:37 a.m. UTC | #5
On 20/05/2022 12:39, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Timo
>> Rothenpieler
>> Sent: Friday, May 20, 2022 12:18 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
>> X264_API_IMPORTS when compiling static
>>
>> On 20/05/2022 00:52, softworkz wrote:
>>> From: softworkz <softworkz@hotmail.com>
>>>
>>> The definition of X264_API_IMPORTS is required for shared linking
>>> (when MSVC is used) but it must not be defined in case of static
>>> builds as is stated in x264.h:
>>
>> This doesn't seem right. It's about shared or static linking of
>> libx264
>> itself, not ffmpeg.
> 
> How about some custom macro like DISABLE_X264_API_IMPORTS that one
> can set when desired?
> 
> In that case there wouldn't be any logical irritation.
> 

I'm still quite confused what the actual issue here is.
Countless libraries ffmpeg depends on need those kind of macros to set 
the correct function import preamble.
Why does x264 need special treatment? It correctly sets the desired flag 
via its pkg-config file.

Is this some "pkg-config does not exist with msvc" thing?
Soft Works May 20, 2022, 12:07 p.m. UTC | #6
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Timo
> Rothenpieler
> Sent: Friday, May 20, 2022 1:38 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
> X264_API_IMPORTS when compiling static
> 
> On 20/05/2022 12:39, Soft Works wrote:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Timo
> >> Rothenpieler
> >> Sent: Friday, May 20, 2022 12:18 PM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
> >> X264_API_IMPORTS when compiling static
> >>
> >> On 20/05/2022 00:52, softworkz wrote:
> >>> From: softworkz <softworkz@hotmail.com>
> >>>
> >>> The definition of X264_API_IMPORTS is required for shared linking
> >>> (when MSVC is used) but it must not be defined in case of static
> >>> builds as is stated in x264.h:
> >>
> >> This doesn't seem right. It's about shared or static linking of
> >> libx264
> >> itself, not ffmpeg.
> >
> > How about some custom macro like DISABLE_X264_API_IMPORTS that one
> > can set when desired?
> >
> > In that case there wouldn't be any logical irritation.
> >
> 
> I'm still quite confused what the actual issue here is.
> Countless libraries ffmpeg depends on need those kind of macros to set
> the correct function import preamble.
> Why does x264 need special treatment? It correctly sets the desired
> flag
> via its pkg-config file.

The current code is 

#if defined(_MSC_VER)
#define X264_API_IMPORTS 1
#endif

Which means that this macro is always set then building with MSVC.
But the macro may only be set when linking to x264.dll, not 
when linking statically to libx264.
(pkg-config can't do anything about that)


This problem was introduced by this change in libx264:

https://code.videolan.org/videolan/x264/-/commit/a615f027ed172e2dd5380e736d487aa858a0c4ff#98b74dd0a8bf575bfdf90bbccf5142a555f06d4f_56_69


Previously, they had this line

#define X264_API __declspec(dllimport)

Which handled the situation automatically by checking whether
it's linked as dll or static lib.

But after that change, you are required to set this 
yourself (as a consumer).

But ffmpeg has no proper condition to set this only when
linking to x264.dll. That's why the current code is
essentially wrong:

#if defined(_MSC_VER)
#define X264_API_IMPORTS 1
#endif

And besides that, I don't think that those things belong into
a code file.


> Is this some "pkg-config does not exist with msvc" thing?


The "official" way of building ffmpeg with MSVC is to run
configure and make from MSYS2/MinGW which then only calls
cl.exe and link.exe from a Visual Studio installation.
In this case pkg-config is used.


I'm working with regular Visual Studio projects, though.
Even dependencies like libx264 are compiled in their own
VS projects.
There's no MSYS2, no make, no pkg-conf involved.


I _think_ that just nobody has ever tried to link libx264
statically when compiling in the "official way", which is
probably rarely used anyway and even more rare that somebody
would bother to link with x264 and once again even more rare
that the one would on top of that decide to link to libx264
statically. That's my guess why nobody has complained about
this during the past 3 years.


Kind regards,
softworkz
Derek Buitenhuis May 20, 2022, 12:49 p.m. UTC | #7
On 5/20/2022 1:07 PM, Soft Works wrote:
> I'm working with regular Visual Studio projects, though.
> Even dependencies like libx264 are compiled in their own
> VS projects.
> There's no MSYS2, no make, no pkg-conf involved.

Things should not be added to FFmpeg in support of
non-standard build systems.

As per Thilo's explanation, I think this is a more appropraite fix:

    http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html

Matt sent that last October, but it seems to have been missed.


- Derek
Derek Buitenhuis May 20, 2022, 12:51 p.m. UTC | #8
On 5/20/2022 1:49 PM, Derek Buitenhuis wrote:
> As per Thilo's explanation, I think this is a more appropraite fix:

Apologies, I meant to type *Timo*.

- Derek
Soft Works May 20, 2022, 12:54 p.m. UTC | #9
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Derek Buitenhuis
> Sent: Friday, May 20, 2022 2:51 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
> X264_API_IMPORTS when compiling static
> 
> On 5/20/2022 1:49 PM, Derek Buitenhuis wrote:
> > As per Thilo's explanation, I think this is a more appropraite fix:
> 
> Apologies, I meant to type *Timo*.

Still wrong. Because I had posted that link.

softworkz
Derek Buitenhuis May 20, 2022, 1 p.m. UTC | #10
On 5/20/2022 1:54 PM, Soft Works wrote:
> Still wrong. Because I had posted that link.

No, not wrong. I was alluding to: http://ffmpeg.org/pipermail/ffmpeg-devel/2022-May/296605.html - and not the link.

In any case, thanks for the needlessly aggressive email.

- Derek
Soft Works May 20, 2022, 1:06 p.m. UTC | #11
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Derek Buitenhuis
> Sent: Friday, May 20, 2022 3:00 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
> X264_API_IMPORTS when compiling static
> 
> On 5/20/2022 1:54 PM, Soft Works wrote:
> > Still wrong. Because I had posted that link.
> 
> No, not wrong. I was alluding to: http://ffmpeg.org/pipermail/ffmpeg-
> devel/2022-May/296605.html - and not the link.
> 
> In any case, thanks for the needlessly aggressive email.

I didn't mean to be aggressive. But you posted the exact link
that I had posted 2 messages before:


> As per Thilo's explanation, I think this is a more appropraite fix:
> 
>     http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html

You say you think this is a more appropriate fix "as per Timo's explanation"
because of the following message:

> No, not wrong. I was alluding to: http://ffmpeg.org/pipermail/ffmpeg-
> devel/2022-May/296605.html - and not the link.


...where Timo said "This doesn't seem right"?


I'm not sure but I think this doesn't seem right, :-)

Kind regards,
softworkz
Derek Buitenhuis May 20, 2022, 1:13 p.m. UTC | #12
On 5/20/2022 2:06 PM, Soft Works wrote:
>> As per Thilo's explanation, I think this is a more appropraite fix:
>>
>>     http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html
> 
> You say you think this is a more appropriate fix "as per Timo's explanation"
> because of the following message:
> 
>> No, not wrong. I was alluding to: http://ffmpeg.org/pipermail/ffmpeg-
>> devel/2022-May/296605.html - and not the link.
> 
> 
> ...where Timo said "This doesn't seem right"?

The fix he suggests, using pkg-config, is what the patch I linked does.

I don't know why it is important who is 'right' here, so I'll conclude replying,
as it is rather unpleasant.

- Derek
Soft Works May 20, 2022, 1:14 p.m. UTC | #13
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Derek Buitenhuis
> Sent: Friday, May 20, 2022 2:50 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
> X264_API_IMPORTS when compiling static
> 
> On 5/20/2022 1:07 PM, Soft Works wrote:
> > I'm working with regular Visual Studio projects, though.
> > Even dependencies like libx264 are compiled in their own
> > VS projects.
> > There's no MSYS2, no make, no pkg-conf involved.
> 
> Things should not be added to FFmpeg in support of
> non-standard build systems.
> 
> As per Thilo's explanation, I think this is a more appropraite fix:
> 
>     http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html
> 
> Matt sent that last October, but it seems to have been missed.

Actually I had talked to Matt about it and he gave me permission to 
rebase and resubmit his patch. (due to lack of time)

But here comes my dilemma: I can't submit something which I'm not 
working with usually and where I don't have sufficient experience 
to be confident that it's all well.

There have ben several discussions about it last year, last one 
with Michael in December IIRC. 
I wouldn't be able to go through defending and evolving this patch,
that's why I wanted to do something very very basic and simple.


Kind regards,
softworkz
Soft Works May 20, 2022, 1:24 p.m. UTC | #14
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Derek Buitenhuis
> Sent: Friday, May 20, 2022 3:13 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/libx264: don't define
> X264_API_IMPORTS when compiling static
> 
> On 5/20/2022 2:06 PM, Soft Works wrote:
> >> As per Thilo's explanation, I think this is a more appropraite fix:
> >>
> >>     http://ffmpeg.org/pipermail/ffmpeg-devel/2021-
> October/287426.html
> >
> > You say you think this is a more appropriate fix "as per Timo's
> explanation"
> > because of the following message:
> >
> >> No, not wrong. I was alluding to:
> http://ffmpeg.org/pipermail/ffmpeg-
> >> devel/2022-May/296605.html - and not the link.
> >
> >
> > ...where Timo said "This doesn't seem right"?
> 
> The fix he suggests, using pkg-config, is what the patch I linked
> does.
> 
> I don't know why it is important who is 'right' here, so I'll conclude
> replying,
> as it is rather unpleasant.

Let me be honest: you caught me with this:

> Things should not be added to FFmpeg in support of
> non-standard build systems.

So many adaptions have been made over the years for whatever kind of
platforms and builds to work, but as soon as it's about MS, even
making a super-trivial macro definition configurable is already
too much and it's "non-standard"...

Though, I apologize for my slightly overdriven reaction.

sw
diff mbox series

Patch

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 4ce3791ae8..2304bbb774 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -37,7 +37,7 @@ 
 #include "atsc_a53.h"
 #include "sei.h"
 
-#if defined(_MSC_VER)
+#if defined(_MSC_VER) && !defined(_LIB)
 #define X264_API_IMPORTS 1
 #endif