[v4,1/2] pipeline: virtual: Provide and validate colorspace
diff mbox series

Message ID 20251026125650.117468-2-uajain@igalia.com
State Superseded
Headers show
Series
  • lc-compliance: Ensure stream's colorspace is set after validate()
Related show

Commit Message

Umang Jain Oct. 26, 2025, 12:56 p.m. UTC 
Virtual pipeline handler should provide colorSpace in
generateConfiguration() and validate the colorspace in validate().
It is mandatory for a pipeline handler to set the colorspace if it
is unset in the stream configuration, during validate().

For choosing the colorspace for the generated NV12 frames, following
points have been taken into account:
- The transfer function should be Rec.709 for NV12
- The YCbCr encoding has been chosen Rec.709 as it is the most common
  than Rec.601/Rec.2020
- Range should be 'Limited' as with the NV12 pixel format.

Hence, the closest colorspace match is ColorSpace::Rec709 which is
set for the virtual pipeline handler.

Signed-off-by: Umang Jain <uajain@igalia.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Robert Mader <robert.mader@collabora.com>
---
 src/libcamera/pipeline/virtual/virtual.cpp | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Barnabás Pőcze Oct. 27, 2025, 10:02 a.m. UTC | #1 
Hi

2025. 10. 26. 13:56 keltezéssel, Umang Jain írta:
> Virtual pipeline handler should provide colorSpace in
> generateConfiguration() and validate the colorspace in validate().
> It is mandatory for a pipeline handler to set the colorspace if it
> is unset in the stream configuration, during validate().
> 
> For choosing the colorspace for the generated NV12 frames, following
> points have been taken into account:
> - The transfer function should be Rec.709 for NV12
> - The YCbCr encoding has been chosen Rec.709 as it is the most common
                                                             more ?

And the "The YCbCr encoding has been chosen Rec.709" part is not clear to me.


>    than Rec.601/Rec.2020
> - Range should be 'Limited' as with the NV12 pixel format.
> 
> Hence, the closest colorspace match is ColorSpace::Rec709 which is
> set for the virtual pipeline handler.
> 
> Signed-off-by: Umang Jain <uajain@igalia.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Tested-by: Robert Mader <robert.mader@collabora.com>
> ---
>   src/libcamera/pipeline/virtual/virtual.cpp | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index 23eae852..c0247b4d 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -214,6 +214,18 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
>   			adjusted = true;
>   		}
> 
> +		if (!cfg.colorSpace ||
> +		    cfg.colorSpace != ColorSpace::Rec709) {

You can drop the `!cfg.colorSpace` part. An empty optional is only
equal to another empty optional.


> +			cfg.colorSpace = ColorSpace::Rec709;
> +			status = Adjusted;
> +			adjusted = true;
> +		}
> +
> +		if (validateColorSpaces() == Adjusted) {

This will always be called with `formats::NV12` and `ColorSpace::Rec709`
on every stream, and so it will never make any adjustments, right?

I think it's probably fine to leave it here even in that case, should this
pipeline handler be extended to support more things; just want to confirm it.


Regards,
Barnabás Pőcze

> +			status = Adjusted;
> +			adjusted = true;
> +		}
> +
>   		if (adjusted)
>   			LOG(Virtual, Info)
>   				<< "Stream configuration adjusted to " << cfg.toString();
> @@ -278,6 +290,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,
>   		cfg.pixelFormat = pixelFormat;
>   		cfg.size = data->config_.maxResolutionSize;
>   		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> +		cfg.colorSpace = ColorSpace::Rec709;
> 
>   		config->addConfiguration(cfg);
>   	}
> --
> 2.51.0
>
Robert Mader Oct. 27, 2025, 3:07 p.m. UTC | #2 
Hi,

