[PATCH] libcamera: software_isp: Assign colour spaces in configurations

[Kieran Bingham]

Quoting Milan Zamazal (2025-03-20 16:26:24)
> StreamConfiguration's should have colorSpace set.  This is not the case
> in the simple pipeline.  Let's set it there, basically mimicking what
> rpi and rkisp1 pipelines do (having a common helper may be a
> consideration).
> 
> This also fixes a crash in `cam' due to accessing an unset colorSpace.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6e039bf3..62db96ca 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>
> @@ -1196,11 +1197,24 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>          *
>          * \todo Implement a better way to pick the default format
>          */
> -       for ([[maybe_unused]] StreamRole role : roles) {
> +       for (StreamRole role : roles) {
>                 StreamConfiguration cfg{ StreamFormats{ formats } };
>                 cfg.pixelFormat = formats.begin()->first;
>                 cfg.size = formats.begin()->second[0].max;
>  
> +               switch (role) {
> +               case StreamRole::Raw:
> +                       cfg.colorSpace = ColorSpace::Raw;
> +                       break;
> +               case StreamRole::StillCapture:
> +               case StreamRole::Viewfinder:
> +                       cfg.colorSpace = ColorSpace::Sycc;
> +                       break;
> +               case StreamRole::VideoRecording:
> +                       cfg.colorSpace = ColorSpace::Rec709;
> +                       break;
> +               }
> +

I'm not convinced we can set these like this yet. I don't think it's
correct/accurate yet - The debayering isn't performing any CCM or color
space conversion that I can see - so I 'suspect' they are always in
ColorSpace::Raw...  but I'm not sure if that's actually valid for a
debayered stream ?

But until there's any form of CSC ... I don't think we can report that
we're in Rec709 for instance...

So maybe ColorSpace::Raw is the correct thing to report for all cases
now.

And crucially, it needs to be set in validate() so that if an
application 'requests' a specific colorspace, the validation specifies
what will actually be provided. (so in the future if there is any CSC,
it would ensure that the correct colorspace is reported back)...

--
Kieran


>                 config->addConfiguration(cfg);
>         }
>  
> -- 
> 2.49.0
>