[FFmpeg-devel] [PATCH] lavc/libopenh264: Drop openh264 runtime version checks

[FFmpeg-devel] [PATCH] lavc/libopenh264: Drop openh264 runtime version checks

[Kalev Lember]

Years ago, openh264 releases often changed their ABI without changing
the library soname. To avoid running into ABI issues, a version check
was added to lavc libopenh264 code to error out at runtime in case the
build time and runtime openh264 versions don't match.

This should no longer be an issue with newer openh264 releases and we
can drop the runtime version check and rely on upstream doing the right
thing and bump the library soname if the ABI changes, similar to how
other libraries are consumed in ffmpeg.

Almost all major distributions now include openh264 and this means there
are more eyes on ABI changes and issues are discovered and reported
quickly. See e.g. https://github.com/cisco/openh264/issues/3564 where an
ABI issue was quickly discovered and fixed.

Relaxing the check allows downstream distributions to build ffmpeg
against e.g. openh264 2.3.1 and ship an update to ABI-compatible
openh264 2.4.0, without needing to coordinate a lock step update between
ffmpeg and openh264 (which can be difficult if openh264 is distributed
by Cisco and ffmpeg comes from the distro, such as is the case for
Fedora).

Signed-off-by: Kalev Lember <klember@redhat.com>
---
 libavcodec/libopenh264.c    | 15 ---------------
 libavcodec/libopenh264.h    |  2 --
 libavcodec/libopenh264dec.c |  4 ----
 libavcodec/libopenh264enc.c |  4 ----
 4 files changed, 25 deletions(-)

diff --git a/libavcodec/libopenh264.c b/libavcodec/libopenh264.c
index 0f6d28ed88..c80c85ea8b 100644
--- a/libavcodec/libopenh264.c
+++ b/libavcodec/libopenh264.c
@@ -46,18 +46,3 @@ void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg)
     int equiv_ffmpeg_log_level = libopenh264_to_ffmpeg_log_level(level);
     av_log(ctx, equiv_ffmpeg_log_level, "%s\n", msg);
 }
-
-int ff_libopenh264_check_version(void *logctx)
-{
-    // Mingw GCC < 4.7 on x86_32 uses an incorrect/buggy ABI for the WelsGetCodecVersion
-    // function (for functions returning larger structs), thus skip the check in those
-    // configurations.
-#if !defined(_WIN32) || !defined(__GNUC__) || !ARCH_X86_32 || AV_GCC_VERSION_AT_LEAST(4, 7)
-    OpenH264Version libver = WelsGetCodecVersion();
-    if (memcmp(&libver, &g_stCodecVersion, sizeof(libver))) {
-        av_log(logctx, AV_LOG_ERROR, "Incorrect library version loaded\n");
-        return AVERROR(EINVAL);
-    }
-#endif
-    return 0;
-}
diff --git a/libavcodec/libopenh264.h b/libavcodec/libopenh264.h
index dbb9c5d429..0b462d6fdc 100644
--- a/libavcodec/libopenh264.h
+++ b/libavcodec/libopenh264.h
@@ -34,6 +34,4 @@
 
 void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg);
 
-int ff_libopenh264_check_version(void *logctx);
-
 #endif /* AVCODEC_LIBOPENH264_H */
diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
index 7d650ae03e..b6a9bba2dc 100644
--- a/libavcodec/libopenh264dec.c
+++ b/libavcodec/libopenh264dec.c
@@ -52,13 +52,9 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
 {
     SVCContext *s = avctx->priv_data;
     SDecodingParam param = { 0 };
-    int err;
     int log_level;
     WelsTraceCallback callback_function;
 
-    if ((err = ff_libopenh264_check_version(avctx)) < 0)
-        return AVERROR_DECODER_NOT_FOUND;
-
     if (WelsCreateDecoder(&s->decoder)) {
         av_log(avctx, AV_LOG_ERROR, "Unable to create decoder\n");
         return AVERROR_UNKNOWN;
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index f518d0894e..6f231d22b2 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -110,14 +110,10 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
 {
     SVCContext *s = avctx->priv_data;
     SEncParamExt param = { 0 };
-    int err;
     int log_level;
     WelsTraceCallback callback_function;
     AVCPBProperties *props;
 
-    if ((err = ff_libopenh264_check_version(avctx)) < 0)
-        return AVERROR_ENCODER_NOT_FOUND;
-
     if (WelsCreateSVCEncoder(&s->encoder)) {
         av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
         return AVERROR_UNKNOWN;
-- 
2.43.0

_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Martin Storsjö]

Hi,

On Fri, 8 Dec 2023, Kalev Lember wrote:

> This should no longer be an issue with newer openh264 releases and we
> can drop the runtime version check and rely on upstream doing the right
> thing and bump the library soname if the ABI changes, similar to how
> other libraries are consumed in ffmpeg.
>
> Almost all major distributions now include openh264 and this means there
> are more eyes on ABI changes and issues are discovered and reported
> quickly. See e.g. https://github.com/cisco/openh264/issues/3564 where an
> ABI issue was quickly discovered and fixed.
>
> Relaxing the check allows downstream distributions to build ffmpeg
> against e.g. openh264 2.3.1 and ship an update to ABI-compatible
> openh264 2.4.0, without needing to coordinate a lock step update between
> ffmpeg and openh264 (which can be difficult if openh264 is distributed
> by Cisco and ffmpeg comes from the distro, such as is the case for
> Fedora).
>
> Signed-off-by: Kalev Lember <klember@redhat.com>
> ---
> libavcodec/libopenh264.c    | 15 ---------------
> libavcodec/libopenh264.h    |  2 --
> libavcodec/libopenh264dec.c |  4 ----
> libavcodec/libopenh264enc.c |  4 ----
> 4 files changed, 25 deletions(-)

I guess this seems reasonable to me, so LGTM.

The version check would be more relevant if we would be dlopening the 
OpenH264 library (which pretty much is what one has to do in order to take 
advantage of the patent offer from Cisco), but the libavcodec wrapper 
doesn't dlopen this library (and doing the dlopen dance for ffmpeg is kind 
of pointless, there's more of a point to it if individual apps want to 
integrate OpenH264 directly), so this should indeed be fine.

// 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] lavc/libopenh264: Drop openh264 runtime version checks

[Kalev Lember]

On Fri, Dec 8, 2023 at 9:39 AM Martin Storsjö <martin@martin.st> wrote:

> I guess this seems reasonable to me, so LGTM.
>
> The version check would be more relevant if we would be dlopening the
> OpenH264 library (which pretty much is what one has to do in order to take
> advantage of the patent offer from Cisco), but the libavcodec wrapper
> doesn't dlopen this library (and doing the dlopen dance for ffmpeg is kind
> of pointless, there's more of a point to it if individual apps want to
> integrate OpenH264 directly), so this should indeed be fine.
>

Thanks!

As for dlopening, I think instead of version checks, it would make sense to
try to dlsym() all of the actual required symbols, and error out in init if
anything is missing. That should make it all super flexible and resilient
to e.g. struct size changes that would normally be an ABI change.

In Fedora, we are planning on changing things up a bit and starting to
build packages that link with openh264 against the "noopenh264" stub
implementation and replacing it at runtime with the actual openh264 library
downloaded directly from Cisco. Flathub flatpak runtimes already use that
approach and it seems to work well there. This should hopefully let us take
advantage of the Cisco patent grant and fit well in the build system
architecture that we have.

https://gitlab.com/freedesktop-sdk/noopenh264

-- 
Kalev
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Martin Storsjö]

On Fri, 8 Dec 2023, Kalev Lember wrote:

> As for dlopening, I think instead of version checks, it would make sense to
> try to dlsym() all of the actual required symbols, and error out in init if
> anything is missing. That should make it all super flexible and resilient to
> e.g. struct size changes that would normally be an ABI change.

How would that help, if e.g. the SEncParamExt struct in svc_encode_init 
would change layout/size - which part would notice that change?

> In Fedora, we are planning on changing things up a bit and starting to build
> packages that link with openh264 against the "noopenh264" stub
> implementation and replacing it at runtime with the actual openh264 library
> downloaded directly from Cisco. Flathub flatpak runtimes already use that
> approach and it seems to work well there. This should hopefully let us take
> advantage of the Cisco patent grant and fit well in the build system
> architecture that we have.

Ah, interesting, that sounds like a reasonable way to take advantage of 
that patent grant without having everybody to do the dlopening.

// 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] lavc/libopenh264: Drop openh264 runtime version checks

[Kalev Lember]

On Fri, Dec 8, 2023 at 1:00 PM Martin Storsjö <martin@martin.st> wrote:

> On Fri, 8 Dec 2023, Kalev Lember wrote:
>
> > As for dlopening, I think instead of version checks, it would make sense
> to
> > try to dlsym() all of the actual required symbols, and error out in init
> if
> > anything is missing. That should make it all super flexible and
> resilient to
> > e.g. struct size changes that would normally be an ABI change.
>
> How would that help, if e.g. the SEncParamExt struct in svc_encode_init
> would change layout/size - which part would notice that change?
>

Ah, hm, I didn't think this through apparently :) This would indeed still
be an issue.

I guess maybe dlopening the soname version that matches the headers (e.g.
libopenh264.so.7) would work then? With the expectation that upstream bumps
soname whenever the struct layout/size changes.

-- 
Kalev
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Martin Storsjö]

On Fri, 8 Dec 2023, Kalev Lember wrote:

> 
> On Fri, Dec 8, 2023 at 1:00 PM Martin Storsjö <martin@martin.st> wrote:
>       On Fri, 8 Dec 2023, Kalev Lember wrote:
>
>       > As for dlopening, I think instead of version checks, it would
>       make sense to
>       > try to dlsym() all of the actual required symbols, and error
>       out in init if
>       > anything is missing. That should make it all super flexible
>       and resilient to
>       > e.g. struct size changes that would normally be an ABI change.
>
>       How would that help, if e.g. the SEncParamExt struct in
>       svc_encode_init
>       would change layout/size - which part would notice that change?
> 
> 
> Ah, hm, I didn't think this through apparently :) This would indeed still be
> an issue.
> 
> I guess maybe dlopening the soname version that matches the headers (e.g.
> libopenh264.so.7) would work then? With the expectation that upstream bumps
> soname whenever the struct layout/size changes.

Yeah I guess that would work, it's all up to who has the responsibility 
for keeping it in sync. At some point, I envisioned that one could run it 
with e.g. -openh264_lib /path/to/my/libopenh264.so, and in such a 
scenario, we definitely would need some sort of reassurance that we're 
using the right ABI.

But anyway, good that we agree how this works. And this wasn't relevant 
for our current way of linking it here anyway, so the patch still is ok 
(and can be pushed later if nobody else has further opinions on it).

// 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] lavc/libopenh264: Drop openh264 runtime version checks

[Neal Gompa]

