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

[Jacopo Mondi]

Hi David,

On Wed, Jan 04, 2023 at 09:41:54AM +0000, David Plowman wrote:
> 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).

It seems to me as well it's the only other user apart RPi

>
> 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

I've missed good parts of the discussion, but why "YUV and RGB streams
are -no longer- allowed to share a ColorSpace" ?

> about removing it, but one would have to understand the rkisp1
> requirements first! Would you know anything about that?

I don't know the details, but looking at the commit message of

4267e0bab86d ("libcamera: pipeline: rkisp1: Implement color space support")

As all the processing related to the color space is performed in the
part of the pipeline shared by all streams, a single color space must
cover all stream configurations. This is enforced manually when
generating the configuration, and with the validateColorSpaces()
helper when validating it.

and the comment in the code

+       /*
+        * As the ISP can't output different color spaces for the main and self
+        * path, pick a sensible default color space based on the role of the
+        * first stream and use it for all streams.
+        */
+       std::optional<ColorSpace> colorSpace;

It seems clear to me that the RkISP1 requires a single colorspace for
all outputs. That's why it's interesting to know why "YUV and RGB are
no longer allowed to share a ColorSpace".

Ofc if you move away from CameraConfiguration::validateColorSpaces()
and RkISP1 it's the only remaining user we can consider moving the
function back to the pipeline handler implementation.

Thanks
  j

>
> 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
> > >