[libcamera-devel] [PATCH v3 6/6] cam: Add command line option to test IspCrop control
David Plowman
david.plowman at raspberrypi.com
Wed Sep 30 09:00:18 CEST 2020
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!
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
More information about the libcamera-devel
mailing list