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

10094 mbox series

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.

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.

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, and I find the
idea of translating by a Point mildly offensive, so I'm not sure about
all that. Thoughts?

Well, I think that's everything!

Thanks and best regards
David

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(-)

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(-)