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

[Laurent Pinchart]

On Thu, Mar 20, 2025 at 05:18:39PM +0000, Kieran Bingham wrote:
> 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...

The soft ISP outputs RGB only, so ycbcrEncoding must be
YcbcrEncoding::None and range must be Range::Full.

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

Don't forget that the simpler pipeline handler also works with RGB/YUV
sensors, without the soft ISP. This needs to be taken into account too.

> 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)...
> 
> >                 config->addConfiguration(cfg);
> >         }
> >  

-- 
Regards,

Laurent Pinchart