[libcamera-devel] [PATCH v6 6/7] libcamera: Add validateColorSpaces to CameraConfiguration class

[David Plowman]

Hi Jacopo

Thanks for reviewing this.

On Wed, 24 Nov 2021 at 23:55, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David,
>
> On Thu, Nov 18, 2021 at 03:19:32PM +0000, David Plowman wrote:
> > This method forces raw streams to have the "raw" color space, and also
> > optionally makes all output streams to share the same color space as
> > some platforms may require this.
>
> I feel like it's a bit early, we don't have much platforms supporting
> colorspaces and it might be the requirements are different ?

I added this function because - a very long time ago now! - there had
been a worry about different pipeline handlers writing their own bits
of code, so I wanted to create something that people could share. But
I agree that the jury is still out on how useful this is.

>
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  include/libcamera/camera.h |  2 ++
> >  src/libcamera/camera.cpp   | 51 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 601ee46e..fdab4410 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -69,6 +69,8 @@ public:
> >  protected:
> >       CameraConfiguration();
> >
> > +     Status validateColorSpaces(bool sharedColorSpace);
> > +
> >       std::vector<StreamConfiguration> config_;
> >  };
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 400a7cf0..c1541685 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -20,6 +20,7 @@
> >
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/camera_controls.h"
> > +#include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> >
> >  /**
> > @@ -314,6 +315,56 @@ std::size_t CameraConfiguration::size() const
> >       return config_.size();
> >  }
> >
> > +static bool isRaw(const PixelFormat &pixFmt)
> > +{
> > +     const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > +     return info.isValid() &&
> > +            info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> > +}
>
> I've seen this pattern so many times I'm surprised we don't have an
> helper
>
> > +
> > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)
>
> Shouldn't this be documented ?

Of course, I must have forgotten.

>
> > +{
> > +     Status status = Valid;
> > +
> > +     /* Find the largest non-raw stream (if any). */
> > +     int index = -1;
> > +     for (unsigned int i = 0; i < config_.size(); i++) {
>
> for (auto &[i, cfg] utils::enumerate(config_)) is nicer

Ooh, it gets ever more like Python! I didn't know you could do that...

>
> > +             const StreamConfiguration &cfg = config_[i];
> > +             if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))
> > +                     index = i;
> > +     }
>
> If you make sure the selected index has a colorspace the rest of the
> function can be simplified
>
>         StreamConfiguration largerYUV;
>         for (auto const &cfg : configs_) {
>                 if (isRaw(cfg.pixelformat))
>                         continue;
>
>                 if (cfg.size > largerYUV.size && cfg.colorSpace)
>                         largerYUV = cfg;
>         }
>
>         if (!largerYUV.colorSpace)
>                 ...
>
> And you could catch the case where there are no colorspaces at all
> earlier (what happens in that case ?)
>
>
> > +
> > +     /*
> > +      * Here we force raw streams to the correct color space. We handle
> > +      * the case where all output streams are to share a color space,
> > +      * which will match the color space of the largest (non-raw) stream.
> > +      */
> > +     for (auto &cfg : config_) {
> > +             std::optional<ColorSpace> initialColorSpace = cfg.colorSpace;
> > +
> > +             if (isRaw(cfg.pixelFormat))
> > +                     cfg.colorSpace = ColorSpace::Raw;
>
>                 if (isRaw(cfg.pixelFormat)) {
>                         if (cfg.colorSpace == ColorSpace::Raw)
>                                 continue;
>
>                         cfg.colorSpace = ColorSpace::Raw;
>                         status = Adjusted;
>                         continue;
>                 }
>
> > +             else if (sharedColorSpace) {
>
> the rest of the function can then be
>
>                 if (!sharedColorSpace)
>                         continue;
>
>                 if (cfg->colorSpace == largerYUV.colorSpace)
>                         continue;
>
>                 cfg.colorspace = largerYUV.colorspace
>                 status = Adjusted;
>         }
>
>         return status;
>
> Again not tested, I might have missed something indeed
>
> I would also call the "sharedColorSpace" flag "forceColorSpace" ?

With "forceColorSpace" I'm left wondering what I am "forcing"... at
least "sharedColorSpace" implies that some colour spaces are somehow
being shared. Perhaps we could have "shareOutputColorSpaces" if that's
not too long? That's 100% clear, I think!

>
> > +                     /*
> > +                      * When all outputs share a color space, use the biggest
> > +                      * output if that was set. If that was blank, but this
> > +                      * stream's is not, then use this stream's color space for
> > +                      * largest stream.
> > +                      * (index must be != -1 if there are any non-raw streams.)
> > +                      */
> > +                     if (config_[index].colorSpace)
>
> Can index be -1 here ?
>

I'll have another look at all this and see if I can make it all a bit
"more obvious".

Thanks!
David

> > +                             cfg.colorSpace = config_[index].colorSpace;
> > +                     else if (cfg.colorSpace)
>
> Thanks
>    j
>
> > +                             config_[index].colorSpace = cfg.colorSpace;
> > +             }
> > +
> > +             if (cfg.colorSpace != initialColorSpace)
> > +                     status = Adjusted;
> > +     }
> > +
> > +     return status;
> > +}
> > +
> >  /**
> >   * \var CameraConfiguration::transform
> >   * \brief User-specified transform to be applied to the image
> > --
> > 2.20.1
> >