Hi, On Sat, Aug 8, 2015 at 12:58 PM, Paul B Mahol <[email protected]> wrote:
> On 8/8/15, wm4 <[email protected]> wrote: > > On Sat, 8 Aug 2015 18:17:37 +0200 > > Michael Niedermayer <[email protected]> wrote: > > > >> On Sat, Aug 08, 2015 at 05:58:29PM +0200, wm4 wrote: > >> > On Sat, 8 Aug 2015 17:14:58 +0200 > >> > Michael Niedermayer <[email protected]> wrote: > >> > > >> > > From: Michael Niedermayer <[email protected]> > >> > > > >> > > Signed-off-by: Michael Niedermayer <[email protected]> > >> > > --- > >> > > 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. > > +1 More +1 from me. The current sws API is a bloody pain (the double param joke is just one of several terrible examples). Ronald _______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
