[libcamera-devel] [PATCH v3 7/8] test: v4l2_device: Add format handling test

[Kieran Bingham]

Hi Jacopo, Laurent,

On 28/02/2019 08:54, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Wed, Feb 27, 2019 at 08:44:02PM +0200, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> On Tue, Feb 26, 2019 at 05:26:40PM +0100, Jacopo Mondi wrote:
>>> Add test for V4L2Device set and get format methods.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>  test/v4l2_device/meson.build      |  1 +
>>>  test/v4l2_device/test_formats.cpp | 65 +++++++++++++++++++++++++++++++
>>>  2 files changed, 66 insertions(+)
>>>  create mode 100644 test/v4l2_device/test_formats.cpp
>>>
>>> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
>>> index 9f7a7545ac9b..e5e50faac282 100644
>>> --- a/test/v4l2_device/meson.build
>>> +++ b/test/v4l2_device/meson.build
>>> @@ -6,6 +6,7 @@ v4l2_device_tests = [
>>>    [ 'stream_on_off',      'stream_on_off.cpp' ],
>>>    [ 'capture_async',      'capture_async.cpp' ],
>>>    [ 'buffer_sharing',     'buffer_sharing.cpp' ],
>>> +  [ 'test_formats',	  'test_formats.cpp' ],
>>
>> V4L2Device tests are soretd by order of increasing complexity, I think
>> think this one should be last in the list.


Yes, the intention was that the tests are increasing in complexity, and
the simplest test gets run first.

Also - the tests should somewhat follow the order of the expected API
usage... so hence open/request_buffers is before stream_on_off.


The tests execute in the order that they are listed in the array.

meson test --suite v4l2_device
1/6 libcamera:v4l2_device / double_open     SKIP     0.00 s
2/6 libcamera:v4l2_device / request_buffers  SKIP     0.00 s
3/6 libcamera:v4l2_device / stream_on_off   SKIP     0.00 s
4/6 libcamera:v4l2_device / capture_async   SKIP     0.00 s
5/6 libcamera:v4l2_device / buffer_sharing  SKIP     0.00 s
6/6 libcamera:v4l2_device / test_formats    SKIP     0.00 s

(Yes, they all skip on my x86 host because I'm still on v4.18 which
doesn't have a media controlled vivid)


> May I say this is not the most intuitive ordering method?
> last in the complexity list == first in the array?

I think laurent made a typo, and should have said:

> I don't think this one should be last in the list.


I would consider this 'test_formats' to be after double open, and before
stream_on_off.

Actually, as setting the formats is an API that you would use before
getting buffers, I would say it goes before request_buffers too.

So that would put in at position 2?

I might also suggest dropping the "test_" prefix from the file name too
... but it's not important. The other tests are not for example called
"test_buffer_sharing".

--
Kieran


> 
>>>  ]
>>>
>>>  foreach t : v4l2_device_tests
>>> diff --git a/test/v4l2_device/test_formats.cpp b/test/v4l2_device/test_formats.cpp
>>> new file mode 100644
>>> index 000000000000..dcb05a3904f7
>>> --- /dev/null
>>> +++ b/test/v4l2_device/test_formats.cpp
>>> @@ -0,0 +1,65 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright (C) 2019, Google Inc.
>>> + *
>>> + * libcamera V4L2 device format handling test
>>> + */
>>> +
>>> +#include <climits>
>>> +#include <iostream>
>>> +
>>> +#include "v4l2_device.h"
>>> +
>>> +#include "v4l2_device_test.h"
>>> +
>>> +using namespace std;
>>> +using namespace libcamera;
>>> +
>>> +class Format : public V4L2DeviceTest
>>> +{
>>> +protected:
>>> +	int run();
>>
>> override
>>
>>> +};
>>> +
>>> +int Format::run()
>>> +{
>>> +	V4L2DeviceFormat format = {};
>>> +
>>> +	int ret = capture_->getFormat(&format);
>>> +	if (ret) {
>>> +		cerr << "Failed to get format" << endl;
>>> +		return TestFail;
>>> +	}
>>> +
>>> +	format.width = UINT_MAX;
>>> +	format.height = UINT_MAX;
>>> +	ret = capture_->setFormat(&format);
>>> +	if (ret) {
>>> +		cerr << "Failed to set format: image resolution is wrong, but "
>>> +		     << "setFormat() should not fail." << endl;
>>> +		return TestFail;
>>> +	}
>>> +
>>> +	if (format.width == UINT_MAX || format.height == UINT_MAX) {
>>> +		cerr << "Failed to update image format" << endl;
>>> +		return TestFail;
>>> +	}
>>> +
>>> +	format.width = 0;
>>> +	format.height = 0;
>>> +	ret = capture_->setFormat(&format);
>>> +	if (ret) {
>>> +		cerr << "Failed to set format: image resolution is wrong, but "
>>> +		     << "setFormat() should not fail." << endl;
>>> +		return TestFail;
>>> +	}
>>> +
>>> +	if (format.width == 0 || format.height == 0) {
>>> +		cerr << "Failed to update image format" << endl;
>>> +		return TestFail;
>>> +	}
>>> +
>>> +	return TestPass;
>>
>> With Kieran's comment addressed,
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> (I however wonder if we need both the 0 and UINT_MAX tests. Testing that
>> the setFormat() function correctly updates the passed format is useful
>> to test our implementation, but testing both 0 and UINT_MAX feels like
>> testing the kernel instead, which is a bit out of scope)
> 
> Ah, I re-wrote this stupid tests a million times, as everytime I did
> something a bit more complex I was actually testing the kernel driver
> instead of the library. This seemed to me as a compromise between
> going for testing the driver and a test with no actual value, but I
> can drop one of the cases.
> Thanks
>    j
> 
>>
>>> +}
>>> +
>>> +TEST_REGISTER(Format);
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran