[libcamera-devel,v3,6/8] test: v4l2_subdevice: Add ListFormat test
diff mbox series

Message ID 20190226162641.12116-7-jacopo@jmondi.org
State Superseded
Headers show
Series
  • v4l2_(sub)dev: improvements and tests
Related show

Commit Message

Jacopo Mondi Feb. 26, 2019, 4:26 p.m. UTC 
Add test to list formats on a v4l2 subdevice.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 test/v4l2_subdevice/list_formats.cpp | 76 ++++++++++++++++++++++++++++
 test/v4l2_subdevice/meson.build      |  1 +
 2 files changed, 77 insertions(+)
 create mode 100644 test/v4l2_subdevice/list_formats.cpp

Comments

Kieran Bingham Feb. 26, 2019, 11:50 p.m. UTC | #1 
Hi Jacopo,

On 26/02/2019 16:26, Jacopo Mondi wrote:
> Add test to list formats on a v4l2 subdevice.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  test/v4l2_subdevice/list_formats.cpp | 76 ++++++++++++++++++++++++++++
>  test/v4l2_subdevice/meson.build      |  1 +
>  2 files changed, 77 insertions(+)
>  create mode 100644 test/v4l2_subdevice/list_formats.cpp
> 
> diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
> new file mode 100644
> index 000000000000..3c4737a17afc
> --- /dev/null
> +++ b/test/v4l2_subdevice/list_formats.cpp
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * libcamera V4L2 Subdevice format handling test
> + */
> +
> +#include <iostream>
> +#include <vector>
> +
> +#include "v4l2_subdevice.h"
> +
> +#include "v4l2_subdevice_test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +/* List image formats on the "Scaler" subdevice of vimc media device.  */
> +
> +class ListFormatsTest : public V4L2SubdeviceTest
> +{
> +protected:
> +	int run();
> +
> +private:
> +	void printFormats(unsigned int pad,
> +			  std::vector<V4L2SubdeviceFormat> &formats);
> +};
> +
> +void ListFormatsTest::printFormats(unsigned int pad,
> +				   std::vector<V4L2SubdeviceFormat> &formats)
> +{
> +	cout << "Enumerate formats on pad " << pad << endl;
> +	for (V4L2SubdeviceFormat &format : formats) {
> +		cout << "	Mbus code: 0x" << hex << format.mbus_code << endl;
> +		cout << "	Width: " << dec << format.width << endl;
> +		cout << "	Height: " << dec << format.height << endl;
> +	}
> +}
> +
> +int ListFormatsTest::run()
> +{
> +	/* List all formats available on existing "Scaler" pads. */
> +	std::vector<V4L2SubdeviceFormat> formats = {};
> +
> +	formats = scaler_->formats(0);
> +	if (formats.empty()) {
> +		cerr << "Failed to list formats on pad 0 of sudevice "

Is the 'list' a verb or a noun in listFormats?

Should it be getFormats()? or enumFormats() in the Subdevice
implementation as it seems formats() will name clash :)


> +		     << scaler_->deviceNode() << endl;
> +		return TestFail;
> +	}
> +	printFormats(0, formats);
> +> +	formats = {};
> +	formats = scaler_->formats(1);
> +	if (formats.empty()) {
> +		cerr << "Failed to list formats on pad 1 of sudevice "

s/sudevice/subdevice/ here and above on pad 0.

> +		     << scaler_->deviceNode() << endl;
> +		return TestFail;
> +	}
> +	printFormats(1, formats);
> +
> +	/* List format on a non-existing pad, format vector shall be empty. */
> +	formats = {};
> +	formats = scaler_->formats(2);
> +	if (!formats.empty()) {
> +		cerr << "Listing formats on non-existing pad 2 of subdevice "
> +		     << scaler_->deviceNode()
> +		     << " should return an empty format list" << endl;
> +		return TestFail;
> +	}
> +
> +	return TestPass;
> +}
> +
> +TEST_REGISTER(ListFormatsTest);
> diff --git a/test/v4l2_subdevice/meson.build b/test/v4l2_subdevice/meson.build
> index a4359fe1bc19..6023d15e1558 100644
> --- a/test/v4l2_subdevice/meson.build
> +++ b/test/v4l2_subdevice/meson.build
> @@ -1,4 +1,5 @@
>  v4l2_subdevice_tests = [
> +  [ 'list_formats',             'list_formats.cpp'],
>    [ 'test_formats',             'test_formats.cpp'],
>  ]
>  
>
Jacopo Mondi Feb. 27, 2019, 8:37 a.m. UTC | #2 
Hi Kieran,