On Fri, Dec 8, 2023 at 3:16 AM Kalev Lember <klember@redhat.com> wrote:
>
> Years ago, openh264 releases often changed their ABI without changing
> the library soname. To avoid running into ABI issues, a version check
> was added to lavc libopenh264 code to error out at runtime in case the
> build time and runtime openh264 versions don't match.
>
> This should no longer be an issue with newer openh264 releases and we
> can drop the runtime version check and rely on upstream doing the right
> thing and bump the library soname if the ABI changes, similar to how
> other libraries are consumed in ffmpeg.
>
> Almost all major distributions now include openh264 and this means there
> are more eyes on ABI changes and issues are discovered and reported
> quickly. See e.g. https://github.com/cisco/openh264/issues/3564 where an
> ABI issue was quickly discovered and fixed.
>
> Relaxing the check allows downstream distributions to build ffmpeg
> against e.g. openh264 2.3.1 and ship an update to ABI-compatible
> openh264 2.4.0, without needing to coordinate a lock step update between
> ffmpeg and openh264 (which can be difficult if openh264 is distributed
> by Cisco and ffmpeg comes from the distro, such as is the case for
> Fedora).
>
> Signed-off-by: Kalev Lember <klember@redhat.com>
> ---
>  libavcodec/libopenh264.c    | 15 ---------------
>  libavcodec/libopenh264.h    |  2 --
>  libavcodec/libopenh264dec.c |  4 ----
>  libavcodec/libopenh264enc.c |  4 ----
>  4 files changed, 25 deletions(-)
>
> diff --git a/libavcodec/libopenh264.c b/libavcodec/libopenh264.c
> index 0f6d28ed88..c80c85ea8b 100644
> --- a/libavcodec/libopenh264.c
> +++ b/libavcodec/libopenh264.c
> @@ -46,18 +46,3 @@ void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg)
>      int equiv_ffmpeg_log_level = libopenh264_to_ffmpeg_log_level(level);
>      av_log(ctx, equiv_ffmpeg_log_level, "%s\n", msg);
>  }
> -
> -int ff_libopenh264_check_version(void *logctx)
> -{
> -    // Mingw GCC < 4.7 on x86_32 uses an incorrect/buggy ABI for the WelsGetCodecVersion
> -    // function (for functions returning larger structs), thus skip the check in those
> -    // configurations.
> -#if !defined(_WIN32) || !defined(__GNUC__) || !ARCH_X86_32 || AV_GCC_VERSION_AT_LEAST(4, 7)
> -    OpenH264Version libver = WelsGetCodecVersion();
> -    if (memcmp(&libver, &g_stCodecVersion, sizeof(libver))) {
> -        av_log(logctx, AV_LOG_ERROR, "Incorrect library version loaded\n");
> -        return AVERROR(EINVAL);
> -    }
> -#endif
> -    return 0;
> -}
> diff --git a/libavcodec/libopenh264.h b/libavcodec/libopenh264.h
> index dbb9c5d429..0b462d6fdc 100644
> --- a/libavcodec/libopenh264.h
> +++ b/libavcodec/libopenh264.h
> @@ -34,6 +34,4 @@
>
>  void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg);
>
> -int ff_libopenh264_check_version(void *logctx);
> -
>  #endif /* AVCODEC_LIBOPENH264_H */
> diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
> index 7d650ae03e..b6a9bba2dc 100644
> --- a/libavcodec/libopenh264dec.c
> +++ b/libavcodec/libopenh264dec.c
> @@ -52,13 +52,9 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
>  {
>      SVCContext *s = avctx->priv_data;
>      SDecodingParam param = { 0 };
> -    int err;
>      int log_level;
>      WelsTraceCallback callback_function;
>
> -    if ((err = ff_libopenh264_check_version(avctx)) < 0)
> -        return AVERROR_DECODER_NOT_FOUND;
> -
>      if (WelsCreateDecoder(&s->decoder)) {
>          av_log(avctx, AV_LOG_ERROR, "Unable to create decoder\n");
>          return AVERROR_UNKNOWN;
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index f518d0894e..6f231d22b2 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -110,14 +110,10 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
>  {
>      SVCContext *s = avctx->priv_data;
>      SEncParamExt param = { 0 };
> -    int err;
>      int log_level;
>      WelsTraceCallback callback_function;
>      AVCPBProperties *props;
>
> -    if ((err = ff_libopenh264_check_version(avctx)) < 0)
> -        return AVERROR_ENCODER_NOT_FOUND;
> -
>      if (WelsCreateSVCEncoder(&s->encoder)) {
>          av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
>          return AVERROR_UNKNOWN;
> --
> 2.43.0
>

Thank you for this. It looks good to me.

Reviewed-by: Neal Gompa <ngompa13@gmail.com>



-- 
真実はいつも一つ！/ Always, there's only one truth!
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[James Almer]

On 12/8/2023 5:15 AM, Kalev Lember wrote:
> Years ago, openh264 releases often changed their ABI without changing
> the library soname. To avoid running into ABI issues, a version check
> was added to lavc libopenh264 code to error out at runtime in case the
> build time and runtime openh264 versions don't match.
> 
> This should no longer be an issue with newer openh264 releases and we
> can drop the runtime version check and rely on upstream doing the right
> thing and bump the library soname if the ABI changes, similar to how
> other libraries are consumed in ffmpeg.

Does the configure check ensure a new enough openh264 version is the 
minimum supported?

> 
> Almost all major distributions now include openh264 and this means there
> are more eyes on ABI changes and issues are discovered and reported
> quickly. See e.g. https://github.com/cisco/openh264/issues/3564 where an
> ABI issue was quickly discovered and fixed.
> 
> Relaxing the check allows downstream distributions to build ffmpeg
> against e.g. openh264 2.3.1 and ship an update to ABI-compatible
> openh264 2.4.0, without needing to coordinate a lock step update between
> ffmpeg and openh264 (which can be difficult if openh264 is distributed
> by Cisco and ffmpeg comes from the distro, such as is the case for
> Fedora).
> 
> Signed-off-by: Kalev Lember <klember@redhat.com>
> ---
>   libavcodec/libopenh264.c    | 15 ---------------
>   libavcodec/libopenh264.h    |  2 --
>   libavcodec/libopenh264dec.c |  4 ----
>   libavcodec/libopenh264enc.c |  4 ----
>   4 files changed, 25 deletions(-)
> 
> diff --git a/libavcodec/libopenh264.c b/libavcodec/libopenh264.c
> index 0f6d28ed88..c80c85ea8b 100644
> --- a/libavcodec/libopenh264.c
> +++ b/libavcodec/libopenh264.c
> @@ -46,18 +46,3 @@ void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg)
>       int equiv_ffmpeg_log_level = libopenh264_to_ffmpeg_log_level(level);
>       av_log(ctx, equiv_ffmpeg_log_level, "%s\n", msg);
>   }
> -
> -int ff_libopenh264_check_version(void *logctx)
> -{
> -    // Mingw GCC < 4.7 on x86_32 uses an incorrect/buggy ABI for the WelsGetCodecVersion
> -    // function (for functions returning larger structs), thus skip the check in those
> -    // configurations.
> -#if !defined(_WIN32) || !defined(__GNUC__) || !ARCH_X86_32 || AV_GCC_VERSION_AT_LEAST(4, 7)
> -    OpenH264Version libver = WelsGetCodecVersion();
> -    if (memcmp(&libver, &g_stCodecVersion, sizeof(libver))) {
> -        av_log(logctx, AV_LOG_ERROR, "Incorrect library version loaded\n");
> -        return AVERROR(EINVAL);
> -    }
> -#endif
> -    return 0;
> -}
> diff --git a/libavcodec/libopenh264.h b/libavcodec/libopenh264.h
> index dbb9c5d429..0b462d6fdc 100644
> --- a/libavcodec/libopenh264.h
> +++ b/libavcodec/libopenh264.h
> @@ -34,6 +34,4 @@
>   
>   void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg);
>   
> -int ff_libopenh264_check_version(void *logctx);
> -
>   #endif /* AVCODEC_LIBOPENH264_H */
> diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
> index 7d650ae03e..b6a9bba2dc 100644
> --- a/libavcodec/libopenh264dec.c
> +++ b/libavcodec/libopenh264dec.c
> @@ -52,13 +52,9 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
>   {
>       SVCContext *s = avctx->priv_data;
>       SDecodingParam param = { 0 };
> -    int err;
>       int log_level;
>       WelsTraceCallback callback_function;
>   
> -    if ((err = ff_libopenh264_check_version(avctx)) < 0)
> -        return AVERROR_DECODER_NOT_FOUND;
> -
>       if (WelsCreateDecoder(&s->decoder)) {
>           av_log(avctx, AV_LOG_ERROR, "Unable to create decoder\n");
>           return AVERROR_UNKNOWN;
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index f518d0894e..6f231d22b2 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -110,14 +110,10 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
>   {
>       SVCContext *s = avctx->priv_data;
>       SEncParamExt param = { 0 };
> -    int err;
>       int log_level;
>       WelsTraceCallback callback_function;
>       AVCPBProperties *props;
>   
> -    if ((err = ff_libopenh264_check_version(avctx)) < 0)
> -        return AVERROR_ENCODER_NOT_FOUND;
> -
>       if (WelsCreateSVCEncoder(&s->encoder)) {
>           av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
>           return AVERROR_UNKNOWN;
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Kalev Lember]

On Fri, Dec 8, 2023 at 4:59 PM James Almer <jamrial@gmail.com> wrote:

> Does the configure check ensure a new enough openh264 version is the
> minimum supported?
>

Hm, I'd say that configure minimum version check is mostly orthogonal to
the patch here.

This patch just removes a check that made it error out if the build time
and runtime versions didn't perfectly match (as in, 1.0.0 at build time and
1.0.1 at run time would have resulted in erroring out). Basically just
makes it behave like with all other libraries :)

-- 
Kalev
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[James Almer]

On 12/8/2023 4:07 PM, Kalev Lember wrote:
> On Fri, Dec 8, 2023 at 4:59 PM James Almer <jamrial@gmail.com> wrote:
> 
>> Does the configure check ensure a new enough openh264 version is the
>> minimum supported?
>>
> 
> Hm, I'd say that configure minimum version check is mostly orthogonal to
> the patch here.
> 
> This patch just removes a check that made it error out if the build time
> and runtime versions didn't perfectly match (as in, 1.0.0 at build time and
> 1.0.1 at run time would have resulted in erroring out). Basically just
> makes it behave like with all other libraries :)

