[libcamera-devel] [PATCH v3 6/6] cam: Add command line option to test IspCrop control
Jacopo Mondi
jacopo at jmondi.org
Wed Sep 30 10:42:28 CEST 2020
Hi David,
On Wed, Sep 30, 2020 at 08:00:18AM +0100, David Plowman wrote:
> Hi Niklas
>
> Thanks for the comments, and sorry for not addressing them sooner!
>
> On Tue, 29 Sep 2020 at 23:33, Niklas Söderlund
> <niklas.soderlund at ragnatech.se> wrote:
> >
> > Hi David,
> >
> > Thanks for your patch.
> >
> > 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
>
> Correct, apart from a couple of minor mods (though one rather important one!)
>
> > 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.
>
> Yes, I was kind of hoping not to get too involved with all that! How about this:
>
> 1. Add an OptionFloat type
>
> 2. Change the "isArray" option field to "size" or something, and make
> it an unsigned int. We'd make its default value 1, meaning "not an
> array" and edit any existing usage that sets it to False to use 1
> instead.
>
> 3. It seems to me that the current semantics for an "array" option is
> that they're variable length, i.e. it accepts as many as you type in.
> Perhaps we could use zero size for such an array, and otherwise insist
> that we get exactly the number that were wanted?
>
> 4. Then in my case I could supply a size of 4, and get an array of 4 floats.
>
> 5. It's true there is still some checking that needs to be done on the
> values to be sure they're sensible - my view here is that pipeline
> handlers should deal with garbage as applications can easily submit
> garbage, and here we have a way of checking that the pipeline handler
> actually does this correctly!
>
> >
> > > + 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?
>
> Indeed you're right, there is a bit of a dilemma here. There was some
> discussion of this a while back, I think with Jacopo, unfortunately I
> don't have any email references to hand.
>
> I think the feeling was that in all the real use cases that we can
> imagine, you generally want the same field of view on all your
> outputs. I think situations where something else would be useful are
> certainly less "mainstream".
>
> Secondly, some ISPs will let you crop outputs differently, and some
> won't. That's an awkward thing to handle - how would you know what the
> underlying hardware does, and then would you write different code for
> each case? The pragmatic answer may just be to implement the "usual"
> case here, and it becomes a question for the future whether we get
> per-stream crop controls, or whether a StreamConfiguration gets crop
> rectangles...
>
> Maybe Jacopo can comment if I've got anything wrong there!
Yes, we've gone through that, and I think my intereptation was that
you're better positioned than me, having a large user base and knowing
what they expect, to decide if it was worth blocking this feature
until we have decided how to implement per-Stream controls and how to
handle the situation you have described here when a platform supports
such a feature at the Camera level or at the Stream level.
My opinion is that digital zoom it's a feature it's worth having, even
if in sub-optimal shape (same applies to the SensorOutputSize
property, which ideally should be reported as part of the
CameraConfiguration). The alternative is to post-pone this until we
have augmented CameraConfiguration with properties (relatively easy)
and implemented per-Stream control (less easy if you ask me).
I know Laurent wanted to have a look here, so let's see what his
opinion is.
Thanks
j
>
> Thanks again and best regards
> David
>
> >
> > > + } 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,
> > > };
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list