[PATCH v4 07/11] libcamera: simple: Consider raw output configurations

[Milan Zamazal]

Hi Barnabás,

thank you for review.

Barnabás Pőcze <barnabas.pocze at ideasonboard.com> writes:

> Hi
>
> 2025. 04. 07. 10:56 keltezéssel, Milan Zamazal írta:
>> When generating simple pipeline configuration, raw output configurations
>> are generated but later filtered out.  Let's include processed and/or
>> raw output configurations depending on the requested stream roles.
>> Raw and processed formats are handled separately.  The intention is that
>> in case both raw and processed formats are requested then the raw
>> formats should be left intact to the extent possible and not be
>> influenced by the processed formats (implying that raw parameters
>> compatible with the processed output requirements must be requested by
>> the application).
>> Raw output configurations are identified by colorSpace being set to
>> ColorSpace::Raw.
>> This is another preparatory patch without making raw outputs working.
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>   src/libcamera/pipeline/simple/simple.cpp | 73 ++++++++++++++++--------
>>   1 file changed, 48 insertions(+), 25 deletions(-)
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index bf9d75f4..6b65d79f 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -439,6 +439,8 @@ private:
>>   	const MediaPad *acquirePipeline(SimpleCameraData *data);
>>   	void releasePipeline(SimpleCameraData *data);
>>   +	void setUpFormatSizes(std::map<PixelFormat, std::vector<SizeRange>> &formats);
>> +
>>   	std::map<const MediaEntity *, EntityData> entities_;
>>     	MediaDevice *converter_;
>> @@ -1321,35 +1323,28 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>>   			data->processedRequested_ = true;
>>   		}
>>   -	/* Create the formats map. */
>> -	std::map<PixelFormat, std::vector<SizeRange>> formats;
>> +	/* Create the formats maps. */
>> +	std::map<PixelFormat, std::vector<SizeRange>> processedFormats;
>> +	std::map<PixelFormat, std::vector<SizeRange>> rawFormats;
>>     	for (const SimpleCameraData::Configuration &cfg : data->configs_)
>> -		if (!cfg.raw)
>> -			for (PixelFormat format : cfg.outputFormats)
>> -				formats[format].push_back(cfg.outputSizes);
>> +		for (PixelFormat format : cfg.outputFormats)
>> +			(cfg.raw ? rawFormats : processedFormats)[format].push_back(cfg.outputSizes);
>>   -	/* Sort the sizes and merge any consecutive overlapping ranges. */
>> -	for (auto &[format, sizes] : formats) {
>> -		std::sort(sizes.begin(), sizes.end(),
>> -			  [](SizeRange &a, SizeRange &b) {
>> -				  return a.min < b.min;
>> -			  });
>> -
>> -		auto cur = sizes.begin();
>> -		auto next = cur;
>> -
>> -		while (++next != sizes.end()) {
>> -			if (cur->max.width >= next->min.width &&
>> -			    cur->max.height >= next->min.height)
>> -				cur->max = next->max;
>> -			else if (++cur != next)
>> -				*cur = *next;
>> -		}
>> -
>> -		sizes.erase(++cur, sizes.end());
>> +	if (data->processedRequested_ && processedFormats.empty()) {
>> +		LOG(SimplePipeline, Error)
>> +			<< "Processed stream requsted but no corresponding output configuration found";
>> +		return nullptr;
>> +	}
>> +	if (data->rawRequested_ && rawFormats.empty()) {
>> +		LOG(SimplePipeline, Error)
>> +			<< "Raw stream requsted but no corresponding output configuration found";
>> +		return nullptr;
>>   	}
>>   +	setUpFormatSizes(processedFormats);
>> +	setUpFormatSizes(rawFormats);
>> +
>>   	/*
>>   	 * Create the stream configurations. Take the first entry in the formats
>>   	 * map as the default, for lack of a better option.
>> @@ -1357,11 +1352,13 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>>   	 * \todo Implement a better way to pick the default format
>>   	 */
>>   	for (StreamRole role : roles) {
>> +		bool raw = (role == StreamRole::Raw);
>> +		auto formats = (raw ? rawFormats : processedFormats);
>>   		StreamConfiguration cfg{ StreamFormats{ formats } };
>>   		cfg.pixelFormat = formats.begin()->first;
>>   		cfg.size = formats.begin()->second[0].max;
>>   -		if (role == StreamRole::Raw)
>> +		if (raw)
>>   			cfg.colorSpace = ColorSpace::Raw;
>>   		else if (data->swIsp_)
>>   			cfg.colorSpace = ColorSpace::Srgb;
>> @@ -1374,6 +1371,32 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>>   	return config;
>>   }
>>   +void SimplePipelineHandler::setUpFormatSizes(
>> +	std::map<PixelFormat, std::vector<SizeRange>> &formats)
>
> Does this need to be a member? If not, could it be moved into an anonymous
> namespace in this file? Or since this is only used in a function, maybe
> just save it into a lambda there for now.

Or it could be a loop over the two variables.  I don't have any real
preference, I can do whatever reviewers like.

>> +{
>> +	/* Sort the sizes and merge any consecutive overlapping ranges. */
>
> I see that this has been copied unchanged, but I don't understand how this could
> be correct. It does not seem to account for `SizeRange::{h,v}Step` at all.
> What am I missing something?

It's probably some implicit assumption about the steps, whether correct
or not.  Perhaps this and the followup \todo should be revised some day.

> Regards,
> Barnabás Pőcze
>
>> +
>> +	for (auto &[format, sizes] : formats) {
>> +		std::sort(sizes.begin(), sizes.end(),
>> +			  [](SizeRange &a, SizeRange &b) {
>> +				  return a.min < b.min;
>> +			  });
>> +
>> +		auto cur = sizes.begin();
>> +		auto next = cur;
>> +
>> +		while (++next != sizes.end()) {
>> +			if (cur->max.width >= next->min.width &&
>> +			    cur->max.height >= next->min.height)
>> +				cur->max = next->max;
>> +			else if (++cur != next)
>> +				*cur = *next;
>> +		}
>> +
>> +		sizes.erase(++cur, sizes.end());
>> +	}
>> +}
>> +
>>   int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>>   {
>>   	SimpleCameraConfiguration *config =