[FFmpeg-devel] [PATCH] swscale: Support setting filters through AVOptions
Michael Niedermayer
michaelni at gmx.at
Mon Oct 14 19:06:03 CEST 2013
On Sun, Oct 13, 2013 at 08:57:40PM +0200, Stefano Sabatini wrote:
> On date Friday 2013-10-11 16:45:43 +0200, Michael Niedermayer encoded:
> > On Fri, Oct 11, 2013 at 02:41:50PM +0200, Stefano Sabatini wrote:
> > > On date Saturday 2013-10-05 22:49:55 +0200, Michael Niedermayer encoded:
> > > > anything else could be used or the routine even be made to accept
> > > > any character as separator.
> > > >
> > > > Docs are omitted as i expect long bikesheds on the syntax and which
> > > > separator char to use
> > > >
> > > > Examples:
> > >
> > > > -vf scale=640:480:src_filter=1#h-0.5#0#0#0#1
> > > > -vf scale=640:480:src_filter=1#c1#1#1#1#1#1#1#1#1#1#1#1#1#1#1#1#1#1
> > > > -vf scale=640:480:src_filter=1#lh-0.2#1#-0.2#lv1#0#0#0#0#0#0#0#0#0#0#0#0#1
> > >
> > > 1. this needs sh escaping, since "#" introduces a comment
> > > 2. in case we extend lavfi graph syntac, this may need escaping inner
> > > escaping since are likely going to use "#" for comments (indeed I
> > > was surprised that we already don't)
> > >
> > > Thus I suggest another separator character. We use "|" in such
> > > cases. Ideally we should have a syntax to represent matrixes / vectors
> > > (e.g. employing Matlab notation), but this would cause escaping hell
> > > since we already use both ";" and ":" in the filtergraph.
> > >
> >
> > > In conclusion "|" seems at the moment preferable.
> >
> > fine with me
> >
> >
> > >
> > > >
> > > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > > ---
> > > > libswscale/options.c | 3 ++
> > > > libswscale/swscale_internal.h | 4 +++
> > > > libswscale/utils.c | 79 +++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 86 insertions(+)
> > > >
> > > > diff --git a/libswscale/options.c b/libswscale/options.c
> > > > index 2b3147b..0ebb2b6 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", "source filter", OFFSET(src_filter_string), AV_OPT_TYPE_STRING, { .str = NULL }, INT_MIN, INT_MAX, VE },
> > > > + { "dst_filter", "destination filter", OFFSET(dst_filter_string), AV_OPT_TYPE_STRING, { .str = NULL }, INT_MIN, INT_MAX, VE },
> > >
> > > set source filter
> > > set destination filter
> > >
> > > > +
> > > > { NULL }
> > > > };
> > > >
> > > > 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 a2e3ce1..29faabb 100644
> > > > --- a/libswscale/utils.c
> > > > +++ b/libswscale/utils.c
> > > > @@ -42,6 +42,7 @@
> > > > #include "libavutil/avutil.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 +239,68 @@ const char *sws_format_name(enum AVPixelFormat format)
> > > > }
> > > > #endif
> > > >
> > > > +static SwsVector *parse_subfilter(const SwsContext *c, const char *str, const char *id)
> > > > +{
> > > > + const char *p;
> > > > + int i;
> > > > + SwsVector *v = av_mallocz(sizeof(SwsVector));
> > > > +
> > > > + p = strstr(str, id);
> > >
> > > not very robust (this will ignore leading characters, possibly
> > > introduced because of typos)
> > >
> > > > + if (p) {
> > > > + p++;
> > > > + } else
> > > > + p = strchr(str, id[0]);
> > >
> > > > + if (!p)
> > > > + p = strchr(str, id[1]);
> > >
> > > this is weird
> >
> > all that is required for the parser in its current form to work
> >
> > there are 4 filters, luma horizontal, vertical and chroma horizontal
> > and vertical
> >
> > filter specifications can be for all 4, for 2 (like luma h+v or for
> > luma+chroma horizontal for example) or for just one specific
> > filter.
> > If there are multiple overlapping specifications, the most specific
> > is used for each of the 4 filters
> > This way you can specify a default and override 1 specific case to
> > get filter A on 3 and B on 1 case.
> >
> > a use case would be to sharpen luma and shift chroma by 1 pixel left
> > to compensate some error
> > (this would need 2 filters to be written while 3 of the 4 would be
> > set)
> > iam sure there exist many such use cases ...
> >
> > also multiple specifications of a filter possibly could/should be
> > combined by convolution to actually apply both filters
> >
> >
> > >
> > > > + if (p)
> > > > + p++;
> > > > + else
> > > > + p = str;
> > > > + for (i=0; ; i++) {
> > > > + char *end;
> > > > + if (av_reallocp(&v->coeff, (i+1) * sizeof(*v->coeff)) < 0)
> > > > + goto fail;
> > > > + v->coeff[i] = av_strtod(p, &end);
> > > > + if (end == p)
> > > > + goto fail;
> > > > + p = (const char *)end;
> > > > + if(*p == '#' && p[1]) {
> > > > + p++;
> > > > + if (*p == 'l' || *p == 'c' || *p == 'h' || *p == 'v')
> > > > + break;
> > > > + } else if(!*p)
> > > > + break;
> > > > + else
> > > > + goto fail;
> > > > + }
> > > > + v->length = i + 1;
> > > > + return v;
> > > > +fail:
> > > > + av_log(c, AV_LOG_ERROR, "parse_subfilter failed at %s\n", p);
> > > > + sws_freeVec(v);
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +static SwsFilter *parse_filter(const SwsContext *c, const char *str)
> > > > +{
> > > > + SwsFilter *f;
> > > > + if (!str)
> > > > + return NULL;
> > > > + f = av_mallocz(sizeof(SwsFilter));
> > > > + if (!f)
> > > > + return NULL;
> > > > + f->lumH = parse_subfilter(c, str, "lh");
> > > > + f->lumV = parse_subfilter(c, str, "lv");
> > > > + f->chrH = parse_subfilter(c, str, "ch");
> > > > + f->chrV = parse_subfilter(c, str, "cv");
> > > > + if (!f->lumH || !f->lumV || !f->chrH || !f->chrV) {
> > > > + sws_freeFilter(f);
> > > > + return NULL;
> > > > + }
> > >
> > > I'd prefer to have something like:
> > >
> > > int parse_vector(SwsVector **vec, enun VectorType *vec_type, char **buf, void *log_ctx) {...}
> > >
> > > int parse_filter(SwsFilter **filt, const char *str, void *log_ctx)
> > > {
> > > SwsFilter *f = alloc_filter();
> > > char *p = str;
> > > while (p && *p) {
> > > SwsVector *v;
> > > ret = parse_vector(&v, ...);
> > > if (ret < 0) ERROR;
> > > switch (vec_type) {
> > > LH: filt->lumH = v; break;
> > > LV: filt->lumV = v; break;
> > > ...
> > > }
> > > }
> > > ...
> > > *filt = f;
> > > return 0;
> > > ...
> > > }
> > >
> > > [...]
> > >
> > > I volunteer to write the parsing code during the weekend if you don't
> > > want to do it yourself.
> >
> > do whatever you prefer as long as it doesnt loose features and isnt
> > more messy than mine
> >
> > Also please check with wm4, iam not sure what he is working on and
> > if that includes a filter string parser
>
> Incomplete and partially untested, for your comments.
>
> Result of the test is:
>
> Incomplete filter specification in '', components lumH lumV chrH chrV are not specified
> -> error
> Vector specification '1+2' contains trailing invalid data
> 1+2 -> error
> 1 -> lumH:1.000000 lumV:1.000000 chrH:1.000000 chrV:1.000000
> h1,2,3v2,3,4 -> lumH:1.000000,2.000000,3.000000 lumV:2.000000,3.000000,4.000000 chrH:1.000000,2.000000,3.000000 chrV:2.000000,3.000000,4.000000
> 0.2,0.123 -> lumH:0.200000,0.123000 lumV:0.200000,0.123000 chrH:0.200000,0.123000 chrV:0.200000,0.123000
> Vector specification '1,23,45,343+3' contains trailing invalid data
> 1,23,45,343+3 -> error
> lh1,2,3c4,5.123 -> lumH:1.000000,2.000000,3.000000 lumV:1.000000,2.000000,3.000000 chrH:4.000000,5.123000 chrV:4.000000,5.123000
> Incomplete filter specification in 'lh1,2,3', components chrV are not specified
> lh1,2,3 -> error
> Incomplete filter specification in 'hl1,2,3', components chrV are not specified
> hl1,2,3 -> error
it might make sense to use "1" as default for unspecified cases
(this can be added later too)
otherwise the syntax looks ok to me
the patch breaks fate though
--- ./tests/ref/fate/force_key_frames 2013-10-09 21:13:09.704657757 +0200
+++ tests/data/fate/force_key_frames 2013-10-14 19:03:04.885594388 +0200
@@ -1,4 +0,0 @@
-5423335cd809e631a2e03f97585348e0 *tests/data/fate/force_key_frames.avi
-113308 tests/data/fate/force_key_frames.avi
-8f68ad2e602ecd87a3e0c097ba99d773 *tests/data/fate/force_key_frames.out.framecrc
-stddev:34363.01 PSNR: 5.61 MAXDIFF:56305 bytes: 7603200/ 186
Test force_key_frames failed. Look at tests/data/fate/force_key_frames.err for details.
make: *** [fate-force_key_frames] Error 1
make: *** Waiting for unfinished jobs....
--- ./tests/ref/acodec/pcm-alaw 2013-10-09 21:13:09.692657757 +0200
+++ tests/data/fate/acodec-pcm-alaw 2013-10-14 19:03:04.897594388 +0200
@@ -1,4 +0,0 @@
-a2dd6a934ec6d5ec901a211652e85227 *tests/data/fate/acodec-pcm-alaw.wav
-529258 tests/data/fate/acodec-pcm-alaw.wav
-f323f7551ffad91de8613f44dcb198b6 *tests/data/fate/acodec-pcm-alaw.out.wav
-stddev: 101.67 PSNR: 56.19 MAXDIFF: 515 bytes: 1058400/ 1058400
Test acodec-pcm-alaw failed. Look at tests/data/fate/acodec-pcm-alaw.err for details.
make: *** [fate-acodec-pcm-alaw] Error 1
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131014/4750fca0/attachment.asc>
More information about the ffmpeg-devel
mailing list