[PATCH v4 05/11] libcamera: simple: Add plain output configurations

[Milan Zamazal]

Hi Paul,

Paul Elder <paul.elder at 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 at 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
>>