[3/4] virtual: Validate stride and frameSize
diff mbox series

Message ID 20261008211149.51765-4-robert.mader@collabora.com
State Superseded
Headers show
Series
  • virtual: Support more common pixel formats
Related show

Commit Message

Robert Mader Oct. 8, 2026, 9:11 p.m. UTC
If a client requested a specific stride that's incompatible with what
we'll produce, let the client know.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
---
 src/libcamera/pipeline/virtual/virtual.cpp | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Barnabás Pőcze Oct. 9, 2026, 8:43 a.m. UTC | #1
2026. 10. 08. 23:11 keltezéssel, Robert Mader írta:
> If a client requested a specific stride that's incompatible with what
> we'll produce, let the client know.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> ---
>   src/libcamera/pipeline/virtual/virtual.cpp | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index 565b66477363..349cb88a3f05 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -231,13 +231,20 @@ CameraConfiguration::Status VirtualCameraConfiguration::validate()
>   			adjusted = true;
>   		}
>   
> +		auto stride = info.stride(cfg.size.width, 0);
> +		auto frameSize = info.frameSize(cfg.size);
> +		if ((cfg.stride && cfg.stride != stride) ||
> +		    (cfg.frameSize && cfg.frameSize != frameSize)) {

I think if the fields have been changed, then `Adjusted` should be returned no matter what.
And then maybe the log message can be suppressed if both are 0. And maybe the log message
should be denoted to "debug" and then the special handling of 0 could be removed.


> +			status = Adjusted;
> +			adjusted = true;
> +		}
> +		cfg.stride = stride;
> +		cfg.frameSize = frameSize;
> +
>   		if (adjusted)
>   			LOG(Virtual, Info)
>   				<< "Stream configuration adjusted to " << cfg.toString();
>   
> -		cfg.stride = info.stride(cfg.size.width, 0, 1);
> -		cfg.frameSize = info.frameSize(cfg.size, 1);
> -
>   		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
>   	}
>
Robert Mader Oct. 9, 2026, 7:47 p.m. UTC | #2
Hi,

On 09.10.26 10:43, Barnabás Pőcze wrote:
> 2026. 10. 08. 23:11 keltezéssel, Robert Mader írta:
>> If a client requested a specific stride that's incompatible with what
>> we'll produce, let the client know.
>>
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>> ---
>>   src/libcamera/pipeline/virtual/virtual.cpp | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp 
>> b/src/libcamera/pipeline/virtual/virtual.cpp
>> index 565b66477363..349cb88a3f05 100644
>> --- a/src/libcamera/pipeline/virtual/virtual.cpp
>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
>> @@ -231,13 +231,20 @@ CameraConfiguration::Status 
>> VirtualCameraConfiguration::validate()
>>               adjusted = true;
>>           }
>>   +        auto stride = info.stride(cfg.size.width, 0);
>> +        auto frameSize = info.frameSize(cfg.size);
>> +        if ((cfg.stride && cfg.stride != stride) ||
>> +            (cfg.frameSize && cfg.frameSize != frameSize)) {
>
> I think if the fields have been changed, then `Adjusted` should be 
> returned no matter what.
> And then maybe the log message can be suppressed if both are 0. And 
> maybe the log message
> should be denoted to "debug" and then the special handling of 0 could 
> be removed.

I didn't apply this suggestion in V2 as I'm not sure if the expected 
behaviour here is well defined (or there is at least consensus) - i.e. 
if setting "unset" values should result in an adjusted config. Happy to 
change it in v3 if you or somebody else can confirm what we need here.

>
>
>> +            status = Adjusted;
>> +            adjusted = true;
>> +        }
>> +        cfg.stride = stride;
>> +        cfg.frameSize = frameSize;
>> +
>>           if (adjusted)
>>               LOG(Virtual, Info)
>>                   << "Stream configuration adjusted to " << 
>> cfg.toString();
>>   -        cfg.stride = info.stride(cfg.size.width, 0, 1);
>> -        cfg.frameSize = info.frameSize(cfg.size, 1);
>> -
>>           cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
>>       }
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
index 565b66477363..349cb88a3f05 100644
--- a/src/libcamera/pipeline/virtual/virtual.cpp
+++ b/src/libcamera/pipeline/virtual/virtual.cpp
@@ -231,13 +231,20 @@  CameraConfiguration::Status VirtualCameraConfiguration::validate()
 			adjusted = true;
 		}
 
+		auto stride = info.stride(cfg.size.width, 0);
+		auto frameSize = info.frameSize(cfg.size);
+		if ((cfg.stride && cfg.stride != stride) ||
+		    (cfg.frameSize && cfg.frameSize != frameSize)) {
+			status = Adjusted;
+			adjusted = true;
+		}
+		cfg.stride = stride;
+		cfg.frameSize = frameSize;
+
 		if (adjusted)
 			LOG(Virtual, Info)
 				<< "Stream configuration adjusted to " << cfg.toString();
 
-		cfg.stride = info.stride(cfg.size.width, 0, 1);
-		cfg.frameSize = info.frameSize(cfg.size, 1);
-
 		cfg.bufferCount = VirtualCameraConfiguration::kBufferCount;
 	}