[FFmpeg-devel] avfilter:vf_libvmaf: fix errors while running with psnr=1 and improve docs

Submitted by Ashish Singh on Oct. 11, 2017, 1:53 p.m.

Details

Message ID 20171011135348.4347-1-ashk43712@gmail.com
State New
Headers show

Commit Message

Ashish Singh Oct. 11, 2017, 1:53 p.m.
From: ashk43712 <ashk43712@gmail.com>

Hi, this patch fixes the seg fault which ocuured while running libvmaf filter
with option psnr=1. This also improves libvmaf doc a bit.

Signed-off-by: ashk43712 <ashk43712@gmail.com>
---
 doc/filters.texi         | 19 ++++++-------------
 libavfilter/vf_libvmaf.c |  7 ++++---
 2 files changed, 10 insertions(+), 16 deletions(-)

Comments

Carl Eugen Hoyos Oct. 12, 2017, 8:43 p.m.
2017-10-11 15:53 GMT+02:00 Ashish Pratap Singh <ashk43712@gmail.com>:

> Hi, this patch fixes the seg fault which ocuured while running libvmaf filter
> with option psnr=1. This also improves libvmaf doc a bit.

Please split in two patches.

Carl Eugen
Ronald S. Bultje Nov. 6, 2017, 12:52 p.m.
Hi,

On Thu, Oct 12, 2017 at 4:43 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
wrote:

> 2017-10-11 15:53 GMT+02:00 Ashish Pratap Singh <ashk43712@gmail.com>:
>
> > Hi, this patch fixes the seg fault which ocuured while running libvmaf
> filter
> > with option psnr=1. This also improves libvmaf doc a bit.
>
> Please split in two patches.


Split, and pushed.

Ronald
Carl Eugen Hoyos Nov. 6, 2017, 1:21 p.m.
2017-11-06 13:52 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
> Hi,
>
> On Thu, Oct 12, 2017 at 4:43 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>
>> 2017-10-11 15:53 GMT+02:00 Ashish Pratap Singh <ashk43712@gmail.com>:
>>
>> > Hi, this patch fixes the seg fault which ocuured while running libvmaf
>> > filter with option psnr=1. This also improves libvmaf doc a bit.
>>
>> Please split in two patches.
>
> Split, and pushed.

+    format = (char *) s->desc->name;

Isn't this cast a bad idea?

Thank you for reviving this, Carl Eugen
Ashish Singh Nov. 6, 2017, 2:09 p.m.
Hi,

On Nov 6, 2017 18:22, "Ronald S. Bultje" <rsbultje@gmail.com> wrote:

Hi,

On Thu, Oct 12, 2017 at 4:43 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
wrote:

> 2017-10-11 15:53 GMT+02:00 Ashish Pratap Singh <ashk43712@gmail.com>:
>
> > Hi, this patch fixes the seg fault which ocuured while running libvmaf
> filter
> > with option psnr=1. This also improves libvmaf doc a bit.
>
> Please split in two patches.


Split, and pushed.

Ronald

Thanks.
Ronald S. Bultje Nov. 6, 2017, 2:09 p.m.
Hi,

On Mon, Nov 6, 2017 at 8:21 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-11-06 13:52 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
> > Hi,
> >
> > On Thu, Oct 12, 2017 at 4:43 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
> > wrote:
> >
> >> 2017-10-11 15:53 GMT+02:00 Ashish Pratap Singh <ashk43712@gmail.com>:
> >>
> >> > Hi, this patch fixes the seg fault which ocuured while running libvmaf
> >> > filter with option psnr=1. This also improves libvmaf doc a bit.
> >>
> >> Please split in two patches.
> >
> > Split, and pushed.
>
> +    format = (char *) s->desc->name;
>
> Isn't this cast a bad idea?


It's an API thing. We can either strdup(), or use a cast, or have a
compiler warning. libvmaf does not modify the argument even if it doesn't
use const here.

Ronald
Carl Eugen Hoyos Nov. 6, 2017, 6:58 p.m.
2017-11-06 15:09 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
> Hi,
>
> On Mon, Nov 6, 2017 at 8:21 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-11-06 13:52 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:

>> > Split, and pushed.
>>
>> +    format = (char *) s->desc->name;
>>
>> Isn't this cast a bad idea?
>
> It's an API thing. We can either strdup(), or use a cast, or have a
> compiler warning.

Yes.

> libvmaf does not modify the argument even if it doesn't
> use const here.

I hoped (and assumed) so.

What I meant was:
It is one thing to know that within FFmpeg, we do not modify
a pointer target and therefore decide to cast, but isn't it
another (bad) thing to assume this for an external library?

Carl Eugen
Ronald S. Bultje Nov. 6, 2017, 8:27 p.m.
Hi,

On Mon, Nov 6, 2017 at 1:58 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> It is one thing to know that within FFmpeg, we do not modify
> a pointer target and therefore decide to cast, but isn't it
> another (bad) thing to assume this for an external library?


I'm guessing that what you mean is "libvmaf may not touch it right now, but
the API doesn't guarantee that such an implementation detail doesn't change
in the future", right?

Yes, you're right. Potential solutions are to make libvmaf const-correct or
to strdup in our implementation. I'm fine with both, although I'd prefer if
we didn't strdup every frame, i.e. move it to some global init/uninit
function.

Ronald
Carl Eugen Hoyos Nov. 6, 2017, 9:58 p.m.
2017-11-06 21:27 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
> Hi,
>
> On Mon, Nov 6, 2017 at 1:58 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> It is one thing to know that within FFmpeg, we do not modify
>> a pointer target and therefore decide to cast, but isn't it
>> another (bad) thing to assume this for an external library?
>
> I'm guessing that what you mean is "libvmaf may not touch it right now, but
> the API doesn't guarantee that such an implementation detail doesn't change
> in the future", right?
>
> Yes, you're right. Potential solutions are to make libvmaf const-correct or
> to strdup in our implementation.

> I'm fine with both

Me too.

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/doc/filters.texi b/doc/filters.texi
index e26dde4b1a..90790c2261 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -9885,25 +9885,16 @@  distances from the focal point in the source and target images, respectively.
 
 @section libvmaf
 
-Obtain the average VMAF (Video Multi-Method Assessment Fusion)
+Obtain the VMAF (Video Multi-Method Assessment Fusion)
 score between two input videos.
 
-This filter takes two input videos.
-
-Both video inputs must have the same resolution and pixel format for
-this filter to work correctly. Also it assumes that both inputs
-have the same number of frames, which are compared one by one.
-
-The obtained average VMAF score is printed through the logging system.
+The obtained VMAF score is printed through the logging system.
 
 It requires Netflix's vmaf library (libvmaf) as a pre-requisite.
 After installing the library it can be enabled using:
 @code{./configure --enable-libvmaf}.
 If no model path is specified it uses the default model: @code{vmaf_v0.6.1.pkl}.
 
-On the below examples the input file @file{main.mpg} being processed is
-compared with the reference file @file{ref.mpg}.
-
 The filter has following options:
 
 @table @option
@@ -9934,12 +9925,14 @@  Enables computing ssim along with vmaf.
 Enables computing ms_ssim along with vmaf.
 
 @item pool
-Set the pool method to be used for computing vmaf.
+Set the pool method (mean, min or harmonic mean) to be used for computing vmaf.
 @end table
 
 This filter also supports the @ref{framesync} options.
 
-For example:
+On the below examples the input file @file{main.mpg} being processed is
+compared with the reference file @file{ref.mpg}.
+
 @example
 ffmpeg -i main.mpg -i ref.mpg -lavfi libvmaf -f null -
 @end example
diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c
index 2c3a9f3349..7670c51e21 100644
--- a/libavfilter/vf_libvmaf.c
+++ b/libavfilter/vf_libvmaf.c
@@ -40,7 +40,6 @@  typedef struct LIBVMAFContext {
     const AVClass *class;
     FFFrameSync fs;
     const AVPixFmtDescriptor *desc;
-    char *format;
     int width;
     int height;
     double vmaf_score;
@@ -149,6 +148,7 @@  static void compute_vmaf_score(LIBVMAFContext *s)
 {
     int (*read_frame)(float *ref_data, float *main_data, float *temp_data,
                       int stride, void *ctx);
+    char *format;
 
     if (s->desc->comp[0].depth <= 8) {
         read_frame = read_frame_8bit;
@@ -156,7 +156,9 @@  static void compute_vmaf_score(LIBVMAFContext *s)
         read_frame = read_frame_10bit;
     }
 
-    s->vmaf_score = compute_vmaf(s->format, s->width, s->height, read_frame, 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);
@@ -258,7 +260,6 @@  static int config_input_ref(AVFilterLink *inlink)
     return 0;
 }
 
-
 static int config_output(AVFilterLink *outlink)
 {
     AVFilterContext *ctx = outlink->src;