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

14199 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>
---
 include/libcamera/camera.h |  2 ++
 src/libcamera/camera.cpp   | 53 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

Hi David,

Thank you for your work.


On Wed, 20 Oct 2021 at 12:08, David Plowman <david.plowman@raspberrypi.com>
wrote:

> 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>
> ---
>  include/libcamera/camera.h |  2 ++
>  src/libcamera/camera.cpp   | 53 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 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 71809bcd..79451ef4 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -19,6 +19,7 @@
>  #include <libcamera/stream.h>
>
>  #include "libcamera/internal/camera.h"
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/pipeline_handler.h"
>
>  /**
> @@ -313,6 +314,58 @@ 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;
> +}
>

Not as part of this series, but we ought to move this to the
PixelFomat class,
there is an identical function in our pipeline handler :-)


> +
> +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.width * cfg.size.height >
> config_[i].size.width * config_[i].size.height))
>

You can replace this with "cfg.size > config_[i].size". Apart from that:

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> +                       index = i;
> +       }
> +
> +       /*
> +        * 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;
> +
> +               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
>
>

Hi Naush

Thanks for the review.

On Mon, 25 Oct 2021 at 09:34, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for your work.
>
>
> On Wed, 20 Oct 2021 at 12:08, David Plowman <david.plowman@raspberrypi.com> wrote:
>>
>> 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>
>> ---
>>  include/libcamera/camera.h |  2 ++
>>  src/libcamera/camera.cpp   | 53 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 55 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 71809bcd..79451ef4 100644
>> --- a/src/libcamera/camera.cpp
>> +++ b/src/libcamera/camera.cpp
>> @@ -19,6 +19,7 @@
>>  #include <libcamera/stream.h>
>>
>>  #include "libcamera/internal/camera.h"
>> +#include "libcamera/internal/formats.h"
>>  #include "libcamera/internal/pipeline_handler.h"
>>
>>  /**
>> @@ -313,6 +314,58 @@ 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;
>> +}
>
>
> Not as part of this series, but we ought to move this to the PixelFomat class,
> there is an identical function in our pipeline handler :-)

Hehe. I think I made the same comment about one of your patches recently!

>
>>
>> +
>> +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.width * cfg.size.height > config_[i].size.width * config_[i].size.height))
>
>
> You can replace this with "cfg.size > config_[i].size". Apart from that:

Wow, you can do that? How cool, I never knew!

Thanks

David

>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
>>
>> +                       index = i;
>> +       }
>> +
>> +       /*
>> +        * 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;
>> +
>> +               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
>>