[libcamera-devel] [RFC PATCH 2/4] libcamera: Add SensorCrop control

[David Plowman]

Hi Jacopo

Thanks for the reply.

On Mon, 21 Sep 2020 at 14:02, Jacopo Mondi <jacopo at jmondi.org> wrote:

> Hi David,
>   super nit: in Subject: "libcamera: controls: Add SensorCrop"
>
> On Mon, Sep 07, 2020 at 05:44:48PM +0100, David Plowman wrote:
> > The SensorCrop control selects how much of the sensor's output image
> > will be scaled to form the output image. It can be used to implement
> > digital zoom.
> > ---
> >  src/libcamera/control_ids.yaml | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> > index 3560d4a..cebaa25 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -273,4 +273,13 @@ controls:
> >          order in an array of 9 floating point values.
> >
> >        size: [3x3]
> > +
> > +  - SensorCrop:
>
> I wonder, is this a Sensor related property ? Roughly speaking that's
> a selection on the input frame that is provided to the ISP for
> processing, if I think about the possibly forthcoming namespacing of
> the controls IDs I don't see this being well placed as Sensor::Crop
> (but rather as ISP::Crop possibly ?)
>
> I recall you had 'PipelineCrop' in previous version, am I wrong ?
>

Hehe. Every time I come back to the topic of digital zoom it seems to
change its name! Yes, it was PipelineCrop before, now it's SensorCrop but I
can go with IspCrop (becoming ISP::Crop) too.  :)


>
> > +      type: Rectangle
> > +      description: |
> > +        Sets the portion of the full sensor image, in pixels, that will
> be
> > +        scaled up to form the whole of the final output image. This
> control
>
> As a suggestion, take whatever you like in:
>
>            Selection rectangle (in pixel units) that defines the
>            portion of the image that will be scaled up to form the
>            final output image. This control can be used to implement
>            digital zoom.
>
> > +        can be used to implement digital zoom. The size of the full
> sensor
> > +        image within which an application can crop is available from the
> > +        SensorOutputSize property.
>
>            The rectangle is defined in respect to the size of the
>            image which is processed by the ISP to produce the output
>            streams, whose size is reported by the SensorOutputSize
>            property.
>
>            \sa properties::SensorOutputSize
>
> How bad is it in your opinion that this will apply to all streams ? I
> don't see many ways around and to me it's fine, but maybe we have
> different expectations ?
>

Well, I'm OK with it too, I think it makes life really quite complicated if
different output streams have different FOVs. I certainly know of ISPs that
can do this, but I don't think I've ever found myself working on a camera
application and thinking "I really need this feature". Generally speaking I
think you want the same image - just in a different resolution, or colour
space or format, something like that.


>
> How does it work if the Rectangle sizes are larger than the input
> frame sizes ? Should we enforce a behaviour like always clamping the
> SensorCrop size to the SensorOutputSize ones ?
>

In the Raspberry Pi pipeline handler I always use the Rectangle::clamp
method, so it forces it to fit whatever the application passes in. This
seems fair behaviour to me - application gives you rubbish, coerce it to
the nearest reasonable thing and report what you did - and is, I think,
more helpful than reporting an error and failing.

Thanks also for the other suggestions!

Best regards
David


>
> Thanks
>   j
>
>
> >  ...
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200921/89174f60/attachment.htm>