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

[Laurent Pinchart]

Hi Paul,

On Tue, Mar 02, 2021 at 02:45:26PM +0900, paul.elder at ideasonboard.com wrote:
> On Mon, Feb 01, 2021 at 12:46:57AM +0200, 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>
> > ---
> >  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_;
> >  };
> >  
> >  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;
> > +	}
> > +
> >  	properties_ = sensor_->properties();
> >  
> >  	return 0;
> > @@ -548,7 +554,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> >  		status = Adjusted;
> >  	}
> >  
> > -	pipeConfig_ = &it->second;
> > +	pipeConfig_ = it->second;
> 
> I think this belongs in the previous patch.

I don't think so. it iterates over data_->formats_, whose type has
changed in this patch.

> The rest looks good to me.
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> >  	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,

Laurent Pinchart