[libcamera-devel,14/20] libcamera: pipeline: simple: Cache pipeline config in SimpleCameraConfiguration
diff mbox series

Message ID 20210131224702.8838-15-laurent.pinchart@ideasonboard.com
State Accepted
Delegated to: Laurent Pinchart
Headers show
Series
  • [libcamera-devel,01/20] libcamera: pipeline: simple: Manage converter with std::unique_ptr<>
Related show

Commit Message

Laurent Pinchart Jan. 31, 2021, 10:46 p.m. UTC 
As the pipeline configuration is selected in
SimpleCameraConfiguration::validate() already, cache it in the
SimpleCameraConfiguration instead of looking it up in
SimplePipelineHandler::configure(). This makes little difference at the
moment, but will save duplication of more complex logic between
validate() and configure() when adding support for multiple streams.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 38 +++++++++++++-----------
 1 file changed, 21 insertions(+), 17 deletions(-)

Comments

Paul Elder March 2, 2021, 2 a.m. UTC | #1 
Hi Laurent,

On Mon, Feb 01, 2021 at 12:46:56AM +0200, Laurent Pinchart wrote:
> As the pipeline configuration is selected in
> SimpleCameraConfiguration::validate() already, cache it in the
> SimpleCameraConfiguration instead of looking it up in
> SimplePipelineHandler::configure(). This makes little difference at the
> moment, but will save duplication of more complex logic between
> validate() and configure() when adding support for multiple streams.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 38 +++++++++++++-----------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 72a18cc0b97f..c44fa9ee28ed 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -177,6 +177,11 @@ public:
>  
>  	Status validate() override;
>  
> +	const SimpleCameraData::Configuration *pipeConfig() const
> +	{
> +		return pipeConfig_;
> +	}
> +
>  	bool needConversion() const { return needConversion_; }
>  
>  private:
> @@ -188,6 +193,7 @@ private:
>  	std::shared_ptr<Camera> camera_;
>  	const SimpleCameraData *data_;
>  
> +	const SimpleCameraData::Configuration *pipeConfig_;
>  	bool needConversion_;
>  };
>  
> @@ -506,7 +512,7 @@ int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
>  SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
>  						     SimpleCameraData *data)
>  	: CameraConfiguration(), camera_(camera->shared_from_this()),
> -	  data_(data)
> +	  data_(data), pipeConfig_(nullptr)
>  {
>  }
>  
> @@ -542,17 +548,17 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  		status = Adjusted;
>  	}
>  
> -	const SimpleCameraData::Configuration &pipeConfig = it->second;
> -	if (!pipeConfig.outputSizes.contains(cfg.size)) {
> +	pipeConfig_ = &it->second;
> +	if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>  		LOG(SimplePipeline, Debug)
>  			<< "Adjusting size from " << cfg.size.toString()
> -			<< " to " << pipeConfig.captureSize.toString();
> -		cfg.size = pipeConfig.captureSize;
> +			<< " to " << pipeConfig_->captureSize.toString();
> +		cfg.size = pipeConfig_->captureSize;
>  		status = Adjusted;
>  	}
>  
> -	needConversion_ = cfg.pixelFormat != pipeConfig.captureFormat
> -			|| cfg.size != pipeConfig.captureSize;
> +	needConversion_ = cfg.pixelFormat != pipeConfig_->captureFormat
> +			|| cfg.size != pipeConfig_->captureSize;
>  
>  	cfg.bufferCount = 3;
>  
> @@ -646,21 +652,19 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	if (ret < 0)
>  		return ret;
>  
> -	const SimpleCameraData::Configuration &pipeConfig =
> -		data->formats_[cfg.pixelFormat];
> -
> -	V4L2SubdeviceFormat format{ pipeConfig.code, data->sensor_->resolution() };
> +	const SimpleCameraData::Configuration *pipeConfig = config->pipeConfig();
> +	V4L2SubdeviceFormat format{ pipeConfig->code, data->sensor_->resolution() };
>  
>  	ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
>  	if (ret < 0)
>  		return ret;
>  
>  	/* Configure the video node. */
> -	V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig.captureFormat);
> +	V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig->captureFormat);
>  
>  	V4L2DeviceFormat captureFormat;
>  	captureFormat.fourcc = videoFormat;
> -	captureFormat.size = pipeConfig.captureSize;
> +	captureFormat.size = pipeConfig->captureSize;
>  
>  	ret = video->setFormat(&captureFormat);
>  	if (ret)
> @@ -673,10 +677,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	}
>  
>  	if (captureFormat.fourcc != videoFormat ||
> -	    captureFormat.size != pipeConfig.captureSize) {
> +	    captureFormat.size != pipeConfig->captureSize) {
>  		LOG(SimplePipeline, Error)
>  			<< "Unable to configure capture in "
> -			<< pipeConfig.captureSize.toString() << "-"
> +			<< pipeConfig->captureSize.toString() << "-"
>  			<< videoFormat.toString();
>  		return -EINVAL;
>  	}
> @@ -686,8 +690,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  
>  	if (useConverter_) {
>  		StreamConfiguration inputCfg;
> -		inputCfg.pixelFormat = pipeConfig.captureFormat;
> -		inputCfg.size = pipeConfig.captureSize;
> +		inputCfg.pixelFormat = pipeConfig->captureFormat;
> +		inputCfg.size = pipeConfig->captureSize;
>  		inputCfg.stride = captureFormat.planes[0].bpl;
>  		inputCfg.bufferCount = cfg.bufferCount;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham March 2, 2021, 10:13 a.m. UTC | #2 
On 31/01/2021 22:46, Laurent Pinchart wrote:
> As the pipeline configuration is selected in
> SimpleCameraConfiguration::validate() already, cache it in the
> SimpleCameraConfiguration instead of looking it up in
> SimplePipelineHandler::configure(). This makes little difference at the
> moment, but will save duplication of more complex logic between
> validate() and configure() when adding support for multiple streams.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 38 +++++++++++++-----------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 72a18cc0b97f..c44fa9ee28ed 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -177,6 +177,11 @@ public:
>  
>  	Status validate() override;
>  
> +	const SimpleCameraData::Configuration *pipeConfig() const
> +	{
> +		return pipeConfig_;
> +	}
> +
>  	bool needConversion() const { return needConversion_; }
>  
>  private:
> @@ -188,6 +193,7 @@ private:
>  	std::shared_ptr<Camera> camera_;
>  	const SimpleCameraData *data_;
>  
> +	const SimpleCameraData::Configuration *pipeConfig_;
>  	bool needConversion_;
>  };
>  
> @@ -506,7 +512,7 @@ int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
>  SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
>  						     SimpleCameraData *data)
>  	: CameraConfiguration(), camera_(camera->shared_from_this()),
> -	  data_(data)
> +	  data_(data), pipeConfig_(nullptr)
>  {
>  }
>  
> @@ -542,17 +548,17 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  		status = Adjusted;
>  	}
>  
> -	const SimpleCameraData::Configuration &pipeConfig = it->second;
> -	if (!pipeConfig.outputSizes.contains(cfg.size)) {
> +	pipeConfig_ = &it->second;
> +	if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>  		LOG(SimplePipeline, Debug)
>  			<< "Adjusting size from " << cfg.size.toString()
> -			<< " to " << pipeConfig.captureSize.toString();
> -		cfg.size = pipeConfig.captureSize;
> +			<< " to " << pipeConfig_->captureSize.toString();
> +		cfg.size = pipeConfig_->captureSize;
>  		status = Adjusted;
>  	}
>  
> -	needConversion_ = cfg.pixelFormat != pipeConfig.captureFormat
> -			|| cfg.size != pipeConfig.captureSize;
> +	needConversion_ = cfg.pixelFormat != pipeConfig_->captureFormat
> +			|| cfg.size != pipeConfig_->captureSize;
>  
>  	cfg.bufferCount = 3;
>  
> @@ -646,21 +652,19 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	if (ret < 0)
>  		return ret;
>  
> -	const SimpleCameraData::Configuration &pipeConfig =
> -		data->formats_[cfg.pixelFormat];
> -
> -	V4L2SubdeviceFormat format{ pipeConfig.code, data->sensor_->resolution() };
> +	const SimpleCameraData::Configuration *pipeConfig = config->pipeConfig();
> +	V4L2SubdeviceFormat format{ pipeConfig->code, data->sensor_->resolution() };
>  
>  	ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
>  	if (ret < 0)
>  		return ret;
>  
>  	/* Configure the video node. */
> -	V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig.captureFormat);
> +	V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig->captureFormat);
>  
>  	V4L2DeviceFormat captureFormat;
>  	captureFormat.fourcc = videoFormat;
> -	captureFormat.size = pipeConfig.captureSize;
> +	captureFormat.size = pipeConfig->captureSize;
>  
>  	ret = video->setFormat(&captureFormat);
>  	if (ret)
> @@ -673,10 +677,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	}
>  
>  	if (captureFormat.fourcc != videoFormat ||
> -	    captureFormat.size != pipeConfig.captureSize) {
> +	    captureFormat.size != pipeConfig->captureSize) {
>  		LOG(SimplePipeline, Error)
>  			<< "Unable to configure capture in "
> -			<< pipeConfig.captureSize.toString() << "-"
> +			<< pipeConfig->captureSize.toString() << "-"
>  			<< videoFormat.toString();
>  		return -EINVAL;
>  	}
> @@ -686,8 +690,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  
>  	if (useConverter_) {
>  		StreamConfiguration inputCfg;
> -		inputCfg.pixelFormat = pipeConfig.captureFormat;
> -		inputCfg.size = pipeConfig.captureSize;
> +		inputCfg.pixelFormat = pipeConfig->captureFormat;
> +		inputCfg.size = pipeConfig->captureSize;
>  		inputCfg.stride = captureFormat.planes[0].bpl;
>  		inputCfg.bufferCount = cfg.bufferCount;
>  
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 72a18cc0b97f..c44fa9ee28ed 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -177,6 +177,11 @@  public:
 
 	Status validate() override;
 
+	const SimpleCameraData::Configuration *pipeConfig() const
+	{
+		return pipeConfig_;
+	}
+
 	bool needConversion() const { return needConversion_; }
 
 private:
@@ -188,6 +193,7 @@  private:
 	std::shared_ptr<Camera> camera_;
 	const SimpleCameraData *data_;
 
+	const SimpleCameraData::Configuration *pipeConfig_;
 	bool needConversion_;
 };
 
