| 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 |
| 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 |
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
> -----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
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.
> -----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
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?
> -----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
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
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
> -----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
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
> -----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
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
> -----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
> -----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 --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