[libcamera-devel,18/30] cam: Move CameraConfiguration creation to CameraSession class
diff mbox series

Message ID 20210707021941.20804-19-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Multi-camera support in the cam application
Related show

Commit Message

Laurent Pinchart July 7, 2021, 2:19 a.m. UTC 
Creating a configuration for a camera is an operation that logically
belongs to the CameraSession class. Move it there.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/cam/camera_session.cpp | 47 +++++++++++++++++++++++++---
 src/cam/camera_session.h   |  7 +++--
 src/cam/main.cpp           | 63 +++++---------------------------------
 3 files changed, 56 insertions(+), 61 deletions(-)

Comments

Kieran Bingham July 12, 2021, 3:37 p.m. UTC | #1 
On 07/07/2021 03:19, Laurent Pinchart wrote:
> Creating a configuration for a camera is an operation that logically
> belongs to the CameraSession class. Move it there.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

> ---
>  src/cam/camera_session.cpp | 47 +++++++++++++++++++++++++---
>  src/cam/camera_session.h   |  7 +++--
>  src/cam/main.cpp           | 63 +++++---------------------------------
>  3 files changed, 56 insertions(+), 61 deletions(-)
> 
> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index 7bb75e86eab5..edc7205d6bda 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -15,14 +15,53 @@
>  #include "camera_session.h"
>  #include "event_loop.h"
>  #include "main.h"
> +#include "stream_options.h"
>  
>  using namespace libcamera;
>  
>  CameraSession::CameraSession(std::shared_ptr<Camera> camera,
> -			     CameraConfiguration *config)
> -	: camera_(camera), config_(config), last_(0), queueCount_(0),
> -	  captureCount_(0), captureLimit_(0), printMetadata_(false)
> +			     const OptionsParser::Options &options)
> +	: camera_(camera), last_(0), queueCount_(0), captureCount_(0),
> +	  captureLimit_(0), printMetadata_(false)
>  {
> +	StreamRoles roles = StreamKeyValueParser::roles(options[OptStream]);
> +
> +	std::unique_ptr<CameraConfiguration> config =
> +		camera_->generateConfiguration(roles);
> +	if (!config || config->size() != roles.size()) {
> +		std::cerr << "Failed to get default stream configuration"
> +			  << std::endl;
> +		return;
> +	}
> +
> +	/* Apply configuration if explicitly requested. */
> +	if (StreamKeyValueParser::updateConfiguration(config.get(),
> +						      options[OptStream])) {
> +		std::cerr << "Failed to update configuration" << std::endl;
> +		return;
> +	}
> +
> +	bool strictFormats = options.isSet(OptStrictFormats);
> +
> +	switch (config->validate()) {
> +	case CameraConfiguration::Valid:
> +		break;
> +
> +	case CameraConfiguration::Adjusted:
> +		if (strictFormats) {
> +			std::cout << "Adjusting camera configuration disallowed by --strict-formats argument"
> +				  << std::endl;
> +			return;
> +		}
> +		std::cout << "Camera configuration adjusted" << std::endl;
> +		break;
> +
> +	case CameraConfiguration::Invalid:
> +		std::cout << "Camera configuration invalid" << std::endl;
> +		return;
> +	}
> +
> +	config_ = std::move(config);
>  }
>  
>  int CameraSession::start(const OptionsParser::Options &options)
> @@ -34,7 +73,7 @@ int CameraSession::start(const OptionsParser::Options &options)
>  	captureLimit_ = options[OptCapture].toInteger();
>  	printMetadata_ = options.isSet(OptMetadata);
>  
> -	ret = camera_->configure(config_);
> +	ret = camera_->configure(config_.get());
>  	if (ret < 0) {
>  		std::cout << "Failed to configure camera" << std::endl;
>  		return ret;
> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h
> index 31e8d6db01d9..39dbbdf37913 100644
> --- a/src/cam/camera_session.h
> +++ b/src/cam/camera_session.h
> @@ -26,7 +26,10 @@ class CameraSession
>  {
>  public:
>  	CameraSession(std::shared_ptr<libcamera::Camera> camera,
> -		      libcamera::CameraConfiguration *config);
> +		      const OptionsParser::Options &options);
> +
> +	bool isValid() const { return config_ != nullptr; }
> +	libcamera::CameraConfiguration *config() { return config_.get(); }
>  
>  	int start(const OptionsParser::Options &options);
>  	void stop();
> @@ -41,7 +44,7 @@ private:
>  	void processRequest(libcamera::Request *request);
>  
>  	std::shared_ptr<libcamera::Camera> camera_;
> -	libcamera::CameraConfiguration *config_;
> +	std::unique_ptr<libcamera::CameraConfiguration> config_;
>  
>  	std::map<const libcamera::Stream *, std::string> streamName_;
>  	std::unique_ptr<BufferWriter> writer_;
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 2c6500a5682b..082a053efeda 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -40,7 +40,6 @@ private:
>  	void cameraRemoved(std::shared_ptr<Camera> cam);
>  	void captureDone();
>  	int parseOptions(int argc, char *argv[]);
> -	int prepareConfig();
>  	int listControls();
>  	int listProperties();
>  	int infoConfiguration();
> @@ -53,19 +52,15 @@ private:
>  	CameraManager *cm_;
>  
>  	std::shared_ptr<Camera> camera_;
> -	std::unique_ptr<libcamera::CameraConfiguration> config_;
>  	std::unique_ptr<CameraSession> session_;
>  
>  	EventLoop loop_;
> -
> -	bool strictFormats_;
>  };
>  
>  CamApp *CamApp::app_ = nullptr;
>  
>  CamApp::CamApp()
> -	: cm_(nullptr), camera_(nullptr), config_(nullptr),
> -	  strictFormats_(false)
> +	: cm_(nullptr), camera_(nullptr)
>  {
>  	CamApp::app_ = this;
>  }
> @@ -88,9 +83,6 @@ int CamApp::init(int argc, char **argv)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (options_.isSet(OptStrictFormats))
> -		strictFormats_ = true;
> -
>  	cm_ = new CameraManager();
>  
>  	ret = cm_->start();
> @@ -125,13 +117,13 @@ int CamApp::init(int argc, char **argv)
>  
>  		std::cout << "Using camera " << camera_->id() << std::endl;
>  
> -		ret = prepareConfig();
> -		if (ret) {
> +		session_ = std::make_unique<CameraSession>(camera_, options_);
> +		if (!session_->isValid()) {
> +			std::cout << "Failed to create camera session" << std::endl;
>  			cleanup();
> -			return ret;
> +			return -EINVAL;
>  		}
>  
> -		session_ = std::make_unique<CameraSession>(camera_, config_.get());
>  		session_->captureDone.connect(this, &CamApp::captureDone);
>  	}
>  
> @@ -151,7 +143,7 @@ void CamApp::cleanup()
>  		camera_.reset();
>  	}
>  
> -	config_.reset();
> +	session_.reset();
>  
>  	cm_->stop();
>  }
> @@ -220,45 +212,6 @@ int CamApp::parseOptions(int argc, char *argv[])
>  	return 0;
>  }
>  
> -int CamApp::prepareConfig()
> -{
> -	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> -
> -	config_ = camera_->generateConfiguration(roles);
> -	if (!config_ || config_->size() != roles.size()) {
> -		std::cerr << "Failed to get default stream configuration"
> -			  << std::endl;
> -		return -EINVAL;
> -	}
> -
> -	/* Apply configuration if explicitly requested. */
> -	if (StreamKeyValueParser::updateConfiguration(config_.get(),
> -						      options_[OptStream])) {
> -		std::cerr << "Failed to update configuration" << std::endl;
> -		return -EINVAL;
> -	}
> -
> -	switch (config_->validate()) {
> -	case CameraConfiguration::Valid:
> -		break;
> -	case CameraConfiguration::Adjusted:
> -		if (strictFormats_) {
> -			std::cout << "Adjusting camera configuration disallowed by --strict-formats argument"
> -				  << std::endl;
> -			config_.reset();
> -			return -EINVAL;
> -		}
> -		std::cout << "Camera configuration adjusted" << std::endl;
> -		break;
> -	case CameraConfiguration::Invalid:
> -		std::cout << "Camera configuration invalid" << std::endl;
> -		config_.reset();
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
>  int CamApp::listControls()
>  {
>  	if (!camera_) {
> @@ -299,14 +252,14 @@ int CamApp::listProperties()
>  
>  int CamApp::infoConfiguration()
>  {
> -	if (!config_) {
> +	if (!camera_) {
>  		std::cout << "Cannot print stream information without a camera"
>  			  << std::endl;
>  		return -EINVAL;
>  	}
>  
>  	unsigned int index = 0;
> -	for (const StreamConfiguration &cfg : *config_) {
> +	for (const StreamConfiguration &cfg : *session_->config()) {
>  		std::cout << index << ": " << cfg.toString() << std::endl;
>  
>  		const StreamFormats &formats = cfg.formats();
>

Patch
diff mbox series 

diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
index 7bb75e86eab5..edc7205d6bda 100644
--- a/src/cam/camera_session.cpp
+++ b/src/cam/camera_session.cpp
@@ -15,14 +15,53 @@ 
 #include "camera_session.h"
 #include "event_loop.h"
 #include "main.h"
+#include "stream_options.h"
 
 using namespace libcamera;
 
 CameraSession::CameraSession(std::shared_ptr<Camera> camera,
-			     CameraConfiguration *config)
-	: camera_(camera), config_(config), last_(0), queueCount_(0),
-	  captureCount_(0), captureLimit_(0), printMetadata_(false)
+			     const OptionsParser::Options &options)
+	: camera_(camera), last_(0), queueCount_(0), captureCount_(0),
+	  captureLimit_(0), printMetadata_(false)
 {
+	StreamRoles roles = StreamKeyValueParser::roles(options[OptStream]);
+
+	std::unique_ptr<CameraConfiguration> config =
+		camera_->generateConfiguration(roles);
+	if (!config || config->size() != roles.size()) {
+		std::cerr << "Failed to get default stream configuration"
+			  << std::endl;
+		return;
+	}
+
+	/* Apply configuration if explicitly requested. */
+	if (StreamKeyValueParser::updateConfiguration(config.get(),
+						      options[OptStream])) {
+		std::cerr << "Failed to update configuration" << std::endl;
+		return;
+	}
+
+	bool strictFormats = options.isSet(OptStrictFormats);
+
+	switch (config->validate()) {
+	case CameraConfiguration::Valid:
+		break;
+
+	case CameraConfiguration::Adjusted:
+		if (strictFormats) {
+			std::cout << "Adjusting camera configuration disallowed by --strict-formats argument"
+				  << std::endl;
+			return;
+		}
+		std::cout << "Camera configuration adjusted" << std::endl;
+		break;
+
+	case CameraConfiguration::Invalid:
+		std::cout << "Camera configuration invalid" << std::endl;
+		return;
+	}
+
+	config_ = std::move(config);
 }
 
 int CameraSession::start(const OptionsParser::Options &options)
@@ -34,7 +73,7 @@  int CameraSession::start(const OptionsParser::Options &options)
 	captureLimit_ = options[OptCapture].toInteger();
 	printMetadata_ = options.isSet(OptMetadata);
 
-	ret = camera_->configure(config_);
+	ret = camera_->configure(config_.get());
 	if (ret < 0) {
 		std::cout << "Failed to configure camera" << std::endl;
 		return ret;
diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h
index 31e8d6db01d9..39dbbdf37913 100644
--- a/src/cam/camera_session.h
+++ b/src/cam/camera_session.h
@@ -26,7 +26,10 @@  class CameraSession
 {
 public:
 	CameraSession(std::shared_ptr<libcamera::Camera> camera,
-		      libcamera::CameraConfiguration *config);
+		      const OptionsParser::Options &options);
+
+	bool isValid() const { return config_ != nullptr; }
+	libcamera::CameraConfiguration *config() { return config_.get(); }
 
 	int start(const OptionsParser::Options &options);
 	void stop();
@@ -41,7 +44,7 @@  private:
 	void processRequest(libcamera::Request *request);
 
 	std::shared_ptr<libcamera::Camera> camera_;
-	libcamera::CameraConfiguration *config_;
+	std::unique_ptr<libcamera::CameraConfiguration> config_;
 
 	std::map<const libcamera::Stream *, std::string> streamName_;
 	std::unique_ptr<BufferWriter> writer_;
diff --git a/src/cam/main.cpp b/src/cam/main.cpp
index 2c6500a5682b..082a053efeda 100644
--- a/src/cam/main.cpp
+++ b/src/cam/main.cpp
@@ -40,7 +40,6 @@  private:
 	void cameraRemoved(std::shared_ptr<Camera> cam);
 	void captureDone();
 	int parseOptions(int argc, char *argv[]);
-	int prepareConfig();
 	int listControls();
 	int listProperties();
 	int infoConfiguration();
@@ -53,19 +52,15 @@  private:
 	CameraManager *cm_;
 
 	std::shared_ptr<Camera> camera_;
-	std::unique_ptr<libcamera::CameraConfiguration> config_;
 	std::unique_ptr<CameraSession> session_;
 
 	EventLoop loop_;
-
-	bool strictFormats_;
 };
 
 CamApp *CamApp::app_ = nullptr;
 
 CamApp::CamApp()
-	: cm_(nullptr), camera_(nullptr), config_(nullptr),
-	  strictFormats_(false)
+	: cm_(nullptr), camera_(nullptr)
 {
 	CamApp::app_ = this;
 }
@@ -88,9 +83,6 @@  int CamApp::init(int argc, char **argv)
 	if (ret < 0)
 		return ret;
 
-	if (options_.isSet(OptStrictFormats))
-		strictFormats_ = true;
-
 	cm_ = new CameraManager();
 
 	ret = cm_->start();
@@ -125,13 +117,13 @@  int CamApp::init(int argc, char **argv)
 
 		std::cout << "Using camera " << camera_->id() << std::endl;
 
-		ret = prepareConfig();
-		if (ret) {
+		session_ = std::make_unique<CameraSession>(camera_, options_);
+		if (!session_->isValid()) {
+			std::cout << "Failed to create camera session" << std::endl;
 			cleanup();
-			return ret;
+			return -EINVAL;
 		}
 
-		session_ = std::make_unique<CameraSession>(camera_, config_.get());
 		session_->captureDone.connect(this, &CamApp::captureDone);
 	}
 
@@ -151,7 +143,7 @@  void CamApp::cleanup()
 		camera_.reset();
 	}
 
-	config_.reset();
+	session_.reset();
 
 	cm_->stop();
 }
@@ -220,45 +212,6 @@  int CamApp::parseOptions(int argc, char *argv[])
 	return 0;
 }
 
-int CamApp::prepareConfig()
-{
-	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
-
-	config_ = camera_->generateConfiguration(roles);
-	if (!config_ || config_->size() != roles.size()) {
-		std::cerr << "Failed to get default stream configuration"
-			  << std::endl;
-		return -EINVAL;
-	}
-
-	/* Apply configuration if explicitly requested. */
-	if (StreamKeyValueParser::updateConfiguration(config_.get(),
-						      options_[OptStream])) {
-		std::cerr << "Failed to update configuration" << std::endl;
-		return -EINVAL;
-	}
-
-	switch (config_->validate()) {
-	case CameraConfiguration::Valid:
-		break;
-	case CameraConfiguration::Adjusted:
-		if (strictFormats_) {
-			std::cout << "Adjusting camera configuration disallowed by --strict-formats argument"
-				  << std::endl;
-			config_.reset();
-			return -EINVAL;
-		}
-		std::cout << "Camera configuration adjusted" << std::endl;
-		break;
-	case CameraConfiguration::Invalid:
-		std::cout << "Camera configuration invalid" << std::endl;
-		config_.reset();
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 int CamApp::listControls()
 {
 	if (!camera_) {
@@ -299,14 +252,14 @@  int CamApp::listProperties()
 
 int CamApp::infoConfiguration()
 {
-	if (!config_) {
+	if (!camera_) {
 		std::cout << "Cannot print stream information without a camera"
 			  << std::endl;
 		return -EINVAL;
 	}
 
 	unsigned int index = 0;
-	for (const StreamConfiguration &cfg : *config_) {
+	for (const StreamConfiguration &cfg : *session_->config()) {
 		std::cout << index << ": " << cfg.toString() << std::endl;
 
 		const StreamFormats &formats = cfg.formats();