| Message ID | 20180807223742.96627-1-k@ylo.ph |
|---|---|
| State | New |
| Headers | show |
On 8/7/2018 7:37 PM, Kyle Swanson wrote: > From: Kyle Swanson <kswanson@netflix.com> > > Signed-off-by: Kyle Swanson <kswanson@netflix.com> > --- > configure | 2 +- > libavfilter/vf_libvmaf.c | 9 ++++++++- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/configure b/configure > index 15a58935de..e718c1531c 100755 > --- a/configure > +++ b/configure > @@ -6089,7 +6089,7 @@ enabled libtwolame && require libtwolame twolame.h twolame_init -ltwolame > die "ERROR: libtwolame must be installed and version must be >= 0.3.10"; } > enabled libv4l2 && require_pkg_config libv4l2 libv4l2 libv4l2.h v4l2_ioctl > enabled libvidstab && require_pkg_config libvidstab "vidstab >= 0.98" vid.stab/libvidstab.h vsMotionDetectInit > -enabled libvmaf && require_pkg_config libvmaf "libvmaf >= 0.6.2" libvmaf.h compute_vmaf > +enabled libvmaf && require_pkg_config libvmaf "libvmaf >= 1.3.9" libvmaf.h compute_vmaf > enabled libvo_amrwbenc && require libvo_amrwbenc vo-amrwbenc/enc_if.h E_IF_init -lvo-amrwbenc > enabled libvorbis && require_pkg_config libvorbis vorbis vorbis/codec.h vorbis_info_init && > require_pkg_config libvorbisenc vorbisenc vorbis/vorbisenc.h vorbis_encode_init > diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c > index 5d47a74375..249e50c720 100644 > --- a/libavfilter/vf_libvmaf.c > +++ b/libavfilter/vf_libvmaf.c > @@ -62,6 +62,9 @@ typedef struct LIBVMAFContext { > int ssim; > int ms_ssim; > char *pool; > + int n_threads; > + int n_subsample; > + int enable_conf_interval; > int error; > } LIBVMAFContext; > > @@ -78,6 +81,9 @@ static const AVOption libvmaf_options[] = { > {"ssim", "Enables computing ssim along with vmaf.", OFFSET(ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS}, > {"ms_ssim", "Enables computing ms-ssim along with vmaf.", OFFSET(ms_ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS}, > {"pool", "Set the pool method to be used for computing vmaf.", OFFSET(pool), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS}, > + {"n_threads", "Set number of threads to be used when computing vmaf.", OFFSET(n_threads), AV_OPT_TYPE_INT, {.i64=0}, 0, UINT_MAX, FLAGS}, > + {"n_subsample", "Set interval for frame subsampling used when computing vmaf.", OFFSET(n_subsample), AV_OPT_TYPE_INT, {.i64=1}, 1, UINT_MAX, FLAGS}, > + {"enable_conf_interval", "Enables confidence interval.", OFFSET(enable_conf_interval), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS}, > { NULL } > }; > > @@ -166,7 +172,8 @@ static void compute_vmaf_score(LIBVMAFContext *s) > read_frame, s, s->model_path, s->log_path, > s->log_fmt, 0, 0, s->enable_transform, > s->phone_model, s->psnr, s->ssim, > - s->ms_ssim, s->pool); > + s->ms_ssim, s->pool, > + s->n_threads, s->n_subsample, s->enable_conf_interval); It would be ideal if this library could stop breaking the API so often. If the current API is not extensible, maybe you should consider introducing a new one that is. > } > > static void *call_vmaf(void *ctx) >
Hi, On Tue, Aug 7, 2018 at 8:02 PM, James Almer <jamrial@gmail.com> wrote: > On 8/7/2018 7:37 PM, Kyle Swanson wrote: > > From: Kyle Swanson <kswanson@netflix.com> > > > > Signed-off-by: Kyle Swanson <kswanson@netflix.com> > > --- > > configure | 2 +- > > libavfilter/vf_libvmaf.c | 9 ++++++++- > > 2 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/configure b/configure > > index 15a58935de..e718c1531c 100755 > > --- a/configure > > +++ b/configure > > @@ -6089,7 +6089,7 @@ enabled libtwolame && require libtwolame > twolame.h twolame_init -ltwolame > > die "ERROR: libtwolame must be installed > and version must be >= 0.3.10"; } > > enabled libv4l2 && require_pkg_config libv4l2 libv4l2 > libv4l2.h v4l2_ioctl > > enabled libvidstab && require_pkg_config libvidstab "vidstab >= > 0.98" vid.stab/libvidstab.h vsMotionDetectInit > > -enabled libvmaf && require_pkg_config libvmaf "libvmaf >= > 0.6.2" libvmaf.h compute_vmaf > > +enabled libvmaf && require_pkg_config libvmaf "libvmaf >= > 1.3.9" libvmaf.h compute_vmaf > > enabled libvo_amrwbenc && require libvo_amrwbenc > vo-amrwbenc/enc_if.h E_IF_init -lvo-amrwbenc > > enabled libvorbis && require_pkg_config libvorbis vorbis > vorbis/codec.h vorbis_info_init && > > require_pkg_config libvorbisenc vorbisenc > vorbis/vorbisenc.h vorbis_encode_init > > diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c > > index 5d47a74375..249e50c720 100644 > > --- a/libavfilter/vf_libvmaf.c > > +++ b/libavfilter/vf_libvmaf.c > > @@ -62,6 +62,9 @@ typedef struct LIBVMAFContext { > > int ssim; > > int ms_ssim; > > char *pool; > > + int n_threads; > > + int n_subsample; > > + int enable_conf_interval; > > int error; > > } LIBVMAFContext; > > > > @@ -78,6 +81,9 @@ static const AVOption libvmaf_options[] = { > > {"ssim", "Enables computing ssim along with vmaf.", > OFFSET(ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS}, > > {"ms_ssim", "Enables computing ms-ssim along with vmaf.", > OFFSET(ms_ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS}, > > {"pool", "Set the pool method to be used for computing vmaf.", > OFFSET(pool), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, > FLAGS}, > > + {"n_threads", "Set number of threads to be used when computing > vmaf.", OFFSET(n_threads), AV_OPT_TYPE_INT, {.i64=0}, 0, > UINT_MAX, FLAGS}, > > + {"n_subsample", "Set interval for frame subsampling used when > computing vmaf.", OFFSET(n_subsample), AV_OPT_TYPE_INT, {.i64=1}, 1, > UINT_MAX, FLAGS}, > > + {"enable_conf_interval", "Enables confidence interval.", > OFFSET(enable_conf_interval), AV_OPT_TYPE_BOOL, {.i64=0}, > 0, 1, FLAGS}, > > { NULL } > > }; > > > > @@ -166,7 +172,8 @@ static void compute_vmaf_score(LIBVMAFContext *s) > > read_frame, s, s->model_path, s->log_path, > > s->log_fmt, 0, 0, s->enable_transform, > > s->phone_model, s->psnr, s->ssim, > > - s->ms_ssim, s->pool); > > + s->ms_ssim, s->pool, > > + s->n_threads, s->n_subsample, > s->enable_conf_interval); > > It would be ideal if this library could stop breaking the API so often. > If the current API is not extensible, maybe you should consider > introducing a new one that is. > Yes, I'm in agreement with you. The libvmaf API hasn't been stable and has more or less been evolving over time. I've had a few discussions with Zhi about a new API that is both more manageable to use and also extensible. For now this is the interface, but I plan to invest some of my time in the future to help with a new API. At that point there will be another breaking change, but it should be more stable going forward. I can also manage the updates to this filter during that time as well. > > > } > > > > static void *call_vmaf(void *ctx) > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > Thanks, Kyle
2018-08-08 5:02 GMT+02:00, James Almer <jamrial@gmail.com>:
> It would be ideal if this library could stop breaking the API so often.
Not that I care much but don't we have a requirement
for external libs to have a stable api?
Carl Eugen
Hi, On Tue, Aug 7, 2018 at 3:37 PM, Kyle Swanson <k@ylo.ph> wrote: > From: Kyle Swanson <kswanson@netflix.com> > > Signed-off-by: Kyle Swanson <kswanson@netflix.com> > --- > configure | 2 +- > libavfilter/vf_libvmaf.c | 9 ++++++++- > 2 files changed, 9 insertions(+), 2 deletions(-) > > I'll update docs and push this sometime in the next 24 hours unless anyone has something. Thanks, Kyle
Hi, > On Aug 9, 2018, at 10:10 AM, Kyle Swanson <k@ylo.ph> wrote: > > Hi, > >> On Tue, Aug 7, 2018 at 3:37 PM, Kyle Swanson <k@ylo.ph> wrote: >> From: Kyle Swanson <kswanson@netflix.com> >> >> Signed-off-by: Kyle Swanson <kswanson@netflix.com> >> --- >> configure | 2 +- >> libavfilter/vf_libvmaf.c | 9 ++++++++- >> 2 files changed, 9 insertions(+), 2 deletions(-) >> > > I'll update docs and push this sometime in the next 24 hours unless anyone has something. > > Thanks, > Kyle Pushed. Thanks. Kyle
2018-08-08 0:37 GMT+02:00, Kyle Swanson <k@ylo.ph>: > + {"n_threads", "Set number of threads to be used when computing vmaf.", > OFFSET(n_threads), AV_OPT_TYPE_INT, {.i64=0}, 0, UINT_MAX, > FLAGS}, Why does this not use -filter_threads? Carl Eugen
Hi, On Mon, Aug 13, 2018 at 11:56 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-08-08 0:37 GMT+02:00, Kyle Swanson <k@ylo.ph>: > > > + {"n_threads", "Set number of threads to be used when computing > vmaf.", > > OFFSET(n_threads), AV_OPT_TYPE_INT, {.i64=0}, 0, UINT_MAX, > > FLAGS}, > > Why does this not use -filter_threads? Didn't know it was an option. I can certainly change the code to use it. I assume this sets AVFilterContext::nb_threads? > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > Thanks, Kyle
2018-08-13 23:12 GMT+02:00, Kyle Swanson <k@ylo.ph>: > Hi, > > On Mon, Aug 13, 2018 at 11:56 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > >> 2018-08-08 0:37 GMT+02:00, Kyle Swanson <k@ylo.ph>: >> >> > + {"n_threads", "Set number of threads to be used when computing >> vmaf.", >> > OFFSET(n_threads), AV_OPT_TYPE_INT, {.i64=0}, 0, UINT_MAX, >> > FLAGS}, >> >> Why does this not use -filter_threads? > > > Didn't know it was an option. > I can certainly change the code to use it. Please do it if it works. Carl Eugen
On 14-08-2018 03:24 AM, Carl Eugen Hoyos wrote: >>> >>> Why does this not use -filter_threads? >> >> >> Didn't know it was an option. > >> I can certainly change the code to use it. > > Please do it if it works. filter_threads applies to all simple filtergraphs. The private option is specifically targeted. Isn't that better? Gyan
On 8/14/18, Gyan Doshi <gyandoshi@gmail.com> wrote: > > > On 14-08-2018 03:24 AM, Carl Eugen Hoyos wrote: > >>>> >>>> Why does this not use -filter_threads? >>> >>> >>> Didn't know it was an option. >> >>> I can certainly change the code to use it. >> >> Please do it if it works. > > filter_threads applies to all simple filtergraphs. The private option is > specifically targeted. Isn't that better? filter_threads is used only for internal threading, so using it for something external seems wrong.
On 14-08-2018 01:20 PM, Paul B Mahol wrote: > On 8/14/18, Gyan Doshi <gyandoshi@gmail.com> wrote: >> >> >> On 14-08-2018 03:24 AM, Carl Eugen Hoyos wrote: >> >>>>> >>>>> Why does this not use -filter_threads? >>>> >>>> >>>> Didn't know it was an option. >>> >>>> I can certainly change the code to use it. >>> >>> Please do it if it works. >> >> filter_threads applies to all simple filtergraphs. The private option is >> specifically targeted. Isn't that better? > > filter_threads is used only for internal threading, so using it for something > external seems wrong. I also realized that libvmaf requires two inputs and should/will be used within a complex filtergraph so -filter_threads wouldn't apply even if wanted. Gyan
2018-08-14 9:57 GMT+02:00, Gyan Doshi <gyandoshi@gmail.com>: > > > On 14-08-2018 01:20 PM, Paul B Mahol wrote: >> On 8/14/18, Gyan Doshi <gyandoshi@gmail.com> wrote: >>> >>> >>> On 14-08-2018 03:24 AM, Carl Eugen Hoyos wrote: >>> >>>>>> >>>>>> Why does this not use -filter_threads? >>>>> >>>>> >>>>> Didn't know it was an option. >>>> >>>>> I can certainly change the code to use it. >>>> >>>> Please do it if it works. >>> >>> filter_threads applies to all simple filtergraphs. The private option >>> is specifically targeted. Isn't that better? I believe the opposite is true. >> filter_threads is used only for internal threading, so using it for >> something external seems wrong. Like the libavcodec option -threads? > I also realized that libvmaf requires two inputs and should/will be used > within a complex filtergraph so -filter_threads wouldn't apply even if > wanted. Isn't it a bug that -filter_threads does not apply for -filter_complex? Carl Eugen
On 17-08-2018 06:12 PM, Carl Eugen Hoyos wrote: > 2018-08-14 9:57 GMT+02:00, Gyan Doshi <gyandoshi@gmail.com>: >>>> >>>> filter_threads applies to all simple filtergraphs. The private option >>>> is specifically targeted. Isn't that better? > > I believe the opposite is true. Which - preference for private option or application of filter_threads? > Isn't it a bug that -filter_threads does not apply for -filter_complex? We have -filter_complex_threads for that, so no. Gyan
2018-08-17 15:15 GMT+02:00, Gyan Doshi <gyandoshi@gmail.com>: > > > On 17-08-2018 06:12 PM, Carl Eugen Hoyos wrote: >> 2018-08-14 9:57 GMT+02:00, Gyan Doshi <gyandoshi@gmail.com>: > >>>>> >>>>> filter_threads applies to all simple filtergraphs. The private option >>>>> is specifically targeted. Isn't that better? >> >> I believe the opposite is true. > > Which - preference for private option or application of filter_threads? I believe that if a general option exists (as in this case), it is a bad idea to have a specifically targeted option that has to be used instead. >> Isn't it a bug that -filter_threads does not apply for -filter_complex? > > We have -filter_complex_threads for that, so no. For which use-case is this an advantage? Carl Eugen
On 20-08-2018 03:17 AM, Carl Eugen Hoyos wrote: > I believe that if a general option exists (as in this case), it is a bad > idea to have a specifically targeted option that has to be used instead. The user may not want the thread pool to be available for any other threaded filters in the graph. Encoding/decoding threads can have stream specifiers suffixed to limit scope. The filter_[complex_]threads options don't. >> We have -filter_complex_threads for that, so no. > > For which use-case is this an advantage? For when the intended recipient is a complex filtergraph. Gyan
2018-08-20 7:15 GMT+02:00, Gyan Doshi <gyandoshi@gmail.com>: > On 20-08-2018 03:17 AM, Carl Eugen Hoyos wrote: > >> I believe that if a general option exists (as in this case), it is a bad >> idea to have a specifically targeted option that has to be used instead. > > The user may not want the thread pool to be available for any other > threaded filters in the graph. I wonder if this is really useful (and especially if the cost of having an additional option for the user to know is really worth this tiny advantage). The more I think about it, the more it is obvious to me that 1) the filter should use the default thread number for all filters and that 2) an option may be specified to overwrite this number (if this is really needed). > Encoding/decoding threads can have stream specifiers suffixed to > limit scope. The filter_[complex_]threads options don't. Ok. >>> We have -filter_complex_threads for that, so no. >> >> For which use-case is this an advantage? > > For when the intended recipient is a complex filtergraph. For which use-case is it an advantage that there is not one option to tell the filter-chain the number of threads to use, no matter if it is a simple or a complex filter-chain, but to have one option to use for the simple and a different option for the complex case? Carl Eugen
On 8/20/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-08-20 7:15 GMT+02:00, Gyan Doshi <gyandoshi@gmail.com>: >> On 20-08-2018 03:17 AM, Carl Eugen Hoyos wrote: >> >>> I believe that if a general option exists (as in this case), it is a bad >>> idea to have a specifically targeted option that has to be used instead. >> >> The user may not want the thread pool to be available for any other >> threaded filters in the graph. > > I wonder if this is really useful (and especially if the cost of having > an additional option for the user to know is really worth this tiny > advantage). > > The more I think about it, the more it is obvious to me that 1) the > filter should use the default thread number for all filters and that 2) > an option may be specified to overwrite this number (if this is really > needed). Both cases are already present. Please learn to read and understand documentation. > >> Encoding/decoding threads can have stream specifiers suffixed to >> limit scope. The filter_[complex_]threads options don't. > > Ok. > >>>> We have -filter_complex_threads for that, so no. >>> >>> For which use-case is this an advantage? >> >> For when the intended recipient is a complex filtergraph. > > For which use-case is it an advantage that there is not one > option to tell the filter-chain the number of threads to use, > no matter if it is a simple or a complex filter-chain, but to > have one option to use for the simple and a different option > for the complex case? Again, there is already option, please consult documentation. This filter does not use lavfi threads so it should not use lavfi threads option.
2018-08-20 19:09 GMT+02:00, Paul B Mahol <onemda@gmail.com>: > On 8/20/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2018-08-20 7:15 GMT+02:00, Gyan Doshi <gyandoshi@gmail.com>: >>> On 20-08-2018 03:17 AM, Carl Eugen Hoyos wrote: >>>>> We have -filter_complex_threads for that, so no. >>>> >>>> For which use-case is this an advantage? >>> >>> For when the intended recipient is a complex filtergraph. >> >> For which use-case is it an advantage that there is not one >> option to tell the filter-chain the number of threads to use, >> no matter if it is a simple or a complex filter-chain, but to >> have one option to use for the simple and a different option >> for the complex case? > > Again, there is already option, please consult documentation. How is this related to the question? > This filter does not use lavfi threads so it should not use lavfi > threads option. Not sure if I misunderstand but my question above is not related to any filter. Carl Eugen
diff --git a/configure b/configure index 15a58935de..e718c1531c 100755 --- a/configure +++ b/configure @@ -6089,7 +6089,7 @@ enabled libtwolame && require libtwolame twolame.h twolame_init -ltwolame die "ERROR: libtwolame must be installed and version must be >= 0.3.10"; } enabled libv4l2 && require_pkg_config libv4l2 libv4l2 libv4l2.h v4l2_ioctl enabled libvidstab && require_pkg_config libvidstab "vidstab >= 0.98" vid.stab/libvidstab.h vsMotionDetectInit -enabled libvmaf && require_pkg_config libvmaf "libvmaf >= 0.6.2" libvmaf.h compute_vmaf +enabled libvmaf && require_pkg_config libvmaf "libvmaf >= 1.3.9" libvmaf.h compute_vmaf enabled libvo_amrwbenc && require libvo_amrwbenc vo-amrwbenc/enc_if.h E_IF_init -lvo-amrwbenc enabled libvorbis && require_pkg_config libvorbis vorbis vorbis/codec.h vorbis_info_init && require_pkg_config libvorbisenc vorbisenc vorbis/vorbisenc.h vorbis_encode_init diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c index 5d47a74375..249e50c720 100644 --- a/libavfilter/vf_libvmaf.c +++ b/libavfilter/vf_libvmaf.c @@ -62,6 +62,9 @@ typedef struct LIBVMAFContext { int ssim; int ms_ssim; char *pool; + int n_threads; + int n_subsample; + int enable_conf_interval; int error; } LIBVMAFContext; @@ -78,6 +81,9 @@ static const AVOption libvmaf_options[] = { {"ssim", "Enables computing ssim along with vmaf.", OFFSET(ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS}, {"ms_ssim", "Enables computing ms-ssim along with vmaf.", OFFSET(ms_ssim), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS}, {"pool", "Set the pool method to be used for computing vmaf.", OFFSET(pool), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 1, FLAGS}, + {"n_threads", "Set number of threads to be used when computing vmaf.", OFFSET(n_threads), AV_OPT_TYPE_INT, {.i64=0}, 0, UINT_MAX, FLAGS}, + {"n_subsample", "Set interval for frame subsampling used when computing vmaf.", OFFSET(n_subsample), AV_OPT_TYPE_INT, {.i64=1}, 1, UINT_MAX, FLAGS}, + {"enable_conf_interval", "Enables confidence interval.", OFFSET(enable_conf_interval), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS}, { NULL } }; @@ -166,7 +172,8 @@ static void compute_vmaf_score(LIBVMAFContext *s) read_frame, s, s->model_path, s->log_path, s->log_fmt, 0, 0, s->enable_transform, s->phone_model, s->psnr, s->ssim, - s->ms_ssim, s->pool); + s->ms_ssim, s->pool, + s->n_threads, s->n_subsample, s->enable_conf_interval); } static void *call_vmaf(void *ctx)