[libcamera-devel] [PATCH 19/30] cam: Move camera acquire to the CameraSession class

[Kieran Bingham]

On 07/07/2021 03:19, Laurent Pinchart wrote:
> Continue moving towards making the CameraSession class the central point
> to handle a camera by moving the camera acquire operation. A new
> CameraSession::camera() function is needed to allow access to the camera
> from CamApp.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/cam/camera_session.cpp | 29 ++++++++++++++++++++--
>  src/cam/camera_session.h   |  6 ++++-
>  src/cam/main.cpp           | 49 +++++++++-----------------------------
>  3 files changed, 43 insertions(+), 41 deletions(-)
> 
> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index edc7205d6bda..10e66446ee67 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -19,11 +19,30 @@
>  
>  using namespace libcamera;
>  
> -CameraSession::CameraSession(std::shared_ptr<Camera> camera,
> +CameraSession::CameraSession(CameraManager *cm,
>  			     const OptionsParser::Options &options)
> -	: camera_(camera), last_(0), queueCount_(0), captureCount_(0),
> +	: last_(0), queueCount_(0), captureCount_(0),
>  	  captureLimit_(0), printMetadata_(false)

Well I certainly like seeing initialiser lists shrink ;-)


>  {
> +	const std::string &cameraId = options[OptCamera];
> +	char *endptr;
> +	unsigned long index = strtoul(cameraId.c_str(), &endptr, 10);
> +	if (*endptr == '\0' && index > 0 && index <= cm->cameras().size())
> +		camera_ = cm->cameras()[index - 1];
> +	else
> +		camera_ = cm->get(cameraId);
> +
> +	if (!camera_) {
> +		std::cerr << "Camera " << cameraId << " not found" << std::endl;
> +		return;
> +	}
> +
> +	if (camera_->acquire()) {
> +		std::cerr << "Failed to acquire camera " << cameraId
> +			  << std::endl;
> +		return;
> +	}
> +
>  	StreamRoles roles = StreamKeyValueParser::roles(options[OptStream]);
>  
>  	std::unique_ptr<CameraConfiguration> config =
> @@ -64,6 +83,12 @@ CameraSession::CameraSession(std::shared_ptr<Camera> camera,
>  	config_ = std::move(config);
>  }
>  
> +CameraSession::~CameraSession()
> +{
> +	if (camera_)
> +		camera_->release();

Makes me wonder if we should we wrap a shared_ptr<camera> with a
unique_ptr so that it automatically releases ?

And I can't quite tell where the sarcasm stops or the ... why we might
need that starts.... eeek


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

> +}
> +
>  int CameraSession::start(const OptionsParser::Options &options)
>  {
>  	int ret;
> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h
> index 39dbbdf37913..88baf9061629 100644
> --- a/src/cam/camera_session.h
> +++ b/src/cam/camera_session.h
> @@ -14,6 +14,7 @@
>  #include <libcamera/base/signal.h>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/request.h>
> @@ -25,10 +26,13 @@
>  class CameraSession
>  {
>  public:
> -	CameraSession(std::shared_ptr<libcamera::Camera> camera,
> +	CameraSession(libcamera::CameraManager *cm,
>  		      const OptionsParser::Options &options);
> +	~CameraSession();
>  
>  	bool isValid() const { return config_ != nullptr; }
> +
> +	libcamera::Camera *camera() { return camera_.get(); }
>  	libcamera::CameraConfiguration *config() { return config_.get(); }
>  
>  	int start(const OptionsParser::Options &options);
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 082a053efeda..8ed5e841a1e9 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -51,7 +51,6 @@ private:
>  	OptionsParser::Options options_;
>  	CameraManager *cm_;
>  
> -	std::shared_ptr<Camera> camera_;
>  	std::unique_ptr<CameraSession> session_;
>  
>  	EventLoop loop_;
> @@ -60,7 +59,7 @@ private:
>  CamApp *CamApp::app_ = nullptr;
>  
>  CamApp::CamApp()
> -	: cm_(nullptr), camera_(nullptr)
> +	: cm_(nullptr)
>  {
>  	CamApp::app_ = this;
>  }
> @@ -93,37 +92,16 @@ int CamApp::init(int argc, char **argv)
>  	}
>  
>  	if (options_.isSet(OptCamera)) {
> -		const std::string &cameraId = options_[OptCamera];
> -		char *endptr;
> -		unsigned long index = strtoul(cameraId.c_str(), &endptr, 10);
> -		if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size())
> -			camera_ = cm_->cameras()[index - 1];
> -		else
> -			camera_ = cm_->get(cameraId);
> -
> -		if (!camera_) {
> -			std::cout << "Camera "
> -				  << std::string(options_[OptCamera])
> -				  << " not found" << std::endl;
> -			cleanup();
> -			return -ENODEV;
> -		}
> -
> -		if (camera_->acquire()) {
> -			std::cout << "Failed to acquire camera" << std::endl;
> -			cleanup();
> -			return -EINVAL;
> -		}
> -
> -		std::cout << "Using camera " << camera_->id() << std::endl;
> -
> -		session_ = std::make_unique<CameraSession>(camera_, options_);
> +		session_ = std::make_unique<CameraSession>(cm_, options_);
>  		if (!session_->isValid()) {
>  			std::cout << "Failed to create camera session" << std::endl;
>  			cleanup();
>  			return -EINVAL;
>  		}
>  
> +		std::cout << "Using camera " << session_->camera()->id()
> +			  << std::endl;
> +
>  		session_->captureDone.connect(this, &CamApp::captureDone);
>  	}
>  
> @@ -138,11 +116,6 @@ int CamApp::init(int argc, char **argv)
>  
>  void CamApp::cleanup()
>  {
> -	if (camera_) {
> -		camera_->release();
> -		camera_.reset();
> -	}
> -
>  	session_.reset();
>  
>  	cm_->stop();
> @@ -214,13 +187,13 @@ int CamApp::parseOptions(int argc, char *argv[])
>  
>  int CamApp::listControls()
>  {
> -	if (!camera_) {
> +	if (!session_) {
>  		std::cout << "Cannot list controls without a camera"
>  			  << std::endl;
>  		return -EINVAL;
>  	}
>  
> -	for (const auto &ctrl : camera_->controls()) {
> +	for (const auto &ctrl : session_->camera()->controls()) {
>  		const ControlId *id = ctrl.first;
>  		const ControlInfo &info = ctrl.second;
>  
> @@ -233,13 +206,13 @@ int CamApp::listControls()
>  
>  int CamApp::listProperties()
>  {
> -	if (!camera_) {
> +	if (!session_) {
>  		std::cout << "Cannot list properties without a camera"
>  			  << std::endl;
>  		return -EINVAL;
>  	}
>  
> -	for (const auto &prop : camera_->properties()) {
> +	for (const auto &prop : session_->camera()->properties()) {
>  		const ControlId *id = properties::properties.at(prop.first);
>  		const ControlValue &value = prop.second;
>  
> @@ -252,7 +225,7 @@ int CamApp::listProperties()
>  
>  int CamApp::infoConfiguration()
>  {
> -	if (!camera_) {
> +	if (!session_) {
>  		std::cout << "Cannot print stream information without a camera"
>  			  << std::endl;
>  		return -EINVAL;
> @@ -328,7 +301,7 @@ int CamApp::run()
>  	}
>  
>  	if (options_.isSet(OptCapture)) {
> -		if (!camera_) {
> +		if (!session_) {
>  			std::cout << "Can't capture without a camera" << std::endl;
>  			return -ENODEV;
>  		}
>