[v4,07/11] libcamera: simple: Consider raw output configurations
diff mbox series

Message ID 20250407085639.16180-8-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 
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@redhat.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 73 ++++++++++++++++--------
 1 file changed, 48 insertions(+), 25 deletions(-)

Comments

Barnabás Pőcze May 9, 2025, 3:46 p.m. UTC | #1 
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@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.


> +{
> +	/* 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?


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 =
Milan Zamazal May 13, 2025, 10:31 a.m. UTC | #2 
Hi Barnabás,

thank you for review.

Barnabás Pőcze <barnabas.pocze@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@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 =

Patch
diff mbox series 

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)
+{
+	/* 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());
+	}
+}
+
 int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 {
 	SimpleCameraConfiguration *config =