[libcamera-devel] [PATCH v3 6/6] cam: Add command line option to test IspCrop control
David Plowman
david.plowman at raspberrypi.com
Thu Oct 1 16:00:30 CEST 2020
Hi Niklas
Thanks for your comments.
On Wed, 30 Sep 2020 at 12:32, Niklas Söderlund
<niklas.soderlund at ragnatech.se> wrote:
>
> 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.
Well, any help is always appreciated! As I said initially, I'm very
happy to drop this patch, and we could let another one replace it.
>
> >
> > >
> > > > + 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.
So yes, I agree there are use cases, and indeed I've seen this kind of
use case before. But I've seen it done using software too, which kind
of underlines the whole problem. Namely, are applications going to
write different versions of code for underlying hardware with
different features? Unless the developer is targeting particular
hardware known to have this feature they will try to avoid it.
But in any case, we have to expose what we regard as our minimum
expected hardware features, which would probably be a single zoom
control, and make that convenient to use.
>
> 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.
Agreed, rotation (or transform) control is a case in point too. To be
fair, we've followed a similar procedure there and allowed for a
single per-camera transform (the CameraConfiguration has a field to
request this transform). The question of per-stream transforms is
tricky - many ISPs won't support it which limits its usefulness,
others may support them only at startup, others per-frame, and so that
whole question is deferred for the moment.
>
> >
> > 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.
Well, I'm happy to go with this approach too. For us, digital zoom is
an important feature, so if we could come to some consensus that would
be very helpful!
Best regards
David
>
> 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