On 10/27/25 11:02, Barnabás Pőcze wrote:
> Hi
>
> 2025. 10. 26. 13:56 keltezéssel, Umang Jain írta:
>> Virtual pipeline handler should provide colorSpace in
>> generateConfiguration() and validate the colorspace in validate().
>> It is mandatory for a pipeline handler to set the colorspace if it
>> is unset in the stream configuration, during validate().
>>
>> For choosing the colorspace for the generated NV12 frames, following
>> points have been taken into account:
>> - The transfer function should be Rec.709 for NV12
>> - The YCbCr encoding has been chosen Rec.709 as it is the most common
>                                                             more ?
>
> And the "The YCbCr encoding has been chosen Rec.709" part is not clear 
> to me.
>
>
>>    than Rec.601/Rec.2020
>> - Range should be 'Limited' as with the NV12 pixel format.
>>
>> Hence, the closest colorspace match is ColorSpace::Rec709 which is
>> set for the virtual pipeline handler.
>>
>> Signed-off-by: Umang Jain <uajain@igalia.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Tested-by: Robert Mader <robert.mader@collabora.com>
>> ---
>>   src/libcamera/pipeline/virtual/virtual.cpp | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp 
>> b/src/libcamera/pipeline/virtual/virtual.cpp
>> index 23eae852..c0247b4d 100644
>> --- a/src/libcamera/pipeline/virtual/virtual.cpp
>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
>> @@ -214,6 +214,18 @@ CameraConfiguration::Status 
>> VirtualCameraConfiguration::validate()
>>               adjusted = true;
>>           }
>>
>> +        if (!cfg.colorSpace ||
>> +            cfg.colorSpace != ColorSpace::Rec709) {
>
> You can drop the `!cfg.colorSpace` part. An empty optional is only
> equal to another empty optional.
FTR., other pipelines like the rkisp1 only set the adjusted state if 
cfg.colorSpace was not empty - so that might be worth doing here as 
well. But the current form is indeed redundant.

>
>> +            cfg.colorSpace = ColorSpace::Rec709;
>> +            status = Adjusted;
>> +            adjusted = true;
>> +        }
>> +
>> +        if (validateColorSpaces() == Adjusted) {
>
> This will always be called with `formats::NV12` and `ColorSpace::Rec709`
> on every stream, and so it will never make any adjustments, right?
>
> I think it's probably fine to leave it here even in that case, should 
> this
> pipeline handler be extended to support more things; just want to 
> confirm it.
>
>
> Regards,
> Barnabás Pőcze
>
>> +            status = Adjusted;
>> +            adjusted = true;
>> +        }
>> +
>>           if (adjusted)
>>               LOG(Virtual, Info)
>>                   << "Stream configuration adjusted to " << 
>> cfg.toString();
>> @@ -278,6 +290,7 @@ 
>> PipelineHandlerVirtual::generateConfiguration(Camera *camera,
>>           cfg.pixelFormat = pixelFormat;
>>           cfg.size = data->config_.maxResolutionSize;
>>           cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
>> +        cfg.colorSpace = ColorSpace::Rec709;
>>
>>           config->addConfiguration(cfg);
>>       }
>> -- 
>> 2.51.0
>>
>
Umang Jain Oct. 31, 2025, 7:03 a.m. UTC | #3 
On Mon, Oct 27, 2025 at 11:02:47AM +0100, Barnabás Pőcze wrote:
> Hi
> 
> 2025. 10. 26. 13:56 keltezéssel, Umang Jain írta:
> > Virtual pipeline handler should provide colorSpace in
> > generateConfiguration() and validate the colorspace in validate().
> > It is mandatory for a pipeline handler to set the colorspace if it
> > is unset in the stream configuration, during validate().
> > 
> > For choosing the colorspace for the generated NV12 frames, following
> > points have been taken into account:
> > - The transfer function should be Rec.709 for NV12
> > - The YCbCr encoding has been chosen Rec.709 as it is the most common
>                                                             more ?
> 
> And the "The YCbCr encoding has been chosen Rec.709" part is not clear to me.

YUV formats need to have Y'CbCr encoding, as it's referred in
ColorSpace::adjust()

> 
> 
> >    than Rec.601/Rec.2020
> > - Range should be 'Limited' as with the NV12 pixel format.
> > 
> > Hence, the closest colorspace match is ColorSpace::Rec709 which is
> > set for the virtual pipeline handler.
> > 
> > Signed-off-by: Umang Jain <uajain@igalia.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Tested-by: Robert Mader <robert.mader@collabora.com>
> > ---
> >   src/libcamera/pipeline/virtual/virtual.cpp | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> > 
> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> > index 23eae852..c0247b4d 100644
> > --- a/src/libcamera/pipeline/virtual/virtual.cpp
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -214,6 +214,18 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
> >   			adjusted = true;
> >   		}
> > 
> > +		if (!cfg.colorSpace ||
> > +		    cfg.colorSpace != ColorSpace::Rec709) {
> 
> You can drop the `!cfg.colorSpace` part. An empty optional is only
> equal to another empty optional.
> 
> 
> > +			cfg.colorSpace = ColorSpace::Rec709;
> > +			status = Adjusted;
> > +			adjusted = true;
> > +		}
> > +
> > +		if (validateColorSpaces() == Adjusted) {
> 
> This will always be called with `formats::NV12` and `ColorSpace::Rec709`
> on every stream, and so it will never make any adjustments, right?
> 
> I think it's probably fine to leave it here even in that case, should this
> pipeline handler be extended to support more things; just want to confirm it.

that's true for this pipeline handler, but we should always call validateXYZ()
regardless if libcamera core provides it.
> 
> 
> Regards,
> Barnabás Pőcze
> 
> > +			status = Adjusted;
> > +			adjusted = true;
> > +		}
> > +
> >   		if (adjusted)
> >   			LOG(Virtual, Info)
> >   				<< "Stream configuration adjusted to " << cfg.toString();
> > @@ -278,6 +290,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,
> >   		cfg.pixelFormat = pixelFormat;
> >   		cfg.size = data->config_.maxResolutionSize;
> >   		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
> > +		cfg.colorSpace = ColorSpace::Rec709;
> > 
> >   		config->addConfiguration(cfg);
> >   	}
> > --
> > 2.51.0
> > 
>
Barnabás Pőcze Oct. 31, 2025, 10 a.m. UTC | #4 
2025. 10. 31. 8:03 keltezéssel, Umang Jain írta:
> On Mon, Oct 27, 2025 at 11:02:47AM +0100, Barnabás Pőcze wrote:
>> Hi
>>
>> 2025. 10. 26. 13:56 keltezéssel, Umang Jain írta:
>>> Virtual pipeline handler should provide colorSpace in
>>> generateConfiguration() and validate the colorspace in validate().
>>> It is mandatory for a pipeline handler to set the colorspace if it
>>> is unset in the stream configuration, during validate().
>>>
>>> For choosing the colorspace for the generated NV12 frames, following
>>> points have been taken into account:
>>> - The transfer function should be Rec.709 for NV12
>>> - The YCbCr encoding has been chosen Rec.709 as it is the most common
>>                                                              more ?
>>
>> And the "The YCbCr encoding has been chosen Rec.709" part is not clear to me.
> 
> YUV formats need to have Y'CbCr encoding, as it's referred in
> ColorSpace::adjust()

