[FFmpeg-devel] [PATCH] swscale: Support setting filters through AVOptions
Stefano Sabatini
stefasab at gmail.com
Mon Nov 4 14:32:23 CET 2013
On date Tuesday 2013-10-29 20:16:21 +0100, wm4 encoded:
> 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
> ','?
I could easily support gauss() and const() vector operators.
At this point we need to extend the syntax. Probably something like:
vector ::= [domain=][function|sequence]
vectors ::= vector [&vector]
filter ::= vectors
> (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's the problem with API users? Can't they set them directly in the
SwsContext with sws_getCachedContext()?
> 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.
Why "more awkward"? The idea is to be able to set it through external
components (e.g. libavfilter) and to let the high-level user (with no
direct interface to libswscale internals) to define them. Note that
this is orthogonal to setting it programmatically.
> 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?
"," is useful to remind standard matrix notation, '|' is useful to
avoid escaping in other context (e.g. in a lavfi graph where "," is a
special character.
[...]
> > +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?
Yes.
> > 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.
This is useful for debugging.
--
FFmpeg = Furious and Formidable MultiPurpose Elfic Gladiator
More information about the ffmpeg-devel
mailing list