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

[Laurent Pinchart]

Hi Niklas,

On Tue, May 19, 2020 at 04:27:50PM +0200, Niklas Söderlund wrote:
> 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?

I'll do so.

> > 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