[FFmpeg-devel] [PATCH] swscale: Support setting filters through AVOptions
wm4
nfxjfg at googlemail.com
Mon Nov 4 22:30:25 CET 2013
On Mon, 4 Nov 2013 14:32:23 +0100
Stefano Sabatini <stefasab at gmail.com> wrote:
> 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
Yes, if you want to go this way, this should be added. The new API
should cover all uses of the old API (unless they're deemed as not
worth supporting).
> > (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()?
Using a set of functions is easier than generating a string. (Both for
users and implementers of an API!)
> > 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.
User interface issues shouldn't be the job of a very low level core
library like libswscale.
> > 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.
Well, I tend to think having a single way is better than having two.
Maybe the doxygen should mention there is no difference between them.
> [...]
> > > +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.
That would be nice.
> > > 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.
More information about the ffmpeg-devel
mailing list