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

[Jacopo Mondi]

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 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)

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