[libcamera-devel] [PATCH v3 5/8] test: v4l2_subdevice: Add format handling test
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Feb 27 19:04:16 CET 2019
Hi Jacopo,
Thank you for the patch.
On Tue, Feb 26, 2019 at 05:26:38PM +0100, Jacopo Mondi wrote:
> Add test for video format get and set operations on V4L2Subdevice class.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> test/meson.build | 1 +
> test/v4l2_subdevice/meson.build | 10 +++
> test/v4l2_subdevice/test_formats.cpp | 79 ++++++++++++++++++
> test/v4l2_subdevice/v4l2_subdevice_test.cpp | 90 +++++++++++++++++++++
> test/v4l2_subdevice/v4l2_subdevice_test.h | 36 +++++++++
> 5 files changed, 216 insertions(+)
> create mode 100644 test/v4l2_subdevice/meson.build
> create mode 100644 test/v4l2_subdevice/test_formats.cpp
> create mode 100644 test/v4l2_subdevice/v4l2_subdevice_test.cpp
> create mode 100644 test/v4l2_subdevice/v4l2_subdevice_test.h
>
> diff --git a/test/meson.build b/test/meson.build
> index d515a716207e..5fb16fa6afb6 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -3,6 +3,7 @@ subdir('libtest')
> subdir('media_device')
> subdir('pipeline')
> subdir('v4l2_device')
> +subdir('v4l2_subdevice')
>
> public_tests = [
> ['event', 'event.cpp'],
> diff --git a/test/v4l2_subdevice/meson.build b/test/v4l2_subdevice/meson.build
> new file mode 100644
> index 000000000000..a4359fe1bc19
> --- /dev/null
> +++ b/test/v4l2_subdevice/meson.build
> @@ -0,0 +1,10 @@
> +v4l2_subdevice_tests = [
> + [ 'test_formats', 'test_formats.cpp'],
> +]
> +
> +foreach t : v4l2_subdevice_tests
> + exe = executable(t[0], [t[1], 'v4l2_subdevice_test.cpp'],
> + link_with : test_libraries,
> + include_directories : test_includes_internal)
> + test(t[0], exe, suite: 'v4l2_subdevice', is_parallel: false)
> +endforeach
4 spaces for indentation ?
> diff --git a/test/v4l2_subdevice/test_formats.cpp b/test/v4l2_subdevice/test_formats.cpp
> new file mode 100644
> index 000000000000..91b460f64e7e
> --- /dev/null
> +++ b/test/v4l2_subdevice/test_formats.cpp
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * libcamera V4L2 Subdevice format handling test
> + */
> +
> +#include <climits>
> +#include <iostream>
> +
> +#include "v4l2_subdevice.h"
> +
Do we need a blank line here ?
> +#include "v4l2_subdevice_test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +/* Test format handling on the "Scaler" subdevice of vimc media device. */
> +
> +class FormatHandlingTest : public V4L2SubdeviceTest
> +{
> +protected:
> + int run();
override ?
> +};
> +
> +int FormatHandlingTest::run()
> +{
> + V4L2SubdeviceFormat format = {};
> +
> + /*
> + * Get format on a non-existing Scaler pad: expect failure.
> + */
> + int ret = scaler_->getFormat(2, &format);
> + if (!ret) {
> + cerr << "Get format on a non existing pad should fail" << endl;
s/Get/Getting/ or s/Get format/getFormat()/ ?
> + return TestFail;
> + }
> +
> + ret = scaler_->getFormat(0, &format);
> + if (ret) {
> + cerr << "Failed to get format" << endl;
> + return TestFail;
> + }
> +
> + /*
> + * Set unrealistic image resolutions and make sure it gets updated.
> + */
> + format.width = UINT_MAX;
> + format.height = UINT_MAX;
> + ret = scaler_->setFormat(0, &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 = scaler_->setFormat(0, &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;
> +}
> +
> +TEST_REGISTER(FormatHandlingTest);
> diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> new file mode 100644
> index 000000000000..10fe2655e5e4
> --- /dev/null
> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> @@ -0,0 +1,90 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_subdevice_test.cpp - V4L2 subdevice test
Maybe "VIMC-based V4L2 subdevice test" given that the test class
requires VIMC ?
> + */
> +
> +#include <iostream>
> +#include <string.h>
> +#include <sys/stat.h>
> +
> +#include "device_enumerator.h"
> +#include "media_device.h"
> +#include "v4l2_subdevice.h"
> +
> +#include "v4l2_subdevice_test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +/*
> + * This test runs on vimc media device. For a description of vimc, in the
> + * context of libcamera testingrefer to
> + * 'test/media_device/media_device_link_test.cpp' file.
> + *
> + * If the vimc module is not loaded, the test gets skipped.
> + */
> +
> +int V4L2SubdeviceTest::init()
> +{
> + enumerator_ = DeviceEnumerator::create();
> + if (!enumerator_) {
> + cerr << "Failed to create device enumerator" << endl;
> + return TestFail;
> + }
> +
> + if (enumerator_->enumerate()) {
> + cerr << "Failed to enumerate media devices" << endl;
> + return TestFail;
> + }
> +
> + DeviceMatch dm("vimc");
> + media_ = std::move(enumerator_->search(dm));
> + if (!media_) {
> + cerr << "Unable to find \'vimc\' media device node" << endl;
> + return TestSkip;
> + }
> +
> + media_->acquire();
> +
> + int ret = media_->open();
> + if (ret) {
> + cerr << "Unable to open media device: " << media_->deviceNode()
> + << ": " << strerror(ret) << endl;
> + media_->release();
> + return TestSkip;
> + }
> +
> + MediaEntity *videoEntity = media_->getEntityByName("Scaler");
> + if (!videoEntity) {
> + cerr << "Unable to find media entity 'Scaler'" << endl;
> + media_->release();
> + return TestSkip;
Shouldn't this be TestFail ?
> + }
> +
> + scaler_ = new V4L2Subdevice(videoEntity);
> + if (!scaler_) {
> + cerr << "Unable to create media device from media entity: "
> + << videoEntity->deviceNode();
> + media_->release();
> + return TestSkip;
And here too ?
> + }
> +
> + ret = scaler_->open();
> + if (ret) {
> + cerr << "Unable to open video subdevice "
> + << scaler_->deviceNode() << endl;
> + media_->release();
> + return TestSkip;
This one can be a skip as there could be permission issues on the subdev
node.
> + }
> +
> + return 0;
> +}
> +
> +void V4L2SubdeviceTest::cleanup()
> +{
> + media_->release();
> +
> + delete scaler_;
> +}
> diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.h b/test/v4l2_subdevice/v4l2_subdevice_test.h
> new file mode 100644
> index 000000000000..7b64c6122745
> --- /dev/null
> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_subdevice_test.h - V4L2 subdevice test
> + */
> +
> +#ifndef __LIBCAMERA_V4L2_SUBDEVICE_TEST_H_
> +#define __LIBCAMERA_V4L2_SUBDEVICE_TEST_H_
Double underscore at the end of both line ?
> +
> +#include <libcamera/buffer.h>
> +
> +#include "device_enumerator.h"
> +#include "media_device.h"
> +#include "test.h"
> +#include "v4l2_subdevice.h"
> +
> +using namespace libcamera;
> +
> +class V4L2SubdeviceTest : public Test
> +{
> +public:
> + V4L2SubdeviceTest()
> + : scaler_(nullptr){};
> +
> +protected:
> + int init();
override ?
> + virtual int run() = 0;
This isn't needed, the Test class already declares the virtual function.
> + void cleanup();
override here too ?
> +
> + std::unique_ptr<DeviceEnumerator> enumerator_;
> + std::shared_ptr<MediaDevice> media_;
> + V4L2Subdevice *scaler_;
> +};
> +
> +#endif /* __LIBCAMERA_V4L2_SUBDEVICE_TEST_H_ */
And double underscore here too ?
Otherwise this looks good to me.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list