| Message ID | 1512783313-97469-1-git-send-email-rsbultje@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 12/9/2017 7:05 AM, Ronald S. Bultje wrote: > Fixes trac issue #6884 and Netflix/vmaf issue #124. > static void *call_vmaf(void *ctx) > { > ... > pthread_exit(NULL); > } Fails to build unless I add 'return NULL;' in call_vmaf. Pre-existing issue. Open ticket is #6878. Is it possible to have distinctive error messages? I get the same message whether the model is not found or found but not successfully parsed. Pasted below. Earlier, the console would have the exception reporting if "No newline at end o string". The error for missing file was generic, like now. ---- t C:\avutils\ffmpeg-libs\compiled\share\model\nflxtrain_vmfv3a.pkl cannot be read successfully. Caught VmafException: Error loading model (.pkl): Trouble reading the file:C:\avutils\ffmpeg-libs\compiled\share\model\nflxtrain_vmfv3a.pkl [Parsed_libvmaf_0 @ 000000000050b4a0] libvmaf encountered an error, check log for details Error while filtering: Invalid argument Failed to inject frame into filter network: Invalid argument Error while processing the decoded data for stream #1:0 Conversion failed! ---- But no more "This application has requested the Runtime to terminate it in an unusual way.". Thanks. Regards, Gyan
Hi, On Sat, Dec 9, 2017 at 2:34 AM, Gyan Doshi <gyandoshi@gmail.com> wrote: > > On 12/9/2017 7:05 AM, Ronald S. Bultje wrote: > >> Fixes trac issue #6884 and Netflix/vmaf issue #124. >> > > static void *call_vmaf(void *ctx) >> { >> > > ... > >> pthread_exit(NULL); >> } >> > > Fails to build unless I add 'return NULL;' in call_vmaf. Pre-existing > issue. Open ticket is #6878. > I can fix that in the same patch also, sure. Is it possible to have distinctive error messages? I get the same message > whether the model is not found or found but not successfully parsed. Pasted > below. Earlier, the console would have the exception reporting if "No > newline at end o string". The error for missing file was generic, like now. > > ---- > t C:\avutils\ffmpeg-libs\compiled\share\model\nflxtrain_vmfv3a.pkl cannot > be read successfully. > Caught VmafException: Error loading model (.pkl): Trouble reading the > file:C:\avutils\ffmpeg-libs\compiled\share\model\nflxtrain_vmfv3a.pkl > [Parsed_libvmaf_0 @ 000000000050b4a0] libvmaf encountered an error, check > log for details > Error while filtering: Invalid argument > Failed to inject frame into filter network: Invalid argument > Error while processing the decoded data for stream #1:0 > Conversion failed! > ---- > > But no more "This application has requested the Runtime to terminate it in > an unusual way.". Thanks. This should be done in libvmaf. Right now it returns 0 or 1, and that's all FFmpeg knows. If we want more, we have to ask Zhi to return more specific error codes (or messages), so we can display more specific error messages. Ronald
2017-12-09 2:35 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > Fixes trac issue #6884 and Netflix/vmaf issue #124. > - s->vmaf_score = compute_vmaf(format, s->width, s->height, 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->error = compute_vmaf(&s->vmaf_score, format, s->width, s->height, > + 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); Sorry if I miss something: This looks completely broken to me, how can the same function of the same external library have two different amounts of arguments (in C)? Carl Eugen
Carl Eugen Hoyos (2017-12-10): > Sorry if I miss something: > This looks completely broken to me, how can the same function > of the same external library have two different amounts of > arguments (in C)? https://github.com/Netflix/vmaf/commit/96b99b9416135a0dfeb42c3b148852cce2b3f531 This commit makes an incompatible API change and ABI change. I hope the libvmaf developers will learn not to do that before considering the library stable. Regards,
Hi Ronald, When do you expect to apply this? Regards, Gyan
Hi, On Mon, Dec 18, 2017 at 12:28 AM, Gyan Doshi <gyandoshi@gmail.com> wrote: > Hi Ronald, > > When do you expect to apply this? Oops, forgot; pushed. Nicolas makes a good point that we may want to add a version check for libvmaf, but that can be done separately... Ronald
Hi, On Mon, Dec 18, 2017 at 6:31 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-12-18 14:00 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > Hi, > > > > On Mon, Dec 18, 2017 at 12:28 AM, Gyan Doshi wrote: > > > >> Hi Ronald, > >> > >> When do you expect to apply this? > > > Oops, forgot; pushed. > > Sorry if my email was really so unclear! > > Even by my (low) standards, this patch was horrible and should > not have been committed, sorry if you disagree and sorry if you > believe I didn't make this clear in my first email. > > Please fix this mess, see #6921. > > Thank you, Carl Eugen > Please keep discussions on the list. I've mentioned (in my earlier email) that this could indeed use a version bump in libvmaf. If #6921 is related to this patch, updating libvmaf would probably be the correct solution both before and after such a version bump. Ronald
2017-12-19 0:49 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > Hi, > > On Mon, Dec 18, 2017 at 6:31 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > >> 2017-12-18 14:00 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >> > Hi, >> > >> > On Mon, Dec 18, 2017 at 12:28 AM, Gyan Doshi wrote: >> > >> >> Hi Ronald, >> >> >> >> When do you expect to apply this? >> >> > Oops, forgot; pushed. >> >> Sorry if my email was really so unclear! >> >> Even by my (low) standards, this patch was horrible and should >> not have been committed, sorry if you disagree and sorry if you >> believe I didn't make this clear in my first email. >> >> Please fix this mess, see #6921. >> >> Thank you, Carl Eugen >> > > Please keep discussions on the list. > > I've mentioned (in my earlier email) that this could indeed use a version > bump in libvmaf. If #6921 is related to this patch, updating libvmaf would > probably be the correct solution both before and after such a version bump. How is this related to the missing configure check? Carl Eugen
On 12/8/2017 10:35 PM, Ronald S. Bultje wrote: > Fixes trac issue #6884 and Netflix/vmaf issue #124. > --- > libavfilter/vf_libvmaf.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c > index 7670c51..d628b95 100644 > --- a/libavfilter/vf_libvmaf.c > +++ b/libavfilter/vf_libvmaf.c > @@ -61,6 +61,7 @@ typedef struct LIBVMAFContext { > int ssim; > int ms_ssim; > char *pool; > + int error; > } LIBVMAFContext; > > #define OFFSET(x) offsetof(LIBVMAFContext, x) > @@ -158,17 +159,24 @@ static void compute_vmaf_score(LIBVMAFContext *s) > > format = (char *) s->desc->name; > > - s->vmaf_score = compute_vmaf(format, s->width, s->height, 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->error = compute_vmaf(&s->vmaf_score, format, s->width, s->height, This is an awful API break. It broke compilation for everyone using the latest stable release of libvmaf. See ticket #6921. How hard was for them to add a new function instead? compute_vmaf_score() or whatever. You can't just go and change a public function signature like this... This can still implement this properly before a new release is tagged, for that matter. Could you suggest that to them? > + 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); > } > > static void *call_vmaf(void *ctx) > { > LIBVMAFContext *s = (LIBVMAFContext *) ctx; > compute_vmaf_score(s); > - av_log(ctx, AV_LOG_INFO, "VMAF score: %f\n",s->vmaf_score); > + if (!s->error) { > + av_log(ctx, AV_LOG_INFO, "VMAF score: %f\n",s->vmaf_score); > + } else { > + pthread_mutex_lock(&s->lock); > + pthread_cond_signal(&s->cond); > + pthread_mutex_unlock(&s->lock); > + } > pthread_exit(NULL); > } > > @@ -187,10 +195,17 @@ static int do_vmaf(FFFrameSync *fs) > > pthread_mutex_lock(&s->lock); > > - while (s->frame_set != 0) { > + while (s->frame_set && !s->error) { > pthread_cond_wait(&s->cond, &s->lock); > } > > + if (s->error) { > + av_log(ctx, AV_LOG_ERROR, > + "libvmaf encountered an error, check log for details\n"); > + pthread_mutex_unlock(&s->lock); > + return AVERROR(EINVAL); > + } > + > av_frame_ref(s->gref, ref); > av_frame_ref(s->gmain, main); > > @@ -208,6 +223,7 @@ static av_cold int init(AVFilterContext *ctx) > > s->gref = av_frame_alloc(); > s->gmain = av_frame_alloc(); > + s->error = 0; > > pthread_mutex_init(&s->lock, NULL); > pthread_cond_init (&s->cond, NULL); >
Hi, On Mon, Dec 18, 2017 at 11:55 PM, James Almer <jamrial@gmail.com> wrote: > On 12/8/2017 10:35 PM, Ronald S. Bultje wrote: > > Fixes trac issue #6884 and Netflix/vmaf issue #124. > > --- > > libavfilter/vf_libvmaf.c | 28 ++++++++++++++++++++++------ > > 1 file changed, 22 insertions(+), 6 deletions(-) > > > > diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c > > index 7670c51..d628b95 100644 > > --- a/libavfilter/vf_libvmaf.c > > +++ b/libavfilter/vf_libvmaf.c > > @@ -61,6 +61,7 @@ typedef struct LIBVMAFContext { > > int ssim; > > int ms_ssim; > > char *pool; > > + int error; > > } LIBVMAFContext; > > > > #define OFFSET(x) offsetof(LIBVMAFContext, x) > > @@ -158,17 +159,24 @@ static void compute_vmaf_score(LIBVMAFContext *s) > > > > format = (char *) s->desc->name; > > > > - s->vmaf_score = compute_vmaf(format, s->width, s->height, > 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->error = compute_vmaf(&s->vmaf_score, format, s->width, s->height, > > This is an awful API break. It broke compilation for everyone using the > latest stable release of libvmaf. See ticket #6921. > How hard was for them to add a new function instead? > compute_vmaf_score() or whatever. You can't just go and change a public > function signature like this... > > This can still implement this properly before a new release is tagged, > for that matter. Could you suggest that to them? Both requests made in https://github.com/Netflix/vmaf/issues/124. Ronald
diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c index 7670c51..d628b95 100644 --- a/libavfilter/vf_libvmaf.c +++ b/libavfilter/vf_libvmaf.c @@ -61,6 +61,7 @@ typedef struct LIBVMAFContext { int ssim; int ms_ssim; char *pool; + int error; } LIBVMAFContext; #define OFFSET(x) offsetof(LIBVMAFContext, x) @@ -158,17 +159,24 @@ static void compute_vmaf_score(LIBVMAFContext *s) format = (char *) s->desc->name; - s->vmaf_score = compute_vmaf(format, s->width, s->height, 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->error = compute_vmaf(&s->vmaf_score, format, s->width, s->height, + 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); } static void *call_vmaf(void *ctx) { LIBVMAFContext *s = (LIBVMAFContext *) ctx; compute_vmaf_score(s); - av_log(ctx, AV_LOG_INFO, "VMAF score: %f\n",s->vmaf_score); + if (!s->error) { + av_log(ctx, AV_LOG_INFO, "VMAF score: %f\n",s->vmaf_score); + } else { + pthread_mutex_lock(&s->lock); + pthread_cond_signal(&s->cond); + pthread_mutex_unlock(&s->lock); + } pthread_exit(NULL); } @@ -187,10 +195,17 @@ static int do_vmaf(FFFrameSync *fs) pthread_mutex_lock(&s->lock); - while (s->frame_set != 0) { + while (s->frame_set && !s->error) { pthread_cond_wait(&s->cond, &s->lock); } + if (s->error) { + av_log(ctx, AV_LOG_ERROR, + "libvmaf encountered an error, check log for details\n"); + pthread_mutex_unlock(&s->lock); + return AVERROR(EINVAL); + } + av_frame_ref(s->gref, ref); av_frame_ref(s->gmain, main); @@ -208,6 +223,7 @@ static av_cold int init(AVFilterContext *ctx) s->gref = av_frame_alloc(); s->gmain = av_frame_alloc(); + s->error = 0; pthread_mutex_init(&s->lock, NULL); pthread_cond_init (&s->cond, NULL);