On Tue, Feb 26, 2019 at 11:50:37PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 26/02/2019 16:26, Jacopo Mondi wrote:
> > Add test to list formats on a v4l2 subdevice.
> >

[snip]

> > +int ListFormatsTest::run()
> > +{
> > +	/* List all formats available on existing "Scaler" pads. */
> > +	std::vector<V4L2SubdeviceFormat> formats = {};
> > +
> > +	formats = scaler_->formats(0);
> > +	if (formats.empty()) {
> > +		cerr << "Failed to list formats on pad 0 of sudevice "
>
> Is the 'list' a verb or a noun in listFormats?
>

I interpreted it as a verb.

> Should it be getFormats()? or enumFormats() in the Subdevice
> implementation as it seems formats() will name clash :)

getFormats() is easily confusable with getFormat(), enumFormats()
might make sense, but recalls the SUBDEV_ENUM_ ioctls, which have a
different behaviour.

As the 'formats()' methods return the 'formats_' field, which is a
vector of formats, according to out policy that getters should just
repeat the name of the field they access, I would keep 'formats()' as
the method name, even if I might have missed which name clash you're
reporting here...

Thanks
  j

>
>
> > +		     << scaler_->deviceNode() << endl;
> > +		return TestFail;
> > +	}
> > +	printFormats(0, formats);
> > +> +	formats = {};
> > +	formats = scaler_->formats(1);
> > +	if (formats.empty()) {
> > +		cerr << "Failed to list formats on pad 1 of sudevice "
>
> s/sudevice/subdevice/ here and above on pad 0.
>
> > +		     << scaler_->deviceNode() << endl;
> > +		return TestFail;
> > +	}
> > +	printFormats(1, formats);
> > +
> > +	/* List format on a non-existing pad, format vector shall be empty. */
> > +	formats = {};
> > +	formats = scaler_->formats(2);
> > +	if (!formats.empty()) {
> > +		cerr << "Listing formats on non-existing pad 2 of subdevice "
> > +		     << scaler_->deviceNode()
> > +		     << " should return an empty format list" << endl;
> > +		return TestFail;
> > +	}
> > +
> > +	return TestPass;
> > +}
> > +
> > +TEST_REGISTER(ListFormatsTest);
> > diff --git a/test/v4l2_subdevice/meson.build b/test/v4l2_subdevice/meson.build
> > index a4359fe1bc19..6023d15e1558 100644
> > --- a/test/v4l2_subdevice/meson.build
> > +++ b/test/v4l2_subdevice/meson.build
> > @@ -1,4 +1,5 @@
> >  v4l2_subdevice_tests = [
> > +  [ 'list_formats',             'list_formats.cpp'],
> >    [ 'test_formats',             'test_formats.cpp'],
> >  ]
> >
> >
>
> --
> Regards
> --
> Kieran
Laurent Pinchart Feb. 27, 2019, 6:27 p.m. UTC | #3 
Hi Jacopo,

Thank you for the patch.

On Tue, Feb 26, 2019 at 05:26:39PM +0100, Jacopo Mondi wrote:
> Add test to list formats on a v4l2 subdevice.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  test/v4l2_subdevice/list_formats.cpp | 76 ++++++++++++++++++++++++++++
>  test/v4l2_subdevice/meson.build      |  1 +
>  2 files changed, 77 insertions(+)
>  create mode 100644 test/v4l2_subdevice/list_formats.cpp
> 
> diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
> new file mode 100644
> index 000000000000..3c4737a17afc
> --- /dev/null
> +++ b/test/v4l2_subdevice/list_formats.cpp
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * libcamera V4L2 Subdevice format handling test
> + */
> +
> +#include <iostream>
> +#include <vector>
> +
> +#include "v4l2_subdevice.h"
> +

Maybe no need for a blank line here either ?

> +#include "v4l2_subdevice_test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +/* List image formats on the "Scaler" subdevice of vimc media device.  */
> +
> +class ListFormatsTest : public V4L2SubdeviceTest
> +{
> +protected:
> +	int run();

override ?

> +
> +private:
> +	void printFormats(unsigned int pad,
> +			  std::vector<V4L2SubdeviceFormat> &formats);
> +};
> +
> +void ListFormatsTest::printFormats(unsigned int pad,
> +				   std::vector<V4L2SubdeviceFormat> &formats)
> +{
> +	cout << "Enumerate formats on pad " << pad << endl;
> +	for (V4L2SubdeviceFormat &format : formats) {
> +		cout << "	Mbus code: 0x" << hex << format.mbus_code << endl;
> +		cout << "	Width: " << dec << format.width << endl;
> +		cout << "	Height: " << dec << format.height << endl;
> +	}
> +}
> +
> +int ListFormatsTest::run()
> +{
> +	/* List all formats available on existing "Scaler" pads. */
> +	std::vector<V4L2SubdeviceFormat> formats = {};

No need to initialize the vector as you overwrite it on the next line.

> +
> +	formats = scaler_->formats(0);
> +	if (formats.empty()) {
> +		cerr << "Failed to list formats on pad 0 of sudevice "

s/sudevice/subdevice/

> +		     << scaler_->deviceNode() << endl;
> +		return TestFail;
> +	}
> +	printFormats(0, formats);
> +
> +	formats = {};

No need for this.

> +	formats = scaler_->formats(1);
> +	if (formats.empty()) {
> +		cerr << "Failed to list formats on pad 1 of sudevice "

s/sudevice/subdevice/

> +		     << scaler_->deviceNode() << endl;
> +		return TestFail;
> +	}
> +	printFormats(1, formats);
> +
> +	/* List format on a non-existing pad, format vector shall be empty. */
> +	formats = {};

Same here.

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

> +	formats = scaler_->formats(2);
> +	if (!formats.empty()) {
> +		cerr << "Listing formats on non-existing pad 2 of subdevice "
> +		     << scaler_->deviceNode()
> +		     << " should return an empty format list" << endl;
> +		return TestFail;
> +	}
> +
> +	return TestPass;
> +}
> +
> +TEST_REGISTER(ListFormatsTest);
> diff --git a/test/v4l2_subdevice/meson.build b/test/v4l2_subdevice/meson.build
> index a4359fe1bc19..6023d15e1558 100644
> --- a/test/v4l2_subdevice/meson.build
> +++ b/test/v4l2_subdevice/meson.build
> @@ -1,4 +1,5 @@
>  v4l2_subdevice_tests = [
> +  [ 'list_formats',             'list_formats.cpp'],
>    [ 'test_formats',             'test_formats.cpp'],
>  ]
>
Jacopo Mondi Feb. 28, 2019, 8:51 a.m. UTC | #4 
Hi Laurent,

