[PATCH 0/4] kms++util: Provide validation helpers

[PATCH 0/4] kms++util: Provide validation helpers

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Our kms-tests test suite for verifying the R-Car DU is based on kmsxx.

We are implementing a 'loop-back' validator, by providing an HDMI cable from
the display output directly into the HDMI receiver on the same development
board.

By using the existing VideoStreamer and pyvid objects, we can capture the output
and compare to provide automated display validation.

As an effort to commence this process, this series implements a basic frame
buffer comparison, which expects identical frame sizes to operate.

This could be could be expanded upon later to provide a more detailed
comparison at the pixel level later, but this serves as an initial starting
point to quickly verify if the data captured matches the data that we expected.

Kieran Bingham (4):
  videodevice: Fix minor spacing
  py: pyvid: Provide stream_off binding
  kms++util: Add verification module
  kms++util: Add frame compare functionality

 kms++util/inc/kms++util/kms++util.h |  3 ++-
 kms++util/src/verification.cpp      | 52 ++++++++++++++++++++++++++++++-
 kms++util/src/videodevice.cpp       |  2 +-
 py/pykms/pykmsutil.cpp              |  7 ++++-
 py/pykms/pyvid.cpp                  |  1 +-
 5 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 kms++util/src/verification.cpp

base-commit: fee73296cae1a4984e18c8d8ee8e50ab217b1e4a
-- 
git-series 0.9.1

[PATCH 1/4] videodevice: Fix minor spacing

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a space between the return type and the function definition

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 kms++util/src/videodevice.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kms++util/src/videodevice.cpp b/kms++util/src/videodevice.cpp
index e21916bcd06b..efe1678d031e 100644
--- a/kms++util/src/videodevice.cpp
+++ b/kms++util/src/videodevice.cpp
@@ -436,7 +436,7 @@ void VideoStreamer::queue(DumbFramebuffer* fb)
 	v4l2_queue_dmabuf(m_fd, idx, fb, get_buf_type(m_type));
 }
 
-DumbFramebuffer*VideoStreamer::dequeue()
+DumbFramebuffer* VideoStreamer::dequeue()
 {
 	uint32_t idx = v4l2_dequeue(m_fd, get_buf_type(m_type));
 
-- 
git-series 0.9.1

Re: [PATCH 1/4] videodevice: Fix minor spacing

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 14 December 2017 01:10:09 EET Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Provide a space between the return type and the function definition
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  kms++util/src/videodevice.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kms++util/src/videodevice.cpp b/kms++util/src/videodevice.cpp
> index e21916bcd06b..efe1678d031e 100644
> --- a/kms++util/src/videodevice.cpp
> +++ b/kms++util/src/videodevice.cpp
> @@ -436,7 +436,7 @@ void VideoStreamer::queue(DumbFramebuffer* fb)
>  	v4l2_queue_dmabuf(m_fd, idx, fb, get_buf_type(m_type));
>  }
> 
> -DumbFramebuffer*VideoStreamer::dequeue()
> +DumbFramebuffer* VideoStreamer::dequeue()
>  {
>  	uint32_t idx = v4l2_dequeue(m_fd, get_buf_type(m_type));

-- 
Regards,

Laurent Pinchart

[PATCH 2/4] py: pyvid: Provide stream_off binding

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

The videodevice module defines a stream_off function call, but this is
not available in the python bindings interface.

Provide the binding of VideoStreamer::stream_off.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 py/pykms/pyvid.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/py/pykms/pyvid.cpp b/py/pykms/pyvid.cpp
index 6a6080e4b2b7..92006c404038 100644
--- a/py/pykms/pyvid.cpp
+++ b/py/pykms/pyvid.cpp
@@ -34,5 +34,6 @@ void init_pyvid(py::module &m)
 			.def("queue", &VideoStreamer::queue)
 			.def("dequeue", &VideoStreamer::dequeue)
 			.def("stream_on", &VideoStreamer::stream_on)
+			.def("stream_off", &VideoStreamer::stream_off)
 			;
 }
-- 
git-series 0.9.1

Re: [PATCH 2/4] py: pyvid: Provide stream_off binding

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 14 December 2017 01:10:10 EET Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> The videodevice module defines a stream_off function call, but this is
> not available in the python bindings interface.
> 
> Provide the binding of VideoStreamer::stream_off.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  py/pykms/pyvid.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/py/pykms/pyvid.cpp b/py/pykms/pyvid.cpp
> index 6a6080e4b2b7..92006c404038 100644
> --- a/py/pykms/pyvid.cpp
> +++ b/py/pykms/pyvid.cpp
> @@ -34,5 +34,6 @@ void init_pyvid(py::module &m)
>  			.def("queue", &VideoStreamer::queue)
>  			.def("dequeue", &VideoStreamer::dequeue)
>  			.def("stream_on", &VideoStreamer::stream_on)
> +			.def("stream_off", &VideoStreamer::stream_off)

