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

[Stefano Sabatini]

On date Sunday 2009-05-17 12:00:14 +0200, Michael Niedermayer encoded:
> 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]

The only alternative I see would be to redefine in the ScaleContext
every single option of SWScaleContext, then map them to the
corresponding SWScaleContext options when setting them, I avoided it
since that looks more complicate.

Would that be acceptable?

> > 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.

OK, thanks for the clarification. 

[...]
> > > >  vf_scale.c |   18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > > 73e499a1b3e71f943252611d96841f033003d9f6  scale-support-cpuflags.patch
> > > > Index: libavfilter-soc/ffmpeg/libavfilter/vf_scale.c
[...]
> > > >  
> > > > +    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

lsws already depends on lavc, at least at compilation stage, since it
depends on libavcodec/opt.h.

What about to move opt.{h,c}+eval.{h,c} to lavu?

Same consideration apply to the CPU detection stuff.

Also pixdesc.{h,c} may be used in lsws to avoid code duplication, so
maybe that could be moved (to lavu or maybe to a new lib
libavpixels?).

Regards.
-- 
FFmpeg = Fostering and Fast Meaningful Puristic Elastic Game