Ahh, thanks, I think I understand now. I was expecting something like
"... chosen >>to be<< Rec.709 ..." and I couldn't parse the "Rec.709" part.


> 
>>
>>
>>>     than Rec.601/Rec.2020
>>> - Range should be 'Limited' as with the NV12 pixel format.
>>>
>>> Hence, the closest colorspace match is ColorSpace::Rec709 which is
>>> set for the virtual pipeline handler.
>>>
>>> Signed-off-by: Umang Jain <uajain@igalia.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> Tested-by: Robert Mader <robert.mader@collabora.com>
>>> ---
>>>    src/libcamera/pipeline/virtual/virtual.cpp | 13 +++++++++++++
>>>    1 file changed, 13 insertions(+)
>>>
>>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
>>> index 23eae852..c0247b4d 100644
>>> --- a/src/libcamera/pipeline/virtual/virtual.cpp
>>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
>>> @@ -214,6 +214,18 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
>>>    			adjusted = true;
>>>    		}
>>>
>>> +		if (!cfg.colorSpace ||
>>> +		    cfg.colorSpace != ColorSpace::Rec709) {
>>
>> You can drop the `!cfg.colorSpace` part. An empty optional is only
>> equal to another empty optional.
>>
>>
>>> +			cfg.colorSpace = ColorSpace::Rec709;
>>> +			status = Adjusted;
>>> +			adjusted = true;
>>> +		}
>>> +
>>> +		if (validateColorSpaces() == Adjusted) {
>>
>> This will always be called with `formats::NV12` and `ColorSpace::Rec709`
>> on every stream, and so it will never make any adjustments, right?
>>
>> I think it's probably fine to leave it here even in that case, should this
>> pipeline handler be extended to support more things; just want to confirm it.
> 
> that's true for this pipeline handler, but we should always call validateXYZ()
> regardless if libcamera core provides it.
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>> +			status = Adjusted;
>>> +			adjusted = true;
>>> +		}
>>> +
>>>    		if (adjusted)
>>>    			LOG(Virtual, Info)
>>>    				<< "Stream configuration adjusted to " << cfg.toString();
>>> @@ -278,6 +290,7 @@ PipelineHandlerVirtual::generateConfiguration(Camera *camera,
>>>    		cfg.pixelFormat = pixelFormat;
>>>    		cfg.size = data->config_.maxResolutionSize;
>>>    		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
>>> +		cfg.colorSpace = ColorSpace::Rec709;
>>>
>>>    		config->addConfiguration(cfg);
>>>    	}
>>> --
>>> 2.51.0
>>>
>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
index 23eae852..c0247b4d 100644
--- a/src/libcamera/pipeline/virtual/virtual.cpp
+++ b/src/libcamera/pipeline/virtual/virtual.cpp
@@ -214,6 +214,18 @@  CameraConfiguration::Status VirtualCameraConfiguration::validate()
 			adjusted = true;
 		}
 
+		if (!cfg.colorSpace ||
+		    cfg.colorSpace != ColorSpace::Rec709) {
+			cfg.colorSpace = ColorSpace::Rec709;
+			status = Adjusted;
+			adjusted = true;
+		}
+
+		if (validateColorSpaces() == Adjusted) {
+			status = Adjusted;
+			adjusted = true;
+		}
+
 		if (adjusted)
 			LOG(Virtual, Info)
 				<< "Stream configuration adjusted to " << cfg.toString();
@@ -278,6 +290,7 @@  PipelineHandlerVirtual::generateConfiguration(Camera *camera,
 		cfg.pixelFormat = pixelFormat;
 		cfg.size = data->config_.maxResolutionSize;
 		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
+		cfg.colorSpace = ColorSpace::Rec709;
 
 		config->addConfiguration(cfg);
 	}