[libcamera-devel] [PATCH v3 6/6] cam: Add command line option to test IspCrop control
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Sep 30 13:32:41 CEST 2020
Hi David,
On 2020-09-30 08:00:18 +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!
I think cam is the wrong tool to test error injection into pipelines. I
also understand you wish to avoid hacking on the input parsers. I on the
other hand enjoy that work so I would be more then happy to supply you
with a patch you could squash into this patch.
Only thing blocking me from writing one now is that I'm not sure this
should be a new --zoom option (ZoomKeyValueParser) or added to the
existing --stream option (extend StreamKeyValueParser). This is effect
what we are discussing below so I will postpone until we have resolved
this.
>
> >
> > > + 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".
I'm sure there are use cases for different fields of view. I have seen
video conferencing gear that in the participant list zoom in on the
center of the screen while the speakers video is a larger FoV. I thought
this was Google HO but a quick test proves me wrong. In any case I'm
sure this is done in software without any support from the camera, but I
think there are use-cases for this.
I also think the flavor of a zoom control is in the same area as a
rotation control. And rotation is something we sure wish to have per
stream. So I think no matter which controls go first we are reaching the
point we need to add support for per stream controls.
>
> 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?
Applications must be able to enumerate the features of the camera and
then enable or disable certain use-cases based on the enumerated
features. Of course this might lead to the applications rejecting a
camera as it don't support the features it needs (A raw capture
application would likely reject a camera that can't capture in a raw
format it knows how to process).
> 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...
I think we need to implement both the usual and special cases, but we
need to do so in a user-friendly way. One way to do that is to provide
helpers for applications.
I'm thinking as we know controls like digital zoom and rotation
depending on the hardware capabilities can apply to individual or all
streams the low-level API must allow for the most complex setup,
controls on the stream level. But as you say their are use-cases where
an application just wish to zoom or rotate all streams and then we could
add helpers for that, maybe something simple like this would work?
Request::setControlAllStreams(Control, Value);
Then applications who want to rotate or zoom all streams use such
helpers while special case applications will enumerate the camera
features and detect if it's possible to rotate or zoom on indivudual
streams and if so do it.
As Jacopo points out, we know Laurent is working in this area so I'm
sure he has more comments or at least a different viewpoint then mine
;-)
>
> 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list