[FFmpeg-devel] libvmaf: exit gracefully if the library fails.

Submitted by Ronald S. Bultje on Dec. 9, 2017, 1:35 a.m.

Details

Message ID 1512783313-97469-1-git-send-email-rsbultje@gmail.com
State New
Headers show

Commit Message

Ronald S. Bultje Dec. 9, 2017, 1:35 a.m.
Fixes trac issue #6884 and Netflix/vmaf issue #124.
---
 libavfilter/vf_libvmaf.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Comments

Gyan Dec. 9, 2017, 7:34 a.m.
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
Ronald S. Bultje Dec. 9, 2017, 1:47 p.m.
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
Carl Eugen Hoyos Dec. 10, 2017, 1:53 p.m.
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
Nicolas George Dec. 10, 2017, 1:56 p.m.
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,
Gyan Dec. 18, 2017, 5:28 a.m.
Hi Ronald,

When do you expect to apply this?

Regards,
Gyan
Ronald S. Bultje Dec. 18, 2017, 1 p.m.
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
Ronald S. Bultje Dec. 18, 2017, 11:49 p.m.
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
Carl Eugen Hoyos Dec. 18, 2017, 11:53 p.m.
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
James Almer Dec. 19, 2017, 4:55 a.m.
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);
>
Ronald S. Bultje Dec. 19, 2017, 2:55 p.m.
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

Patch hide | download patch | download mbox

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);