| Message ID | 20200907164450.13082-3-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 ? > + 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 ? 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 ? Thanks j > ... > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo Thanks for the reply. On Mon, 21 Sep 2020 at 14:02, Jacopo Mondi <jacopo@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
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: + 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 + 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. ...