I understand that, but you said "This should no longer be an issue with 
newer openh264 releases", meaning this used to be a problem until the 
project started being more careful about breakages, so shouldn't we bump 
the minimum required version to one where it's know there will be no 
issues at runtime if the runtime library is ABI incompatible with the 
link time one?
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Cosmin Stejerean via ffmpeg-devel]

> On Dec 8, 2023, at 11:07 AM, Kalev Lember <klember@redhat.com> wrote:
> 
> On Fri, Dec 8, 2023 at 4:59 PM James Almer <jamrial@gmail.com> wrote:
> 
>> Does the configure check ensure a new enough openh264 version is the
>> minimum supported?
>> 
> 
> Hm, I'd say that configure minimum version check is mostly orthogonal to
> the patch here.
> 
> This patch just removes a check that made it error out if the build time
> and runtime versions didn't perfectly match (as in, 1.0.0 at build time and
> 1.0.1 at run time would have resulted in erroring out). Basically just
> makes it behave like with all other libraries :)

As of what version of openh264 though is it safe to assume that ABI won't change without soname changes? Since years ago ABI changes without soname changes were present there's likely to be some minimum version above which runtime version checks are not needed.

- Cosmin
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Kalev Lember]

On Fri, Dec 8, 2023 at 8:12 PM Cosmin Stejerean via ffmpeg-devel <
ffmpeg-devel@ffmpeg.org> wrote:

