[libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix validateColorSpaces to choose the correct "main" colour space

[Jacopo Mondi]

Hi David

On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote:
> The intention is that the "main" colour space is the colour space of
> the largest non-raw stream. Unfortunately the use of "config_[i].size"
> is clearly incorrect, and has been copied from prior versions of the
> code. This small change merely corrects the error.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/libcamera/camera.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 2d947a44..0da167a7 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>  	 * largest non-raw stream with a defined color space (if there is one).
>  	 */
>  	std::optional<ColorSpace> colorSpace;
> +	Size size;
>
>  	for (auto [i, cfg] : utils::enumerate(config_)) {
>  		if (!cfg.colorSpace)
> @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>  		if (cfg.colorSpace->adjust(cfg.pixelFormat))
>  			status = Adjusted;
>
> -		if (cfg.colorSpace != ColorSpace::Raw &&
> -		    (!colorSpace || cfg.size > config_[i].size))
> +		if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) {

I'm not too familiar with this part of the code, but seeing

 	for (auto [i, cfg] : utils::enumerate(config_)) {
                ...
		if (cfg.size > config_[i].size))

makes me indeed think that cfg.size == config_[i].size

However I wonder why the condition was preceded by !colorSpace..
The same condition was expressed in
5e5eadabd812 ("libcamera: camera: Add validateColorSpaces to CameraConfiguration class")

as

               } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {
                       index = i;

And I guess it was because the second part of the condition was always
false :)

Now that I've tracked down the full history of the bug
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>  			colorSpace = cfg.colorSpace;
> +			size = cfg.size;
> +		}
>  	}
>
>  	if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
> --
> 2.30.2
>