[v10,4/8] libcamera: simple: Handle processed and raw formats separately
diff mbox series

Message ID 20250711175345.90318-5-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Enable raw streams with software ISP
Related show

Commit Message

Milan Zamazal July 11, 2025, 5:53 p.m. UTC 
Let's handle both processed and/or raw output configurations, depending
on the requested stream roles.  In addition to the already handled
processed formats and sizes, this patch adds handling of raw formats and
sizes, which correspond to the capture formats and sizes.

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).

Accompanying checks for invalid role specifications are introduced.

This is another preparatory patch without making raw outputs working.

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

Comments

Umang Jain July 21, 2025, 5:26 a.m. UTC | #1 
Hi Milan

On Fri, Jul 11, 2025 at 07:53:37PM +0200, Milan Zamazal wrote:
> Let's handle both processed and/or raw output configurations, depending
> on the requested stream roles.  In addition to the already handled
> processed formats and sizes, this patch adds handling of raw formats and
> sizes, which correspond to the capture formats and sizes.
> 
> 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).
> 
> Accompanying checks for invalid role specifications are introduced.
> 
> This is another preparatory patch without making raw outputs working.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 80 +++++++++++++++++-------
>  1 file changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 87b26d4a2..37abaa0e0 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1310,35 +1310,67 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>  	if (roles.empty())
>  		return config;
>  
> -	/* Create the formats map. */
> -	std::map<PixelFormat, std::vector<SizeRange>> formats;
> +	bool processedRequested = false;
> +	bool rawRequested = false;
> +	for (const auto &role : roles)
> +		if (role == StreamRole::Raw) {
> +			if (rawRequested) {
> +				LOG(SimplePipeline, Error)
> +					<< "Can't capture multiple raw streams";
> +				return nullptr;
> +			}
> +			rawRequested = true;
> +		} else {
> +			processedRequested = true;
> +		}
> +
> +	/* 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_) {

You can simply try to iterate over data->formats_
data->formats_ contain both raw and processed formats already, so we can
populate the local formats map from that.

> +		rawFormats[cfg.captureFormat].push_back(cfg.captureSize);
>  		for (PixelFormat format : cfg.outputFormats)
> -			formats[format].push_back(cfg.outputSizes);
> +			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 (processedRequested && processedFormats.empty()) {
> +		LOG(SimplePipeline, Error)
> +			<< "Processed stream requsted but no corresponding output configuration found";
> +		return nullptr;
> +	}
> +	if (rawRequested && rawFormats.empty()) {
> +		LOG(SimplePipeline, Error)
> +			<< "Raw stream requsted but no corresponding output configuration found";
> +		return nullptr;
>  	}

You can simply keep one formats map locally. No need to split between
the raw and processed. Depending on the role requested, you can simply
pick appropriate configurations from the formats map.

>  
> +	auto setUpFormatSizes = [](std::map<PixelFormat, std::vector<SizeRange>> &formats) {
> +		/* 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());
> +		}
> +	};
> +	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.
> @@ -1346,11 +1378,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);
> +		const 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) {
>  			/* Enforce raw colour space for raw roles. */
>  			cfg.colorSpace = ColorSpace::Raw;
>  		} else if (data->swIsp_) {

What I have done is the following:

	for (StreamRole role : roles) {
		StreamConfiguration cfg{ StreamFormats{ formats } };

		switch (role) {
		case StreamRole::Raw:
			for (auto &[format, sizes] : formats) {
				if (!isFormatRaw(format))
					continue;
				cfg.pixelFormat = format;
				cfg.size = sizes.back().max;
				cfg.colorSpace = ColorSpace::Raw;
				break;
			}
			break;
		default:
			for (auto &[format, sizes] : formats) {
				if (isFormatRaw(format))
					continue;
				cfg.pixelFormat = format;
				cfg.size = sizes[0].max;
			}
		}

		config->addConfiguration(cfg);
	}

I have `formats` which contain both the RAW and processed formats
(populated from data->formats_). Hence in this loop, I simply pick up
the first entry - which would satisfy the role.

> -- 
> 2.50.1
>
Milan Zamazal July 22, 2025, 10:44 a.m. UTC | #2 
Hi Umang,

Umang Jain <uajain@igalia.com> writes:

> Hi Milan
>
> On Fri, Jul 11, 2025 at 07:53:37PM +0200, Milan Zamazal wrote:
>> Let's handle both processed and/or raw output configurations, depending
>> on the requested stream roles.  In addition to the already handled
>> processed formats and sizes, this patch adds handling of raw formats and
>> sizes, which correspond to the capture formats and sizes.
>> 
>> 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).
>> 
>> Accompanying checks for invalid role specifications are introduced.
>> 
>> This is another preparatory patch without making raw outputs working.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/libcamera/pipeline/simple/simple.cpp | 80 +++++++++++++++++-------
>>  1 file changed, 57 insertions(+), 23 deletions(-)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 87b26d4a2..37abaa0e0 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -1310,35 +1310,67 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>>  	if (roles.empty())
>>  		return config;
>>  
>> -	/* Create the formats map. */
>> -	std::map<PixelFormat, std::vector<SizeRange>> formats;
>> +	bool processedRequested = false;
>> +	bool rawRequested = false;
>> +	for (const auto &role : roles)
>> +		if (role == StreamRole::Raw) {
>> +			if (rawRequested) {
>> +				LOG(SimplePipeline, Error)
>> +					<< "Can't capture multiple raw streams";
>> +				return nullptr;
>> +			}
>> +			rawRequested = true;
>> +		} else {
>> +			processedRequested = true;
>> +		}
>> +
>> +	/* 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_) {
>
> You can simply try to iterate over data->formats_
> data->formats_ contain both raw and processed formats already, so we can
> populate the local formats map from that.
>
>> +		rawFormats[cfg.captureFormat].push_back(cfg.captureSize);
>>  		for (PixelFormat format : cfg.outputFormats)
>> -			formats[format].push_back(cfg.outputSizes);
>> +			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 (processedRequested && processedFormats.empty()) {
>> +		LOG(SimplePipeline, Error)
>> +			<< "Processed stream requsted but no corresponding output configuration found";
>> +		return nullptr;
>> +	}
>> +	if (rawRequested && rawFormats.empty()) {
>> +		LOG(SimplePipeline, Error)
>> +			<< "Raw stream requsted but no corresponding output configuration found";
>> +		return nullptr;
>>  	}
>
> You can simply keep one formats map locally. No need to split between
> the raw and processed. Depending on the role requested, you can simply
> pick appropriate configurations from the formats map.

I'm not sure this is correct.  I'd think that if a raw role is
requested, it means an unprocessed input from the camera.  Which may or
may not be a format for which isFormatRaw returns true.  And a processed
output can, at least in theory, produce a format for which isFormatRaw
returns true.

>>  
>> +	auto setUpFormatSizes = [](std::map<PixelFormat, std::vector<SizeRange>> &formats) {
>> +		/* 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());
>> +		}
>> +	};
>> +	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.
>> @@ -1346,11 +1378,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);
>> +		const 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) {
>>  			/* Enforce raw colour space for raw roles. */
>>  			cfg.colorSpace = ColorSpace::Raw;
>>  		} else if (data->swIsp_) {
>
> What I have done is the following:
>
> 	for (StreamRole role : roles) {
> 		StreamConfiguration cfg{ StreamFormats{ formats } };
>
> 		switch (role) {
> 		case StreamRole::Raw:
> 			for (auto &[format, sizes] : formats) {
> 				if (!isFormatRaw(format))
> 					continue;
> 				cfg.pixelFormat = format;
> 				cfg.size = sizes.back().max;
> 				cfg.colorSpace = ColorSpace::Raw;
> 				break;
> 			}
> 			break;
> 		default:
> 			for (auto &[format, sizes] : formats) {
> 				if (isFormatRaw(format))
> 					continue;
> 				cfg.pixelFormat = format;
> 				cfg.size = sizes[0].max;
> 			}
> 		}
>
> 		config->addConfiguration(cfg);
> 	}
>
> I have `formats` which contain both the RAW and processed formats
> (populated from data->formats_). Hence in this loop, I simply pick up
> the first entry - which would satisfy the role.
>
>> -- 
>> 2.50.1
>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 87b26d4a2..37abaa0e0 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -1310,35 +1310,67 @@  SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
 	if (roles.empty())
 		return config;
 
-	/* Create the formats map. */
-	std::map<PixelFormat, std::vector<SizeRange>> formats;
+	bool processedRequested = false;
+	bool rawRequested = false;
+	for (const auto &role : roles)
+		if (role == StreamRole::Raw) {
+			if (rawRequested) {
+				LOG(SimplePipeline, Error)
+					<< "Can't capture multiple raw streams";
+				return nullptr;
+			}
+			rawRequested = true;
+		} else {
+			processedRequested = true;
+		}
+
+	/* 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_) {
+		rawFormats[cfg.captureFormat].push_back(cfg.captureSize);
 		for (PixelFormat format : cfg.outputFormats)
-			formats[format].push_back(cfg.outputSizes);
+			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 (processedRequested && processedFormats.empty()) {
+		LOG(SimplePipeline, Error)
+			<< "Processed stream requsted but no corresponding output configuration found";
+		return nullptr;
+	}
+	if (rawRequested && rawFormats.empty()) {
+		LOG(SimplePipeline, Error)
+			<< "Raw stream requsted but no corresponding output configuration found";
+		return nullptr;
 	}
 
+	auto setUpFormatSizes = [](std::map<PixelFormat, std::vector<SizeRange>> &formats) {
+		/* 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());
+		}
+	};
+	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.
@@ -1346,11 +1378,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);
+		const 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) {
 			/* Enforce raw colour space for raw roles. */
 			cfg.colorSpace = ColorSpace::Raw;
 		} else if (data->swIsp_) {