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

[Kieran Bingham]

Quoting David Plowman (2021-11-04 13:58:04)
> This method checks that the requested color spaces are sensible and do
> not contain any undefined enum values. It also initialises the
> "actual" color space field to the same value as the one requested, in
> the expectation that the rest of the validate() method will be able to
> check this.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush 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..90e9460b 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)

This looks like it should be a helper added to the PixelFormat class as
a patch of it's own? I'm not sure it belongs here in the camera class.

> +{
> +       const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> +       return info.isValid() &&
> +              info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> +}
> +

I think this is missing doxygen documentation.

What's the definition of a 'sharedColorSpace' ?

> +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;
> +       }
> +

Given the below usage of index, I'd be tempted to put an 

	ASSERT(index != -1);
here.


> +       /*
> +        * Here we force raw streams to the correct color space and signal
> +        * an error if we encounter anything undefined. We handle the case
> +        * where all output streams are to share a color space, which we
> +        * choose to be the color space of the largest stream.
> +        */
> +       for (auto &cfg : config_) {
> +               ColorSpace initialColorSpace = cfg.requestedColorSpace;
> +
> +               if (isRaw(cfg.pixelFormat))
> +                       cfg.requestedColorSpace = ColorSpace::Raw;
> +               else if (!cfg.requestedColorSpace.isFullyDefined()) {
> +                       LOG(Camera, Error) << "Stream has undefined color space";
> +                       cfg.requestedColorSpace = ColorSpace::Jpeg;
> +               } else if (sharedColorSpace)
> +                       cfg.requestedColorSpace = config_[index].requestedColorSpace;

Can index ever be -1 here?


> +
> +               if (cfg.requestedColorSpace != initialColorSpace)
> +                       status = Adjusted;
> +
> +               /*
> +                * We also initialise the actual color space as if the
> +                * hardware can do what we want. But note that the rest
> +                * of the validate() method may change this.
> +                */
> +               cfg.actualColorSpace = cfg.requestedColorSpace;
> +       }
> +
> +       return status;
> +}
> +
>  /**
>   * \var CameraConfiguration::transform
>   * \brief User-specified transform to be applied to the image
> -- 
> 2.20.1
>