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

[David Plowman]

Hi Jacopo

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

On Mon, 28 Sep 2020 at 11:40, Jacopo Mondi <jacopo at 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 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)) {
>
> 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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel