[libcamera-devel] [PATCH 15/20] libcamera: pipeline: simple: Add output formats to Configuration

[Kieran Bingham]

On 31/01/2021 22:46, Laurent Pinchart wrote:
> Store the list of converter output formats in the Configuration
> structure, to be used to implement multi-stream support. As the
> Configuration structure grows bigger, avoid duplicating it in the
> formats_ map for each supported pixel format by storing it in a configs_
> vector instead, and storing pointers only in the map.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++----------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index c44fa9ee28ed..6a8253101a61 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -159,6 +159,7 @@ public:
>  		uint32_t code;
>  		PixelFormat captureFormat;
>  		Size captureSize;
> +		std::vector<PixelFormat> outputFormats;
>  		SizeRange outputSizes;
>  	};
>  
> @@ -167,7 +168,8 @@ public:
>  	std::list<Entity> entities_;
>  	V4L2VideoDevice *video_;
>  
> -	std::map<PixelFormat, Configuration> formats_;
> +	std::vector<Configuration> configs_;
> +	std::map<PixelFormat, const Configuration *> formats_;

Eeep, this scares me ...
Are they kept in sync?

Only used having pre-reserved, to ensure the pointers stay valid etc...


>  };
>  
>  class SimpleCameraConfiguration : public CameraConfiguration
> @@ -371,13 +373,6 @@ int SimpleCameraData::init()
>  				       })
>  			<< " ]";
>  
> -		/*
> -		 * Store the configuration in the formats_ map, mapping the
> -		 * PixelFormat to the corresponding configuration. Any
> -		 * previously stored value is overwritten, as the pipeline
> -		 * handler currently doesn't care about how a particular
> -		 * PixelFormat is achieved.
> -		 */
>  		for (const auto &videoFormat : videoFormats) {
>  			PixelFormat pixelFormat = videoFormat.first.toPixelFormat();
>  			if (!pixelFormat)
> @@ -389,23 +384,34 @@ int SimpleCameraData::init()
>  			config.captureSize = format.size;
>  
>  			if (!converter) {
> +				config.outputFormats = { pixelFormat };
>  				config.outputSizes = config.captureSize;
> -				formats_[pixelFormat] = config;
> -				continue;
> +			} else {
> +				config.outputFormats = converter->formats(pixelFormat);
> +				config.outputSizes = converter->sizes(format.size);
>  			}
>  
> -			config.outputSizes = converter->sizes(format.size);
> -
> -			for (PixelFormat fmt : converter->formats(pixelFormat))
> -				formats_[fmt] = config;
> +			configs_.push_back(config);
>  		}
>  	}
>  
> -	if (formats_.empty()) {
> +	if (configs_.empty()) {
>  		LOG(SimplePipeline, Error) << "No valid configuration found";
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Map the pixel formats to configurations. Any previously stored value
> +	 * is overwritten, as the pipeline handler currently doesn't care about
> +	 * how a particular PixelFormat is achieved.
> +	 */
> +	for (const Configuration &config : configs_) {
> +		formats_[config.captureFormat] = &config;
> +
> +		for (PixelFormat fmt : config.outputFormats)
> +			formats_[fmt] = &config;
> +	}

Phew, ok, so configs_ never grows or changes after init, and the
formats_ is generated after the configs_ are prepared. So I think we're
good.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +
>  	properties_ = sensor_->properties();
>  
>  	return 0;
> @@ -548,7 +554,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  		status = Adjusted;
>  	}
>  
> -	pipeConfig_ = &it->second;
> +	pipeConfig_ = it->second;
>  	if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>  		LOG(SimplePipeline, Debug)
>  			<< "Adjusting size from " << cfg.size.toString()
> @@ -614,7 +620,7 @@ CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera
>  		       std::inserter(formats, formats.end()),
>  		       [](const auto &format) -> decltype(formats)::value_type {
>  			       const PixelFormat &pixelFormat = format.first;
> -			       const Size &size = format.second.captureSize;
> +			       const Size &size = format.second->captureSize;
>  			       return { pixelFormat, { size } };
>  		       });
>  
> 

-- 
Regards
--
Kieran