On Wed, Feb 27, 2019 at 08:27:22PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Feb 26, 2019 at 05:26:39PM +0100, Jacopo Mondi wrote:
> > Add test to list formats on a v4l2 subdevice.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  test/v4l2_subdevice/list_formats.cpp | 76 ++++++++++++++++++++++++++++
> >  test/v4l2_subdevice/meson.build      |  1 +
> >  2 files changed, 77 insertions(+)
> >  create mode 100644 test/v4l2_subdevice/list_formats.cpp
> >
> > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
> > new file mode 100644
> > index 000000000000..3c4737a17afc
> > --- /dev/null
> > +++ b/test/v4l2_subdevice/list_formats.cpp
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * libcamera V4L2 Subdevice format handling test
> > + */
> > +
> > +#include <iostream>
> > +#include <vector>
> > +
> > +#include "v4l2_subdevice.h"
> > +
>
> Maybe no need for a blank line here either ?
>
> > +#include "v4l2_subdevice_test.h"
> > +
> > +using namespace std;
> > +using namespace libcamera;
> > +
> > +/* List image formats on the "Scaler" subdevice of vimc media device.  */
> > +
> > +class ListFormatsTest : public V4L2SubdeviceTest
> > +{
> > +protected:
> > +	int run();
>
> override ?
>
> > +
> > +private:
> > +	void printFormats(unsigned int pad,
> > +			  std::vector<V4L2SubdeviceFormat> &formats);
> > +};
> > +
> > +void ListFormatsTest::printFormats(unsigned int pad,
> > +				   std::vector<V4L2SubdeviceFormat> &formats)
> > +{
> > +	cout << "Enumerate formats on pad " << pad << endl;
> > +	for (V4L2SubdeviceFormat &format : formats) {
> > +		cout << "	Mbus code: 0x" << hex << format.mbus_code << endl;
> > +		cout << "	Width: " << dec << format.width << endl;
> > +		cout << "	Height: " << dec << format.height << endl;
> > +	}
> > +}
> > +
> > +int ListFormatsTest::run()
> > +{
> > +	/* List all formats available on existing "Scaler" pads. */
> > +	std::vector<V4L2SubdeviceFormat> formats = {};
>
> No need to initialize the vector as you overwrite it on the next line.
>
> > +
> > +	formats = scaler_->formats(0);
> > +	if (formats.empty()) {
> > +		cerr << "Failed to list formats on pad 0 of sudevice "
>
> s/sudevice/subdevice/
>
> > +		     << scaler_->deviceNode() << endl;
> > +		return TestFail;
> > +	}
> > +	printFormats(0, formats);
> > +
> > +	formats = {};
>
> No need for this.
>

I -really- wanted this vector to be initialized!

Thanks
  j

> > +	formats = scaler_->formats(1);
> > +	if (formats.empty()) {
> > +		cerr << "Failed to list formats on pad 1 of sudevice "
>
> s/sudevice/subdevice/
>
> > +		     << scaler_->deviceNode() << endl;
> > +		return TestFail;
> > +	}
> > +	printFormats(1, formats);
> > +
> > +	/* List format on a non-existing pad, format vector shall be empty. */
> > +	formats = {};
>
> Same here.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +	formats = scaler_->formats(2);
> > +	if (!formats.empty()) {
> > +		cerr << "Listing formats on non-existing pad 2 of subdevice "
> > +		     << scaler_->deviceNode()
> > +		     << " should return an empty format list" << endl;
> > +		return TestFail;
> > +	}
> > +
> > +	return TestPass;
> > +}
> > +
> > +TEST_REGISTER(ListFormatsTest);
> > diff --git a/test/v4l2_subdevice/meson.build b/test/v4l2_subdevice/meson.build
> > index a4359fe1bc19..6023d15e1558 100644
> > --- a/test/v4l2_subdevice/meson.build
> > +++ b/test/v4l2_subdevice/meson.build
> > @@ -1,4 +1,5 @@
> >  v4l2_subdevice_tests = [
> > +  [ 'list_formats',             'list_formats.cpp'],
> >    [ 'test_formats',             'test_formats.cpp'],
> >  ]
> >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
new file mode 100644
index 000000000000..3c4737a17afc
--- /dev/null
+++ b/test/v4l2_subdevice/list_formats.cpp
@@ -0,0 +1,76 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2019, Google Inc.
+ *
+ * libcamera V4L2 Subdevice format handling test
+ */
+
+#include <iostream>
+#include <vector>
+
+#include "v4l2_subdevice.h"
+
+#include "v4l2_subdevice_test.h"
+
+using namespace std;
+using namespace libcamera;
+
+/* List image formats on the "Scaler" subdevice of vimc media device.  */
+
+class ListFormatsTest : public V4L2SubdeviceTest
+{
+protected:
+	int run();
+
+private:
+	void printFormats(unsigned int pad,
+			  std::vector<V4L2SubdeviceFormat> &formats);
+};
+
+void ListFormatsTest::printFormats(unsigned int pad,
+				   std::vector<V4L2SubdeviceFormat> &formats)
+{
+	cout << "Enumerate formats on pad " << pad << endl;
+	for (V4L2SubdeviceFormat &format : formats) {
+		cout << "	Mbus code: 0x" << hex << format.mbus_code << endl;
+		cout << "	Width: " << dec << format.width << endl;
+		cout << "	Height: " << dec << format.height << endl;
+	}
+}
+
+int ListFormatsTest::run()
+{
+	/* List all formats available on existing "Scaler" pads. */
+	std::vector<V4L2SubdeviceFormat> formats = {};
+
+	formats = scaler_->formats(0);
+	if (formats.empty()) {
+		cerr << "Failed to list formats on pad 0 of sudevice "
+		     << scaler_->deviceNode() << endl;
+		return TestFail;
+	}
+	printFormats(0, formats);
+
+	formats = {};
+	formats = scaler_->formats(1);
+	if (formats.empty()) {
+		cerr << "Failed to list formats on pad 1 of sudevice "
+		     << scaler_->deviceNode() << endl;
+		return TestFail;
+	}
+	printFormats(1, formats);
+
+	/* List format on a non-existing pad, format vector shall be empty. */
+	formats = {};
+	formats = scaler_->formats(2);
+	if (!formats.empty()) {
+		cerr << "Listing formats on non-existing pad 2 of subdevice "
+		     << scaler_->deviceNode()
+		     << " should return an empty format list" << endl;
+		return TestFail;
+	}
+
+	return TestPass;
+}
+
+TEST_REGISTER(ListFormatsTest);
diff --git a/test/v4l2_subdevice/meson.build b/test/v4l2_subdevice/meson.build
index a4359fe1bc19..6023d15e1558 100644
--- a/test/v4l2_subdevice/meson.build
+++ b/test/v4l2_subdevice/meson.build
@@ -1,4 +1,5 @@ 
 v4l2_subdevice_tests = [
+  [ 'list_formats',             'list_formats.cpp'],
   [ 'test_formats',             'test_formats.cpp'],
 ]