@@ -506,7 +512,7 @@  int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
 SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
 						     SimpleCameraData *data)
 	: CameraConfiguration(), camera_(camera->shared_from_this()),
-	  data_(data)
+	  data_(data), pipeConfig_(nullptr)
 {
 }
 
@@ -542,17 +548,17 @@  CameraConfiguration::Status SimpleCameraConfiguration::validate()
 		status = Adjusted;
 	}
 
-	const SimpleCameraData::Configuration &pipeConfig = it->second;
-	if (!pipeConfig.outputSizes.contains(cfg.size)) {
+	pipeConfig_ = &it->second;
+	if (!pipeConfig_->outputSizes.contains(cfg.size)) {
 		LOG(SimplePipeline, Debug)
 			<< "Adjusting size from " << cfg.size.toString()
-			<< " to " << pipeConfig.captureSize.toString();
-		cfg.size = pipeConfig.captureSize;
+			<< " to " << pipeConfig_->captureSize.toString();
+		cfg.size = pipeConfig_->captureSize;
 		status = Adjusted;
 	}
 
-	needConversion_ = cfg.pixelFormat != pipeConfig.captureFormat
-			|| cfg.size != pipeConfig.captureSize;
+	needConversion_ = cfg.pixelFormat != pipeConfig_->captureFormat
+			|| cfg.size != pipeConfig_->captureSize;
 
 	cfg.bufferCount = 3;
 
@@ -646,21 +652,19 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 	if (ret < 0)
 		return ret;
 
-	const SimpleCameraData::Configuration &pipeConfig =
-		data->formats_[cfg.pixelFormat];
-
-	V4L2SubdeviceFormat format{ pipeConfig.code, data->sensor_->resolution() };
+	const SimpleCameraData::Configuration *pipeConfig = config->pipeConfig();
+	V4L2SubdeviceFormat format{ pipeConfig->code, data->sensor_->resolution() };
 
 	ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
 	if (ret < 0)
 		return ret;
 
 	/* Configure the video node. */
-	V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig.captureFormat);
+	V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig->captureFormat);
 
 	V4L2DeviceFormat captureFormat;
 	captureFormat.fourcc = videoFormat;
-	captureFormat.size = pipeConfig.captureSize;
+	captureFormat.size = pipeConfig->captureSize;
 
 	ret = video->setFormat(&captureFormat);
 	if (ret)
@@ -673,10 +677,10 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 	}
 
 	if (captureFormat.fourcc != videoFormat ||
-	    captureFormat.size != pipeConfig.captureSize) {
+	    captureFormat.size != pipeConfig->captureSize) {
 		LOG(SimplePipeline, Error)
 			<< "Unable to configure capture in "
-			<< pipeConfig.captureSize.toString() << "-"
+			<< pipeConfig->captureSize.toString() << "-"
 			<< videoFormat.toString();
 		return -EINVAL;
 	}
@@ -686,8 +690,8 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 
 	if (useConverter_) {
 		StreamConfiguration inputCfg;
-		inputCfg.pixelFormat = pipeConfig.captureFormat;
-		inputCfg.size = pipeConfig.captureSize;
+		inputCfg.pixelFormat = pipeConfig->captureFormat;
+		inputCfg.size = pipeConfig->captureSize;
 		inputCfg.stride = captureFormat.planes[0].bpl;
 		inputCfg.bufferCount = cfg.bufferCount;