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

14463 diff mbox series

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@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/camera.h |  2 ++
 src/libcamera/camera.cpp   | 51 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

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@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@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
>