> As of what version of openh264 though is it safe to assume that ABI won't
> change without soname changes? Since years ago ABI changes without soname
> changes were present there's likely to be some minimum version above which
> runtime version checks are not needed.
>

I don't have a very good answer here, sorry. It was more of a general
observation that upstream is trying to keep the soname updated whenever
there is an ABI change.

However, if I had to pick a version, I did some digging and maybe version
1.3.0 because before that there was just an unversioned libopenh264.so, and
1.3.0 added an actual versioned libopenh264.so.0, which has been updated
since then whenever there have been ABI changes.

Do you guys want me to add a minimum 1.3.0 check?

Martin mentioned earlier that he once envisioned something like passing
-openh264_lib /path/to/my/libopenh264.so to ffmpeg and that must have been
before the versioned soname was introduced.

-- 
Kalev
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Martin Storsjö]

On Fri, 8 Dec 2023, Kalev Lember wrote:

> On Fri, Dec 8, 2023 at 8:12 PM Cosmin Stejerean via ffmpeg-devel <
> ffmpeg-devel@ffmpeg.org> wrote:
>
>> As of what version of openh264 though is it safe to assume that ABI won't
>> change without soname changes? Since years ago ABI changes without soname
>> changes were present there's likely to be some minimum version above which
>> runtime version checks are not needed.
>>
>
> I don't have a very good answer here, sorry. It was more of a general
> observation that upstream is trying to keep the soname updated whenever
> there is an ABI change.
>
> However, if I had to pick a version, I did some digging and maybe version
> 1.3.0 because before that there was just an unversioned libopenh264.so, and
> 1.3.0 added an actual versioned libopenh264.so.0, which has been updated
> since then whenever there have been ABI changes.
>
> Do you guys want me to add a minimum 1.3.0 check?

