diff mbox series

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

Message ID pull.29.v2.ffstaging.FFmpeg.1653060204841.ffmpegagent@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avcodec/libx264: allow to disable definition of X264_API_IMPORTS macro | 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 20, 2022, 3:23 p.m. UTC
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(-)


base-commit: 41a558fea06cc0a23b8d2d0dfb03ef6a25cf5100

Comments

Derek Buitenhuis May 20, 2022, 4:22 p.m. UTC | #1
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
Hendrik Leppkes May 20, 2022, 4:34 p.m. UTC | #2
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
Soft Works May 20, 2022, 4:37 p.m. UTC | #3
> -----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
Derek Buitenhuis May 20, 2022, 4:47 p.m. UTC | #4
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
Martin Storsjö May 20, 2022, 4:51 p.m. UTC | #5
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
Derek Buitenhuis May 20, 2022, 4:55 p.m. UTC | #6
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
Soft Works May 20, 2022, 5:47 p.m. UTC | #7
> -----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
diff mbox series

Patch

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