[libcamera-devel] [PATCH v4 0/5] Digital zoom

[Laurent Pinchart]

Hi David,

Thank you for the patches.

On Mon, Oct 19, 2020 at 01:51:51PM +0100, David Plowman wrote:
> Hi everyone
> 
> Here's a new version of the digital zoom patches, following on from
> the previous implementations and the discussions that have been
> ongoing since then. Firstly I should point out that I've dropped the
> patch that added digital zoom parameters to the cam application as
> someone had suggested they might do something there (so thanks for
> that!). Also, I'm happy to add some geometry tests once there's a bit
> less churn in this patch set as a whole. In view of the current churn,
> I've dropped other "Reviewed by" tags - I hope that's reasonable.

Sure. Thank you so much for coping with my reviews.

> That leaves 5 patches, as follows:
> 
> 1. Adds the ScalerCropMaximum property.
> 
> This is the property that expresses the part of the sensor array
> within which an application can crop to implement digital zoom. The
> property name has been changed, obviously, and the documentation
> updated to be in line with recent discussion, otherwise nothing here
> is very different.
> 
> 2. Initialisation of the ScalerCropMaximum property.
> 
> This is unchanged. Given that the property isn't really very
> meaningful until you know what camera mode you mean, I do wonder
> whether we might be better to initialise this to all zeroes?
> Applications could always call isNull to be sure that they have
> something sensible.

Seems reasonable to me.

> 3. Adds the ScalerCrop property.
> 
> No great changes, apart from the property name and description.
> 
> 4. Geometry helpers.
> 
> Mostly as before, with the addition of a Point class and translation
> functions. There's also a Rectangle::rescaledTo function to make it
> easier to scale crop regions between sensor native and binned/scaled
> coordinates. Maybe someone would prefer a different name?
> 
> I've split Rectangle::boundedTo into a separate true intersection
> function (taking the name Rectangle::boundedTo) and a function I've
> named Rectangle::clampedTo. I'm open to suggestions on the name, I
> wanted to choose something that didn't sound like it might be the
> intersection...
> 
> 5. Implementation in the Raspberry Pi pipeline.
> 
> Mostly as before, with the modifications required by earlier changes
> (notably scaling to and from native sensor coordinates). Some of the
> code here might look a little nicer if we added a
> "Point Rectangle::offset() const { return { x, y }; }" function, and
> changed the translate methods to take a Point instead of a pair of
> ints.
> 
> Though I realise that the word "offset" is not popular,

It could be called Rectangle::topLeft().

> and I find the
> idea of translating by a Point mildly offensive, so I'm not sure about
> all that. Thoughts?

I'd be fine with it.

> Well, I think that's everything!
> 
> David Plowman (5):
>   libcamera: Add SensorCropMaximum property
>   libcamera: Initialise the ScalerCropMaximum property
>   libcamera: Add ScalerCrop control
>   libcamera: Add geometry helper functions
>   libcamera: pipeline: raspberrypi: Implementation of digital zoom
> 
>  include/libcamera/geometry.h                  |  53 ++++
>  include/libcamera/ipa/raspberrypi.h           |   1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           |   7 +
>  src/libcamera/camera_sensor.cpp               |   6 +
>  src/libcamera/control_ids.yaml                |  12 +
>  src/libcamera/geometry.cpp                    | 296 ++++++++++++++++++
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  70 ++++-
>  src/libcamera/property_ids.yaml               |  23 ++
>  8 files changed, 453 insertions(+), 15 deletions(-)

-- 
Regards,

Laurent Pinchart