I've always wondered why V4L2 has two ioctls for stream on and stream off, 
when a single one with an on/off parameter would be perfectly fine. I'm thus 
tempted to implement stream control through a single function, but that's a 
matter of taste. Feel free to give it a go if you want, otherwise

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			;
>  }

-- 
Regards,

Laurent Pinchart

[PATCH 3/4] kms++util: Add verification module

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a util module to provide helpers involved in validation and verification
of data frames.

The first addition is a raw frame binary output with bindings to python modelled
on Tomi's implementation in wbcap.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 kms++util/inc/kms++util/kms++util.h |  2 ++
 kms++util/src/verification.cpp      | 21 +++++++++++++++++++++
 py/pykms/pykmsutil.cpp              |  5 +++++
 3 files changed, 28 insertions(+)
 create mode 100644 kms++util/src/verification.cpp

diff --git a/kms++util/inc/kms++util/kms++util.h b/kms++util/inc/kms++util/kms++util.h
index 8e45b0df3cde..431de0bb159a 100644
--- a/kms++util/inc/kms++util/kms++util.h
+++ b/kms++util/inc/kms++util/kms++util.h
@@ -27,6 +27,8 @@ void draw_text(IFramebuffer& buf, uint32_t x, uint32_t y, const std::string& str
 void draw_color_bar(IFramebuffer& buf, int old_xpos, int xpos, int width);
 
 void draw_test_pattern(IFramebuffer &fb, YUVType yuvt = YUVType::BT601_Lim);
+
+void save_raw_frame(IFramebuffer& fb, const char *filename);
 }
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
diff --git a/kms++util/src/verification.cpp b/kms++util/src/verification.cpp
new file mode 100644
index 000000000000..3210bb144d2b
--- /dev/null
+++ b/kms++util/src/verification.cpp
@@ -0,0 +1,21 @@
+
+#include <kms++/kms++.h>
+#include <kms++util/kms++util.h>
+
+#include <fstream>
+
+using namespace std;
+
+namespace kms
+{
+
+void save_raw_frame(IFramebuffer& fb, const char *filename)
+{
+	unique_ptr<ofstream> os;
+	os = unique_ptr<ofstream>(new ofstream(filename, ofstream::binary));
+
+	for (unsigned i = 0; i < fb.num_planes(); ++i)
+		os->write((char*)fb.map(i), fb.size(i));
+}
+
+}
diff --git a/py/pykms/pykmsutil.cpp b/py/pykms/pykmsutil.cpp
index 518d5eaa88f0..2d741751ba75 100644
--- a/py/pykms/pykmsutil.cpp
+++ b/py/pykms/pykmsutil.cpp
@@ -59,4 +59,9 @@ void init_pykmstest(py::module &m)
 	} );
 	m.def("draw_text", [](Framebuffer& fb, uint32_t x, uint32_t y, const string& str, RGB color) {
 		draw_text(fb, x, y, str, color); } );
+
+	// Verification and Validation
+	m.def("save_raw_frame", [](Framebuffer& fb, const char * filename) {
+		save_raw_frame(fb, filename);
+	});
 }
-- 
git-series 0.9.1

Re: [PATCH 3/4] kms++util: Add verification module

[Tomi Valkeinen]

Hi,

On 14/12/17 01:10, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Provide a util module to provide helpers involved in validation and verification
> of data frames.
> 
> The first addition is a raw frame binary output with bindings to python modelled
> on Tomi's implementation in wbcap.

I don't think verification.cpp is a good name for this. A function to 
save the fb sounds like a generic fb utility.

In fact, I'd like to have a helper utility function to save as png, as 
converting a raw image to png is a bit of a hassle. Then again, we'd 
need a png library for that, and possibly bigger pieces of code to 
handle all the different pixel formats...

So, a function to save the raw bits is fine, but how about "fbutils.cpp" 
or such?

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   kms++util/inc/kms++util/kms++util.h |  2 ++
>   kms++util/src/verification.cpp      | 21 +++++++++++++++++++++
>   py/pykms/pykmsutil.cpp              |  5 +++++
>   3 files changed, 28 insertions(+)
>   create mode 100644 kms++util/src/verification.cpp
> 
> diff --git a/kms++util/inc/kms++util/kms++util.h b/kms++util/inc/kms++util/kms++util.h
> index 8e45b0df3cde..431de0bb159a 100644
> --- a/kms++util/inc/kms++util/kms++util.h
> +++ b/kms++util/inc/kms++util/kms++util.h
> @@ -27,6 +27,8 @@ void draw_text(IFramebuffer& buf, uint32_t x, uint32_t y, const std::string& str
>   void draw_color_bar(IFramebuffer& buf, int old_xpos, int xpos, int width);
>   
>   void draw_test_pattern(IFramebuffer &fb, YUVType yuvt = YUVType::BT601_Lim);
> +
> +void save_raw_frame(IFramebuffer& fb, const char *filename);
>   }
>   
>   #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> diff --git a/kms++util/src/verification.cpp b/kms++util/src/verification.cpp
> new file mode 100644
> index 000000000000..3210bb144d2b
> --- /dev/null
> +++ b/kms++util/src/verification.cpp
> @@ -0,0 +1,21 @@
> +
> +#include <kms++/kms++.h>
> +#include <kms++util/kms++util.h>
> +
> +#include <fstream>
> +
> +using namespace std;
> +
> +namespace kms
> +{
> +
> +void save_raw_frame(IFramebuffer& fb, const char *filename)
> +{
> +	unique_ptr<ofstream> os;
> +	os = unique_ptr<ofstream>(new ofstream(filename, ofstream::binary));
> +
> +	for (unsigned i = 0; i < fb.num_planes(); ++i)
> +		os->write((char*)fb.map(i), fb.size(i));
> +}

You don't need any of that unique_ptr stuff here. I needed it as the 
code needed to handle the case where we don't save, i.e. os = null.

And I'm not fond of the function name, "frame" doesn't sound good. Maybe 
rather save_raw_fb(). Or save_fb_raw(), so in the future we might have 
also save_fb_png().

> +
> +}
> diff --git a/py/pykms/pykmsutil.cpp b/py/pykms/pykmsutil.cpp
> index 518d5eaa88f0..2d741751ba75 100644
> --- a/py/pykms/pykmsutil.cpp
> +++ b/py/pykms/pykmsutil.cpp
> @@ -59,4 +59,9 @@ void init_pykmstest(py::module &m)
>   	} );
>   	m.def("draw_text", [](Framebuffer& fb, uint32_t x, uint32_t y, const string& str, RGB color) {
>   		draw_text(fb, x, y, str, color); } );
> +
> +	// Verification and Validation
> +	m.def("save_raw_frame", [](Framebuffer& fb, const char * filename) {
> +		save_raw_frame(fb, filename);
> +	});
>   }
> 

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

Re: [PATCH 3/4] kms++util: Add verification module

[Kieran Bingham]

Hi Tomi,

Thank you for taking the time to look at this topic.

On 15/12/17 13:43, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/12/17 01:10, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Provide a util module to provide helpers involved in validation and verification
>> of data frames.
>>
>> The first addition is a raw frame binary output with bindings to python modelled
>> on Tomi's implementation in wbcap.
> 
> I don't think verification.cpp is a good name for this. A function to save the
> fb sounds like a generic fb utility.

Yes, I was originally going to put it in one of the existing framebuffer classes
- but then realised they were abstracted classes, and it had to use the base type :)

I was looking at verifying frames, so this was the first name that came to my
head :D

> In fact, I'd like to have a helper utility function to save as png, as
> converting a raw image to png is a bit of a hassle. Then again, we'd need a png
> library for that, and possibly bigger pieces of code to handle all the different
> pixel formats...

So would I ! I'm glad you bring the topic up :)

Are you happy to bring in external libraries to support such functionality?

I guess we can make it so the configure scripts select the feature if libraries
are available or such.

Being able to save directly to easily viewable file formats would certainly ease
things, (while still being able to save raw files for some manual verifications)

> So, a function to save the raw bits is fine, but how about "fbutils.cpp" or such?

Yes, that sounds reasonable.


>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   kms++util/inc/kms++util/kms++util.h |  2 ++
>>   kms++util/src/verification.cpp      | 21 +++++++++++++++++++++
>>   py/pykms/pykmsutil.cpp              |  5 +++++
>>   3 files changed, 28 insertions(+)
>>   create mode 100644 kms++util/src/verification.cpp
>>
>> diff --git a/kms++util/inc/kms++util/kms++util.h
>> b/kms++util/inc/kms++util/kms++util.h
>> index 8e45b0df3cde..431de0bb159a 100644
>> --- a/kms++util/inc/kms++util/kms++util.h
>> +++ b/kms++util/inc/kms++util/kms++util.h
>> @@ -27,6 +27,8 @@ void draw_text(IFramebuffer& buf, uint32_t x, uint32_t y,
>> const std::string& str
>>   void draw_color_bar(IFramebuffer& buf, int old_xpos, int xpos, int width);
>>     void draw_test_pattern(IFramebuffer &fb, YUVType yuvt = YUVType::BT601_Lim);
>> +
>> +void save_raw_frame(IFramebuffer& fb, const char *filename);
>>   }
>>     #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>> diff --git a/kms++util/src/verification.cpp b/kms++util/src/verification.cpp
>> new file mode 100644
>> index 000000000000..3210bb144d2b
>> --- /dev/null
>> +++ b/kms++util/src/verification.cpp
>> @@ -0,0 +1,21 @@
>> +
>> +#include <kms++/kms++.h>
>> +#include <kms++util/kms++util.h>
>> +
>> +#include <fstream>
>> +
>> +using namespace std;
>> +
>> +namespace kms
>> +{
>> +
>> +void save_raw_frame(IFramebuffer& fb, const char *filename)
>> +{
>> +    unique_ptr<ofstream> os;
>> +    os = unique_ptr<ofstream>(new ofstream(filename, ofstream::binary));
>> +
>> +    for (unsigned i = 0; i < fb.num_planes(); ++i)
>> +        os->write((char*)fb.map(i), fb.size(i));
>> +}
> 
> You don't need any of that unique_ptr stuff here. I needed it as the code needed
> to handle the case where we don't save, i.e. os = null.

Ah OK - I thought it was providing the hook up to automatically close the stream
at the end of the function.

I guess an explicit close would be just as clean :)

As the commit message suggests, this was a direct copy paste from your
implementation after I saw it as a better implementation than my previous 'C'
version. (I don't spend a lot of time in C++ land)


> And I'm not fond of the function name, "frame" doesn't sound good. Maybe rather
> save_raw_fb(). Or save_fb_raw(), so in the future we might have also save_fb_png().

Agreed, sounds fine. I'm definitely looking forward to a save_fb_png().


>> +
>> +}
>> diff --git a/py/pykms/pykmsutil.cpp b/py/pykms/pykmsutil.cpp
>> index 518d5eaa88f0..2d741751ba75 100644
>> --- a/py/pykms/pykmsutil.cpp
>> +++ b/py/pykms/pykmsutil.cpp
>> @@ -59,4 +59,9 @@ void init_pykmstest(py::module &m)
>>       } );
>>       m.def("draw_text", [](Framebuffer& fb, uint32_t x, uint32_t y, const
>> string& str, RGB color) {
>>           draw_text(fb, x, y, str, color); } );
>> +
>> +    // Verification and Validation
>> +    m.def("save_raw_frame", [](Framebuffer& fb, const char * filename) {
>> +        save_raw_frame(fb, filename);
>> +    });
>>   }
>>
> 

Regards
--
Kieran

Re: [PATCH 3/4] kms++util: Add verification module

[Laurent Pinchart]

Hi Kieran,

On Saturday, 16 December 2017 18:13:51 EET Kieran Bingham wrote:
> On 15/12/17 13:43, Tomi Valkeinen wrote:
> > On 14/12/17 01:10, Kieran Bingham wrote:
> >> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> 
> >> Provide a util module to provide helpers involved in validation and
> >> verification of data frames.
> >> 
> >> The first addition is a raw frame binary output with bindings to python
> >> modelled on Tomi's implementation in wbcap.
> > 
> > I don't think verification.cpp is a good name for this. A function to save
> > the fb sounds like a generic fb utility.
> 
> Yes, I was originally going to put it in one of the existing framebuffer
> classes - but then realised they were abstracted classes, and it had to use
> the base type :)
> 
> I was looking at verifying frames, so this was the first name that came to
> my head :D
> 
> > In fact, I'd like to have a helper utility function to save as png, as
> > converting a raw image to png is a bit of a hassle. Then again, we'd need
> > a png library for that, and possibly bigger pieces of code to handle all
> > the different pixel formats...
> 
> So would I ! I'm glad you bring the topic up :)
> 
> Are you happy to bring in external libraries to support such functionality?
> 
> I guess we can make it so the configure scripts select the feature if
> libraries are available or such.
> 
> Being able to save directly to easily viewable file formats would certainly
> ease things, (while still being able to save raw files for some manual
> verifications)

The problem with PNG (or any other format really) is that you not only need to 
encode the image into the target format (PNG or JPG would require external 
libraries, simpler formats such as BMP or PNM could be handled internally), 
but you also need to convert the image to a particular RGB or YUV format 
depending on what the output format requires.

If you want to do so, I would like to reuse code from the v4l2convert library. 
The code should be moved to a library that doesn't depend on V4L2, as the 
current API encapsulate conversion in other operations. Other tools such as 
raw2rgbpnm could then be ported to use that library;

> > So, a function to save the raw bits is fine, but how about "fbutils.cpp"
> > or such?
> 
> Yes, that sounds reasonable.
> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>   kms++util/inc/kms++util/kms++util.h |  2 ++
> >>   kms++util/src/verification.cpp      | 21 +++++++++++++++++++++
> >>   py/pykms/pykmsutil.cpp              |  5 +++++
> >>   3 files changed, 28 insertions(+)
> >>   create mode 100644 kms++util/src/verification.cpp
> >> 
> >> diff --git a/kms++util/inc/kms++util/kms++util.h
> >> b/kms++util/inc/kms++util/kms++util.h
> >> index 8e45b0df3cde..431de0bb159a 100644
> >> --- a/kms++util/inc/kms++util/kms++util.h
> >> +++ b/kms++util/inc/kms++util/kms++util.h
> >> @@ -27,6 +27,8 @@ void draw_text(IFramebuffer& buf, uint32_t x, uint32_t
> >> y,
> >> const std::string& str
> >>   void draw_color_bar(IFramebuffer& buf, int old_xpos, int xpos, int
> >> width); void draw_test_pattern(IFramebuffer &fb, YUVType yuvt =
> >> YUVType::BT601_Lim); +
> >> +void save_raw_frame(IFramebuffer& fb, const char *filename);
> >>   }
> >>     #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> >> diff --git a/kms++util/src/verification.cpp
> >> b/kms++util/src/verification.cpp new file mode 100644
> >> index 000000000000..3210bb144d2b
> >> --- /dev/null
> >> +++ b/kms++util/src/verification.cpp
> >> @@ -0,0 +1,21 @@
> >> +
> >> +#include <kms++/kms++.h>
> >> +#include <kms++util/kms++util.h>
> >> +
> >> +#include <fstream>
> >> +
> >> +using namespace std;
> >> +
> >> +namespace kms
> >> +{
> >> +
> >> +void save_raw_frame(IFramebuffer& fb, const char *filename)
> >> +{
> >> +    unique_ptr<ofstream> os;
> >> +    os = unique_ptr<ofstream>(new ofstream(filename, ofstream::binary));
> >> +
> >> +    for (unsigned i = 0; i < fb.num_planes(); ++i)
> >> +        os->write((char*)fb.map(i), fb.size(i));
> >> +}
> > 
> > You don't need any of that unique_ptr stuff here. I needed it as the code
> > needed to handle the case where we don't save, i.e. os = null.
> 
> Ah OK - I thought it was providing the hook up to automatically close the
> stream at the end of the function.
> 
> I guess an explicit close would be just as clean :)
> 
> As the commit message suggests, this was a direct copy paste from your
> implementation after I saw it as a better implementation than my previous
> 'C' version. (I don't spend a lot of time in C++ land)
> 
> > And I'm not fond of the function name, "frame" doesn't sound good. Maybe
> > rather save_raw_fb(). Or save_fb_raw(), so in the future we might have
> > also save_fb_png().
> 
> Agreed, sounds fine. I'm definitely looking forward to a save_fb_png().
> 
> >> +
> >> +}
> >> diff --git a/py/pykms/pykmsutil.cpp b/py/pykms/pykmsutil.cpp
> >> index 518d5eaa88f0..2d741751ba75 100644
> >> --- a/py/pykms/pykmsutil.cpp
> >> +++ b/py/pykms/pykmsutil.cpp
> >> @@ -59,4 +59,9 @@ void init_pykmstest(py::module &m)
> >>       } );
> >>       m.def("draw_text", [](Framebuffer& fb, uint32_t x, uint32_t y,
> >> const
> >> string& str, RGB color) {
> >>           draw_text(fb, x, y, str, color); } );
> >> +
> >> +    // Verification and Validation
> >> +    m.def("save_raw_frame", [](Framebuffer& fb, const char * filename) {
> >> +        save_raw_frame(fb, filename);
> >> +    });
> >>   }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] kms++util: Add verification module

[Tomi Valkeinen]

On 18/12/17 13:36, Laurent Pinchart wrote:

> The problem with PNG (or any other format really) is that you not only need to
> encode the image into the target format (PNG or JPG would require external
> libraries, simpler formats such as BMP or PNM could be handled internally),
> but you also need to convert the image to a particular RGB or YUV format
> depending on what the output format requires.

Yes, that's the "hassle" part I was referring to =).

