[libcamera-devel,v4,1/5] libcamera: Add SensorCropMaximum property

10095 diff mbox series

The SensorCropMaximum camera property reports the location of that
part of the image sensor array that is scaled to produce the output
images, given in native sensor pixels. It will normally change when a
new camera mode is selected, and can be used to implement digital
zoom.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Hi David,

Thank you for the patch.

On Mon, Oct 19, 2020 at 01:51:52PM +0100, David Plowman wrote:
> The SensorCropMaximum camera property reports the location of that
> part of the image sensor array that is scaled to produce the output
> images, given in native sensor pixels. It will normally change when a
> new camera mode is selected, and can be used to implement digital
> zoom.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 7261263a..022cf65d 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -663,4 +663,24 @@ controls:
>          \todo Rename this property to ActiveAreas once we will have property
>                categories (i.e. Properties::PixelArray::ActiveAreas)
>  
> +  - ScalerCropMaximum:
> +      type: Rectangle
> +      description: |
> +        The size and location, in native sensor pixels, of the part of the
> +        sensor that is rescaled to produce the output images. Note that the
> +        units remain native sensor pixels, even if the sensor is being used in
> +        a binning skipping or scaling mode.

Now that I've reviewed 5/5 I think we agree on the meaning of this
property, but the text here doesn't seem to correctly match what I
understand is our agreement :-) The above paragraph is actually what I
was expecting for the ScalerCrop control, not the ScalerCropMaximum
property.

How about simply defining this property as the maximum value for
ScalerCrop ? It could be written as just

        The maximum valid rectangle for the controls::ScalerCrop control.

with the last two paragraphs kept. The rest would then be moved to the
documentation of ScalerCrop (if they're not there already, I'll review
that patch next).

If you agree with this, and with the commit message updated too,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +        The (x,y) location of this rectangle is relative to the
> +        PixelArrayActiveArea that is being used. The property also takes into
> +        account any further cropping being done by the CSI-2 receiver or
> +        elsewhere.
> +
> +        This property is valid only after the Camera has been successfully
> +        configured and its value changes whenever a new configuration is
> +        applied.
> +
> +        \todo Turn this property into a "maximum control value" for the
> +        ScalerCrop control once "dynamic" controls have been implemented.
> +
>  ...

Hi David,

An additional comment.

On Thu, Oct 22, 2020 at 09:04:28AM +0300, Laurent Pinchart wrote:
> Hi David,
> 
> Thank you for the patch.
> 
> On Mon, Oct 19, 2020 at 01:51:52PM +0100, David Plowman wrote:
> > The SensorCropMaximum camera property reports the location of that
> > part of the image sensor array that is scaled to produce the output
> > images, given in native sensor pixels. It will normally change when a
> > new camera mode is selected, and can be used to implement digital
> > zoom.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index 7261263a..022cf65d 100644
> > --- a/src/libcamera/property_ids.yaml
> > +++ b/src/libcamera/property_ids.yaml
> > @@ -663,4 +663,24 @@ controls:
> >          \todo Rename this property to ActiveAreas once we will have property
> >                categories (i.e. Properties::PixelArray::ActiveAreas)
> >  
> > +  - ScalerCropMaximum:
> > +      type: Rectangle
> > +      description: |
> > +        The size and location, in native sensor pixels, of the part of the
> > +        sensor that is rescaled to produce the output images. Note that the
> > +        units remain native sensor pixels, even if the sensor is being used in
> > +        a binning skipping or scaling mode.
> 
> Now that I've reviewed 5/5 I think we agree on the meaning of this
> property, but the text here doesn't seem to correctly match what I
> understand is our agreement :-) The above paragraph is actually what I
> was expecting for the ScalerCrop control, not the ScalerCropMaximum
> property.
> 
> How about simply defining this property as the maximum value for
> ScalerCrop ? It could be written as just
> 
>         The maximum valid rectangle for the controls::ScalerCrop control.

This should be expanded a little bit, as, after reviewing the ScalerCrop
control, the second paragraph of this property doesn't fit there.

        The maximum valid rectangle for the controls::ScalerCrop control. This
        reflects the minimum mandatory cropping applied in the camera sensor and
        the rest of the pipeline.

(I wouldn't mention CSI-2 receiver explicitly as we may be dealing with
a parallel sensor.)

> with the last two paragraphs kept. The rest would then be moved to the
> documentation of ScalerCrop (if they're not there already, I'll review
> that patch next).
> 
> If you agree with this, and with the commit message updated too,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> > +        The (x,y) location of this rectangle is relative to the
> > +        PixelArrayActiveArea that is being used. The property also takes into
> > +        account any further cropping being done by the CSI-2 receiver or
> > +        elsewhere.
> > +
> > +        This property is valid only after the Camera has been successfully
> > +        configured and its value changes whenever a new configuration is
> > +        applied.
> > +
> > +        \todo Turn this property into a "maximum control value" for the
> > +        ScalerCrop control once "dynamic" controls have been implemented.
> > +
> >  ...