libcamera: software_isp: Assign colour spaces in configurations

25149 diff mbox series

StreamConfiguration's should have colorSpace set.  This is not the case
in the simple pipeline.  Let's set it there.  This also fixes a crash in
`cam' due to accessing an unset colorSpace.

We set the colour spaces according to the pixel format.  This is not
completely correct because pixel formats and colour spaces are
different, although not completely independent, things.  But for the
lack of a better practical option to determine the colour space, we use
this.

Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/294
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Hi Milan,

Thank you for the patch!

Quoting Milan Zamazal (2025-11-21 15:18:02)
> StreamConfiguration's should have colorSpace set.  This is not the case
> in the simple pipeline.  Let's set it there.  This also fixes a crash in
> `cam' due to accessing an unset colorSpace.
> 
> We set the colour spaces according to the pixel format.  This is not
> completely correct because pixel formats and colour spaces are
> different, although not completely independent, things.  But for the
> lack of a better practical option to determine the colour space, we use
> this.
> 
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/294
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 118b4186c..a8bfe0cf7 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -25,6 +25,7 @@
>  #include <libcamera/base/log.h>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/color_space.h>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
> @@ -36,6 +37,7 @@
>  #include "libcamera/internal/converter.h"
>  #include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/global_configuration.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
> @@ -1227,6 +1229,44 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>                         status = Adjusted;
>                 }
>  
> +               /*
> +                * Best effort to fix the color space. If the color space is not set,
> +                * set it according to the pixel format, which may not be correct (pixel
> +                * formats and color spaces are different things, although somewhat
> +                * related) but we don't have a better option at the moment. Then in any
> +                * case, perform the standard pixel format based color space adjustment.
> +                */
> +               if (!cfg.colorSpace) {
> +                       const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> +                       switch (info.colourEncoding) {
> +                       case PixelFormatInfo::ColourEncodingRGB:
> +                               cfg.colorSpace = ColorSpace::Srgb;
> +                               break;
> +                       case libcamera::PixelFormatInfo::ColourEncodingYUV:
> +                               cfg.colorSpace = ColorSpace::Sycc;
> +                               break;

Does this need to be libcamera::PixelFormatInfo::ColourEncoding as opposed to the
PixelFormatInfo::ColourEncoding used above?

If it does, looks good to me!

Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>

> +                       default:
> +                               cfg.colorSpace = ColorSpace::Raw;
> +                       }
> +                       LOG(SimplePipeline, Debug)
> +                               << "Unspecified color space set to "
> +                               << cfg.colorSpace.value().toString();
> +                       /*
> +                        * Adjust the assigned color space to make sure everything is OK.
> +                        * Since this is assigning an unspecified color space rather than
> +                        * adjusting a requested one, changes here shouldn't set the status
> +                        * to Adjusted.
> +                        */
> +                       cfg.colorSpace->adjust(pixelFormat);
> +               } else {
> +                       if (cfg.colorSpace->adjust(pixelFormat)) {
> +                               LOG(SimplePipeline, Debug)
> +                                       << "Color space adjusted to "
> +                                       << cfg.colorSpace.value().toString();
> +                               status = Adjusted;
> +                       }
> +               }
> +
>                 if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>                         Size adjustedSize = pipeConfig_->captureSize;
>                         /*
> -- 
> 2.51.1
>

Hi Isaac,

thank you for review.

Isaac Scott <isaac.scott@ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch!
>
> Quoting Milan Zamazal (2025-11-21 15:18:02)
>> StreamConfiguration's should have colorSpace set.  This is not the case
>> in the simple pipeline.  Let's set it there.  This also fixes a crash in
>> `cam' due to accessing an unset colorSpace.
>> 
>> We set the colour spaces according to the pixel format.  This is not
>> completely correct because pixel formats and colour spaces are
>> different, although not completely independent, things.  But for the
>> lack of a better practical option to determine the colour space, we use
>> this.
>> 
>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/294
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 118b4186c..a8bfe0cf7 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -25,6 +25,7 @@
>>  #include <libcamera/base/log.h>
>>  
>>  #include <libcamera/camera.h>
>> +#include <libcamera/color_space.h>
>>  #include <libcamera/control_ids.h>
>>  #include <libcamera/request.h>
>>  #include <libcamera/stream.h>
>> @@ -36,6 +37,7 @@
>>  #include "libcamera/internal/converter.h"
>>  #include "libcamera/internal/delayed_controls.h"
>>  #include "libcamera/internal/device_enumerator.h"
>> +#include "libcamera/internal/formats.h"
>>  #include "libcamera/internal/global_configuration.h"
>>  #include "libcamera/internal/media_device.h"
>>  #include "libcamera/internal/pipeline_handler.h"
>> @@ -1227,6 +1229,44 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>                         status = Adjusted;
>>                 }
>>  
>> +               /*
>> +                * Best effort to fix the color space. If the color space is not set,
>> +                * set it according to the pixel format, which may not be correct (pixel
>> +                * formats and color spaces are different things, although somewhat
>> +                * related) but we don't have a better option at the moment. Then in any
>> +                * case, perform the standard pixel format based color space adjustment.
>> +                */
>> +               if (!cfg.colorSpace) {
>> +                       const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
>> +                       switch (info.colourEncoding) {
>> +                       case PixelFormatInfo::ColourEncodingRGB:
>> +                               cfg.colorSpace = ColorSpace::Srgb;
>> +                               break;
>> +                       case libcamera::PixelFormatInfo::ColourEncodingYUV:
>> +                               cfg.colorSpace = ColorSpace::Sycc;
>> +                               break;
>
> Does this need to be libcamera::PixelFormatInfo::ColourEncoding as opposed to the
> PixelFormatInfo::ColourEncoding used above?

No, I'll post v2 with the prefix removed.

> If it does, looks good to me!
>
> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>
>> +                       default:
>> +                               cfg.colorSpace = ColorSpace::Raw;
>> +                       }
>> +                       LOG(SimplePipeline, Debug)
>> +                               << "Unspecified color space set to "
>> +                               << cfg.colorSpace.value().toString();
>> +                       /*
>> +                        * Adjust the assigned color space to make sure everything is OK.
>> +                        * Since this is assigning an unspecified color space rather than
>> +                        * adjusting a requested one, changes here shouldn't set the status
>> +                        * to Adjusted.
>> +                        */
>> +                       cfg.colorSpace->adjust(pixelFormat);
>> +               } else {
>> +                       if (cfg.colorSpace->adjust(pixelFormat)) {
>> +                               LOG(SimplePipeline, Debug)
>> +                                       << "Color space adjusted to "
>> +                                       << cfg.colorSpace.value().toString();
>> +                               status = Adjusted;
>> +                       }
>> +               }
>> +
>>                 if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>>                         Size adjustedSize = pipeConfig_->captureSize;
>>                         /*
>> -- 
>> 2.51.1
>>