[libcamera-devel] [PATCH v2 1/2] libcamera: camera: fix validateColorSpaces to choose the correct "main" colour space

[David Plowman]

Hi Jacopo

Thanks for the review.

I was wondering if you knew anything about how rkisp1 uses this
function (I think it's the only other caller apart from Raspberry Pi,
at least at the moment).

The reason I ask is because I think the whole "streams sharing a
colour space" thing isn't terribly useful any more (YUV and RGB
streams are no longer allowed to share a ColorSpace), so I'm wondering
about removing it, but one would have to understand the rkisp1
requirements first! Would you know anything about that?

Thanks!
David

On Tue, 3 Jan 2023 at 17:37, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
>
> Hi David
>
> On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote:
> > The intention is that the "main" colour space is the colour space of
> > the largest non-raw stream. Unfortunately the use of "config_[i].size"
> > is clearly incorrect, and has been copied from prior versions of the
> > code. This small change merely corrects the error.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/libcamera/camera.cpp | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 2d947a44..0da167a7 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >        * largest non-raw stream with a defined color space (if there is one).
> >        */
> >       std::optional<ColorSpace> colorSpace;
> > +     Size size;
> >
> >       for (auto [i, cfg] : utils::enumerate(config_)) {
> >               if (!cfg.colorSpace)
> > @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >               if (cfg.colorSpace->adjust(cfg.pixelFormat))
> >                       status = Adjusted;
> >
> > -             if (cfg.colorSpace != ColorSpace::Raw &&
> > -                 (!colorSpace || cfg.size > config_[i].size))
> > +             if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) {
>
> I'm not too familiar with this part of the code, but seeing
>
>         for (auto [i, cfg] : utils::enumerate(config_)) {
>                 ...
>                 if (cfg.size > config_[i].size))
>
> makes me indeed think that cfg.size == config_[i].size
>
> However I wonder why the condition was preceded by !colorSpace..
> The same condition was expressed in
> 5e5eadabd812 ("libcamera: camera: Add validateColorSpaces to CameraConfiguration class")
>
> as
>
>                } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {
>                        index = i;
>
> And I guess it was because the second part of the condition was always
> false :)
>
> Now that I've tracked down the full history of the bug
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Thanks
>   j
>
> >                       colorSpace = cfg.colorSpace;
> > +                     size = cfg.size;
> > +             }
> >       }
> >
> >       if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
> > --
> > 2.30.2
> >