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

[David Plowman]

Hi Jacopo

On Thu, 5 Jan 2023 at 09:24, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
>
> 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" ?

A change was made whereby streams that are producing an RGB output are
forced to have "None" for the YCbCr encoding, and "Full" for the
range. This means if you have a YUV and an RGB stream, they can't
share the same ColorSpace. Previously those fields were simply ignored
for output formats that didn't need them.

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

It depends if RkISP1 can create a YUV and an RGB stream
simultaneously. If so, someone who understands it needs to look into
the ColorSpace question - it may actually be broken at the moment, I
can't tell.

David

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