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

Michael Niedermayer michaelni
Mon May 18 03:03:00 CEST 2009


On Sun, May 17, 2009 at 01:33:48PM +0200, Stefano Sabatini wrote:
> 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?

no you seem to be missing the point
i want, none, ZERO not even a trace of any of these options in the filter
the filter should pass the stuff generically to swscale one way or another
This may not be achiveable and compromises might be needed but what your
patches do goes too far ...

The various ways by which you dupllicate the AVOption list from swscale
is why iam so unhappy with this patch

I dont know if it can be done without changes to swscale ...


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

compilation and runtime are 2 totally seperate things


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

not today and not because of this patch.


> 
> Same consideration apply to the CPU detection stuff.

remove it -> one issue less that complicates this patch


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

iam not going to approve any new libs in relation to this patch.
libmpcodecs surely can use swscale without such stuff, libavfilter
should too

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20090518/0a1ed9d2/attachment.pgp>



More information about the ffmpeg-devel mailing list