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

[David Plowman]

Hi Umang

Thanks for the question below!

On Tue, 23 Nov 2021 at 11:43, Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi David,
>
> One clarification below please,
>
> On 11/18/21 8:49 PM, 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.
> >
> > 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;
> > +}
> > +
> > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)
> > +{
> > +     Status status = Valid;
> > +
> > +     /* Find the largest non-raw stream (if any). */
> > +     int index = -1;
> > +     for (unsigned int i = 0; i < config_.size(); i++) {
> > +             const StreamConfiguration &cfg = config_[i];
> > +             if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))
> > +                     index = i;
> > +     }
> > +
> > +     /*
> > +      * 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.
> > +      */
>
>
> I am questioning this policy, whether or not, let it best be left to
> specific platform instead? Or let me take a step back and ask how does
> one arrive at this policy or is it somewhat a convention?

Yes, it's a good point. I think it's mostly a platform-specific thing.
On the other hand, I can imagine that at least some other platforms
will be like the Pi in this respect (having a single output colour
space, though more than one output stream). So that's why I decided to
put the code somewhere common, where pipeline handlers can call it if
they want.

If there are other behaviours or constraints that we think might be
"common", maybe we could add those too?

Best regards
David

>
> > +     for (auto &cfg : config_) {
> > +             std::optional<ColorSpace> initialColorSpace = cfg.colorSpace;
> > +
> > +             if (isRaw(cfg.pixelFormat))
> > +                     cfg.colorSpace = ColorSpace::Raw;
> > +             else if (sharedColorSpace) {
> > +                     /*
> > +                      * 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)
> > +                             cfg.colorSpace = config_[index].colorSpace;
> > +                     else if (cfg.colorSpace)
> > +                             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