[libcamera-devel,v3,6/7] tests: stream: Add a colorspace adjustment test
diff mbox series

Message ID 20220829163742.1006102-7-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • Colorspace adjustments and gstreamer mappings
Related show

Commit Message

Umang Jain Aug. 29, 2022, 4:37 p.m. UTC 
ColorSpace can be adjusted based on the stream's pixelFormat being
requested. Add a test to check the adjustment logic defined in
ColorSpace::adjust().

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 test/stream/meson.build           |  1 +
 test/stream/stream_colorspace.cpp | 87 +++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 test/stream/stream_colorspace.cpp

Comments

Laurent Pinchart Aug. 29, 2022, 10:28 p.m. UTC | #1 
Hi Umang,

Thank you for the patch.

On Mon, Aug 29, 2022 at 10:07:41PM +0530, Umang Jain via libcamera-devel wrote:
> ColorSpace can be adjusted based on the stream's pixelFormat being
> requested. Add a test to check the adjustment logic defined in
> ColorSpace::adjust().
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  test/stream/meson.build           |  1 +
>  test/stream/stream_colorspace.cpp | 87 +++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
>  create mode 100644 test/stream/stream_colorspace.cpp
> 
> diff --git a/test/stream/meson.build b/test/stream/meson.build
> index 73608ffd..89f51c18 100644
> --- a/test/stream/meson.build
> +++ b/test/stream/meson.build
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  stream_tests = [
> +    ['stream_colorspace', 'stream_colorspace.cpp'],
>      ['stream_formats',  'stream_formats.cpp'],
>  ]
>  
> diff --git a/test/stream/stream_colorspace.cpp b/test/stream/stream_colorspace.cpp
> new file mode 100644
> index 00000000..7b7e84e0
> --- /dev/null
> +++ b/test/stream/stream_colorspace.cpp
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Ideas on Board Oy.
> + *
> + * stream_colorspace.cpp - Stream colorspace tests
> + */
> +
> +#include <iostream>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/formats.h>
> +#include <libcamera/stream.h>
> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +using namespace std;
> +
> +class TestCameraConfiguration : public CameraConfiguration
> +{
> +public:
> +	TestCameraConfiguration()
> +		: CameraConfiguration()
> +	{
> +	}
> +
> +	Status validate() override
> +	{
> +		return validateColorSpaces();

Clever way to test this :-)

> +	}
> +};
> +
> +class StreamColorSpaceTest : public Test
> +{
> +protected:
> +	int run()
> +	{
> +		StreamConfiguration cfg;
> +		cfg.size = { 640, 320 };
> +		cfg.pixelFormat = formats::YUV422;
> +		cfg.colorSpace = ColorSpace::Srgb;
> +		config_.addConfiguration(cfg);
> +
> +		StreamConfiguration &streamCfg = config_.at(0);
> +
> +		/*
> +		 * YUV pixelformat with sRGB colorspace should have Y'CbCr encoding
> +		 * adjusted.
> +		 */
> +		config_.validate();
> +		if (streamCfg.colorSpace->ycbcrEncoding == ColorSpace::YcbcrEncoding::None)

Test failures should print an error message, to identify the test that
fails. Something like.

			std::cerr << "YUV format must have YCbCr encoding" << std::endl;

Same below.

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

> +			return TestFail;
> +
> +		/*
> +		 * For YUV pixelFormat, encoding should picked up according to

s/should picked/should be picked/

> +		 * primaries and transfer function, if 'None' is specified.
> +		 */
> +		streamCfg.pixelFormat = formats::YUV422;
> +		streamCfg.colorSpace = ColorSpace(ColorSpace::Primaries::Rec2020,
> +						  ColorSpace::TransferFunction::Rec709,
> +						  ColorSpace::YcbcrEncoding::None,
> +						  ColorSpace::Range::Limited);
> +		config_.validate();
> +		if (streamCfg.colorSpace->ycbcrEncoding != ColorSpace::YcbcrEncoding::Rec2020)
> +			return TestFail;
> +
> +		/* For RGB pixelFormat, sRGB colorspace shouldn't get adjusted */

s/adjusted/adjusted./

Same below.

> +		streamCfg.pixelFormat = formats::RGB888;
> +		streamCfg.colorSpace = ColorSpace::Srgb;

I'd set it to Sycc here to test that it gets adjusted to Srgb.

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

> +		config_.validate();
> +		if (streamCfg.colorSpace != ColorSpace::Srgb)
> +			return TestFail;
> +
> +		/* Raw formats should always set colorspace to ColorSpace::Raw */
> +		streamCfg.pixelFormat = formats::SBGGR8;
> +		streamCfg.colorSpace = ColorSpace::Rec709;
> +		config_.validate();
> +		if (streamCfg.colorSpace != ColorSpace::Raw)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +	TestCameraConfiguration config_;
> +};
> +
> +TEST_REGISTER(StreamColorSpaceTest)
Laurent Pinchart Aug. 29, 2022, 10:28 p.m. UTC | #2 
One more comment.

On Tue, Aug 30, 2022 at 01:28:04AM +0300, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Mon, Aug 29, 2022 at 10:07:41PM +0530, Umang Jain via libcamera-devel wrote:
> > ColorSpace can be adjusted based on the stream's pixelFormat being
> > requested. Add a test to check the adjustment logic defined in
> > ColorSpace::adjust().
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  test/stream/meson.build           |  1 +
> >  test/stream/stream_colorspace.cpp | 87 +++++++++++++++++++++++++++++++
> >  2 files changed, 88 insertions(+)
> >  create mode 100644 test/stream/stream_colorspace.cpp
> > 
> > diff --git a/test/stream/meson.build b/test/stream/meson.build
> > index 73608ffd..89f51c18 100644
> > --- a/test/stream/meson.build
> > +++ b/test/stream/meson.build
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >  
> >  stream_tests = [
> > +    ['stream_colorspace', 'stream_colorspace.cpp'],
> >      ['stream_formats',  'stream_formats.cpp'],
> >  ]
> >  
> > diff --git a/test/stream/stream_colorspace.cpp b/test/stream/stream_colorspace.cpp
> > new file mode 100644
> > index 00000000..7b7e84e0
> > --- /dev/null
> > +++ b/test/stream/stream_colorspace.cpp
> > @@ -0,0 +1,87 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2022, Ideas on Board Oy.
> > + *
> > + * stream_colorspace.cpp - Stream colorspace tests
> > + */
> > +
> > +#include <iostream>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "test.h"
> > +
> > +using namespace libcamera;
> > +using namespace std;
> > +
> > +class TestCameraConfiguration : public CameraConfiguration
> > +{
> > +public:
> > +	TestCameraConfiguration()
> > +		: CameraConfiguration()
> > +	{
> > +	}
> > +
> > +	Status validate() override
> > +	{
> > +		return validateColorSpaces();
> 
> Clever way to test this :-)
> 
> > +	}
> > +};
> > +
> > +class StreamColorSpaceTest : public Test
> > +{
> > +protected:
> > +	int run()
> > +	{
> > +		StreamConfiguration cfg;
> > +		cfg.size = { 640, 320 };
> > +		cfg.pixelFormat = formats::YUV422;
> > +		cfg.colorSpace = ColorSpace::Srgb;
> > +		config_.addConfiguration(cfg);
> > +
> > +		StreamConfiguration &streamCfg = config_.at(0);
> > +
> > +		/*
> > +		 * YUV pixelformat with sRGB colorspace should have Y'CbCr encoding
> > +		 * adjusted.
> > +		 */
> > +		config_.validate();
> > +		if (streamCfg.colorSpace->ycbcrEncoding == ColorSpace::YcbcrEncoding::None)
> 
> Test failures should print an error message, to identify the test that
> fails. Something like.
> 
> 			std::cerr << "YUV format must have YCbCr encoding" << std::endl;
> 
> Same below.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +			return TestFail;
> > +
> > +		/*
> > +		 * For YUV pixelFormat, encoding should picked up according to
> 
> s/should picked/should be picked/
> 
> > +		 * primaries and transfer function, if 'None' is specified.
> > +		 */
> > +		streamCfg.pixelFormat = formats::YUV422;
> > +		streamCfg.colorSpace = ColorSpace(ColorSpace::Primaries::Rec2020,
> > +						  ColorSpace::TransferFunction::Rec709,
> > +						  ColorSpace::YcbcrEncoding::None,
> > +						  ColorSpace::Range::Limited);
> > +		config_.validate();
> > +		if (streamCfg.colorSpace->ycbcrEncoding != ColorSpace::YcbcrEncoding::Rec2020)
> > +			return TestFail;
> > +
> > +		/* For RGB pixelFormat, sRGB colorspace shouldn't get adjusted */
> 
> s/adjusted/adjusted./
> 
> Same below.
> 
> > +		streamCfg.pixelFormat = formats::RGB888;
> > +		streamCfg.colorSpace = ColorSpace::Srgb;
> 
> I'd set it to Sycc here to test that it gets adjusted to Srgb.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +		config_.validate();
> > +		if (streamCfg.colorSpace != ColorSpace::Srgb)
> > +			return TestFail;
> > +
> > +		/* Raw formats should always set colorspace to ColorSpace::Raw */
> > +		streamCfg.pixelFormat = formats::SBGGR8;
> > +		streamCfg.colorSpace = ColorSpace::Rec709;
> > +		config_.validate();
> > +		if (streamCfg.colorSpace != ColorSpace::Raw)
> > +			return TestFail;
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	TestCameraConfiguration config_;

This can be a local variable in the run() function.

> > +};
> > +
> > +TEST_REGISTER(StreamColorSpaceTest)

Patch
diff mbox series 

diff --git a/test/stream/meson.build b/test/stream/meson.build
index 73608ffd..89f51c18 100644
--- a/test/stream/meson.build
+++ b/test/stream/meson.build
@@ -1,6 +1,7 @@ 
 # SPDX-License-Identifier: CC0-1.0
 
 stream_tests = [
+    ['stream_colorspace', 'stream_colorspace.cpp'],
     ['stream_formats',  'stream_formats.cpp'],
 ]
 
diff --git a/test/stream/stream_colorspace.cpp b/test/stream/stream_colorspace.cpp
new file mode 100644
index 00000000..7b7e84e0
--- /dev/null
+++ b/test/stream/stream_colorspace.cpp
@@ -0,0 +1,87 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2022, Ideas on Board Oy.
+ *
+ * stream_colorspace.cpp - Stream colorspace tests
+ */
+
+#include <iostream>
+
+#include <libcamera/camera.h>
+#include <libcamera/formats.h>
+#include <libcamera/stream.h>
+
+#include "test.h"
+
+using namespace libcamera;
+using namespace std;
+
+class TestCameraConfiguration : public CameraConfiguration
+{
+public:
+	TestCameraConfiguration()
+		: CameraConfiguration()
+	{
+	}
+
+	Status validate() override
+	{
+		return validateColorSpaces();
+	}
+};
+
+class StreamColorSpaceTest : public Test
+{
+protected:
+	int run()
+	{
+		StreamConfiguration cfg;
+		cfg.size = { 640, 320 };
+		cfg.pixelFormat = formats::YUV422;
+		cfg.colorSpace = ColorSpace::Srgb;
+		config_.addConfiguration(cfg);
+
+		StreamConfiguration &streamCfg = config_.at(0);
+
+		/*
+		 * YUV pixelformat with sRGB colorspace should have Y'CbCr encoding
+		 * adjusted.
+		 */
+		config_.validate();
+		if (streamCfg.colorSpace->ycbcrEncoding == ColorSpace::YcbcrEncoding::None)
+			return TestFail;
+
+		/*
+		 * For YUV pixelFormat, encoding should picked up according to
+		 * primaries and transfer function, if 'None' is specified.
+		 */
+		streamCfg.pixelFormat = formats::YUV422;
+		streamCfg.colorSpace = ColorSpace(ColorSpace::Primaries::Rec2020,
+						  ColorSpace::TransferFunction::Rec709,
+						  ColorSpace::YcbcrEncoding::None,
+						  ColorSpace::Range::Limited);
+		config_.validate();
+		if (streamCfg.colorSpace->ycbcrEncoding != ColorSpace::YcbcrEncoding::Rec2020)
+			return TestFail;
+
+		/* For RGB pixelFormat, sRGB colorspace shouldn't get adjusted */
+		streamCfg.pixelFormat = formats::RGB888;
+		streamCfg.colorSpace = ColorSpace::Srgb;
+		config_.validate();
+		if (streamCfg.colorSpace != ColorSpace::Srgb)
+			return TestFail;
+
+		/* Raw formats should always set colorspace to ColorSpace::Raw */
+		streamCfg.pixelFormat = formats::SBGGR8;
+		streamCfg.colorSpace = ColorSpace::Rec709;
+		config_.validate();
+		if (streamCfg.colorSpace != ColorSpace::Raw)
+			return TestFail;
+
+		return TestPass;
+	}
+
+	TestCameraConfiguration config_;
+};
+
+TEST_REGISTER(StreamColorSpaceTest)