[libcamera-devel,v6,6/7] libcamera: Add validateColorSpaces to CameraConfiguration class
diff mbox series

Message ID 20211118151933.15627-7-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Colour spaces
Related show

Commit Message

David Plowman Nov. 18, 2021, 3:19 p.m. UTC
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@raspberrypi.com>
---
 include/libcamera/camera.h |  2 ++
 src/libcamera/camera.cpp   | 51 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

Comments

Umang Jain Nov. 23, 2021, 11:43 a.m. UTC | #1
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@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?

> +	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
David Plowman Nov. 24, 2021, 12:06 p.m. UTC | #2
Hi Umang

Thanks for the question below!

On Tue, 23 Nov 2021 at 11:43, Umang Jain <umang.jain@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@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?

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
Jacopo Mondi Nov. 24, 2021, 11:56 p.m. UTC | #3
Hi David,

On Thu, Nov 18, 2021 at 03:19:32PM +0000, 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.

I feel like it's a bit early, we don't have much platforms supporting
colorspaces and it might be the requirements are different ?

>
> Signed-off-by: David Plowman <david.plowman@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;
> +}

I've seen this pattern so many times I'm surprised we don't have an
helper

> +
> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)

Shouldn't this be documented ?

> +{
> +	Status status = Valid;
> +
> +	/* Find the largest non-raw stream (if any). */
> +	int index = -1;
> +	for (unsigned int i = 0; i < config_.size(); i++) {

for (auto &[i, cfg] utils::enumerate(config_)) is nicer

> +		const StreamConfiguration &cfg = config_[i];
> +		if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))
> +			index = i;
> +	}

If you make sure the selected index has a colorspace the rest of the
function can be simplified

        StreamConfiguration largerYUV;
        for (auto const &cfg : configs_) {
                if (isRaw(cfg.pixelformat))
                        continue;

                if (cfg.size > largerYUV.size && cfg.colorSpace)
                        largerYUV = cfg;
        }

        if (!largerYUV.colorSpace)
                ...

And you could catch the case where there are no colorspaces at all
earlier (what happens in that case ?)


> +
> +	/*
> +	 * 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.
> +	 */
> +	for (auto &cfg : config_) {
> +		std::optional<ColorSpace> initialColorSpace = cfg.colorSpace;
> +
> +		if (isRaw(cfg.pixelFormat))
> +			cfg.colorSpace = ColorSpace::Raw;

		if (isRaw(cfg.pixelFormat)) {
                        if (cfg.colorSpace == ColorSpace::Raw)
                                continue;

			cfg.colorSpace = ColorSpace::Raw;
                        status = Adjusted;
                        continue;
                }

> +		else if (sharedColorSpace) {

the rest of the function can then be

                if (!sharedColorSpace)
                        continue;

                if (cfg->colorSpace == largerYUV.colorSpace)
                        continue;

                cfg.colorspace = largerYUV.colorspace
                status = Adjusted;
        }

        return status;

Again not tested, I might have missed something indeed

I would also call the "sharedColorSpace" flag "forceColorSpace" ?

> +			/*
> +			 * 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)

Can index be -1 here ?

> +				cfg.colorSpace = config_[index].colorSpace;
> +			else if (cfg.colorSpace)

Thanks
   j

> +				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
> --
> 2.20.1
>
Umang Jain Nov. 25, 2021, 4:26 a.m. UTC | #4
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@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@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
David Plowman Nov. 25, 2021, 12:58 p.m. UTC | #5
Hi Jacopo

Thanks for reviewing this.

On Wed, 24 Nov 2021 at 23:55, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David,
>
> On Thu, Nov 18, 2021 at 03:19:32PM +0000, 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.
>
> I feel like it's a bit early, we don't have much platforms supporting
> colorspaces and it might be the requirements are different ?

I added this function because - a very long time ago now! - there had
been a worry about different pipeline handlers writing their own bits
of code, so I wanted to create something that people could share. But
I agree that the jury is still out on how useful this is.

>
> >
> > Signed-off-by: David Plowman <david.plowman@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;
> > +}
>
> I've seen this pattern so many times I'm surprised we don't have an
> helper
>
> > +
> > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)
>
> Shouldn't this be documented ?

Of course, I must have forgotten.

>
> > +{
> > +     Status status = Valid;
> > +
> > +     /* Find the largest non-raw stream (if any). */
> > +     int index = -1;
> > +     for (unsigned int i = 0; i < config_.size(); i++) {
>
> for (auto &[i, cfg] utils::enumerate(config_)) is nicer

Ooh, it gets ever more like Python! I didn't know you could do that...

>
> > +             const StreamConfiguration &cfg = config_[i];
> > +             if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))
> > +                     index = i;
> > +     }
>
> If you make sure the selected index has a colorspace the rest of the
> function can be simplified
>
>         StreamConfiguration largerYUV;
>         for (auto const &cfg : configs_) {
>                 if (isRaw(cfg.pixelformat))
>                         continue;
>
>                 if (cfg.size > largerYUV.size && cfg.colorSpace)
>                         largerYUV = cfg;
>         }
>
>         if (!largerYUV.colorSpace)
>                 ...
>
> And you could catch the case where there are no colorspaces at all
> earlier (what happens in that case ?)
>
>
> > +
> > +     /*
> > +      * 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.
> > +      */
> > +     for (auto &cfg : config_) {
> > +             std::optional<ColorSpace> initialColorSpace = cfg.colorSpace;
> > +
> > +             if (isRaw(cfg.pixelFormat))
> > +                     cfg.colorSpace = ColorSpace::Raw;
>
>                 if (isRaw(cfg.pixelFormat)) {
>                         if (cfg.colorSpace == ColorSpace::Raw)
>                                 continue;
>
>                         cfg.colorSpace = ColorSpace::Raw;
>                         status = Adjusted;
>                         continue;
>                 }
>
> > +             else if (sharedColorSpace) {
>
> the rest of the function can then be
>
>                 if (!sharedColorSpace)
>                         continue;
>
>                 if (cfg->colorSpace == largerYUV.colorSpace)
>                         continue;
>
>                 cfg.colorspace = largerYUV.colorspace
>                 status = Adjusted;
>         }
>
>         return status;
>
> Again not tested, I might have missed something indeed
>
> I would also call the "sharedColorSpace" flag "forceColorSpace" ?

With "forceColorSpace" I'm left wondering what I am "forcing"... at
least "sharedColorSpace" implies that some colour spaces are somehow
being shared. Perhaps we could have "shareOutputColorSpaces" if that's
not too long? That's 100% clear, I think!

>
> > +                     /*
> > +                      * 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)
>
> Can index be -1 here ?
>

I'll have another look at all this and see if I can make it all a bit
"more obvious".

Thanks!
David

> > +                             cfg.colorSpace = config_[index].colorSpace;
> > +                     else if (cfg.colorSpace)
>
> Thanks
>    j
>
> > +                             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
> > --
> > 2.20.1
> >

Patch
diff mbox series

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.
+	 */
+	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