Maybe we need to invent a new file format that can store all kinds of 
formats? (https://xkcd.com/927/)

> If you want to do so, I would like to reuse code from the v4l2convert library.
> The code should be moved to a library that doesn't depend on V4L2, as the
> current API encapsulate conversion in other operations. Other tools such as
> raw2rgbpnm could then be ported to use that library;

Yep, so I agree that doing color format conversions shouldn't really be 
kms++'s job, but at the same time I'd like to be able to export 
framebuffers.

And I'm fine with adding dependencies to kms++, as long as all those are 
optional.

Aren't there "big" image conversion libraries that would do the job? 
Imagemagick? Or something? I have used "convert" command to do some 
conversions, that comes from ImageMagick.

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

Re: [PATCH 3/4] kms++util: Add verification module

[Laurent Pinchart]

Hi Tomi,

On Monday, 18 December 2017 13:46:22 EET Tomi Valkeinen wrote:
> On 18/12/17 13:36, Laurent Pinchart wrote:
> > The problem with PNG (or any other format really) is that you not only
> > need to encode the image into the target format (PNG or JPG would require
> > external libraries, simpler formats such as BMP or PNM could be handled
> > internally), but you also need to convert the image to a particular RGB
> > or YUV format depending on what the output format requires.
> 
> Yes, that's the "hassle" part I was referring to =).
> 
> Maybe we need to invent a new file format that can store all kinds of
> formats? (https://xkcd.com/927/)
> 
> > If you want to do so, I would like to reuse code from the v4l2convert
> > library. The code should be moved to a library that doesn't depend on
> > V4L2, as the current API encapsulate conversion in other operations.
> > Other tools such as raw2rgbpnm could then be ported to use that library;
> 
> Yep, so I agree that doing color format conversions shouldn't really be
> kms++'s job, but at the same time I'd like to be able to export
> framebuffers.
> 
> And I'm fine with adding dependencies to kms++, as long as all those are
> optional.
> 
> Aren't there "big" image conversion libraries that would do the job?
> Imagemagick? Or something? I have used "convert" command to do some
> conversions, that comes from ImageMagick.

That's an option too. I had a look at the code once to find out how 
ImageMagick was performing scaling and gave up with a headache soon 
afterwards. We need more formats than what ImageMagick currently supports 
(it's mostly focused on image file formats instead of raw image formats), with 
all kind of RGB, YUV (and ideally Bayer) formats, and I don't think extending 
ImageMagick would be the way to go.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] kms++util: Add verification module

[Tomi Valkeinen]

On 18/12/17 13:50, Laurent Pinchart wrote:

> That's an option too. I had a look at the code once to find out how
> ImageMagick was performing scaling and gave up with a headache soon
> afterwards. We need more formats than what ImageMagick currently supports
> (it's mostly focused on image file formats instead of raw image formats), with
> all kind of RGB, YUV (and ideally Bayer) formats, and I don't think extending
> ImageMagick would be the way to go.

Yep, I can image that ImageMagick can't handle it all. I don't really 
have much experience with it. I did find that it has enough tunables to 
manage rgbx8888 with different byte orders and the "extra" alpha channel:

convert -depth 8 -size $1x$2 -color-matrix "0 0 1 0 1 0 1 0 0" -alpha 
deactivate rgba:$3 png:-

But things like argb1555 or yuv422 might prove to be too difficult for 
ImageMagick.

Where's the source for v4l2convert? What does it do?

I wonder how difficult it would be to create a 
brute-force-no-optimizations style of a function to which you give the 
exact bit definition of the buffer's pixel format, and which gives you 
RGB888 or YUV444, depending on the input format (I presume most image 
encoders would accept RGB888 & YUV444).

I think the function wouldn't even need to care whether the data is RGB 
or YUV, it would just unpack it according to the format definition.

  Tomi

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

Re: [PATCH 3/4] kms++util: Add verification module

[Laurent Pinchart]

Hi Tomi,

On Monday, 18 December 2017 14:04:45 EET Tomi Valkeinen wrote:
> On 18/12/17 13:50, Laurent Pinchart wrote:
> > That's an option too. I had a look at the code once to find out how
> > ImageMagick was performing scaling and gave up with a headache soon
> > afterwards. We need more formats than what ImageMagick currently supports
> > (it's mostly focused on image file formats instead of raw image formats),
> > with all kind of RGB, YUV (and ideally Bayer) formats, and I don't think
> > extending ImageMagick would be the way to go.
> 
> Yep, I can image that ImageMagick can't handle it all. I don't really
> have much experience with it. I did find that it has enough tunables to
> manage rgbx8888 with different byte orders and the "extra" alpha channel:
> 
> convert -depth 8 -size $1x$2 -color-matrix "0 0 1 0 1 0 1 0 0" -alpha
> deactivate rgba:$3 png:-
> 
> But things like argb1555 or yuv422 might prove to be too difficult for
> ImageMagick.
> 
> Where's the source for v4l2convert? What does it do?

Sorry, it's v4lconvert, not v4l2convert. The library is part of v4l-utils 
(available at git://linuxtv.org/pinchartl/v4l-utils.git) and is found in the 
lib/libv4lconvert directory.

The purpose of the library is to convert from lots of weird formats supported 
by V4L2 to RGB or YUV. It was initially written to handle vendor-specific 
compression formats used by webcams, has been extended to support Bayer 
formats and different kind of RGB and YUV formats. The code is simple, I think 
it could be a good base.

> I wonder how difficult it would be to create a brute-force-no-optimizations
> style of a function to which you give the exact bit definition of the
> buffer's pixel format, and which gives you RGB888 or YUV444, depending on
> the input format (I presume most image encoders would accept RGB888 &
> YUV444).

Can you come up with a bit definition format that could describe Bayer or 
tiled formats without requiring the definition to be written in an interpreted 
language ? :-)

> I think the function wouldn't even need to care whether the data is RGB
> or YUV, it would just unpack it according to the format definition.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] kms++util: Add verification module

[Tomi Valkeinen]

On 18/12/17 17:41, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Monday, 18 December 2017 14:04:45 EET Tomi Valkeinen wrote:
>> On 18/12/17 13:50, Laurent Pinchart wrote:
>>> That's an option too. I had a look at the code once to find out how
>>> ImageMagick was performing scaling and gave up with a headache soon
>>> afterwards. We need more formats than what ImageMagick currently supports
>>> (it's mostly focused on image file formats instead of raw image formats),
>>> with all kind of RGB, YUV (and ideally Bayer) formats, and I don't think
>>> extending ImageMagick would be the way to go.
>>
>> Yep, I can image that ImageMagick can't handle it all. I don't really
>> have much experience with it. I did find that it has enough tunables to
>> manage rgbx8888 with different byte orders and the "extra" alpha channel:
>>
>> convert -depth 8 -size $1x$2 -color-matrix "0 0 1 0 1 0 1 0 0" -alpha
>> deactivate rgba:$3 png:-
>>
>> But things like argb1555 or yuv422 might prove to be too difficult for
>> ImageMagick.
>>
>> Where's the source for v4l2convert? What does it do?
> 
> Sorry, it's v4lconvert, not v4l2convert. The library is part of v4l-utils
> (available at git://linuxtv.org/pinchartl/v4l-utils.git) and is found in the
> lib/libv4lconvert directory.
> 
> The purpose of the library is to convert from lots of weird formats supported
> by V4L2 to RGB or YUV. It was initially written to handle vendor-specific
> compression formats used by webcams, has been extended to support Bayer
> formats and different kind of RGB and YUV formats. The code is simple, I think
> it could be a good base.
> 
>> I wonder how difficult it would be to create a brute-force-no-optimizations
>> style of a function to which you give the exact bit definition of the
>> buffer's pixel format, and which gives you RGB888 or YUV444, depending on
>> the input format (I presume most image encoders would accept RGB888 &
>> YUV444).
> 
> Can you come up with a bit definition format that could describe Bayer or
> tiled formats without requiring the definition to be written in an interpreted
> language ? :-)

I have only worked with RGB and YUV, so that should be enough for 
everybody. ;)

But more seriously, I think kms++ is mainly for display. Do we have any 
display devices that support Bayer? And tiled formats sound a bit 
special platform specific cases.

That said, if we have a library that just does it all, that's obviously 
the best option.

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

Re: [PATCH 3/4] kms++util: Add verification module

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Dec 18, 2017 at 12:50 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday, 18 December 2017 13:46:22 EET Tomi Valkeinen wrote:
>> On 18/12/17 13:36, Laurent Pinchart wrote:
>> > The problem with PNG (or any other format really) is that you not only
>> > need to encode the image into the target format (PNG or JPG would require
>> > external libraries, simpler formats such as BMP or PNM could be handled
>> > internally), but you also need to convert the image to a particular RGB
>> > or YUV format depending on what the output format requires.
>>
>> Yes, that's the "hassle" part I was referring to =).
>>
>> Maybe we need to invent a new file format that can store all kinds of
>> formats? (https://xkcd.com/927/)
>>
>> > If you want to do so, I would like to reuse code from the v4l2convert
>> > library. The code should be moved to a library that doesn't depend on
>> > V4L2, as the current API encapsulate conversion in other operations.
>> > Other tools such as raw2rgbpnm could then be ported to use that library;
>>
>> Yep, so I agree that doing color format conversions shouldn't really be
>> kms++'s job, but at the same time I'd like to be able to export
>> framebuffers.
>>
>> And I'm fine with adding dependencies to kms++, as long as all those are
>> optional.
>>
>> Aren't there "big" image conversion libraries that would do the job?
>> Imagemagick? Or something? I have used "convert" command to do some
>> conversions, that comes from ImageMagick.
>
> That's an option too. I had a look at the code once to find out how
> ImageMagick was performing scaling and gave up with a headache soon
> afterwards. We need more formats than what ImageMagick currently supports
> (it's mostly focused on image file formats instead of raw image formats), with
> all kind of RGB, YUV (and ideally Bayer) formats, and I don't think extending
> ImageMagick would be the way to go.

netpbm is your friend.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

Re: [PATCH 3/4] kms++util: Add verification module

[Tomi Valkeinen]

On 16/12/17 18:13, Kieran Bingham wrote:

>>> +void save_raw_frame(IFramebuffer& fb, const char *filename)
>>> +{
>>> +    unique_ptr<ofstream> os;
>>> +    os = unique_ptr<ofstream>(new ofstream(filename, ofstream::binary));
>>> +
>>> +    for (unsigned i = 0; i < fb.num_planes(); ++i)
>>> +        os->write((char*)fb.map(i), fb.size(i));
>>> +}
>>
>> You don't need any of that unique_ptr stuff here. I needed it as the code needed
>> to handle the case where we don't save, i.e. os = null.
> 
> Ah OK - I thought it was providing the hook up to automatically close the stream
> at the end of the function.
> 
> I guess an explicit close would be just as clean :)

You don't need an explicit close. The ofstream instance will be 
destructed automatically, and thus the stream will be closed.

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

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

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

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 <kieran.bingham@ideasonboard.com>
---
 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 &fb, 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...");
+	}
+
+	// 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]);
+
+	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)
 	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); } );
 }
