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

[Laurent Pinchart]

Hi Niklas,

On Wed, Sep 30, 2020 at 12:33:56AM +0200, Niklas Söderlund wrote:
> On 2020-09-29 17:40:00 +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>
> 
> If I understand the cover letter correctly this is just a rebase so I 
> understand not all comments from v2 are addressed. But just to reiterate 
> my comments from v2.
> 
> > ---
> >  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 5510c009..3b50c591 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. If it is to be moved a stream 
> level StreamKeyValueParser should be extended.
> 
> > +			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?

(Copied from a previous version, to centralize all discussions in this
series)

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 0aebdac9..52806164 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 244720b4..b871d049 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,
> > +			 "Zoom in on the image portion described by the x,y,width,height rectangle (fractions of the maximum FOV) 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 ea8dfd33..a1fd899f 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