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

[David Plowman]

Hi Naush

Thanks for the review.

On Mon, 25 Oct 2021 at 09:34, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for your work.
>
>
> On Wed, 20 Oct 2021 at 12:08, David Plowman <david.plowman at 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 at 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 at 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
>>