[libcamera-devel] [PATCH v11 7/8] libcamera: Add validateColorSpaces to CameraConfiguration class

[David Plowman]

Hi Laurent

Thanks for the review.

On Fri, 10 Dec 2021 at 12:50, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Fri, Dec 10, 2021 at 11:21:41AM +0000, David Plowman wrote:
> > This function forces raw streams to have the "raw" color space, and
> > also optionally makes all non-raw output streams to share the same
> > color space as some platforms may require this.
> >
> > When sharing color spaces we take the shared value to be the one from
> > the largest of these streams. This choice is ultimately arbitrary, but
> > can be appropriate if smaller output streams are used for image
> > analysis rather than human consumption, when the precise colours may
> > be less important.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  include/libcamera/camera.h | 10 +++++
> >  src/libcamera/camera.cpp   | 80 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 90 insertions(+)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index a7759ccb..5bb06584 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -13,6 +13,7 @@
> >  #include <string>
> >
> >  #include <libcamera/base/class.h>
> > +#include <libcamera/base/flags.h>
> >  #include <libcamera/base/object.h>
> >  #include <libcamera/base/signal.h>
> >
> > @@ -69,6 +70,15 @@ public:
> >  protected:
> >       CameraConfiguration();
> >
> > +     enum class ColorSpaceFlag {
> > +             None,
> > +             StreamsShareColorSpace,
> > +     };
> > +
> > +     using ColorSpaceFlags = Flags<ColorSpaceFlag>;
> > +
> > +     Status validateColorSpaces(ColorSpaceFlags flags = ColorSpaceFlag::None);
> > +
> >       std::vector<StreamConfiguration> config_;
> >  };
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 400a7cf0..5f8533e8 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -14,12 +14,14 @@
> >  #include <libcamera/base/log.h>
> >  #include <libcamera/base/thread.h>
> >
> > +#include <libcamera/color_space.h>
> >  #include <libcamera/framebuffer_allocator.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/camera_controls.h"
> > +#include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> >
> >  /**
> > @@ -314,6 +316,84 @@ std::size_t CameraConfiguration::size() const
> >       return config_.size();
> >  }
> >
> > +namespace {
> > +
> > +bool isRaw(const PixelFormat &pixFmt)
> > +{
> > +     const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > +     return info.isValid() &&
> > +            info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> > +}
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \enum CameraConfiguration::ColorSpaceFlag
> > + * \brief Specify the behaviour of validateColorSpaces
> > + * \var CameraConfiguration::ColorSpaceFlag::None
> > + * \brief No extra validation of color spaces is required
> > + * \var CameraConfiguration::ColorSpaceFlag::StreamsShareColorSpace
> > + * \brief Non-raw output streams must share the same color space
> > + */
> > +
> > +/**
> > + * \typedef CameraConfiguration::ColorSpaceFlags
> > + * \brief A bitwise combination of ColorSpaceFlag values
> > + */
> > +
> > +/**
> > + * \brief Check the color spaces requested for each stream
> > + * \param[in] flags Flags to control the behaviour of this function
> > + *
> > + * This function performs certain consistency checks on the color spaces of
> > + * the streams and may adjust them so that:
> > + *
> > + * - Any raw streams have the Raw color space
> > + * - If the StreamsShareColorSpace flag is set, all output streams are forced
> > + * to share the same color space (this may be a constraint on some platforms).
> > + *
> > + * It is optional for a pipeline handler to use this function.
> > + *
> > + * \return A CameraConfiguration::Status value that describes the validation
> > + * status.
> > + * \retval CameraConfigutation::Adjusted The configuration has been adjusted
> > + * and is now valid. The color space of some or all of the streams may bave
> > + * benn changed. The caller shall check the color spaces carefully.
> > + * \retval CameraConfiguration::Valid The configuration was already valid and
> > + * hasn't been adjusted.
> > + */
> > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceFlags flags)
> > +{
> > +     Status status = Valid;
> > +
> > +     /*
> > +      * Set all raw streams to the Raw color space, and make a note of the largest
> > +      * non-raw stream with a defined color space (if there is one).
> > +      */
> > +     int index = -1;
> > +     for (auto [i, cfg] : utils::enumerate(config_)) {
> > +             if (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) {
> > +                     cfg.colorSpace = ColorSpace::Raw;
> > +                     status = Adjusted;
> > +             } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size))
> > +                     index = i;
>
>                 } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {
>                         index = i;
>                 }

Yes, will add that!

Best regards

David

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +     }
> > +
> > +     if (index < 0 || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
> > +             return status;
> > +
> > +     /* Make all output color spaces the same, if requested. */
> > +     for (auto &cfg : config_) {
> > +             if (!isRaw(cfg.pixelFormat) &&
> > +                 cfg.colorSpace != config_[index].colorSpace) {
> > +                     cfg.colorSpace = config_[index].colorSpace;
> > +                     status = Adjusted;
> > +             }
> > +     }
> > +
> > +     return status;
> > +}
> > +
> >  /**
> >   * \var CameraConfiguration::transform
> >   * \brief User-specified transform to be applied to the image
>
> --
> Regards,
>
> Laurent Pinchart