-- 
git-series 0.9.1

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 <kbingham@kernel.org> wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> 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 -- geert@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

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 <kbingham@kernel.org> wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> 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 -- geert@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

[Tomi Valkeinen]

Hi,

On 14/12/17 01:10, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 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 <kieran.bingham@ideasonboard.com>
> ---
>   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 &fb, 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 Tomi,

On 15/12/17 14:09, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/12/17 01:10, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> 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 <kieran.bingham@ideasonboard.com>
>> ---
>>   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 &fb, 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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 14 December 2017 01:10:12 EET Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 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 <kieran.bingham@ideasonboard.com>
> ---
>  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 &fb, 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)
>  	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 0/4] kms++util: Provide validation helpers

[Kieran Bingham]

Hi Tomi,

Apologies that I didn't make it particularly clear, but I should have prefixed
this series title with a nice big [RFC] :-D

This is ongoing work to support loop-back validation on our platforms.

Regards

Kieran

On 13/12/17 23:10, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Our kms-tests test suite for verifying the R-Car DU is based on kmsxx.
> 
> We are implementing a 'loop-back' validator, by providing an HDMI cable from
> the display output directly into the HDMI receiver on the same development
> board.
> 
> By using the existing VideoStreamer and pyvid objects, we can capture the output
> and compare to provide automated display validation.
> 
> As an effort to commence this process, this series implements a basic frame
> buffer comparison, which expects identical frame sizes to operate.
> 
> This could be could be expanded upon later to provide a more detailed
> comparison at the pixel level later, but this serves as an initial starting
> point to quickly verify if the data captured matches the data that we expected.
> 
> Kieran Bingham (4):
>   videodevice: Fix minor spacing
>   py: pyvid: Provide stream_off binding
>   kms++util: Add verification module
>   kms++util: Add frame compare functionality
> 
>  kms++util/inc/kms++util/kms++util.h |  3 ++-
>  kms++util/src/verification.cpp      | 52 ++++++++++++++++++++++++++++++-
>  kms++util/src/videodevice.cpp       |  2 +-
>  py/pykms/pykmsutil.cpp              |  7 ++++-
>  py/pykms/pyvid.cpp                  |  1 +-
>  5 files changed, 64 insertions(+), 1 deletion(-)
>  create mode 100644 kms++util/src/verification.cpp
> 
> base-commit: fee73296cae1a4984e18c8d8ee8e50ab217b1e4a
> 

