[FFmpeg-devel] [PATCH] swscale: Support setting filters through AVOptions
wm4
nfxjfg at googlemail.com
Tue Oct 29 20:16:21 CET 2013
On Mon, 21 Oct 2013 17:53:33 +0200
Stefano Sabatini <stefasab at gmail.com> wrote:
> From 43cfd6ac1504f667f081e92a76b394e2f96816d6 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab at gmail.com>
> Date: Sat, 5 Oct 2013 22:49:55 +0200
> Subject: [PATCH] lsws: support setting filters through AVOptions
>
> A parsing utility sws_parse_filter() is added to allow to parse and
> serialize a filter description.
>
> Based on idea and patch by Michael Niedermayer. This patch implements
> more sound error feedback and provides more robust parsing.
>
> TODO: bump minor, update APIchanges
> ---
> doc/scaler.texi | 35 ++++++
> libswscale/Makefile | 1 +
> libswscale/options.c | 3 +
> libswscale/swscale.h | 27 ++++
> libswscale/swscale_internal.h | 4 +
> libswscale/utils.c | 280 +++++++++++++++++++++++++++++++++++++++++-
> 6 files changed, 349 insertions(+), 1 deletion(-)
>
> diff --git a/doc/scaler.texi b/doc/scaler.texi
> index 08d90bc..1e5bd37 100644
> --- a/doc/scaler.texi
> +++ b/doc/scaler.texi
> @@ -112,6 +112,41 @@ bayer dither
>
> @item ed
> error diffusion dither
> +
> + at item src_filter
> + at item dst_filter
> +Set source or destination filter.
> +
> +A filter description must contain a sequence of vector specifications
> +separated by '&' or ';', preceded by a prefix which specifies which
> +filter components (luma horizontal, luma vertical, chroma horizontal,
> +chroma vertical) are defined by the following vector.
> +
> +The prefix must be a combination of the characters 'l' (luma), 'c'
> +(chroma), 'h' (horizontal), 'v' (vertical). Several characters can be
> +combined to specify a more specific component (e.g. 'lh' means luma
> +horizontal). If the prefix is not specified the vector specifies all
> +the components.
> +
> +A vector is specified by a sequence of @code{av_strtod} number
> +specifications, separated either by ',' or '|'.
> +
> +A few syntax examples follow.
> + at itemize
> + at item
> +Set all components to the same vector:
> + at example
> +1,2,3
> + at end example
> +
> + at item
> +Set same vector for all components, but for the luma horizontal
> +component:
> + at example
> +1,2,3&lh5,6
> + at end example
> + at end itemize
> +
> @end table
>
> @end table
The description isn't terribly clear IMHO. It's not
really clear to me how to do what sws_getDefaultFilter() and
sws_getGaussianVec() did (because these were the only real uses for
this filter stuff apparently). Does the user have to reimplement them,
and then convert the array of coefficients to a string separated by
','? (Also what's the difference between '|' and ','?) Or maybe use the
original functions, and then convert the SwsFilter manually to a string?
All in all pretty weird IMO. It's geared towards somehow making the
filter vectors settable through ffmpeg/ffplay (because apparently these
tools are too dumb to have their own option parsers), instead of making
it usable to API users. What exactly is the advantage of this, and the
use case? Everything becomes more awkward, but at least it's supported
by the glorious AVOption API? I'm quite a bit skeptic about this.
At least it should support what sws_getDefaultFilter() did, because
that's 99% of all use-cases. Unless I'm misunderstanding something.
> diff --git a/libswscale/Makefile b/libswscale/Makefile
> index dd00f7d..2c19ed6 100644
> --- a/libswscale/Makefile
> +++ b/libswscale/Makefile
> @@ -17,3 +17,4 @@ OBJS = input.o \
>
> TESTPROGS = colorspace \
> swscale \
> + utils \
> diff --git a/libswscale/options.c b/libswscale/options.c
> index 9e8703f..bb62824 100644
> --- a/libswscale/options.c
> +++ b/libswscale/options.c
> @@ -74,6 +74,9 @@ static const AVOption swscale_options[] = {
> { "bayer", "bayer dither", 0, AV_OPT_TYPE_CONST, { .i64 = SWS_DITHER_BAYER }, INT_MIN, INT_MAX, VE, "sws_dither" },
> { "ed", "error diffusion", 0, AV_OPT_TYPE_CONST, { .i64 = SWS_DITHER_ED }, INT_MIN, INT_MAX, VE, "sws_dither" },
>
> + { "src_filter", "set source filter", OFFSET(src_filter_string), AV_OPT_TYPE_STRING, { .str = NULL }, INT_MIN, INT_MAX, VE },
> + { "dst_filter", "set destination filter", OFFSET(dst_filter_string), AV_OPT_TYPE_STRING, { .str = NULL }, INT_MIN, INT_MAX, VE },
> +
> { NULL }
> };
>
> diff --git a/libswscale/swscale.h b/libswscale/swscale.h
> index 42702b7..ca10e3b 100644
> --- a/libswscale/swscale.h
> +++ b/libswscale/swscale.h
> @@ -299,6 +299,33 @@ void sws_printVec2(SwsVector *a, AVClass *log_ctx, int log_level);
>
> void sws_freeVec(SwsVector *a);
>
> +/**
> + * Parse filter description, and create filter.
> + *
> + * @param filtp pointer to filter which is set to the new created
> + * filter in case of success
> + * @param buf buffer containing the filter description.
> + *
> + * A filter description must contain a sequence of vector
> + * specifications separated by "&", preceded by a prefix which
> + * specifies which filter components (luma horizontal, luma vertical,
> + * chroma horizontal, chroma vertical) are defined by the following
> + * vector.
> + *
> + * The prefix must be a combination of the characters 'l' (luma), 'c'
> + * (chroma), 'h' (horizontal), 'v' (vertical. Several characters can
> + * be combined to specify a more specific component (e.g. 'lh' means
> + * luma horizontal). If the prefix is not specified the vector
> + * specifies all the components.
> + *
> + * A vector is specified by a sequence of av_strtod() number
> + * specifications, separated either by ',' or '|'.
> + *
> + * @param log_ctx log context to use to log parsing errors
> + * @return >= in case of success, a negative error code otherwise
> + */
> +int sws_parse_filter(SwsFilter **filtp, const char *buf, void *log_ctx);
> +
> SwsFilter *sws_getDefaultFilter(float lumaGBlur, float chromaGBlur,
> float lumaSharpen, float chromaSharpen,
> float chromaHShift, float chromaVShift,
> diff --git a/libswscale/swscale_internal.h b/libswscale/swscale_internal.h
> index 33fdfc2..4b6fff6 100644
> --- a/libswscale/swscale_internal.h
> +++ b/libswscale/swscale_internal.h
> @@ -296,6 +296,10 @@ typedef struct SwsContext {
> int vChrDrop; ///< Binary logarithm of extra vertical subsampling factor in source image chroma planes specified by user.
> int sliceDir; ///< Direction that slices are fed to the scaler (1 = top-to-bottom, -1 = bottom-to-top).
> double param[2]; ///< Input parameters for scaling algorithms that need them.
> + char *src_filter_string;
> + char *dst_filter_string;
> + SwsFilter *src_filter;
> + SwsFilter *dst_filter;
>
> uint32_t pal_yuv[256];
> uint32_t pal_rgb[256];
> diff --git a/libswscale/utils.c b/libswscale/utils.c
> index 5f35b41..b4a21a6 100644
> --- a/libswscale/utils.c
> +++ b/libswscale/utils.c
> @@ -39,9 +39,12 @@
>
> #include "libavutil/attributes.h"
> #include "libavutil/avassert.h"
> +#include "libavutil/avstring.h"
> #include "libavutil/avutil.h"
> +#include "libavutil/bprint.h"
> #include "libavutil/bswap.h"
> #include "libavutil/cpu.h"
> +#include "libavutil/eval.h"
> #include "libavutil/intreadwrite.h"
> #include "libavutil/mathematics.h"
> #include "libavutil/opt.h"
> @@ -238,6 +241,187 @@ const char *sws_format_name(enum AVPixelFormat format)
> }
> #endif
>
> +#define VECTOR_ELEMENT_SEP_CHARS "|,"
> +
> +static int parse_vector(SwsVector **vecp, const char *buf, void *log_ctx)
> +{
> + const char *p = buf;
> + int i, ret = 0;
> + SwsVector *vec = av_mallocz(sizeof(SwsVector));
> + if (!vec)
> + return AVERROR(ENOMEM);
> +
> + for (i = 0; ; i++) {
> + char *end;
> + if (av_reallocp(&vec->coeff, (i+1) * sizeof(*vec->coeff)) < 0) {
> + ret = AVERROR(ENOMEM);
> + goto fail;
> + }
> + vec->coeff[i] = av_strtod(p, &end);
> + if (end == p) {
> + av_log(log_ctx, AV_LOG_ERROR,
> + "Could not parse element '%s' in vector '%s' as a number\n", p, buf);
> + ret = AVERROR(EINVAL);
> + goto fail;
> + }
> + p = end;
> + if (!*p)
> + break;
> + else if (*p && strspn(p, VECTOR_ELEMENT_SEP_CHARS)) {
So there's no difference between '|' and ','? Why support both?
> + p++;
> + } else {
> + av_log(log_ctx, AV_LOG_ERROR,
> + "Vector specification '%s' contains trailing invalid data\n", buf);
> + ret = AVERROR(EINVAL);
> + goto fail;
> + }
> + }
> +
> + vec->length = i+1;
> + *vecp = vec;
> + return i;
> +
> +fail:
> + sws_freeVec(vec);
> + return ret;
> +}
> +
> +#define VECTOR_SEP_CHARS "&;"
> +#define VECTOR_DOMAIN_CHARS "vhlc"
> +
> +int sws_parse_filter(SwsFilter **filtp, const char *buf, void *log_ctx)
> +{
> +#define V 1
> +#define H 2
> +#define LUM 4
> +#define CHR 8
> + int domain, ret = 0, len = 0;
> + const char *p = buf;
> + char *spec = av_strdup(buf);
> + SwsFilter *filt;
> +
> + if (!spec)
> + return AVERROR(ENOMEM);
> +
> + filt = av_mallocz(sizeof(SwsFilter));
> + if (!filt) {
> + av_free(spec);
> + return AVERROR(ENOMEM);
> + }
> +
> + while (p && *p) {
> + SwsVector *vec = NULL;
> + char *vecbuf;
> +
> + /* parse vector domain */
> + domain = 0;
> +
> + while (p && *p && !strcspn(p, VECTOR_DOMAIN_CHARS)) {
> + switch (*p) {
> + case 'v': domain |= V; break;
> + case 'h': domain |= H; break;
> + case 'l': domain |= LUM; break;
> + case 'c': domain |= CHR; break;
> + default:
> + av_log(log_ctx, AV_LOG_ERROR,
> + "Unknown character '%c' in filter domain specification '%s', "
> + "only '" VECTOR_DOMAIN_CHARS "' are recognized\n", *p, buf);
> + ret = AVERROR(EINVAL);
> + goto end;
> + }
> + p++;
> + }
> +
> + /* normalize domain*/
> + if (av_popcount(domain) >= 3) {
> + av_log(log_ctx, AV_LOG_ERROR,
> + "Inconsistent filter vector domain specification in '%s', "
> + "only two subdomains can be chosen\n", buf);
> + ret = AVERROR(EINVAL);
> + goto end;
> + }
> +
> + if (!domain)
> + domain = LUM|CHR;
> + vecbuf = av_get_token(&p, VECTOR_SEP_CHARS);
> + if (*p)
> + p++;
> + if (!vecbuf) {
> + ret = AVERROR(ENOMEM);
> + goto end;
> + }
> +
> + ret = parse_vector(&vec, vecbuf, log_ctx);
> + if (ret < 0)
> + goto end;
> + len = FFMAX(ret, len);
> + av_free(vecbuf);
> +
> +#define ADD_VECTOR(D1, D2, type) \
> +do { \
> + if (domain == (LUM|CHR) || domain == (V|H) || \
> + domain == (D1|D2) || domain == D1 || domain == D2) { \
> + sws_freeVec(filt->type); \
> + if (!(filt->type = sws_cloneVec(vec))) { \
> + ret = AVERROR(ENOMEM); \
> + goto end; \
> + } \
> + } \
> +} while (0)
> +
> + ADD_VECTOR(LUM, H, lumH);
> + ADD_VECTOR(LUM, V, lumV);
> + ADD_VECTOR(CHR, H, chrH);
> + ADD_VECTOR(CHR, V, chrV);
> + sws_freeVec(vec);
> + }
> +#undef ADD_VECTOR
> +
> + if (!filt->lumH || !filt->lumV || !filt->chrH || !filt->chrV) {
> + char missing[32];
> + char *p = missing;
> + if (!filt->lumH) { strcpy(p, "lumH "); p += 5; }
> + if (!filt->lumV) { strcpy(p, "lumV "); p += 5; }
> + if (!filt->chrH) { strcpy(p, "chrH "); p += 5; }
> + if (!filt->chrV) { strcpy(p, "chrV "); p += 5; }
> + av_log(log_ctx, AV_LOG_ERROR,
> + "Incomplete filter specification in '%s', the following components "
> + "are not specified: %s\n", buf, missing);
> + ret = AVERROR(EINVAL);
> + goto end;
> + }
> +
> +end:
> + av_free(spec);
> + if (ret < 0) {
> + sws_freeFilter(filt);
> + return ret;
> + }
> +
> + *filtp = filt;
> + return len;
> +
> +#undef V
> +#undef H
> +#undef LUM
> +#undef CHR
> +}
Can't say much about the string parsing.
> +static void bprint_vector(struct AVBPrint *bp, const SwsVector *vec)
> +{
> + int i;
> + for (i = 0; i < vec->length; i++)
> + av_bprintf(bp, "%f%s", vec->coeff[i], i < vec->length-1 ? "," : "");
> +}
> +
> +static void bprint_filter(struct AVBPrint *bp, const SwsFilter *filter)
> +{
> + av_bprintf(bp, "%s", "lh" ); bprint_vector(bp, filter->lumH);
> + av_bprintf(bp, "%s", "&lv"); bprint_vector(bp, filter->lumV);
> + av_bprintf(bp, "%s", "&ch"); bprint_vector(bp, filter->chrH);
> + av_bprintf(bp, "%s", "&cv"); bprint_vector(bp, filter->chrV);
> +}
> +
> static double getSplineCoeff(double a, double b, double c, double d,
> double dist)
> {
> @@ -1084,7 +1268,7 @@ SwsContext *sws_alloc_context(void)
> av_cold int sws_init_context(SwsContext *c, SwsFilter *srcFilter,
> SwsFilter *dstFilter)
> {
> - int i, j;
> + int i, j, ret;
> int usesVFilter, usesHFilter;
> int unscaled;
> SwsFilter dummyFilter = { NULL, NULL, NULL, NULL };
> @@ -1172,6 +1356,37 @@ av_cold int sws_init_context(SwsContext *c, SwsFilter *srcFilter,
> return AVERROR(EINVAL);
> }
>
> + sws_freeFilter(c->src_filter);
> + c->src_filter = NULL;
> + sws_freeFilter(c->dst_filter);
> + c->dst_filter = NULL;
> +
> + if (dstFilter && c->dst_filter_string)
> + av_log(c, AV_LOG_WARNING,
> + "Destination filter was already defined, ignoring destination filter string specification.\n");
> + if (!dstFilter && c->dst_filter_string) {
> + ret = sws_parse_filter(&dstFilter, c->dst_filter_string, c);
> + if (ret < 0) {
> + av_log(c, AV_LOG_ERROR, "Could not parse specified destination filter '%s'\n",
> + c->dst_filter_string);
> + return ret;
> + }
> + c->dst_filter = dstFilter;
> + }
> +
> + if (srcFilter && c->src_filter_string)
> + av_log(c, AV_LOG_WARNING,
> + "Source filter was already defined, ignoring destination filter string specification.\n");
> + if (!srcFilter && c->src_filter_string) {
> + ret = sws_parse_filter(&srcFilter, c->src_filter_string, c);
> + if (ret < 0) {
> + av_log(c, AV_LOG_ERROR, "Could not parse specified source filter '%s'\n",
> + c->src_filter_string);
> + return ret;
> + }
> + c->src_filter = srcFilter;
> + }
> +
sws_init_context() is already the worst function on the planet. Can't
this new code be factored out?
> if (!dstFilter)
> dstFilter = &dummyFilter;
> if (!srcFilter)
> @@ -1624,6 +1839,22 @@ av_cold int sws_init_context(SwsContext *c, SwsFilter *srcFilter,
> "chr srcW=%d srcH=%d dstW=%d dstH=%d xInc=%d yInc=%d\n",
> c->chrSrcW, c->chrSrcH, c->chrDstW, c->chrDstH,
> c->chrXInc, c->chrYInc);
> + {
> + struct AVBPrint bp;
> +
> + av_bprint_init(&bp, 0, AV_BPRINT_SIZE_AUTOMATIC);
> +
> + if (c->src_filter) {
> + bprint_filter(&bp, c->src_filter);
> + av_log(c, AV_LOG_DEBUG, "src_filter:%s ", bp.str);
> + av_bprint_clear(&bp);
> + }
> + if (c->dst_filter) {
> + bprint_filter(&bp, c->dst_filter);
> + av_log(c, AV_LOG_DEBUG, "dst_filter:%s\n", bp.str);
> + av_bprint_finalize(&bp, NULL);
> + }
> + }
> }
Same for this. Not sure why these have to be printed, though.
> c->swscale = ff_getSwsFunc(c);
> @@ -2042,6 +2273,11 @@ void sws_freeContext(SwsContext *c)
> av_freep(&c->yuvTable);
> av_freep(&c->formatConvBuffer);
>
> + sws_freeFilter(c->src_filter);
> + c->src_filter = NULL;
> + sws_freeFilter(c->dst_filter);
> + c->dst_filter = NULL;
> +
> av_free(c);
> }
>
> @@ -2092,3 +2328,45 @@ struct SwsContext *sws_getCachedContext(struct SwsContext *context, int srcW,
> }
> return context;
> }
> +
> +#ifdef TEST
> +
> +int main(void)
> +{
> + printf("\nTesting sws_parse_filter()\n");
> + {
> + AVBPrint bp;
> + int i;
> + SwsFilter *filt = NULL;
> + static const char *const filts[] = {
> + "",
> + "foo",
> + "1+2",
> + "1",
> + "h1,2,3&v2,3,4",
> + "0.2,0.123",
> + "1,23,45,343+3",
> + "lh1,2,3&c4,5.123",
> + "l1,2,3",
> + "cl1,2,3",
> + "1,2,3&lh5,6",
> + };
> +
> + av_log_set_level(AV_LOG_DEBUG);
> +
> + av_bprint_init(&bp, 0, -1);
> + for (i = 0; i < FF_ARRAY_ELEMS(filts); i++) {
> + if (sws_parse_filter(&filt, filts[i], NULL) >= 0) {
> + printf("%s -> ", filts[i]);
> + bprint_filter(&bp, filt);
> + printf("%s", bp.str);
> + av_bprint_clear(&bp);
> + } else {
> + printf("%s -> error\n", filts[i]);
> + }
> + }
> + av_bprint_finalize(&bp, NULL);
> + }
> +}
> +
> +#endif
More information about the ffmpeg-devel
mailing list