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

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

[softworkz]

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(-)

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
 

base-commit: 41a558fea06cc0a23b8d2d0dfb03ef6a25cf5100
-- 
ffmpeg-codebot
_______________________________________________
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".

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

[Derek Buitenhuis]

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
_______________________________________________
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".

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

[Soft Works]

> -----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
_______________________________________________
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".

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

[Timo Rothenpieler]

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.
_______________________________________________
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".

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

[Soft Works]

> -----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
_______________________________________________
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".

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

[Timo Rothenpieler]

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?

_______________________________________________
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".

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

[Soft Works]

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





_______________________________________________
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".

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

[Derek Buitenhuis]

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
_______________________________________________
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".

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

[Derek Buitenhuis]

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
_______________________________________________
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".

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

[Soft Works]

> -----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
_______________________________________________
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".

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

[Derek Buitenhuis]

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
_______________________________________________
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".

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

[Soft Works]

> -----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
_______________________________________________
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".

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

[Derek Buitenhuis]

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
_______________________________________________
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".

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

[Soft Works]

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




_______________________________________________
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".

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

[Soft Works]

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


_______________________________________________
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] [PATCH v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro

[softworkz]

From: softworkz <softworkz@hotmail.com>

When MSVC is used, the definition of X264_API_IMPORTS is
required for shared linking to libx264.dll, but it must
not be defined in case of statically linking to libx264.

Defining DISABLE_X264_API_IMPORTS allows to disable the
definition of X264_API_IMPORTS for those cases.

This has become necessary due to:

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

A better fix would eventually be this one:
http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html

But there has been disagreement and the issue stalled.

This patch is intended to be a stop-gap solution until
the mention fix will have been worked out.

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:
    
    v2 use custom macro for control, so there's no mechanism imposed and no
    change as long as that macro isn't explicitly defined

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

Range-diff vs v1:

 1:  640a17b84f ! 1:  bc86a3e903 avcodec/libx264: don't define X264_API_IMPORTS when compiling static
     @@ Metadata
      Author: softworkz <softworkz@hotmail.com>
      
       ## Commit message ##
     -    avcodec/libx264: don't define X264_API_IMPORTS when compiling static
     +    avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro
      
     -    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:
     +    When MSVC is used, the definition of X264_API_IMPORTS is
     +    required for shared linking to libx264.dll, but it must
     +    not be defined in case of statically linking to libx264.
     +
     +    Defining DISABLE_X264_API_IMPORTS allows to disable the
     +    definition of X264_API_IMPORTS for those cases.
     +
     +    This has become necessary due to:
      
          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.
     +    A better fix would eventually be this one:
     +    http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html
     +
     +    But there has been disagreement and the issue stalled.
     +
     +    This patch is intended to be a stop-gap solution until
     +    the mention fix will have been worked out.
      
          Signed-off-by: softworkz <softworkz@hotmail.com>
      
     @@ libavcodec/libx264.c
       #include "sei.h"
       
      -#if defined(_MSC_VER)
     -+#if defined(_MSC_VER) && !defined(_LIB)
     ++#if defined(_MSC_VER) && !defined(DISABLE_X264_API_IMPORTS)
       #define X264_API_IMPORTS 1
       #endif
       


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

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 4ce3791ae8..adeaf0f52f 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(DISABLE_X264_API_IMPORTS)
 #define X264_API_IMPORTS 1
 #endif
 

base-commit: 41a558fea06cc0a23b8d2d0dfb03ef6a25cf5100
-- 
ffmpeg-codebot
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro

[Derek Buitenhuis]

On 5/20/2022 4:23 PM, softworkz wrote:
> A better fix would eventually be this one:
> http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html
> 
> But there has been disagreement and the issue stalled.

I did not see a single person disagree with Matt's patch? If someone did,
I missed it.

- Derek
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro

[Hendrik Leppkes]

On Fri, May 20, 2022 at 6:23 PM Derek Buitenhuis
<derek.buitenhuis@gmail.com> wrote:
>
> On 5/20/2022 4:23 PM, softworkz wrote:
> > A better fix would eventually be this one:
> > http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html
> >
> > But there has been disagreement and the issue stalled.
>
> I did not see a single person disagree with Matt's patch? If someone did,
> I missed it.
>

Me neither, it just didn't seem to get any feedback and was
overlooked. Using pkg-config and letting CFLAGS control this through
the appropriate define x264 itself created for it is the far better
solution then inventing your own that noone knows about (soon we have
one for every library then, where does it end?).

- Hendrik
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Derek Buitenhuis
> Sent: Friday, May 20, 2022 6:23 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to
> disable definition of X264_API_IMPORTS macro
> 
> On 5/20/2022 4:23 PM, softworkz wrote:
> > A better fix would eventually be this one:
> > http://ffmpeg.org/pipermail/ffmpeg-devel/2021-October/287426.html
> >
> > But there has been disagreement and the issue stalled.
> 
> I did not see a single person disagree with Matt's patch? If someone
> did,
> I missed it.

It continued here:

https://master.gitmailbox.com/ffmpegdev/CAHVN4mhatDXNUe+=Z+TXfhyQB=aVpzOCPHZtHhRZT4iSmrPv9w@mail.gmail.com/

But if Matt's patch would be agreeable, then that would surely be 
the best outcome.

I can rebase and resubmit his patch if you would find it agreeable.

Thanks,
softworkz
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro

[Derek Buitenhuis]

On 5/20/2022 5:37 PM, Soft Works wrote:
> But if Matt's patch would be agreeable, then that would surely be 
> the best outcome.
> 
> I can rebase and resubmit his patch if you would find it agreeable.

Ah - that was not clear to me.

If Ubuntu LTS does indeed ship such an old x264, the fallback discussed there
may be indeed needed, but it's pretty simple to add. It does seem a tad silly
given that the define only affects Windows, but I get where Michael is coming
from.

I think Matt's patch + fallback for older versions seems reasonable, if, in fact
needed. Ideally we'd just drop support for older x264, though. Not sure which most
people would prefer.

- Derek
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro

[Martin Storsjö]

On Fri, 20 May 2022, Derek Buitenhuis wrote:

> On 5/20/2022 5:37 PM, Soft Works wrote:
>> But if Matt's patch would be agreeable, then that would surely be
>> the best outcome.
>>
>> I can rebase and resubmit his patch if you would find it agreeable.
>
> Ah - that was not clear to me.
>
> If Ubuntu LTS does indeed ship such an old x264, the fallback discussed there
> may be indeed needed, but it's pretty simple to add. It does seem a tad silly
> given that the define only affects Windows, but I get where Michael is coming
> from.
>
> I think Matt's patch + fallback for older versions seems reasonable, if, in fact
> needed. Ideally we'd just drop support for older x264, though. Not sure which most
> people would prefer.

Maybe just drop support for older versions when on Windows? That should 
cover those cases (even if ffmpeg is built with msvc but x264 with mingw, 
or vice versa) quite well, while still not bothering other platforms at 
all.

// Martin

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro

[Derek Buitenhuis]

On 5/20/2022 5:51 PM, Martin Storsjö wrote:
> Maybe just drop support for older versions when on Windows? That should 
> cover those cases (even if ffmpeg is built with msvc but x264 with mingw, 
> or vice versa) quite well, while still not bothering other platforms at 
> all.

Yeah, that seems reasonable to me.

- Derek
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Derek Buitenhuis
> Sent: Friday, May 20, 2022 6:55 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2] avcodec/libx264: allow to
> disable definition of X264_API_IMPORTS macro
> 
> On 5/20/2022 5:51 PM, Martin Storsjö wrote:
> > Maybe just drop support for older versions when on Windows? That
> should
> > cover those cases (even if ffmpeg is built with msvc but x264 with
> mingw,
> > or vice versa) quite well, while still not bothering other platforms
> at
> > all.
> 
> Yeah, that seems reasonable to me.

To me as well!

This is the code in configure after (fixing and) applying Matt's patch:


enabled libwebp           && {
    enabled libwebp_encoder      && require_pkg_config libwebp "libwebp >= 0.2.0" webp/encode.h WebPGetEncoderVersion
    enabled libwebp_anim_encoder && check_pkg_config libwebp_anim_encoder "libwebpmux >= 0.4.0" webp/mux.h WebPAnimEncoderOptionsInit; }
enabled libx264           && check_pkg_config libx264 x264 "stdint.h x264.h" x264_encoder_encode &&
                             require_cpp_condition libx264 x264.h "X264_BUILD >= 158"
enabled libx265           && require_pkg_config libx265 x265 x265.h x265_api_get &&
                             require_cpp_condition libx265 x265.h "X265_BUILD >= 70"


How would the check for this need to look like when we want to require version 158 only in 
case of msvc compilation?

And what should be the minimum for non-MSVC builds? Keep 118 like it was?

Thanks,
sw

_______________________________________________
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".