[PATCH 1/2] apps: cam: Add option to configure sensor

[Paul Elder]

Hi Laurent,

On Mon, May 20, 2024 at 03:11:08PM +0300, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Mon, May 20, 2024 at 05:31:09PM +0530, Umang Jain wrote:
> > From: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > 
> > Add a '-f|--sensor_format' option to cam to allow forcing a sensor
> > configuration from the command line.
> > 
> > As an example:
> > cam -c1 -C -S --stream pixelformat=YUYV -f width=3840,height=2160,bitDepth=10
> > 
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> I *think* we discussed this previously, but I may be wrong.
> 
> Before we push this towards applications, I want the sensor
> configuration to be fully specified, including binning/skipping and the
> crop rectangles. Our current API to configure the sensor isn't good
> enough.

afaict the sensor configuration already supports binning and skipping
and crop rectangles. What else is missing?


Paul

> 
> > ---
> >  src/apps/cam/camera_session.cpp    |  7 +++++
> >  src/apps/cam/main.cpp              |  4 +++
> >  src/apps/cam/main.h                |  1 +
> >  src/apps/common/stream_options.cpp | 42 ++++++++++++++++++++++++++++++
> >  src/apps/common/stream_options.h   |  8 ++++++
> >  5 files changed, 62 insertions(+)
> > 
> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > index f13355ba..a51dcfbc 100644
> > --- a/src/apps/cam/camera_session.cpp
> > +++ b/src/apps/cam/camera_session.cpp
> > @@ -90,6 +90,13 @@ CameraSession::CameraSession(CameraManager *cm,
> >  		return;
> >  	}
> >  
> > +	/* Apply a sensor configuration is requested. */
> > +	if (SensorKeyValueParser::updateConfiguration(config.get(),
> > +						      options_[OptSensorFmt])) {
> > +		std::cerr << "Failed to apply sensor configuration" << std::endl;
> > +		return;
> > +	}
> > +
> >  	bool strictFormats = options_.isSet(OptStrictFormats);
> >  
> >  #ifdef HAVE_KMS
> > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> > index 4f87f200..0f751469 100644
> > --- a/src/apps/cam/main.cpp
> > +++ b/src/apps/cam/main.cpp
> > @@ -110,6 +110,7 @@ void CamApp::quit()
> >  int CamApp::parseOptions(int argc, char *argv[])
> >  {
> >  	StreamKeyValueParser streamKeyValue;
> > +	SensorKeyValueParser sensorKeyValue;
> >  
> >  	OptionsParser parser;
> >  	parser.addOption(OptCamera, OptionString,
> > @@ -178,6 +179,9 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  			 "Load a capture session configuration script from a file",
> >  			 "script", ArgumentRequired, "script", false,
> >  			 OptCamera);
> > +	parser.addOption(OptSensorFmt, &sensorKeyValue,
> > +			 "Apply a format to the sensor", "sensor_format", true,
> > +			 OptCamera);
> >  
> >  	options_ = parser.parse(argc, argv);
> >  	if (!options_.valid())
> > diff --git a/src/apps/cam/main.h b/src/apps/cam/main.h
> > index 64e6a20e..65844ac5 100644
> > --- a/src/apps/cam/main.h
> > +++ b/src/apps/cam/main.h
> > @@ -20,6 +20,7 @@ enum {
> >  	OptOrientation = 'o',
> >  	OptSDL = 'S',
> >  	OptStream = 's',
> > +	OptSensorFmt = 'f',
> >  	OptListControls = 256,
> >  	OptStrictFormats = 257,
> >  	OptMetadata = 258,
> > diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp
> > index 99239e07..55a2d3d2 100644
> > --- a/src/apps/common/stream_options.cpp
> > +++ b/src/apps/common/stream_options.cpp
> > @@ -119,3 +119,45 @@ std::optional<libcamera::StreamRole> StreamKeyValueParser::parseRole(const KeyVa
> >  
> >  	return {};
> >  }
> > +
> > +SensorKeyValueParser::SensorKeyValueParser()
> > +{
> > +	addOption("bitDepth", OptionInteger, "Sensor format bit depth",
> > +		  ArgumentRequired);
> > +	addOption("width", OptionInteger, "Sensor frame width in pixels",
> > +		  ArgumentRequired);
> > +	addOption("height", OptionInteger, "Sensor frame height in pixels",
> > +		  ArgumentRequired);
> > +}
> > +
> > +int SensorKeyValueParser::updateConfiguration(CameraConfiguration *config,
> > +					      const OptionValue &values)
> > +{
> > +	if (!config) {
> > +		std::cerr << "No configuration provided" << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* If no configuration values nothing to do. */
> > +	if (values.empty())
> > +		return 0;
> > +
> > +	const std::vector<OptionValue> &streamParameters = values.toArray();
> > +	SensorConfiguration sensorConfig;
> > +
> > +	for (auto const &value : streamParameters) {
> > +		KeyValueParser::Options opts = value.toKeyValues();
> > +
> > +		if (opts.isSet("width") && opts.isSet("height")) {
> > +			sensorConfig.outputSize.width = opts["width"];
> > +			sensorConfig.outputSize.height = opts["height"];
> > +		}
> > +
> > +		if (opts.isSet("bitDepth"))
> > +			sensorConfig.bitDepth = opts["bitDepth"];
> > +	}
> > +
> > +	config->sensorConfig = sensorConfig;
> > +
> > +	return 0;
> > +}
> > diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h
> > index a93f104c..f1020d8c 100644
> > --- a/src/apps/common/stream_options.h
> > +++ b/src/apps/common/stream_options.h
> > @@ -27,3 +27,11 @@ public:
> >  private:
> >  	static std::optional<libcamera::StreamRole> parseRole(const KeyValueParser::Options &options);
> >  };
> > +
> > +class SensorKeyValueParser : public KeyValueParser
> > +{
> > +public:
> > +	SensorKeyValueParser();
> > +	static int updateConfiguration(libcamera::CameraConfiguration *config,
> > +				       const OptionValue &values);
> > +};
> 
> -- 
> Regards,
> 
> Laurent Pinchart