[libcamera-devel,v3,1/2] libcamera: camera: fix validateColorSpaces to choose the correct "main" colour space
diff mbox series

Message ID 20230112121044.28003-2-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Fix colour spaces on Raspberry Pi
Related show

Commit Message

David Plowman Jan. 12, 2023, 12:10 p.m. UTC 
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@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/camera.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Umang Jain Jan. 17, 2023, 7:32 a.m. UTC | #1 
Hi David,

On 1/12/23 5:40 PM, 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.

I would also document dropping of !colorspace condition, as it's vestigial.

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.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) {
>   			colorSpace = cfg.colorSpace;
> +			size = cfg.size;
> +		}
>   	}
>   
>   	if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))

Patch
diff mbox series 

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) {
 			colorSpace = cfg.colorSpace;
+			size = cfg.size;
+		}
 	}
 
 	if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))