On Sat, 8 Aug 2015 18:17:37 +0200 Michael Niedermayer <mich...@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 <michae...@gmx.at> wrote: > > > > > From: Michael Niedermayer <mich...@niedermayer.cc> > > > > > > Signed-off-by: Michael Niedermayer <mich...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
