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

[Laurent Pinchart]

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 at 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 at 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.
> > +
> >  ...

-- 
Regards,

Laurent Pinchart