Re: [PATCH 0/4] kms++util: Provide validation helpers

[Tomi Valkeinen]

On 14/12/17 01:10, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Our kms-tests test suite for verifying the R-Car DU is based on kmsxx.
> 
> We are implementing a 'loop-back' validator, by providing an HDMI cable from
> the display output directly into the HDMI receiver on the same development
> board.
> 
> By using the existing VideoStreamer and pyvid objects, we can capture the output
> and compare to provide automated display validation.
> 
> As an effort to commence this process, this series implements a basic frame
> buffer comparison, which expects identical frame sizes to operate.
> 
> This could be could be expanded upon later to provide a more detailed
> comparison at the pixel level later, but this serves as an initial starting
> point to quickly verify if the data captured matches the data that we expected.
> 
> Kieran Bingham (4):
>    videodevice: Fix minor spacing
>    py: pyvid: Provide stream_off binding

Thanks, I've picked up the first two patches.

  Tomi

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

Re: [PATCH 0/4] kms++util: Provide validation helpers

[Kieran Bingham]

On 15/12/17 14:10, Tomi Valkeinen wrote:
> On 14/12/17 01:10, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Our kms-tests test suite for verifying the R-Car DU is based on kmsxx.
>>
>> We are implementing a 'loop-back' validator, by providing an HDMI cable from
>> the display output directly into the HDMI receiver on the same development
>> board.
>>
>> By using the existing VideoStreamer and pyvid objects, we can capture the output
>> and compare to provide automated display validation.
>>
>> As an effort to commence this process, this series implements a basic frame
>> buffer comparison, which expects identical frame sizes to operate.
>>
>> This could be could be expanded upon later to provide a more detailed
>> comparison at the pixel level later, but this serves as an initial starting
>> point to quickly verify if the data captured matches the data that we expected.
>>
>> Kieran Bingham (4):
>>    videodevice: Fix minor spacing
>>    py: pyvid: Provide stream_off binding
> 
> Thanks, I've picked up the first two patches.
That's great.

Thanks again.

--
Kieran