[v9,04/10] libcamera: simple: Add plain output configurations
diff mbox series

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

Commit Message

Milan Zamazal July 7, 2025, 3:58 p.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.  Unless
only raw configurations are available, in which case we add them to
avoid ...

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 | 34 +++++++++++++++---------
 1 file changed, 22 insertions(+), 12 deletions(-)

Comments

Umang Jain July 11, 2025, 4:38 a.m. UTC | #1 
Hi Milan

On Mon, Jul 07, 2025 at 05:58:49PM +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.  Unless
> only raw configurations are available, in which case we add them to
> avoid ...
> 

... ?
> 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 | 34 +++++++++++++++---------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 368bf3020..1a7474228 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -325,6 +325,7 @@ public:
>  		Size captureSize;
>  		std::vector<PixelFormat> outputFormats;
>  		SizeRange outputSizes;
> +		bool raw;
>  	};
>  
>  	std::vector<Stream> streams_;
> @@ -714,27 +715,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);

I am wondering a bit if we really need to do this[1] and if at all, won't a
better idea is to populate 

	std::map<PixelFormat, std::vector<const Configuration *>> formats_;

with <rawFormat, Configuration> instead? 

[1]
On the other hand, looking at format_ mapping with applied:

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 60034395..abfd3f2d 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -640,6 +640,14 @@ int SimpleCameraData::init()
                        formats_[fmt].push_back(&config);
        }
 
+       /* Log formats_ key mappings */
+       for (const auto &iter : formats_) {
+               const PixelFormatInfo &info = PixelFormatInfo::info(iter.first);
+               if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
+                       LOG(SimplePipeline, Info) << "formats_:<Key> "
+                                                  << iter.first.toString();
+       }
+
        properties_ = sensor_->properties();
 
        /* Find the first subdev that can generate a frame start signal, if any. */

We have:

[0:32:46.178566332] [409]  INFO SimplePipeline simple.cpp:647 formats_:<Key> SRGGB10
[0:32:46.178813311] [409]  INFO SimplePipeline simple.cpp:647 formats_:<Key> SRGGB10_CSI2P
[0:32:46.179011592] [409]  INFO SimplePipeline simple.cpp:647 formats_:<Key> SRGGB8

So I think we already have rawformats supported by the sensor, mapped to
Configuration, in the formats_ map. Should we just detect role=raw
requested by the user and use formats_ to retrieve the Configuration ?

We would have ignore the config.outputFormats and config.outputSizes for the
role=raw, since it won't be relevant. And look at this current patch,

 +             config.outputFormats = { pixelFormat };                         
 +             config.outputSizes = config.captureSize;   

is caching the redundant information to Configuration.

These are my current thoughts on the patch. I haven't read the whole
series yet (probably will take multiple passes).

>  
> +		/* 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);
>  	}
>  }
> @@ -1313,10 +1320,13 @@ 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);
> -	}
> +	const bool rawOnly = std::all_of(data->configs_.cbegin(),
> +					 data->configs_.cend(),
> +					 [](const auto &c) { return c.raw; });
> +	for (const SimpleCameraData::Configuration &cfg : data->configs_)
> +		if (!cfg.raw || rawOnly)
> +			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) {
> -- 
> 2.50.0
>
Milan Zamazal July 11, 2025, 2:15 p.m. UTC | #2 
Hi Umang,

thank you for review.

Umang Jain <uajain@igalia.com> writes:

> Hi Milan
>
> On Mon, Jul 07, 2025 at 05:58:49PM +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.  Unless
>> only raw configurations are available, in which case we add them to
>> avoid ...
>> 
>
> ... ?

Forgotten unfinished thought, I'll fix it.

>> 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 | 34 +++++++++++++++---------
>>  1 file changed, 22 insertions(+), 12 deletions(-)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 368bf3020..1a7474228 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -325,6 +325,7 @@ public:
>>  		Size captureSize;
>>  		std::vector<PixelFormat> outputFormats;
>>  		SizeRange outputSizes;
>> +		bool raw;
>>  	};
>>  
>>  	std::vector<Stream> streams_;
>> @@ -714,27 +715,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);
>
> I am wondering a bit if we really need to do this[1] and if at all, won't a
> better idea is to populate 
>
> 	std::map<PixelFormat, std::vector<const Configuration *>> formats_;
>
> with <rawFormat, Configuration> instead? 

I prefer keeping unnamed data structures simple.

> [1]
> On the other hand, looking at format_ mapping with applied:
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 60034395..abfd3f2d 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -640,6 +640,14 @@ int SimpleCameraData::init()
>                         formats_[fmt].push_back(&config);
>         }
>  
> +       /* Log formats_ key mappings */
> +       for (const auto &iter : formats_) {
> +               const PixelFormatInfo &info = PixelFormatInfo::info(iter.first);
> +               if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +                       LOG(SimplePipeline, Info) << "formats_:<Key> "
> +                                                  << iter.first.toString();
> +       }
> +
>         properties_ = sensor_->properties();
>  
>         /* Find the first subdev that can generate a frame start signal, if any. */
>
> We have:
>
> [0:32:46.178566332] [409]  INFO SimplePipeline simple.cpp:647 formats_:<Key> SRGGB10
> [0:32:46.178813311] [409]  INFO SimplePipeline simple.cpp:647 formats_:<Key> SRGGB10_CSI2P
> [0:32:46.179011592] [409]  INFO SimplePipeline simple.cpp:647 formats_:<Key> SRGGB8
>
> So I think we already have rawformats supported by the sensor, mapped to
> Configuration, in the formats_ map. Should we just detect role=raw
> requested by the user and use formats_ to retrieve the Configuration ?
>
> We would have ignore the config.outputFormats and config.outputSizes for the
> role=raw, since it won't be relevant. And look at this current patch,
>
>  +             config.outputFormats = { pixelFormat };                         
>  +             config.outputSizes = config.captureSize;   
>
> is caching the redundant information to Configuration.

Makes sense and looks working, with additional adjustments.  I'll try to
change it this way in v10.

> These are my current thoughts on the patch. I haven't read the whole
> series yet (probably will take multiple passes).
>
>>  
>> +		/* 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);
>>  	}
>>  }
>> @@ -1313,10 +1320,13 @@ 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);
>> -	}
>> +	const bool rawOnly = std::all_of(data->configs_.cbegin(),
>> +					 data->configs_.cend(),
>> +					 [](const auto &c) { return c.raw; });
>> +	for (const SimpleCameraData::Configuration &cfg : data->configs_)
>> +		if (!cfg.raw || rawOnly)
>> +			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) {
>> -- 
>> 2.50.0
>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 368bf3020..1a7474228 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -325,6 +325,7 @@  public:
 		Size captureSize;
 		std::vector<PixelFormat> outputFormats;
 		SizeRange outputSizes;
+		bool raw;
 	};
 
 	std::vector<Stream> streams_;
@@ -714,27 +715,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);
 	}
 }
@@ -1313,10 +1320,13 @@  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);
-	}
+	const bool rawOnly = std::all_of(data->configs_.cbegin(),
+					 data->configs_.cend(),
+					 [](const auto &c) { return c.raw; });
+	for (const SimpleCameraData::Configuration &cfg : data->configs_)
+		if (!cfg.raw || rawOnly)
+			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) {