[PATCH] apps: cam: Fix colorSpace access crash in KMSSink::configure

[Kieran Bingham]

Quoting Milan Zamazal (2025-03-14 13:30:18)
> Hi Kieran,
> 
> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 
> > Quoting Milan Zamazal (2025-03-10 11:06:30)
> >> cfg.colorSpace may be unset in KMSSink::configure, resulting in a crash
> >> when it is accessed.  If cfg.colorSpace is unset, simply return, the
> >> same way as when YcbcrEncoding is set to None.
> >
> > I think this is something that we should ensure is trapped by
> > lc-compliance in fact.
> >
> > I believe pipeline handlers /must/ always set the correct colorSpace
> > after validate - so it's incorrect for applications to ever hit an
> > undefined color space ...
> 
> I'm not sure whether all the pipelines do that; at least `simple'
> doesn't.  I can fix `simple' pipeline but maybe some others have the
> problem too.

That's why I think we need to do better on lc-complicance tests to
ensure 'rules' are always validated.

> > Of course crashing isn't nice either ... 
> 
> Let's have an assertion there then to still expose the problem while
> crashing in a civilized way?

In kmssink? I'm fine adding an assertion here - but what about the other
sinks ? What about the other applciations. That's why 'this' should be
validated for /all/ pipeline handlers.

There should be a contract somewhere that states a pipeline handler
/must/ fill in specific fields.

I have an upcoming proposal that could let us add a validation layer to
all runtimes too which could be another place to add runtime
validations.

Hopefully I'll get that out next week as an initial RFC.

> > but is this occuring in SoftISP/simple pipeline handler ?
> 
> Yes.  It started happening to me once I reinstalled my development
> system to Fedora 41.  I can't see any obvious reason why it crashes now
> and not before (maybe some change in gcc? -- using 14.2.1).
> 

It will be curious if this only appears as a result of changing external
tooling though !

> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> >> ---
> >>  src/apps/cam/kms_sink.cpp | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp
> >> index 672c985a..aa9459cf 100644
> >> --- a/src/apps/cam/kms_sink.cpp
> >> +++ b/src/apps/cam/kms_sink.cpp
> >> @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config)
> >>         colorEncoding_ = std::nullopt;
> >>         colorRange_ = std::nullopt;
> >>  
> >> -       if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)
> >> +       if (!cfg.colorSpace ||
> >> +           cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)
> >>                 return 0;
> >>  
> >>         /*
> >> -- 
> >> 2.48.1
> >>
>