[FFmpeg-devel] [PATCH] libavfilter-soc: extend vf_scale.c to make it support colorspace details setting

Michael Niedermayer michaelni
Sun May 17 12:00:14 CEST 2009


On Sun, May 17, 2009 at 10:53:48AM +0200, Stefano Sabatini wrote:
> On date Friday 2009-05-15 21:35:13 +0200, Michael Niedermayer encoded:
> > On Sat, May 02, 2009 at 02:23:42PM +0200, Stefano Sabatini wrote:
> [...]
> > >  vf_scale.c |   29 ++++++++++++++++++++++-------
> > >  1 file changed, 22 insertions(+), 7 deletions(-)
> > > a9bb7a0299379295d9cfbcf45297413d8616201d  scale-add-parse-options-support.patch
> > > Index: libavfilter-soc/ffmpeg/libavfilter/vf_scale.c
> > > ===================================================================
> > > --- libavfilter-soc.orig/ffmpeg/libavfilter/vf_scale.c	2009-05-01 12:33:03.000000000 +0200
> > > +++ libavfilter-soc/ffmpeg/libavfilter/vf_scale.c	2009-05-01 19:48:47.000000000 +0200
> > > @@ -22,11 +22,12 @@
> > >  #include <stdio.h>
> > >  
> > >  #include "avfilter.h"
> > > -#include "libavcodec/opt.h"
> > > +#include "parseutils.h"
> > >  #include "libswscale/swscale.h"
> > >  
> > >  typedef struct
> > >  {
> > > +    const AVClass *av_class;
> > >      struct SwsContext *sws;     ///< software scaler context
> > >  
> > >      /**
> > 
> > > @@ -35,17 +36,29 @@
> > >       *  -1 = keep original aspect
> > >       */
> > >      int w, h;
> > > +    char *sws_flags;
> > 
> > flags is no char*
> >
> > >  
> > >      int sliceY;                 ///< top of current output slice
> > >  } ScaleContext;
> > >  
> > > +#define OFFSET(x) offsetof(ScaleContext, x)
> > > +
> > > +static const AVOption options[]={
> > 
> > > +{"sws_flags", "set SWScaler sws_flags", OFFSET(sws_flags), FF_OPT_TYPE_STRING, 0, CHAR_MIN, CHAR_MAX},
> > 
> > this is outright wrong, flags is no char* not in the actual implementation
> > nor semantically
> 
> Yes but they are set in the SWScaleContext with av_set_string3().  I
> could set all the options in the ScaleContext (redefining the
> SwsContext options in the ScaleContext) but that would be
> brittle/unacceptable.
> 

> Alternatively (as in the libmpcodecs/vf_scale.c implementation), I
> could just pass an int, and let the user fill it using numeric values

you are moving in circles
1. no string
2. no special case for flags, the int still is a special case

grep for sws_flags in your code there should be no match ideally, if
there is one it requires justification of why it cannot be
avoided.
[the cpu stuff may need such a special case but that can be added later]


> which is not very convenient from the UI point of view, while the
> current implmentation allows the use of all the named constants of the
> SwsScaler.
>  

> > > +{NULL},
> > > +};
> > > +
> > 
> > > +AV_DEFINE_CLASS(scale);
> > 
> > i have at least twice rejected this already
> 
> You've never rejected it explicitely, just replied with the error codes:
> MNERROR_SPLIT_THE_PATCH
> MNERROR_NOT_YET_APPROVED

well and i also dont really want to reject it yet but if that the only
way to get rid of it then so be it.
what i want is to defer this code to after +10 filters are in ffmpeg svn
then we can see if it or something else is a good idea, currently you
try to abstract non existing code and it becomes a lot less readable in
the process.


