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

[Milan Zamazal]

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> 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);
>> >         }
>> >  

Well, I don't feel wise about the whole thing, so I posted v2 as an RFC
and let's discuss whether it's an improvement and what's missing.