If that was when the soname version was introduced, that sounds reasonable 
- since then, there's at least an intent not to break it (even if mistakes 
always can happen).

> Martin mentioned earlier that he once envisioned something like passing
> -openh264_lib /path/to/my/libopenh264.so to ffmpeg and that must have been
> before the versioned soname was introduced.

Not necessarily; my point was that if we would have allowed pointing at a 
specific file, we need to check that it matches the expected ABI version. 
As we don't have an ABI version number in the headers (and there was no 
effort to maintain an ABI at the time), checking the full version number 
was my approximation of it.

// 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] lavc/libopenh264: Drop openh264 runtime version checks

[Kalev Lember]

On Fri, Dec 8, 2023 at 9:34 PM Martin Storsjö <martin@martin.st> wrote:

> If that was when the soname version was introduced, that sounds reasonable
> - since then, there's at least an intent not to break it (even if mistakes
> always can happen).
>

Yep, 1.3.0 is when the soname version was introduced. Let's go with that
then, I think it makes sense to go with the intent here, like you say.

I'll send an updated patch.

(And thanks for all the comments and discussion, everybody! This is my
first patch to ffmpeg and it's nice to see so much engagement.)

-- 
Kalev
_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Kalev Lember]

With the way the runtime checks are currently set up, every single
openh264 release, no matter how minor, is considered an ABI break and
requires ffmpeg recompilation. This is unnecessarily strict because it
doesn't allow downstream distributions to ship any openh264 bug fix
version updates without breaking ffmpeg's openh264 support.

Years ago, at the time when ffmpeg's openh264 support was merged,
openh264 releases were done without a versioned soname (the library was
just libopenh264.so, unversioned). Since then, starting with version
1.3.0, openh264 has started using versioned sonames and the intent has
been to bump the soname every time there's a new release with an ABI
change.

This patch drops the exact version check and instead adds a minimum
requirement on 1.3.0 to the configure script.

Signed-off-by: Kalev Lember <klember@redhat.com>
---
 configure                   |  2 +-
 libavcodec/libopenh264.c    | 15 ---------------
 libavcodec/libopenh264.h    |  2 --
 libavcodec/libopenh264dec.c |  4 ----
 libavcodec/libopenh264enc.c |  4 ----
 5 files changed, 1 insertion(+), 26 deletions(-)

diff --git a/configure b/configure
index 7d2ee66000..29b0e463a1 100755
--- a/configure
+++ b/configure
@@ -6799,7 +6799,7 @@ enabled libopencv         && { check_headers opencv2/core/core_c.h &&
                                { check_pkg_config libopencv opencv opencv2/core/core_c.h cvCreateImageHeader ||
                                  require libopencv opencv2/core/core_c.h cvCreateImageHeader -lopencv_core -lopencv_imgproc; } ||
                                require_pkg_config libopencv opencv opencv/cxcore.h cvCreateImageHeader; }
-enabled libopenh264       && require_pkg_config libopenh264 openh264 wels/codec_api.h WelsGetCodecVersion
+enabled libopenh264       && require_pkg_config libopenh264 "openh264 >= 1.3.0" wels/codec_api.h WelsGetCodecVersion
 enabled libopenjpeg       && { check_pkg_config libopenjpeg "libopenjp2 >= 2.1.0" openjpeg.h opj_version ||
                                { require_pkg_config libopenjpeg "libopenjp2 >= 2.1.0" openjpeg.h opj_version -DOPJ_STATIC && add_cppflags -DOPJ_STATIC; } }
 enabled libopenmpt        && require_pkg_config libopenmpt "libopenmpt >= 0.2.6557" libopenmpt/libopenmpt.h openmpt_module_create -lstdc++ && append libopenmpt_extralibs "-lstdc++"
diff --git a/libavcodec/libopenh264.c b/libavcodec/libopenh264.c
index 0f6d28ed88..c80c85ea8b 100644
--- a/libavcodec/libopenh264.c
+++ b/libavcodec/libopenh264.c
@@ -46,18 +46,3 @@ void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg)
     int equiv_ffmpeg_log_level = libopenh264_to_ffmpeg_log_level(level);
     av_log(ctx, equiv_ffmpeg_log_level, "%s\n", msg);
 }
-
-int ff_libopenh264_check_version(void *logctx)
-{
-    // Mingw GCC < 4.7 on x86_32 uses an incorrect/buggy ABI for the WelsGetCodecVersion
-    // function (for functions returning larger structs), thus skip the check in those
-    // configurations.
-#if !defined(_WIN32) || !defined(__GNUC__) || !ARCH_X86_32 || AV_GCC_VERSION_AT_LEAST(4, 7)
-    OpenH264Version libver = WelsGetCodecVersion();
-    if (memcmp(&libver, &g_stCodecVersion, sizeof(libver))) {
-        av_log(logctx, AV_LOG_ERROR, "Incorrect library version loaded\n");
-        return AVERROR(EINVAL);
-    }
-#endif
-    return 0;
-}
diff --git a/libavcodec/libopenh264.h b/libavcodec/libopenh264.h
index dbb9c5d429..0b462d6fdc 100644
--- a/libavcodec/libopenh264.h
+++ b/libavcodec/libopenh264.h
@@ -34,6 +34,4 @@
 
 void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg);
 
-int ff_libopenh264_check_version(void *logctx);
-
 #endif /* AVCODEC_LIBOPENH264_H */
diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
index 7d650ae03e..b6a9bba2dc 100644
--- a/libavcodec/libopenh264dec.c
+++ b/libavcodec/libopenh264dec.c
@@ -52,13 +52,9 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
 {
     SVCContext *s = avctx->priv_data;
     SDecodingParam param = { 0 };
-    int err;
     int log_level;
     WelsTraceCallback callback_function;
 
-    if ((err = ff_libopenh264_check_version(avctx)) < 0)
-        return AVERROR_DECODER_NOT_FOUND;
-
     if (WelsCreateDecoder(&s->decoder)) {
         av_log(avctx, AV_LOG_ERROR, "Unable to create decoder\n");
         return AVERROR_UNKNOWN;
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index f518d0894e..6f231d22b2 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -110,14 +110,10 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
 {
     SVCContext *s = avctx->priv_data;
     SEncParamExt param = { 0 };
-    int err;
     int log_level;
     WelsTraceCallback callback_function;
     AVCPBProperties *props;
 
-    if ((err = ff_libopenh264_check_version(avctx)) < 0)
-        return AVERROR_ENCODER_NOT_FOUND;
-
     if (WelsCreateSVCEncoder(&s->encoder)) {
         av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
         return AVERROR_UNKNOWN;
-- 
2.43.0

_______________________________________________
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] lavc/libopenh264: Drop openh264 runtime version checks

[Martin Storsjö]

On Sat, 9 Dec 2023, Kalev Lember wrote:

> With the way the runtime checks are currently set up, every single
> openh264 release, no matter how minor, is considered an ABI break and
> requires ffmpeg recompilation. This is unnecessarily strict because it
> doesn't allow downstream distributions to ship any openh264 bug fix
> version updates without breaking ffmpeg's openh264 support.
>
> Years ago, at the time when ffmpeg's openh264 support was merged,
> openh264 releases were done without a versioned soname (the library was
> just libopenh264.so, unversioned). Since then, starting with version
> 1.3.0, openh264 has started using versioned sonames and the intent has
> been to bump the soname every time there's a new release with an ABI
> change.
>
> This patch drops the exact version check and instead adds a minimum
> requirement on 1.3.0 to the configure script.
>
> Signed-off-by: Kalev Lember <klember@redhat.com>
> ---

Thanks, pushed now!

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