[v2,2/2] lc-compliance: Ensure stream's colorspace is set after validate()

23704 diff mbox series

StreamConfiguration::colorspace is a std::optional<> and if unset by
the user, it should be populated by the pipeline handler after the
CameraConfiguration::validate().

Add a EXPECT_TRUE() check to ensure that each stream in the
CameraConfiguration has a colorspace set.

Signed-off-by: Umang Jain <uajain@igalia.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/apps/lc-compliance/helpers/capture.cpp | 3 +++
 1 file changed, 3 insertions(+)

Quoting Umang Jain (2025-07-01 12:29:22)
> StreamConfiguration::colorspace is a std::optional<> and if unset by
> the user, it should be populated by the pipeline handler after the
> CameraConfiguration::validate().
> 
> Add a EXPECT_TRUE() check to ensure that each stream in the
> CameraConfiguration has a colorspace set.
> 
> Signed-off-by: Umang Jain <uajain@igalia.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

From Robert, for Patchwork:

Tested-by: Robert Mader <robert.mader@collabora.com>

> ---
>  src/apps/lc-compliance/helpers/capture.cpp | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 2a3fa3b6..e767e45e 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)
>                 FAIL() << "Configuration not valid";
>         }
>  
> +       for (auto &cfg : *config_)
> +               EXPECT_TRUE(cfg.colorSpace) << cfg << " - colorspace not set.";
> +
>         if (camera_->configure(config_.get())) {
>                 config_.reset();
>                 FAIL() << "Failed to configure camera";
> -- 
> 2.50.0
>

Hi

I believe the usual prefix is "apps: lc-compliance: ".

2025. 07. 01. 13:29 keltezéssel, Umang Jain írta:
> StreamConfiguration::colorspace is a std::optional<> and if unset by
> the user, it should be populated by the pipeline handler after the
> CameraConfiguration::validate().
> 
> Add a EXPECT_TRUE() check to ensure that each stream in the
> CameraConfiguration has a colorspace set.
> 
> Signed-off-by: Umang Jain <uajain@igalia.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   src/apps/lc-compliance/helpers/capture.cpp | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 2a3fa3b6..e767e45e 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)
>   		FAIL() << "Configuration not valid";
>   	}
>   
> +	for (auto &cfg : *config_)

   const auto &


> +		EXPECT_TRUE(cfg.colorSpace) << cfg << " - colorspace not set.";

Could you remove the period at the end? Other message don't have it. And I would
probably print `cfg` at the end, i.e. `"Color space not set for stream: " << cfg;`

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


Regards,
Barnabás Pőcze



> +
>   	if (camera_->configure(config_.get())) {
>   		config_.reset();
>   		FAIL() << "Failed to configure camera";

Hi

2025. 07. 01. 13:29 keltezéssel, Umang Jain írta:
> StreamConfiguration::colorspace is a std::optional<> and if unset by
> the user, it should be populated by the pipeline handler after the
> CameraConfiguration::validate().

The documentation of `StreamConfiguration::colorSpace` states:

/**
  * \var StreamConfiguration::colorSpace
  * \brief The ColorSpace for this stream
  *
  * This field allows a ColorSpace to be selected for this Stream.
  *
  * The field is optional and an application can choose to leave it unset.
  * Platforms that support the use of color spaces may provide default
  * values through the generateConfiguration() method. An application can
  * override these when necessary.
  *
  * [...]
  */

I feel that the text allows pipeline handlers to leave it unset. If this
has changed, this documentation should be adjusted, same applies to the
pipeline handler writers' guide.


Regards,
Barnabás Pőcze



> 
> Add a EXPECT_TRUE() check to ensure that each stream in the
> CameraConfiguration has a colorspace set.
> 
> Signed-off-by: Umang Jain <uajain@igalia.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   src/apps/lc-compliance/helpers/capture.cpp | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 2a3fa3b6..e767e45e 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)
>   		FAIL() << "Configuration not valid";
>   	}
>   
> +	for (auto &cfg : *config_)
> +		EXPECT_TRUE(cfg.colorSpace) << cfg << " - colorspace not set.";
> +
>   	if (camera_->configure(config_.get())) {
>   		config_.reset();
>   		FAIL() << "Failed to configure camera";