[libcamera-devel] [PATCH v2 6/6] cam: Add command line option to test IspCrop control

[Laurent Pinchart]

Hi Niklas,

On Mon, Sep 28, 2020 at 01:07:35PM +0200, Niklas Söderlund wrote:
> On 2020-09-25 09:51:27 +0100, David Plowman wrote:
> > This allows the user to request digital zoom by adding, for example,
> > "-z 0.25,0.25,0.5,0.5" which would give a 2x zoom, still centred in
> > the middle of the image.
> > 
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/cam/capture.cpp | 25 +++++++++++++++++++++++--
> >  src/cam/capture.h   |  2 +-
> >  src/cam/main.cpp    |  3 +++
> >  src/cam/main.h      |  1 +
> >  4 files changed, 28 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 5510c00..3b50c59 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -10,6 +10,9 @@
> >  #include <limits.h>
> >  #include <sstream>
> >  
> > +#include <libcamera/control_ids.h>
> > +#include <libcamera/property_ids.h>
> > +
> >  #include "capture.h"
> >  #include "main.h"
> >  
> > @@ -58,7 +61,7 @@ int Capture::run(const OptionsParser::Options &options)
> >  
> >  	FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);
> >  
> > -	ret = capture(allocator);
> > +	ret = capture(allocator, options);
> >  
> >  	if (options.isSet(OptFile)) {
> >  		delete writer_;
> > @@ -70,7 +73,7 @@ int Capture::run(const OptionsParser::Options &options)
> >  	return ret;
> >  }
> >  
> > -int Capture::capture(FrameBufferAllocator *allocator)
> > +int Capture::capture(FrameBufferAllocator *allocator, const OptionsParser::Options &options)
> >  {
> >  	int ret;
> >  
> > @@ -120,6 +123,24 @@ int Capture::capture(FrameBufferAllocator *allocator)
> >  		requests.push_back(request);
> >  	}
> >  
> > +	/* Set up digital zoom if it was requested on the command line. */
> > +	if (options.isSet(OptZoom)) {
> > +		std::string zoom = options[OptZoom].toString();
> > +		float x, y, width, height;
> > +
> > +		if (sscanf(zoom.c_str(), "%f,%f,%f,%f", &x, &y, &width, &height) == 4) {
> 
> This is the wrong place to parse the input from the user. If the zoom 
> options are global something similar like StreamKeyValueParser 
> (ZoomKeyValueParser?) should be created.
> 
> > +			Size sensorArea = camera_->properties().get(properties::SensorOutputSize);
> > +			Rectangle crop(x * sensorArea.width,
> > +				       y * sensorArea.height,
> > +				       width * sensorArea.width,
> > +				       height * sensorArea.height);
> 
> 
> > +
> > +			requests.front()->controls().set(controls::IspCrop, crop);
> 
> Do we wish to expose the ISP in the public API? If we do is the ISP 
> (from an application point of view) the correct level to operate on?  
> Would not setting this on a stream level cover more hardware?
> 
> I'm thinking of an ISP that can apply different crop parameters to two 
> (or more) source pads from a single sink pad. Having a control that sets 
> a global crop on the ISP would that not limit such hardware or am I 
> missing something?

This is a very good question, and a very fundamental design decision we
need to make. I've started thinking that we need to define and document
an explicit abstract pipeline model for cameras, striking the right
balance between flexibility and adaptability to different hardware
architectures, and ease of implementation for pipeline handlers and of
use for applications.

In this context, David mentioned in a previous version of this series
that, while different streams could indeed have different zoom levels,
use cases for this feature were not immediately apparent. We could thus
leave this out of our pipeline model. If we ever want to support this
feature in the future, I think we will be able to do so with the help of
per-stream controls in requests, using the same IspCrop control, so it
appears we would have a way forward.

> > +		} else {
> > +			std::cout << "Invalid zoom values - ignoring" << std::endl;
> > +		}
> > +	}
> > +
> >  	ret = camera_->start();
> >  	if (ret) {
> >  		std::cout << "Failed to start capture" << std::endl;
> > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > index 0aebdac..5280616 100644
> > --- a/src/cam/capture.h
> > +++ b/src/cam/capture.h
> > @@ -29,7 +29,7 @@ public:
> >  
> >  	int run(const OptionsParser::Options &options);
> >  private:
> > -	int capture(libcamera::FrameBufferAllocator *allocator);
> > +	int capture(libcamera::FrameBufferAllocator *allocator, const OptionsParser::Options &options);
> >  
> >  	void requestComplete(libcamera::Request *request);
> >  
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 244720b..8f326d9 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -201,6 +201,9 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  	parser.addOption(OptStrictFormats, OptionNone,
> >  			 "Do not allow requested stream format(s) to be adjusted",
> >  			 "strict-formats");
> > +	parser.addOption(OptZoom, OptionString,
> > +			 "Specify digital zoom by supplying IspCrop control as x,y,width,height e.g. 0.25,0.25,0.5,0.5",
> > +			 "zoom", ArgumentRequired, "zoom");
> >  
> >  	options_ = parser.parse(argc, argv);
> >  	if (!options_.valid())
> > diff --git a/src/cam/main.h b/src/cam/main.h
> > index ea8dfd3..a1fd899 100644
> > --- a/src/cam/main.h
> > +++ b/src/cam/main.h
> > @@ -17,6 +17,7 @@ enum {
> >  	OptListProperties = 'p',
> >  	OptMonitor = 'm',
> >  	OptStream = 's',
> > +	OptZoom = 'z',
> >  	OptListControls = 256,
> >  	OptStrictFormats = 257,
> >  };

-- 
Regards,

Laurent Pinchart