On Fri, Jul 15, 2016 at 01:43:16PM +0300, Martin Storsjö wrote:
> This simplifies use of the patent license, simplifying use with a
> library that has been downloaded at runtime (making it possible
> to actually load and run libavcodec before the corresponding
> OpenH264 library exists).
> ---
> This patch was sent for review earlier, and got some feedback, but it was
> deferred back then due not knowing how much interest there would be in it
> - see https://patches.libav.org/patch/56287/.
>
> Now there seems to be interest
> in it, see
> https://lists.libav.org/pipermail/libav-devel/2016-July/078139.html,
> so I'm resending it, with the feedback from the previous round applied,
> and updated on top of the latest master.
> ---
> configure | 8 ++++--
> libavcodec/Makefile | 1 +
> libavcodec/libopenh264.c | 4 +--
> libavcodec/libopenh264.h | 5 +++-
> libavcodec/libopenh264dec.c | 61
> ++++++++++++++++++++++++++++++++++++++++++---
> libavcodec/libopenh264enc.c | 49 +++++++++++++++++++++++++++++++++---
> 6 files changed, 115 insertions(+), 13 deletions(-)
I'm undecided about the general merits of this patch.
Some implementation remarks below...
> --- a/configure
> +++ b/configure
> @@ -4594,6 +4596,8 @@ enabled libopencore_amrnb && require libopencore_amrnb
> opencore-amrnb/interf_dec
> enabled libopencv && require_pkg_config opencv opencv/cv.h
> cvCreateImageHeader
> enabled libopenh264 && require_pkg_config openh264 wels/codec_api.h
> WelsGetCodecVersion
> +enabled libopenh264_dyn && { { check_header wels/codec_ver.h && enabled
> dyn_lib_open; } ||
> + die "ERROR: OpenH264 1.3 header not found,
> or dlopen/LoadLibrary not found"; }
Not sure if mentioning the version number here is a good idea looking
forward.
> index 8eb7d36..f639ac5 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -771,6 +771,7 @@ SKIPHEADERS += %_tablegen.h
> \
> SKIPHEADERS-$(CONFIG_D3D11VA) += d3d11va.h dxva2_internal.h
> SKIPHEADERS-$(CONFIG_DXVA2) += dxva2.h dxva2_internal.h
> SKIPHEADERS-$(CONFIG_LIBSCHROEDINGER) += libschroedinger.h
> +SKIPHEADERS-$(CONFIG_LIBOPENH264) += libopenh264.h
This will also skip the header if LIBOPENH264_DYN is set, which I think
is not what you intend.
> --- a/libavcodec/libopenh264dec.c
> +++ b/libavcodec/libopenh264dec.c
> @@ -23,6 +23,9 @@
> #include <wels/codec_ver.h>
>
> #include "libavutil/common.h"
> +#if CONFIG_LIBOPENH264_DYN
> +#include "libavutil/dyn_lib_open.h"
> +#endif
You are missing a config.h #include.
> @@ -239,4 +291,5 @@ AVCodec ff_libopenh264_decoder = {
> .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_DR1,
> .caps_internal = FF_CODEC_CAP_SETS_PKT_DTS |
> FF_CODEC_CAP_INIT_THREADSAFE |
> FF_CODEC_CAP_INIT_CLEANUP,
> + .priv_class = &class,
unrelated?
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -24,6 +24,9 @@
>
> #include "libavutil/attributes.h"
> #include "libavutil/common.h"
> +#if CONFIG_LIBOPENH264_DYN
> +#include "libavutil/dyn_lib_open.h"
> +#endif
> #include "libavutil/opt.h"
> #include "libavutil/internal.h"
> #include "libavutil/intreadwrite.h"
You're missing a config.h #include.
> --- a/libavcodec/libopenh264dec.c
> +++ b/libavcodec/libopenh264dec.c
> @@ -67,15 +93,41 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
> WelsTraceCallback callback_function;
> + int (*create_func)(ISVCDecoder **ppDecoder);
> + OpenH264Version (*get_version_func)(void);
> +
> +#if CONFIG_LIBOPENH264_DYN
> + if (!s->libname) {
> + av_log(avctx, AV_LOG_ERROR, "No library name provided\n");
> + return AVERROR(EINVAL);
> + }
> + s->lib = load_library(s->libname);
> + if (!s->lib) {
> + av_log(avctx, AV_LOG_ERROR, "Unable to load %s\n", s->libname);
> + return AVERROR(EINVAL);
> + }
> +#endif
>
> - if ((err = ff_libopenh264_check_version(avctx)) < 0)
> + if ((err = ff_libopenh264_check_version(avctx, get_version_func)) < 0)
> return err;
>
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -97,11 +112,37 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
> WelsTraceCallback callback_function;
> AVCPBProperties *props;
> + int (*create_func)(ISVCEncoder **ppEncoder);
> + OpenH264Version (*get_version_func)(void);
> +
> +#if CONFIG_LIBOPENH264_DYN
> + if (!s->libname) {
> + av_log(avctx, AV_LOG_ERROR, "No library name provided\n");
> + return AVERROR(EINVAL);
> + }
> + s->lib = load_library(s->libname);
> + if (!s->lib) {
> + av_log(avctx, AV_LOG_ERROR, "Unable to load %s\n", s->libname);
> + return AVERROR(EINVAL);
> + }
> +#endif
>
> - if ((err = ff_libopenh264_check_version(avctx)) < 0)
> + if ((err = ff_libopenh264_check_version(avctx, get_version_func)) < 0)
> return err;
This looks similar enough that it could be shared between decoder and
encoder.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
