[libcamera-devel] [PATCH 24/30] cam: Move printing of camera information to CameraSession class

[Laurent Pinchart]

Hi Kieran,

On Mon, Jul 12, 2021 at 04:56:34PM +0100, Kieran Bingham wrote:
> On 07/07/2021 03:19, Laurent Pinchart wrote:
> > The three CamApp functions listControls(), listProperties() and
> > infoConfiguration() operate on a camera. They would thus be better
> > placed in the CameraSession class. Move them there. As they now have no
> > error to return anymore, make them void functions.
> 
> Makes me wonder if the arg parsing for those shouldn't be handled by the
> CameraSession - but I don't know if that's practical, and this is fine.

I've thought about it, but then decided that a function named
listControls() should list controls unconditionally. I've actually
thought about moving arg parsing (or rather interpretation) out of
CameraSession for other arguments, but that became quite impractical.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/cam/camera_session.cpp |  45 +++++++++++++++++
> >  src/cam/camera_session.h   |   4 ++
> >  src/cam/main.cpp           | 101 ++++++++-----------------------------
> >  3 files changed, 70 insertions(+), 80 deletions(-)
> > 
> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > index 10e66446ee67..ceb2c3ab3a92 100644
> > --- a/src/cam/camera_session.cpp
> > +++ b/src/cam/camera_session.cpp
> > @@ -11,6 +11,7 @@
> >  #include <sstream>
> >  
> >  #include <libcamera/control_ids.h>
> > +#include <libcamera/property_ids.h>
> >  
> >  #include "camera_session.h"
> >  #include "event_loop.h"
> > @@ -89,6 +90,50 @@ CameraSession::~CameraSession()
> >  		camera_->release();
> >  }
> >  
> > +void CameraSession::listControls() const
> > +{
> > +	for (const auto &ctrl : camera_->controls()) {
> > +		const ControlId *id = ctrl.first;
> > +		const ControlInfo &info = ctrl.second;
> > +
> > +		std::cout << "Control: " << id->name() << ": "
> > +			  << info.toString() << std::endl;
> > +	}
> > +}
> > +
> > +void CameraSession::listProperties() const
> > +{
> > +	for (const auto &prop : camera_->properties()) {
> > +		const ControlId *id = properties::properties.at(prop.first);
> > +		const ControlValue &value = prop.second;
> > +
> > +		std::cout << "Property: " << id->name() << " = "
> > +			  << value.toString() << std::endl;
> > +	}
> > +}
> > +
> > +void CameraSession::infoConfiguration() const
> > +{
> > +	unsigned int index = 0;
> > +	for (const StreamConfiguration &cfg : *config_) {
> > +		std::cout << index << ": " << cfg.toString() << std::endl;
> > +
> > +		const StreamFormats &formats = cfg.formats();
> > +		for (PixelFormat pixelformat : formats.pixelformats()) {
> > +			std::cout << " * Pixelformat: "
> > +				  << pixelformat.toString() << " "
> > +				  << formats.range(pixelformat).toString()
> > +				  << std::endl;
> > +
> > +			for (const Size &size : formats.sizes(pixelformat))
> > +				std::cout << "  - " << size.toString()
> > +					  << std::endl;
> > +		}
> > +
> > +		index++;
> > +	}
> > +}
> > +
> >  int CameraSession::start(const OptionsParser::Options &options)
> >  {
> >  	int ret;
> > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h
> > index 88baf9061629..6221aadadf90 100644
> > --- a/src/cam/camera_session.h
> > +++ b/src/cam/camera_session.h
> > @@ -35,6 +35,10 @@ public:
> >  	libcamera::Camera *camera() { return camera_.get(); }
> >  	libcamera::CameraConfiguration *config() { return config_.get(); }
> >  
> > +	void listControls() const;
> > +	void listProperties() const;
> > +	void infoConfiguration() const;
> > +
> >  	int start(const OptionsParser::Options &options);
> >  	void stop();
> >  
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index cba0793ac39b..4fe0c4c368d1 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -39,9 +39,6 @@ private:
> >  	void cameraRemoved(std::shared_ptr<Camera> cam);
> >  	void captureDone();
> >  	int parseOptions(int argc, char *argv[]);
> > -	int listControls();
> > -	int listProperties();
> > -	int infoConfiguration();
> >  	int run();
> >  
> >  	static std::string cameraName(const Camera *camera);
> > @@ -158,74 +155,6 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  	return 0;
> >  }
> >  
> > -int CamApp::listControls()
> > -{
> > -	if (!session_) {
> > -		std::cout << "Cannot list controls without a camera"
> > -			  << std::endl;
> > -		return -EINVAL;
> > -	}
> > -
> > -	for (const auto &ctrl : session_->camera()->controls()) {
> > -		const ControlId *id = ctrl.first;
> > -		const ControlInfo &info = ctrl.second;
> > -
> > -		std::cout << "Control: " << id->name() << ": "
> > -			  << info.toString() << std::endl;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -int CamApp::listProperties()
> > -{
> > -	if (!session_) {
> > -		std::cout << "Cannot list properties without a camera"
> > -			  << std::endl;
> > -		return -EINVAL;
> > -	}
> > -
> > -	for (const auto &prop : session_->camera()->properties()) {
> > -		const ControlId *id = properties::properties.at(prop.first);
> > -		const ControlValue &value = prop.second;
> > -
> > -		std::cout << "Property: " << id->name() << " = "
> > -			  << value.toString() << std::endl;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -int CamApp::infoConfiguration()
> > -{
> > -	if (!session_) {
> > -		std::cout << "Cannot print stream information without a camera"
> > -			  << std::endl;
> > -		return -EINVAL;
> > -	}
> > -
> > -	unsigned int index = 0;
> > -	for (const StreamConfiguration &cfg : *session_->config()) {
> > -		std::cout << index << ": " << cfg.toString() << std::endl;
> > -
> > -		const StreamFormats &formats = cfg.formats();
> > -		for (PixelFormat pixelformat : formats.pixelformats()) {
> > -			std::cout << " * Pixelformat: "
> > -				  << pixelformat.toString() << " "
> > -				  << formats.range(pixelformat).toString()
> > -				  << std::endl;
> > -
> > -			for (const Size &size : formats.sizes(pixelformat))
> > -				std::cout << "  - " << size.toString()
> > -					  << std::endl;
> > -		}
> > -
> > -		index++;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >  void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> >  {
> >  	std::cout << "Camera Added: " << cam->id() << std::endl;
> > @@ -269,21 +198,33 @@ int CamApp::run()
> >  	}
> >  
> >  	if (options_.isSet(OptListControls)) {
> > -		ret = listControls();
> > -		if (ret)
> > -			return ret;
> > +		if (!session_) {
> > +			std::cout << "Cannot list controls without a camera"
> > +				  << std::endl;
> > +			return -EINVAL;
> > +		}
> > +
> > +		session_->listControls();
> >  	}
> >  
> >  	if (options_.isSet(OptListProperties)) {
> > -		ret = listProperties();
> > -		if (ret)
> > -			return ret;
> > +		if (!session_) {
> > +			std::cout << "Cannot list properties without a camera"
> > +				  << std::endl;
> > +			return -EINVAL;
> > +		}
> > +
> > +		session_->listProperties();
> >  	}
> >  
> >  	if (options_.isSet(OptInfo)) {
> > -		ret = infoConfiguration();
> > -		if (ret)
> > -			return ret;
> > +		if (!session_) {
> > +			std::cout << "Cannot print stream information without a camera"
> > +				  << std::endl;
> > +			return -EINVAL;
> > +		}
> > +
> > +		session_->infoConfiguration();
> >  	}
> >  
> >  	if (options_.isSet(OptCapture)) {

-- 
Regards,

Laurent Pinchart