> 
> so I really don't know if I should drop the patch or ping it.
> 
> > > +
> > >  static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> > >  {
> > >      ScaleContext *scale = ctx->priv;
> > >      char sws_opts[256];
> > > -    char *p;
> > >  
> > >      /* default to no scaling */
> > > +    scale->av_class = &scale_class;
> > > +    av_opt_set_defaults2(scale, 0, 0);
> > > +
> > >      scale->w =
> > >      scale->h = 0;
> > >  
> > 
> > > @@ -55,11 +68,11 @@
> > >      if(args)
> > >          sscanf(args, "%d:%d:%255s", &scale->w, &scale->h, sws_opts);
> > >  
> > > -    if ((p = strstr(sws_opts, "sws_flags="))) {
> > > -        char sws_flags[256];
> > > -        sscanf(p, "sws_flags=%255[^:]", sws_flags);
> > > +    if (av_set_options_string(scale, sws_opts, "=", ":") < 0)
> > > +        return -1;
> > >  
> > > -        if (av_set_string3(scale->sws, "sws_flags", sws_flags, 1, NULL) < 0) {
> > > +    if (scale->sws_flags) {
> > > +        if (av_set_string3(scale->sws, "sws_flags", scale->sws_flags, 1, NULL) < 0) {
> > 
> > could we get rid of this sws_flags special case?
> 
> [...]
> > >  vf_scale.c |   18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 73e499a1b3e71f943252611d96841f033003d9f6  scale-support-cpuflags.patch
> > > Index: libavfilter-soc/ffmpeg/libavfilter/vf_scale.c
> > > ===================================================================
> > > --- libavfilter-soc.orig/ffmpeg/libavfilter/vf_scale.c	2009-05-02 13:40:56.000000000 +0200
> > > +++ libavfilter-soc/ffmpeg/libavfilter/vf_scale.c	2009-05-02 14:18:42.000000000 +0200
> > > @@ -23,6 +23,7 @@
> > >  
> > >  #include "avfilter.h"
> > >  #include "parseutils.h"
> > > +#include "libavcodec/dsputil.h"
> > >  #include "libswscale/swscale.h"
> > >  
> > >  typedef struct
> > > @@ -37,6 +38,7 @@
> > >       */
> > >      int w, h;
> > >      char *sws_flags;
> > > +    int auto_cpu_caps;
> > >  
> > >      int sliceY;                 ///< top of current output slice
> > >  } ScaleContext;
> > > @@ -45,11 +47,22 @@
> > >  
> > >  static const AVOption options[]={
> > >  {"sws_flags", "set SWScaler sws_flags", OFFSET(sws_flags), FF_OPT_TYPE_STRING, 0, CHAR_MIN, CHAR_MAX},
> > > +{"auto_cpu_caps", "enable SWScaler CPU caps automatic setting" , OFFSET(auto_cpu_caps), FF_OPT_TYPE_INT, 1, 0, 1},
> > >  {NULL},
> > >  };
> > >  
> > >  AV_DEFINE_CLASS(scale);
> > >  
> > > +static av_cold int64_t get_sws_cpu_caps_flags(void) {
> > > +    int64_t cpu_caps_flags = mm_support();
> > > +
> > > +   return
> > > +       (cpu_caps_flags & FF_MM_MMX     ? SWS_CPU_CAPS_MMX     : 0) |
> > > +       (cpu_caps_flags & FF_MM_MMX2    ? SWS_CPU_CAPS_MMX2    : 0) |
> > > +       (cpu_caps_flags & FF_MM_3DNOW   ? SWS_CPU_CAPS_3DNOW   : 0) |
> > > +       (cpu_caps_flags & FF_MM_ALTIVEC ? SWS_CPU_CAPS_ALTIVEC : 0);
> > > +}
> > > +
> > >  static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> > >  {
> > >      ScaleContext *scale = ctx->priv;
> > > @@ -79,6 +92,11 @@
> > >          }
> > >      }
> > >  
> > > +    if (scale->auto_cpu_caps) {
> > > +        int64_t sws_flags = av_get_int(scale->sws, "sws_flags", NULL);
> > > +        av_set_int(scale->sws, "sws_flags", sws_flags | get_sws_cpu_caps_flags());
> > > +    }
> > > +
> > >      /* sanity check parms */
> > >      if(scale->w <  -1 || scale->h <  -1)
> > >          return -1;
> > 
> > we will have to find a cleaner solution for this and if none is found
> > that means this should stay out of svn until a clean solution is found.
> 
> What about to set an auto_cpu flag in the SwsScaler?

no, we will not make sws depend on lavc

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090517/79caa7d5/attachment.pgp>



More information about the ffmpeg-devel mailing list