[libcamera-devel] [PATCH v5 6/7] libcamera: Add validateColorSpaces to CameraConfiguration class
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Nov 5 13:54:07 CET 2021
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
>
More information about the libcamera-devel
mailing list