Re: [PATCH 4/4] kms++util: Add frame compare functionality

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 14 December 2017 01:10:12 EET Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> Provide a means to compare two identically sized framebuffers.
> 
> This basic implementation expects the two buffers to have the same
> formats and sizes, and will return zero for identical frames, or a
> positive float representing and average difference otherwise.
> 
> Signed-off-by: Kieran Bingham 
> ---
>  kms++util/inc/kms++util/kms++util.h |  1 +-
>  kms++util/src/verification.cpp  | 31 ++-
>  py/pykms/pykmsutil.cpp  |  2 ++-
>  3 files changed, 34 insertions(+)
> 
> diff --git a/kms++util/inc/kms++util/kms++util.h
> b/kms++util/inc/kms++util/kms++util.h index 431de0bb159a..753cee189451
> 100644
> --- a/kms++util/inc/kms++util/kms++util.h
> +++ b/kms++util/inc/kms++util/kms++util.h
> @@ -29,6 +29,7 @@ void draw_color_bar(IFramebuffer& buf, int old_xpos, int
> xpos, int width); void draw_test_pattern(IFramebuffer , YUVType yuvt =
> YUVType::BT601_Lim);
> 
>  void save_raw_frame(IFramebuffer& fb, const char *filename);
> +float compare_framebuffers(IFramebuffer& a, IFramebuffer& b);
>  }
> 
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> diff --git a/kms++util/src/verification.cpp b/kms++util/src/verification.cpp
> index 3210bb144d2b..a46d6f924095 100644
> --- a/kms++util/src/verification.cpp
> +++ b/kms++util/src/verification.cpp
> @@ -18,4 +18,35 @@ void save_raw_frame(IFramebuffer& fb, const char
> *filename) os->write((char*)fb.map(i), fb.size(i));
>  }
> 
> +float compare_framebuffers(IFramebuffer& a, IFramebuffer& b)
> +{
> + unsigned int i;
> + unsigned int pixels = a.width() * a.height();
> + uint8_t *pa = a.map(0);
> + uint8_t *pb = b.map(0);
> +
> + float diff = 0;
> +
> + if (a.format() != b.format())
> + throw std::invalid_argument("Pixel formats differ...");
> +
> + if ((a.width() != b.width() ||
> + (a.height() != b.height(
> + throw std::invalid_argument("Frame sizes differ...");
> +
> + // Formats are identical, so num_planes are already identical
> + for (i = 0; i < a.num_planes(); i++) {
> + if ((a.offset(i) != b.offset(i)) ||
> + (a.stride(i) != b.stride(i)))
> + throw std::invalid_argument("Planes differ...");
> + }

Is this required ? Why can't we compare two image of identical size stored in 
frame buffers with different strides ?

> + // Simple byte comparisons to start.
> + // This expects a frame to be identical, including non-visible data.

Do we need to compare the non-visible data ?

> + for (i = 0; i < a.size(0) && i < b.size(0); i++)
> + diff += abs(pa[i] - pb[i]);
> +
> + return diff / pixels;
> +}
> +
>  }
> diff --git a/py/pykms/pykmsutil.cpp b/py/pykms/pykmsutil.cpp
> index 2d741751ba75..b86690a3d306 100644
> --- a/py/pykms/pykmsutil.cpp
> +++ b/py/pykms/pykmsutil.cpp
> @@ -64,4 +64,6 @@ void init_pykmstest(py::module )
>   m.def("save_raw_frame", [](Framebuffer& fb, const char * filename) {
>   save_raw_frame(fb, filename);
>   });
> + m.def("compare_framebuffers", [](Framebuffer& a, Framebuffer& b) {
> + return compare_framebuffers(a, b); } );
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] kms++util: Add frame compare functionality

[Kieran Bingham]

Hi Tomi,

On 15/12/17 14:09, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/12/17 01:10, Kieran Bingham wrote:
>> From: Kieran Bingham 
>>
>> Provide a means to compare two identically sized framebuffers.
>>
>> This basic implementation expects the two buffers to have the same
>> formats and sizes, and will return zero for identical frames, or a
>> positive float representing and average difference otherwise.
>>
>> Signed-off-by: Kieran Bingham 
>> ---
>>   kms++util/inc/kms++util/kms++util.h |  1 +-
>>   kms++util/src/verification.cpp  | 31 ++-
>>   py/pykms/pykmsutil.cpp  |  2 ++-
>>   3 files changed, 34 insertions(+)
>>
>> diff --git a/kms++util/inc/kms++util/kms++util.h
>> b/kms++util/inc/kms++util/kms++util.h
>> index 431de0bb159a..753cee189451 100644
>> --- a/kms++util/inc/kms++util/kms++util.h
>> +++ b/kms++util/inc/kms++util/kms++util.h
>> @@ -29,6 +29,7 @@ void draw_color_bar(IFramebuffer& buf, int old_xpos, int
>> xpos, int width);
>>   void draw_test_pattern(IFramebuffer , YUVType yuvt = 
>> YUVType::BT601_Lim);
>>     void save_raw_frame(IFramebuffer& fb, const char *filename);
>> +float compare_framebuffers(IFramebuffer& a, IFramebuffer& b);
>>   }
>>     #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>> diff --git a/kms++util/src/verification.cpp b/kms++util/src/verification.cpp
>> index 3210bb144d2b..a46d6f924095 100644
>> --- a/kms++util/src/verification.cpp
>> +++ b/kms++util/src/verification.cpp
>> @@ -18,4 +18,35 @@ void save_raw_frame(IFramebuffer& fb, const char 
>> *filename)
>>   os->write((char*)fb.map(i), fb.size(i));
>>   }
>>   +float compare_framebuffers(IFramebuffer& a, IFramebuffer& b)
> 
> I think this needs a short comment above to explain the return value.

