[libcamera-devel] [PATCH 1/8] cam: Pass stream roles to Capture class

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2020-05-19 06:24:58 +0300, Laurent Pinchart wrote:
> The stream roles will be needed in the Capture class to verify the
> configuration. Store they in the CamApp class and pass them to the
> Capture class constructor.

The patch itself is fine but I wonder if this is the right way. We have 
previously endeavored to limit the spread of the roles past the 
generateConfiguration() as they somewhat lose there meaning after that, 
right?

Even if the user requested a viewfinder and video stream they are only 
hints to the pipeline hander is "free" to return any type of camera 
configuration, even one with only 1 stream even if more where requested 
in the role hints.

For this reason I wonder if instead the check for the combination of the 
new option -D and that the stream role hint should be set to viewfinder 
should be moved and be validated right after we have parsed the command 
line options?

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/cam/capture.cpp | 5 +++--
>  src/cam/capture.h   | 4 +++-
>  src/cam/main.cpp    | 9 +++++----
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 55fa2dabcee9..b7e06bcc9463 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -16,8 +16,9 @@
>  
>  using namespace libcamera;
>  
> -Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config)
> -	: camera_(camera), config_(config), writer_(nullptr)
> +Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config,
> +		 const StreamRoles &roles)
> +	: camera_(camera), config_(config), roles_(roles), writer_(nullptr)
>  {
>  }
>  
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> index 9bca5661070e..c0e697b831fb 100644
> --- a/src/cam/capture.h
> +++ b/src/cam/capture.h
> @@ -24,7 +24,8 @@ class Capture
>  {
>  public:
>  	Capture(std::shared_ptr<libcamera::Camera> camera,
> -		libcamera::CameraConfiguration *config);
> +		libcamera::CameraConfiguration *config,
> +		const libcamera::StreamRoles &roles);
>  
>  	int run(EventLoop *loop, const OptionsParser::Options &options);
>  private:
> @@ -35,6 +36,7 @@ private:
>  
>  	std::shared_ptr<libcamera::Camera> camera_;
>  	libcamera::CameraConfiguration *config_;
> +	libcamera::StreamRoles roles_;
>  
>  	std::map<libcamera::Stream *, std::string> streamName_;
>  	BufferWriter *writer_;
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 2512fe9da782..cdd29d500202 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -47,6 +47,7 @@ private:
>  	OptionsParser::Options options_;
>  	CameraManager *cm_;
>  	std::shared_ptr<Camera> camera_;
> +	StreamRoles roles_;
>  	std::unique_ptr<libcamera::CameraConfiguration> config_;
>  	EventLoop *loop_;
>  };
> @@ -194,10 +195,10 @@ int CamApp::parseOptions(int argc, char *argv[])
>  
>  int CamApp::prepareConfig()
>  {
> -	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> +	roles_ = StreamKeyValueParser::roles(options_[OptStream]);
>  
> -	config_ = camera_->generateConfiguration(roles);
> -	if (!config_ || config_->size() != roles.size()) {
> +	config_ = camera_->generateConfiguration(roles_);
> +	if (!config_ || config_->size() != roles_.size()) {
>  		std::cerr << "Failed to get default stream configuration"
>  			  << std::endl;
>  		return -EINVAL;
> @@ -326,7 +327,7 @@ int CamApp::run()
>  	}
>  
>  	if (options_.isSet(OptCapture)) {
> -		Capture capture(camera_, config_.get());
> +		Capture capture(camera_, config_.get(), roles_);
>  		return capture.run(loop_, options_);
>  	}
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund