[v4,05/11] libcamera: simple: Add plain output configurations
diff mbox series

Message ID 20250407085639.16180-6-mzamazal@redhat.com
State New
Headers show
Series
  • Enable raw streams with software ISP
Related show

Commit Message

Milan Zamazal April 7, 2025, 8:56 a.m. UTC 
Output configurations in simple pipeline are added depending on whether
a converter, software ISP or none of them are used.  If a converter or
software ISP is used, no raw output configurations are added.

In order to support raw output at least with software ISP, let's always
add raw output configurations.  A flag is added to
SimpleCameraData::Configuration indicating whether it's for a raw or a
converted output.  We later filter formats and output sizes for
particular stream configurations according to the new configuration
flag.

This is just preparation and raw output is still not supported.  At the
moment, we simply filter out raw configurations unconditionally to keep
the current code working; this will be changed in followup patches.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 31 +++++++++++++++---------
 1 file changed, 19 insertions(+), 12 deletions(-)

Comments

Paul Elder April 18, 2025, 6:56 a.m. UTC | #1 
Hi Milan,

Thank you for the patches.

On Mon, Apr 07, 2025 at 10:56:31AM +0200, Milan Zamazal wrote:
> Output configurations in simple pipeline are added depending on whether
> a converter, software ISP or none of them are used.  If a converter or
> software ISP is used, no raw output configurations are added.
> 
> In order to support raw output at least with software ISP, let's always
> add raw output configurations.  A flag is added to
> SimpleCameraData::Configuration indicating whether it's for a raw or a
> converted output.  We later filter formats and output sizes for
> particular stream configurations according to the new configuration
> flag.
> 
> This is just preparation and raw output is still not supported.  At the
> moment, we simply filter out raw configurations unconditionally to keep
> the current code working; this will be changed in followup patches.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 31 +++++++++++++++---------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 60b7e1b1..82d7a9a5 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -319,6 +319,7 @@ public:
>  		Size captureSize;
>  		std::vector<PixelFormat> outputFormats;
>  		SizeRange outputSizes;
> +		bool raw;
>  	};
>  
>  	std::vector<Stream> streams_;
> @@ -708,27 +709,33 @@ void SimpleCameraData::tryPipeline(unsigned int code, const Size &size)
>  			continue;
>  
>  		Configuration config;
> +
> +		/* Raw configuration */
>  		config.code = code;
>  		config.sensorSize = size;
>  		config.captureFormat = pixelFormat;
>  		config.captureSize = format.size;
> +		config.outputFormats = { pixelFormat };
> +		config.outputSizes = config.captureSize;
> +		config.raw = true;
> +		configs_.push_back(config);
>  
> +		/* Modified, non-raw, configuration */
> +		config.raw = false;
>  		if (converter_) {
>  			config.outputFormats = converter_->formats(pixelFormat);
>  			config.outputSizes = converter_->sizes(format.size);
>  		} else if (swIsp_) {
> -			config.outputFormats = swIsp_->formats(pixelFormat);
> -			config.outputSizes = swIsp_->sizes(pixelFormat, format.size);
> -			if (config.outputFormats.empty()) {
> +			std::vector<PixelFormat> outputFormats = swIsp_->formats(pixelFormat);
> +			if (outputFormats.empty()) {
>  				/* Do not use swIsp for unsupported pixelFormat's. */
> -				config.outputFormats = { pixelFormat };
> -				config.outputSizes = config.captureSize;
> +				continue;
>  			}
> +			config.outputFormats = outputFormats;
> +			config.outputSizes = swIsp_->sizes(pixelFormat, format.size);
>  		} else {
> -			config.outputFormats = { pixelFormat };
> -			config.outputSizes = config.captureSize;
> +			continue;
>  		}
> -
>  		configs_.push_back(config);
>  	}
>  }
> @@ -1301,10 +1308,10 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>  	/* Create the formats map. */
>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>  
> -	for (const SimpleCameraData::Configuration &cfg : data->configs_) {
> -		for (PixelFormat format : cfg.outputFormats)
> -			formats[format].push_back(cfg.outputSizes);
> -	}
> +	for (const SimpleCameraData::Configuration &cfg : data->configs_)
> +		if (!cfg.raw)
> +			for (PixelFormat format : cfg.outputFormats)
> +				formats[format].push_back(cfg.outputSizes);

This can break bisection (at runtime), since if the pipeline only
supports raw formats then !cfg.raw will never be satisfied and formats
will be empty, thus later on causing

	for (StreamRole role : roles) {
		StreamConfiguration cfg{ StreamFormats{ formats } };
		cfg.pixelFormat = formats.begin()->first;
		cfg.size = formats.begin()->second[0].max; <-- this

this to segfault.

I see that later on in the series this is resolved but I'd prefer
avoiding breaking bisection.


Thanks,

Paul

>  
>  	/* Sort the sizes and merge any consecutive overlapping ranges. */
>  	for (auto &[format, sizes] : formats) {
> -- 
> 2.49.0
>
Milan Zamazal April 26, 2025, 7:07 p.m. UTC | #2 
Hi Paul,

Paul Elder <paul.elder@ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patches.
>
> On Mon, Apr 07, 2025 at 10:56:31AM +0200, Milan Zamazal wrote:
>> Output configurations in simple pipeline are added depending on whether
>> a converter, software ISP or none of them are used.  If a converter or
>> software ISP is used, no raw output configurations are added.
>> 
>> In order to support raw output at least with software ISP, let's always
>> add raw output configurations.  A flag is added to
>> SimpleCameraData::Configuration indicating whether it's for a raw or a
>> converted output.  We later filter formats and output sizes for
>> particular stream configurations according to the new configuration
>> flag.
>> 
>> This is just preparation and raw output is still not supported.  At the
>> moment, we simply filter out raw configurations unconditionally to keep
>> the current code working; this will be changed in followup patches.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/libcamera/pipeline/simple/simple.cpp | 31 +++++++++++++++---------
>>  1 file changed, 19 insertions(+), 12 deletions(-)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 60b7e1b1..82d7a9a5 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -319,6 +319,7 @@ public:
>>  		Size captureSize;
>>  		std::vector<PixelFormat> outputFormats;
>>  		SizeRange outputSizes;
>> +		bool raw;
>>  	};
>>  
>>  	std::vector<Stream> streams_;
>> @@ -708,27 +709,33 @@ void SimpleCameraData::tryPipeline(unsigned int code, const Size &size)
>>  			continue;
>>  
>>  		Configuration config;
>> +
>> +		/* Raw configuration */
>>  		config.code = code;
>>  		config.sensorSize = size;
>>  		config.captureFormat = pixelFormat;
>>  		config.captureSize = format.size;
>> +		config.outputFormats = { pixelFormat };
>> +		config.outputSizes = config.captureSize;
>> +		config.raw = true;
>> +		configs_.push_back(config);
>>  
>> +		/* Modified, non-raw, configuration */
>> +		config.raw = false;
>>  		if (converter_) {
>>  			config.outputFormats = converter_->formats(pixelFormat);
>>  			config.outputSizes = converter_->sizes(format.size);
>>  		} else if (swIsp_) {
>> -			config.outputFormats = swIsp_->formats(pixelFormat);
>> -			config.outputSizes = swIsp_->sizes(pixelFormat, format.size);
>> -			if (config.outputFormats.empty()) {
>> +			std::vector<PixelFormat> outputFormats = swIsp_->formats(pixelFormat);
>> +			if (outputFormats.empty()) {
>>  				/* Do not use swIsp for unsupported pixelFormat's. */
>> -				config.outputFormats = { pixelFormat };
>> -				config.outputSizes = config.captureSize;
>> +				continue;
>>  			}
>> +			config.outputFormats = outputFormats;
>> +			config.outputSizes = swIsp_->sizes(pixelFormat, format.size);
>>  		} else {
>> -			config.outputFormats = { pixelFormat };
>> -			config.outputSizes = config.captureSize;
>> +			continue;
>>  		}
>> -
>>  		configs_.push_back(config);
>>  	}
>>  }
>> @@ -1301,10 +1308,10 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>>  	/* Create the formats map. */
>>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>>  
>> -	for (const SimpleCameraData::Configuration &cfg : data->configs_) {
>> -		for (PixelFormat format : cfg.outputFormats)
>> -			formats[format].push_back(cfg.outputSizes);
>> -	}
>> +	for (const SimpleCameraData::Configuration &cfg : data->configs_)
>> +		if (!cfg.raw)
>> +			for (PixelFormat format : cfg.outputFormats)
>> +				formats[format].push_back(cfg.outputSizes);
>
> This can break bisection (at runtime), since if the pipeline only
> supports raw formats then !cfg.raw will never be satisfied and formats
> will be empty, thus later on causing
>
> 	for (StreamRole role : roles) {
> 		StreamConfiguration cfg{ StreamFormats{ formats } };
> 		cfg.pixelFormat = formats.begin()->first;
> 		cfg.size = formats.begin()->second[0].max; <-- this
>
> this to segfault.
>
> I see that later on in the series this is resolved but I'd prefer
> avoiding breaking bisection.

Sure, thank you for pointing this out.  I'll fix it in v5.

> Thanks,
>
> Paul
>
>>  
>>  	/* Sort the sizes and merge any consecutive overlapping ranges. */
>>  	for (auto &[format, sizes] : formats) {
>> -- 
>> 2.49.0
>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 60b7e1b1..82d7a9a5 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -319,6 +319,7 @@  public:
 		Size captureSize;
 		std::vector<PixelFormat> outputFormats;
 		SizeRange outputSizes;
+		bool raw;
 	};
 
 	std::vector<Stream> streams_;
@@ -708,27 +709,33 @@  void SimpleCameraData::tryPipeline(unsigned int code, const Size &size)
 			continue;
 
 		Configuration config;
+
+		/* Raw configuration */
 		config.code = code;
 		config.sensorSize = size;
 		config.captureFormat = pixelFormat;
 		config.captureSize = format.size;
+		config.outputFormats = { pixelFormat };
+		config.outputSizes = config.captureSize;
+		config.raw = true;
+		configs_.push_back(config);
 
+		/* Modified, non-raw, configuration */
+		config.raw = false;
 		if (converter_) {
 			config.outputFormats = converter_->formats(pixelFormat);
 			config.outputSizes = converter_->sizes(format.size);
 		} else if (swIsp_) {
-			config.outputFormats = swIsp_->formats(pixelFormat);
-			config.outputSizes = swIsp_->sizes(pixelFormat, format.size);
-			if (config.outputFormats.empty()) {
+			std::vector<PixelFormat> outputFormats = swIsp_->formats(pixelFormat);
+			if (outputFormats.empty()) {
 				/* Do not use swIsp for unsupported pixelFormat's. */
-				config.outputFormats = { pixelFormat };
-				config.outputSizes = config.captureSize;
+				continue;
 			}
+			config.outputFormats = outputFormats;
+			config.outputSizes = swIsp_->sizes(pixelFormat, format.size);
 		} else {
-			config.outputFormats = { pixelFormat };
-			config.outputSizes = config.captureSize;
+			continue;
 		}
-
 		configs_.push_back(config);
 	}
 }
@@ -1301,10 +1308,10 @@  SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
 	/* Create the formats map. */
 	std::map<PixelFormat, std::vector<SizeRange>> formats;
 
-	for (const SimpleCameraData::Configuration &cfg : data->configs_) {
-		for (PixelFormat format : cfg.outputFormats)
-			formats[format].push_back(cfg.outputSizes);
-	}
+	for (const SimpleCameraData::Configuration &cfg : data->configs_)
+		if (!cfg.raw)
+			for (PixelFormat format : cfg.outputFormats)
+				formats[format].push_back(cfg.outputSizes);
 
 	/* Sort the sizes and merge any consecutive overlapping ranges. */
 	for (auto &[format, sizes] : formats) {