Certainly.

As suggested by Geert, this will become an MSE. I'll document this in the next
version.

> 
>> +{
>> +    unsigned int i;
>> +    unsigned int pixels = a.width() * a.height();
>> +    uint8_t *pa = a.map(0);
>> +    uint8_t *pb = b.map(0);
> 
> This is only checking plane 0, you need to go through all the planes.

Yes, I must confess this version is the exceedingly rushed version after
chopping out my rather failed attempt at supporting multiple image formats doing
pixel level comparisons rather than byte level.

I'm hoping by v2 I'll be back to a much improved state here... but as this was
(eventually marked) RFC, and I had a deadline to post my current state - this
was the quickest basic implementation.


> Also, it's much nicer to introduce the locals near where they are needed,
> instead of having them all in the beginning of the function.

Ahhh - that's the C kernel developer in me. All variables defined at the
beginning of the function.

I can adjust this as you wish.

>> +
>> +    float diff = 0;
>> +
>> +    if (a.format() != b.format())
>> +    throw std::invalid_argument("Pixel formats differ...");
>> +
>> +    if ((a.width() != b.width() ||
>> +    (a.height() != b.height(
>> +    throw std::invalid_argument("Frame sizes differ...");
>> +
>> +    // Formats are identical, so num_planes are already identical
>> +    for (i = 0; i < a.num_planes(); i++) {
>> +    if ((a.offset(i) != b.offset(i)) ||
>> +    (a.stride(i) != b.stride(i)))
>> +    throw std::invalid_argument("Planes differ...");
>> +    }
>> +
>> +    // Simple byte comparisons to start.
>> +    // This expects a frame to be identical, including non-visible data.
>> +    for (i = 0; i < a.size(0) && i < b.size(0); i++)
>> +    diff += abs(pa[i] - pb[i]);
> 
> I don't think it's a good idea compare the non-valid pixels. They could as 
> well
> contain random data.

Absolutely agreed - I apologise for posting a very basic early version here.
I screwed up somewhere in my more complex implementation and came up against a
lack of time - so I fell back to the basic implementation which was still
functional for my needs at the higher level.

> Better to do the check properly, using the pixels inside width and height 
> only,
> without making assumptions about the buffer layout. And that way you can drop
> the check for offset and stride, as they don't have to be the same, as long as
> width & height are the same.

Completely agreed here too.

It was getting the YUYV pixel iterators right that caught me out, and broke my
other implementation. I'll try to find time to fix things up and resubmit.

Thankyou for your time and comments.

I'm pleased that you are positive towards receiving an implementation here, and
I hope it can be useful in other projects.

>  Tomi

Regards

Kieran

Re: [PATCH 4/4] kms++util: Add frame compare functionality

[Tomi Valkeinen]

Hi,

On 14/12/17 01:10, Kieran Bingham wrote:

From: Kieran Bingham 

Provide a means to compare two identically sized framebuffers.

This basic implementation expects the two buffers to have the same
formats and sizes, and will return zero for identical frames, or a
positive float representing and average difference otherwise.

Signed-off-by: Kieran Bingham 
---
  kms++util/inc/kms++util/kms++util.h |  1 +-
  kms++util/src/verification.cpp  | 31 ++-
  py/pykms/pykmsutil.cpp  |  2 ++-
  3 files changed, 34 insertions(+)

diff --git a/kms++util/inc/kms++util/kms++util.h 
b/kms++util/inc/kms++util/kms++util.h
index 431de0bb159a..753cee189451 100644
--- a/kms++util/inc/kms++util/kms++util.h
+++ b/kms++util/inc/kms++util/kms++util.h
@@ -29,6 +29,7 @@ void draw_color_bar(IFramebuffer& buf, int old_xpos, int 
xpos, int width);
  void draw_test_pattern(IFramebuffer , YUVType yuvt = YUVType::BT601_Lim);
  
  void save_raw_frame(IFramebuffer& fb, const char *filename);

+float compare_framebuffers(IFramebuffer& a, IFramebuffer& b);
  }
  
  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))

diff --git a/kms++util/src/verification.cpp b/kms++util/src/verification.cpp
index 3210bb144d2b..a46d6f924095 100644
--- a/kms++util/src/verification.cpp
+++ b/kms++util/src/verification.cpp
@@ -18,4 +18,35 @@ void save_raw_frame(IFramebuffer& fb, const char *filename)
os->write((char*)fb.map(i), fb.size(i));
  }
  
+float compare_framebuffers(IFramebuffer& a, IFramebuffer& b)


I think this needs a short comment above to explain the return value.


+{
+   unsigned int i;
+   unsigned int pixels = a.width() * a.height();
+   uint8_t *pa = a.map(0);
+   uint8_t *pb = b.map(0);


This is only checking plane 0, you need to go through all the planes.

Also, it's much nicer to introduce the locals near where they are 
needed, instead of having them all in the beginning of the function.



+
+   float diff = 0;
+
+   if (a.format() != b.format())
+   throw std::invalid_argument("Pixel formats differ...");
+
+   if ((a.width() != b.width() ||
+   (a.height() != b.height(
+   throw std::invalid_argument("Frame sizes differ...");
+
+   // Formats are identical, so num_planes are already identical
+   for (i = 0; i < a.num_planes(); i++) {
+   if ((a.offset(i) != b.offset(i)) ||
+   (a.stride(i) != b.stride(i)))
+   throw std::invalid_argument("Planes differ...");
+   }
+
+   // Simple byte comparisons to start.
+   // This expects a frame to be identical, including non-visible data.
+   for (i = 0; i < a.size(0) && i < b.size(0); i++)
+   diff += abs(pa[i] - pb[i]);


I don't think it's a good idea compare the non-valid pixels. They could 
as well contain random data.


Better to do the check properly, using the pixels inside width and 
height only, without making assumptions about the buffer layout. And 
that way you can drop the check for offset and stride, as they don't 
have to be the same, as long as width & height are the same.


 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 4/4] kms++util: Add frame compare functionality

[Kieran Bingham]

Hi Geert,

On 14/12/17 08:52, Geert Uytterhoeven wrote:
> Hi Kieran,
> 
> On Thu, Dec 14, 2017 at 12:10 AM, Kieran Bingham  wrote:
>> From: Kieran Bingham 
>>
>> Provide a means to compare two identically sized framebuffers.
>>
>> This basic implementation expects the two buffers to have the same
>> formats and sizes, and will return zero for identical frames, or a
>> positive float representing and average difference otherwise.
> 
> an
> 
>> --- a/kms++util/src/verification.cpp
>> +++ b/kms++util/src/verification.cpp
>> @@ -18,4 +18,35 @@ void save_raw_frame(IFramebuffer& fb, const char 
>> *filename)
>> os->write((char*)fb.map(i), fb.size(i));
>>  }
>>
>> +float compare_framebuffers(IFramebuffer& a, IFramebuffer& b)
>> +{
> 
>> +   // This expects a frame to be identical, including non-visible data.
>> +   for (i = 0; i < a.size(0) && i < b.size(0); i++)
>> +   diff += abs(pa[i] - pb[i]);
> 
> I think it's better to use "diff += abs(pa[i] - pb[i]) * abs(pa[i] - pb[i])",
> to penalize larger differences.
> 
> See also https://en.wikipedia.org/wiki/Peak_signal-to-noise_ratio

Ah yes of course - an excellent suggestion!

Thanks.


>> +
>> +   return diff / pixels;
>> +}
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

-- 
--
Kieran

Re: [PATCH 4/4] kms++util: Add frame compare functionality

[Geert Uytterhoeven]

Hi Kieran,

On Thu, Dec 14, 2017 at 12:10 AM, Kieran Bingham  wrote:
> From: Kieran Bingham 
>
> Provide a means to compare two identically sized framebuffers.
>
> This basic implementation expects the two buffers to have the same
> formats and sizes, and will return zero for identical frames, or a
> positive float representing and average difference otherwise.

an

> --- a/kms++util/src/verification.cpp
> +++ b/kms++util/src/verification.cpp
> @@ -18,4 +18,35 @@ void save_raw_frame(IFramebuffer& fb, const char *filename)
> os->write((char*)fb.map(i), fb.size(i));
>  }
>
> +float compare_framebuffers(IFramebuffer& a, IFramebuffer& b)
> +{

> +   // This expects a frame to be identical, including non-visible data.
> +   for (i = 0; i < a.size(0) && i < b.size(0); i++)
> +   diff += abs(pa[i] - pb[i]);

I think it's better to use "diff += abs(pa[i] - pb[i]) * abs(pa[i] - pb[i])",
to penalize larger differences.

See also https://en.wikipedia.org/wiki/Peak_signal-to-noise_ratio

> +
> +   return diff / pixels;
> +}

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds