[libcamera-devel] [PATCH 1/3] cam: Add option to disallow adjusting of requested formats

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2020-07-24 14:57:28 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> I can see some definite value in this option!
> 
> On 24/07/2020 14:43, Niklas Söderlund wrote:
> > Add an '--strict-formats' option which fails the camera configuration
> 
> s/an/a/
> 
> > step if the format is adjusted,
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/main.cpp | 17 ++++++++++++++++-
> >  src/cam/main.h   |  1 +
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 2512fe9da782165b..a223563ad37e9fe5 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -49,12 +49,15 @@ private:
> >  	std::shared_ptr<Camera> camera_;
> >  	std::unique_ptr<libcamera::CameraConfiguration> config_;
> >  	EventLoop *loop_;
> > +
> > +	bool strictFormats_;
> >  };
> >  
> >  CamApp *CamApp::app_ = nullptr;
> >  
> >  CamApp::CamApp()
> > -	: cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr)
> > +	: cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr),
> > +	  strictFormats_(false)
> >  {
> >  	CamApp::app_ = this;
> >  }
> > @@ -77,6 +80,9 @@ int CamApp::init(int argc, char **argv)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	if (options_.isSet(OptStrictFormats))
> > +		strictFormats_ = true;
> > +
> >  	cm_ = new CameraManager();
> >  
> >  	ret = cm_->start();
> > @@ -179,6 +185,9 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  			 "list-controls");
> >  	parser.addOption(OptListProperties, OptionNone, "List cameras properties",
> >  			 "list-properties");
> > +	parser.addOption(OptStrictFormats, OptionNone,
> > +			 "Do not allow requested stream format(s) to be adjusted",
> > +			 "strict-formats");
> >  
> >  	options_ = parser.parse(argc, argv);
> >  	if (!options_.valid())
> > @@ -214,6 +223,12 @@ int CamApp::prepareConfig()
> >  	case CameraConfiguration::Valid:
> >  		break;
> >  	case CameraConfiguration::Adjusted:
> > +		if (strictFormats_) {
> > +			std::cout << "Adjustng camera configuration not allowed"
> 
> s/Adjustng/Adjusting/
> 
> 
> Could/should we print here what the stream configurations were adjusted
> /to/ to help debugging?

We could, but there are other places we could do this and don't. I think 
we should add this to core at the Debug level so no matter the 
application we could get this information out.

I know you have added something like this in your HAL JPEG work so I 
think we should address this at a common point.

> 
> 
> Otherwise,
> 
> > +				  << std::endl;
> > +			config_.reset();
> 
> Is this (reset) essential?

Yes, we need to reset the config_ variable if we don't want to proceed.  
We do the same here for CameraConfiguration::Invalid (that is not shown 
in the context for this patch).

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

Thanks!

> 
> 
> 
> > +			return -EINVAL;
> > +		}
> >  		std::cout << "Camera configuration adjusted" << std::endl;
> >  		break;
> >  	case CameraConfiguration::Invalid:
> > diff --git a/src/cam/main.h b/src/cam/main.h
> > index 4a130d8dd2906ad4..6f95add31a6341cf 100644
> > --- a/src/cam/main.h
> > +++ b/src/cam/main.h
> > @@ -17,6 +17,7 @@ enum {
> >  	OptListProperties = 'p',
> >  	OptStream = 's',
> >  	OptListControls = 256,
> > +	OptStrictFormats = 257,
> >  };
> >  
> >  #endif /* __CAM_MAIN_H__ */
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund