[FFmpeg-devel] Bug in VMAF calculation for 10 bit comparison

[FFmpeg-devel] Bug in VMAF calculation for 10 bit comparison

[christian.feldmann]

Hi,

I was debugging some VMAF calculation in ffmpeg when I stumbled on this bug in the latest master commit (9450f759748d02d1d284d2e4afd741cb0fe0c04a). Its in libavfilter/vf_libvmaf.c::109. The function copy_picture_data does only work correctly for 8 bit input. For 10 bit input, only half of the width of the input picture is copied. But for 10 bit inputs, 2 bytes per pixel must be copied. Please see this patch:

 

libavfilter/vf_libvmaf.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)

 

diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c

index eee1c280ef..8d5ba4e2d5 100644

--- a/libavfilter/vf_libvmaf.c

+++ b/libavfilter/vf_libvmaf.c

@@ -108,6 +108,7 @@ static enum VmafPixelFormat pix_fmt_map(enum AVPixelFormat av_pix_fmt)

 static int copy_picture_data(AVFrame *src, VmafPicture *dst, unsigned bpc)

{

+    const int bytes_per_value = bpc > 8 ? 2 : 1;

     int err = vmaf_picture_alloc(dst, pix_fmt_map(src->format), bpc,

                                  src->width, src->height);

     if (err)

@@ -117,7 +118,7 @@ static int copy_picture_data(AVFrame *src, VmafPicture *dst, unsigned bpc)

         uint8_t *src_data = src->data[i];

         uint8_t *dst_data = dst->data[i];

         for (unsigned j = 0; j < dst->h[i]; j++) {

-            memcpy(dst_data, src_data, sizeof(*dst_data) * dst->w[i]);

+            memcpy(dst_data, src_data, bytes_per_value * dst->w[i]);

             src_data += src->linesize[i];

             dst_data += dst->stride[i];

         }

--

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Bug in VMAF calculation for 10 bit comparison

[Paul B Mahol]

On 9/14/22, christian.feldmann@gmx.de <christian.feldmann@gmx.de> wrote:
> Hi,
>
> I was debugging some VMAF calculation in ffmpeg when I stumbled on this bug
> in the latest master commit (9450f759748d02d1d284d2e4afd741cb0fe0c04a). Its
> in libavfilter/vf_libvmaf.c::109. The function copy_picture_data does only
> work correctly for 8 bit input. For 10 bit input, only half of the width of
> the input picture is copied. But for 10 bit inputs, 2 bytes per pixel must
> be copied. Please see this patch:
>
>

Patch is messed up, please either attach it as attachment or provide
it in some other way.

>
> libavfilter/vf_libvmaf.c | 3 ++-
>
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>
>
> diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c
>
> index eee1c280ef..8d5ba4e2d5 100644
>
> --- a/libavfilter/vf_libvmaf.c
>
> +++ b/libavfilter/vf_libvmaf.c
>
> @@ -108,6 +108,7 @@ static enum VmafPixelFormat pix_fmt_map(enum
> AVPixelFormat av_pix_fmt)
>
>  static int copy_picture_data(AVFrame *src, VmafPicture *dst, unsigned bpc)
>
> {
>
> +    const int bytes_per_value = bpc > 8 ? 2 : 1;
>
>      int err = vmaf_picture_alloc(dst, pix_fmt_map(src->format), bpc,
>
>                                   src->width, src->height);
>
>      if (err)
>
> @@ -117,7 +118,7 @@ static int copy_picture_data(AVFrame *src, VmafPicture
> *dst, unsigned bpc)
>
>          uint8_t *src_data = src->data[i];
>
>          uint8_t *dst_data = dst->data[i];
>
>          for (unsigned j = 0; j < dst->h[i]; j++) {
>
> -            memcpy(dst_data, src_data, sizeof(*dst_data) * dst->w[i]);
>
> +            memcpy(dst_data, src_data, bytes_per_value * dst->w[i]);
>
>              src_data += src->linesize[i];
>
>              dst_data += dst->stride[i];
>
>          }
>
> --
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Bug in VMAF calculation for 10 bit comparison

[christian.feldmann]

[-- Attachment #1: Type: text/plain, Size: 2757 bytes --]

Sure. sorry for that. Please find attached the patch file.


-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Paul B Mahol
Sent: Wednesday, September 14, 2022 18:49
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] Bug in VMAF calculation for 10 bit comparison

On 9/14/22, christian.feldmann@gmx.de <christian.feldmann@gmx.de> wrote:
> Hi,
>
> I was debugging some VMAF calculation in ffmpeg when I stumbled on 
> this bug in the latest master commit 
> (9450f759748d02d1d284d2e4afd741cb0fe0c04a). Its in 
> libavfilter/vf_libvmaf.c::109. The function copy_picture_data does 
> only work correctly for 8 bit input. For 10 bit input, only half of 
> the width of the input picture is copied. But for 10 bit inputs, 2 bytes per pixel must be copied. Please see this patch:
>
>

Patch is messed up, please either attach it as attachment or provide it in some other way.

>
> libavfilter/vf_libvmaf.c | 3 ++-
>
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>
>
> diff --git a/libavfilter/vf_libvmaf.c b/libavfilter/vf_libvmaf.c
>
> index eee1c280ef..8d5ba4e2d5 100644
>
> --- a/libavfilter/vf_libvmaf.c
>
> +++ b/libavfilter/vf_libvmaf.c
>
> @@ -108,6 +108,7 @@ static enum VmafPixelFormat pix_fmt_map(enum 
> AVPixelFormat av_pix_fmt)
>
>  static int copy_picture_data(AVFrame *src, VmafPicture *dst, unsigned 
> bpc)
>
> {
>
> +    const int bytes_per_value = bpc > 8 ? 2 : 1;
>
>      int err = vmaf_picture_alloc(dst, pix_fmt_map(src->format), bpc,
>
>                                   src->width, src->height);
>
>      if (err)
>
> @@ -117,7 +118,7 @@ static int copy_picture_data(AVFrame *src, 
> VmafPicture *dst, unsigned bpc)
>
>          uint8_t *src_data = src->data[i];
>
>          uint8_t *dst_data = dst->data[i];
>
>          for (unsigned j = 0; j < dst->h[i]; j++) {
>
> -            memcpy(dst_data, src_data, sizeof(*dst_data) * dst->w[i]);
>
> +            memcpy(dst_data, src_data, bytes_per_value * dst->w[i]);
>
>              src_data += src->linesize[i];
>
>              dst_data += dst->stride[i];
>
>          }
>
> --
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email 
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[-- Attachment #2: 0001-Copy-all-values-also-for-10-bit-input.patch --]
[-- Type: application/octet-stream, Size: 1349 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Bug in VMAF calculation for 10 bit comparison

[Kyle Swanson]

Hi,

On Wed, Sep 14, 2022 at 9:52 AM <christian.feldmann@gmx.de> wrote:
>
> Sure. sorry for that. Please find attached the patch file.
>

Was about to test / review, but it looks like this is already pushed by Paul.

Thanks,
Kyle
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Bug in VMAF calculation for 10 bit comparison

[Andreas Rheinhardt]

Kyle Swanson:
> Hi,
> 
> On Wed, Sep 14, 2022 at 9:52 AM <christian.feldmann@gmx.de> wrote:
>>
>> Sure. sorry for that. Please find attached the patch file.
>>
> 
> Was about to test / review, but it looks like this is already pushed by Paul.
> 

Would it actually be possible to avoid the memcpy altogether?

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Bug in VMAF calculation for 10 bit comparison

[Kyle Swanson]

On Wed, Sep 14, 2022 at 12:02 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Would it actually be possible to avoid the memcpy altogether?
>

There's no libvmaf API for user buffers yet, so no, not at this moment.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".