[FFmpeg-devel] [PATCH] swscale: Make sws_alloc_set_opts() public

[wm4]

On Sat, 8 Aug 2015 18:17:37 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Sat, Aug 08, 2015 at 05:58:29PM +0200, wm4 wrote:
> > On Sat,  8 Aug 2015 17:14:58 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > From: Michael Niedermayer <michael at niedermayer.cc>
> > > 
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libswscale/swscale.h          |   11 +++++++++++
> > >  libswscale/swscale_internal.h |   11 -----------
> > >  libswscale/version.h          |    4 ++--
> > >  3 files changed, 13 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/libswscale/swscale.h b/libswscale/swscale.h
> > > index 903e120..055c9fc 100644
> > > --- a/libswscale/swscale.h
> > > +++ b/libswscale/swscale.h
> > > @@ -161,6 +161,17 @@ int sws_isSupportedEndiannessConversion(enum AVPixelFormat pix_fmt);
> > >  struct SwsContext *sws_alloc_context(void);
> > >  
> > >  /**
> > > + * Allocate and return an SwsContext.
> > > + * This is like sws_getContext() but does not perform the init step, allowing
> > > + * the user to set additional AVOptions.
> > > + *
> > > + * @see sws_getContext()
> > > + */
> > > +struct SwsContext *sws_alloc_set_opts(int srcW, int srcH, enum AVPixelFormat srcFormat,
> > > +                                      int dstW, int dstH, enum AVPixelFormat dstFormat,
> > > +                                      int flags, const double *param);
> > > +
> > 
> > This looks excessively useless and un-nice to use.
> 
> what do you suggest ?
> 
> The function is intended to replace:
> 
> context = sws_alloc_context()
> if (!context)
>     handle error
> 
> ret0 = av_opt_set_int(context, "sws_flags",  flags, 0);
> ret1 = av_opt_set_int(context, "srcw",       srcW, 0);
> ret2 = av_opt_set_int(context, "srch",       srcH, 0);
> ret3 = av_opt_set_int(context, "dstw",       dstW, 0);
> ret4 = av_opt_set_int(context, "dsth",       dstH, 0);
> ret5 = av_opt_set_int(context, "src_format", srcFormat, 0);
> ret6 = av_opt_set_int(context, "dst_format", dstFormat, 0);
> ret7 = av_opt_set(context, "alphablend", "none", 0);

This does look better to me than a function which has some required
parameters (and at least 1 optional one!), but not all parameters which
are reasonably needed for general pixfmt conversion. Where are the
input and output colorspaces and ranges set?

The "param" parameter is the worst part if this proposed function. What
does it even mean? I don't know because I didn't read the
implementation.

> if (ret0 <0)
>     handle error, free context, return ret0
> if (ret1 <0)
>     handle error, free context, return ret1
> if (ret2 <0)
>     handle error, free context, return ret2
> if (ret3 <0)
>     handle error, free context, return ret3
> if (ret4 <0)
>     handle error, free context, return ret4
> if (ret5 <0)
>     handle error, free context, return ret5
> if (ret6 <0)
>     handle error, free context, return ret6
> if (ret7 <0)
>     handle error, free context, return ret7

Oh come on, why would you need to check av_opt_set calls for options
that are guaranteed to exist? The codebase has hundreds of such
"unchecked" calls.

And even if you insist that's it's necessary, you wrote it in a way
that makes it look like such error handling is more code. E.g. you
could write:

if ((ret = av_opt_set_int(...)) < 0) goto error;

(Or even put all av_opt_set calls into one if condition; it's not like
the error code will be useful anyway.)

> by this:
> 
> context = sws_alloc_set_opts(srcW, srcH,  srcFormat, dstW, dstH,  dstFormat, flags, NULL);
> if (!context)
>     handle error
> 
> ret = av_opt_set(context, "alphablend", "none", 0);
> 
> if (ret <0)
>     handle error, free context, return ret
> 
> and that looks alot nicer to me,
> but if someone has a even better idea?
> 
> 

There's my very old patch, which made libswscale transparently set the
parameters of an input AVFrame. This would actually be the simplest API.