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

[Umang Jain]

Hi David,

On 11/24/21 5:36 PM, David Plowman wrote:
> Hi Umang
>
> Thanks for the question below!
>
> On Tue, 23 Nov 2021 at 11:43, Umang Jain <umang.jain at ideasonboard.com> wrote:
>> Hi David,
>>
>> One clarification below please,
>>
>> On 11/18/21 8:49 PM, David Plowman wrote:
>>> This method forces raw streams to have the "raw" color space, and also
>>> optionally makes all output streams to share the same color space as
>>> some platforms may require this.
>>>
>>> Signed-off-by: David Plowman <david.plowman 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..c1541685 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)
>>> +{
>>> +     const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
>>> +     return info.isValid() &&
>>> +            info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>>> +}
>>> +
>>> +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;
>>> +     }
>>> +
>>> +     /*
>>> +      * Here we force raw streams to the correct color space. We handle
>>> +      * the case where all output streams are to share a color space,
>>> +      * which will match the color space of the largest (non-raw) stream.
>>> +      */
>>
>> I am questioning this policy, whether or not, let it best be left to
>> specific platform instead? Or let me take a step back and ask how does
>> one arrive at this policy or is it somewhat a convention?
> Yes, it's a good point. I think it's mostly a platform-specific thing.
> On the other hand, I can imagine that at least some other platforms
> will be like the Pi in this respect (having a single output colour
> space, though more than one output stream). So that's why I decided to
> put the code somewhere common, where pipeline handlers can call it if
> they want.
>
> If there are other behaviours or constraints that we think might be
> "common", maybe we could add those too?


I see, it's fine for now, I can think pipeline-handler can chose to do 
their own thing, when diverged cases arrive or spotted. As Jacopo said, 
it seems a bit-early but also seem okay to me.

> Best regards
> David
>
>>> +     for (auto &cfg : config_) {
>>> +             std::optional<ColorSpace> initialColorSpace = cfg.colorSpace;
>>> +
>>> +             if (isRaw(cfg.pixelFormat))
>>> +                     cfg.colorSpace = ColorSpace::Raw;
>>> +             else if (sharedColorSpace) {
>>> +                     /*
>>> +                      * When all outputs share a color space, use the biggest
>>> +                      * output if that was set. If that was blank, but this
>>> +                      * stream's is not, then use this stream's color space for
>>> +                      * largest stream.
>>> +                      * (index must be != -1 if there are any non-raw streams.)
>>> +                      */
>>> +                     if (config_[index].colorSpace)
>>> +                             cfg.colorSpace = config_[index].colorSpace;
>>> +                     else if (cfg.colorSpace)
>>> +                             config_[index].colorSpace = cfg.colorSpace;
>>> +             }
>>> +
>>> +             if (cfg.colorSpace != initialColorSpace)
>>> +                     status = Adjusted;
>>> +     }
>>> +
>>> +     return status;
>>> +}
>>> +
>>>    /**
>>>     * \var CameraConfiguration::transform
>>>     * \brief User-specified transform to be applied to the image