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

Message ID 20200925085127.17214-7-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Digital zoom
Related show

Commit Message

David Plowman Sept. 25, 2020, 8:51 a.m. UTC
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@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(-)

Comments

Jacopo Mondi Sept. 28, 2020, 10:44 a.m. UTC | #1
Hi David,

On Fri, Sep 25, 2020 at 09:51:27AM +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@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)) {

I would have preferred to keep option parsing where the other options
parsing happens, but this is the first control we set in Request, so I
guess this it's fine to do it here.

> +		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) {
> +			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);
> +		} 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",

Could we specify the values are percentages ? Maybe it's a standard
thing, but for the uneducated like me is it worth specifying it ?

			 "Zoom on the image portion described by the x,y,width,height rectangle (in %) e.g. 0.25,0.25,0.5,0.5",

Or something better..

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


> +			 "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,
>  };
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Sept. 28, 2020, 11:07 a.m. UTC | #2
Hi David,

Thanks for your patch.

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

> +		} 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,
>  };
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
David Plowman Sept. 28, 2020, 11:13 a.m. UTC | #3
Hi Jacopo

Thanks for the comments. Just one small clarification...

On Mon, 28 Sep 2020 at 11:40, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David,
>
> On Fri, Sep 25, 2020 at 09:51:27AM +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@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)) {
>
> I would have preferred to keep option parsing where the other options
> parsing happens, but this is the first control we set in Request, so I
> guess this it's fine to do it here.
>
> > +             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) {
> > +                     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);
> > +             } 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",
>
> Could we specify the values are percentages ? Maybe it's a standard
> thing, but for the uneducated like me is it worth specifying it ?
>
>                          "Zoom on the image portion described by the x,y,width,height rectangle (in %) e.g. 0.25,0.25,0.5,0.5",
>
> Or something better..

I think I'd expect a percentage to be out of one hundred, so can we go
with something like:

"Zoom on the image portion described by the x,y,width,height rectangle
(fraction of the maximum FOV) e.g. 0.25,0.25,0.5,0.5"

(percentages would be OK too, but I think there'd be a small code
change where we divide everything by 100)

Are we OK to make this change while applying or would another patch be better?

Thanks
David

>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
>
> > +                      "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,
> >  };
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Sept. 28, 2020, 11:19 a.m. UTC | #4
HI David,

On Mon, Sep 28, 2020 at 12:13:23PM +0100, David Plowman wrote:
> Hi Jacopo
>
> Thanks for the comments. Just one small clarification...
>
> On Mon, 28 Sep 2020 at 11:40, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi David,
> >
> > On Fri, Sep 25, 2020 at 09:51:27AM +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@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)) {
> >
> > I would have preferred to keep option parsing where the other options
> > parsing happens, but this is the first control we set in Request, so I
> > guess this it's fine to do it here.
> >
> > > +             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) {
> > > +                     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);
> > > +             } 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",
> >
> > Could we specify the values are percentages ? Maybe it's a standard
> > thing, but for the uneducated like me is it worth specifying it ?
> >
> >                          "Zoom on the image portion described by the x,y,width,height rectangle (in %) e.g. 0.25,0.25,0.5,0.5",
> >
> > Or something better..
>
> I think I'd expect a percentage to be out of one hundred, so can we go
> with something like:
>
> "Zoom on the image portion described by the x,y,width,height rectangle
> (fraction of the maximum FOV) e.g. 0.25,0.25,0.5,0.5"
>
> (percentages would be OK too, but I think there'd be a small code
> change where we divide everything by 100)

You are right, I should have wrote "fraction"
>
> Are we OK to make this change while applying or would another patch be better?
>

A new patch for this change only is not required imho.

Thanks
  j

> Thanks
> David
>
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Thanks
> >   j
> >
> >
> > > +                      "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,
> > >  };
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 11, 2020, 12:41 a.m. UTC | #5
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@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,
> >  };

Patch

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) {
+			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);
+		} 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,
 };