[libcamera-devel] [PATCH v3 0/6] Digital zoom

[Laurent Pinchart]

Hi David,

Thank you for the patches. And more than sorry for reviewing the series
so late.

On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:
> Hi everyone
> 
> Despite not originally intending to, I've actually made a version 3 of
> the digital zoom patches, just to take care of a few things that might
> be a bit annoying otherwise.
> 
> 1. I've improved the description of the IspCrop control as was
> suggested.
> 
> 2. I've improved the description of the zoom option in cam (if we
> decide to use this patch!), also as was proposed.
> 
> 3. There was actually a problem with the "{}" syntax to denote zero
> Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a
> range type test in ControlInfoMap::generateIdmap() and so the control
> wasn't working. I've replaced "{}" by "Rectangle{}" which seems OK.
> 
> 4. There's been a bit of churn in the RPi pipeline handler lately so
> rebasing gave some conflicts. I've fixed those up.
> 
> Otherwise everything else remains the same.

I've gone through the different versions of this series, and the first
thing I want to see is that it has improved very nicely over time. Nice
job !

Configuring zoom through an absolute crop rectangle is I believe the way
to go, so the overall approach is good in my opinion. There are however
a few points I'd like to discuss, related to the SensorOutputSize
property and IspCrop control.

Reading through the discussion, I become increasingly aware of a topic
that was present in the background without ever being named, and that's
the camera pipeline model. As mentioned yesterday in a few other replies
to selected patches in this series and its previous versions, the job of
libcamera is to expose to applications a camera model that abstract
hardware differences. This is not an easy job, as we have to strike the
right balance between a higher-level inflexible but simple to use model
that can't support many of the less common use cases, and a lower-level
powerful but complex to use model that exposes a very large range of
hardware features. We have implicitly defined the skeleton of such a
model through the API of the Camera and PipelineHandler classes, but
have never made it explicit.

This needs to change. I see no reason to block the digital zoom feature
until we finish documenting the pipeline model, but I would like to
design the feature while thinking about the bigger picture. Here are the
assumptions I think the pipeline model should make for devices that
support digital zoom.

- The camera pipeline starts with a camera  sensor, that may implement
  binning, skipping and/or cropping.

- The subsequent blocks in the pipeline may perform additional cropping,
  either at the direct command of the pipeline handler (e.g. cropping at
  the input of the scaler), or automatically to support image processing
  steps (e.g. colour interpoloation often drops a few lines and columns
  on all edges of the image).

- The pipeline ends with a scaler that can implement digital zoom
  through a combination of cropping followed by scaling.

The exact order of the processing steps at the hardware level doesn't
need to match the pipeline model. For instance, cropping at the input
and output of the scaler are interchangeable (not taking into account
sub-pixel differences). It doesn't matter if the ISP scales before
cropping the output, the hardware scaler parameters and output crop
rectangle can be computed from an abstract input crop rectangle and
output size. This is crucial to consider for the definition of the
pipeline model: we need to design it in a way that ensures all features
can be mapped to how they are implemented in the different types of
hardware we want to support, but we're otherwise free to not map
controls and properties 1:1 with the hardware parameters. When multiple
options are possible, we should be guided by API design criteria such as
coherency, simplicity and flexibility.

Coming back to digital zoom, this series exposes a new SensorOutputSize
property and a new IspCrop control. The SensorOutpuSize property is
introduced to support the IspCrop control, as the base rectangle in
which the scaler can crop. There are two issues here that bother me:

- The property, despite being named SensorOutputSize, potentially takes
  into account the cropping added by the CSI-2 receiver and by the ISP
  for operations that consume lines and columns on the edges of the
  image. The naming can create some confusion, which can possibly be
  addressed by a combination of documentation (you're covering that
  already) and possibly a more explicit name for the property. However,
  as the property bundles crop operations perfomed in different stages
  of the pipeline, I'm worried that it will turn out to be a bit
  ill-defined regardless of how well we document it, with slightly
  different behaviours in different implementations.

- Ignoring the additional cropping performed in the CSI-2 receiver and
  ISP (if any), the property exposes the sensor binning, skipping and
  cropping. It bundles those three operations together, and thus doesn't
  convey how the cropping affects the field of view as a given output
  size can be achieved with different combinations of skipping/binning
  and cropping.

For these reasons, I'm concerned that the SensorOutputCrop property is a
ad-hoc solution to provide a reference for the IspCrop property, and
will not fit clearly in a pipeline model that should be based on simple,
base building blocks. I would thus like to propose an alternative
option.

Instead of expressing the IspCrop controls (which I think we should
rename to ScalerCrop) relatively to the SensorOutputSize property, could
we express it relatively to the existing PixelArrayActiveAreas property
? This would have the advantage, in my opinion, of abstracting binning
and skipping from applications. The pipeline handlers would need to
perform a bit more work to compute the crop rectangle actually applied
to the scaler, in order to take sensor binning/skipping and all
additional cropping in the pipeline into account. The upside is that the
ScalerCrop will directly define how the field of view is affected. It
would also simplify the API, as no intermediate property between
PixelArrayActiveAreas and ScalerCrop would need to be defined, and the
ScalerCrop coordinates wouldn't depend on the active camera
configuration. I think this would be easier to clearly document as part
of a camera pipeline model.

Two additional points I'd like to consider (and which are orthogonal to
the previous one) are:

- Should we automatically adjust the ScalerCrop rectangle to always
  output square pixels, or should we allow modifying the aspect ratio
  when scaling ? Most use cases call for square pixels, but I don't
  think we necessarily want to create an artificial limitation here (as
  long as we make it easy for applications to compute the scaling
  parameters that will give them square pixels)/

- Should we allow a ScalerCrop rectangle smaller than the stream output
  size, or should we restrict scaling to down-scaling only ?

> David Plowman (6):
>   libcamera: Add SensorOutputSize property
>   libcamera: Initialise the SensorOutputSize property
>   libcamera: Add IspCrop control
>   libcamera: Add geometry helper functions
>   libcamera: pipeline: raspberrypi: Implementation of digital zoom
>   cam: Add command line option to test IspCrop control
> 
>  include/libcamera/geometry.h                  |  20 +++
>  include/libcamera/ipa/raspberrypi.h           |   1 +
>  src/cam/capture.cpp                           |  25 +++-
>  src/cam/capture.h                             |   2 +-
>  src/cam/main.cpp                              |   3 +
>  src/cam/main.h                                |   1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           |   7 +
>  src/libcamera/camera_sensor.cpp               |   6 +
>  src/libcamera/control_ids.yaml                |  12 ++
>  src/libcamera/geometry.cpp                    | 129 ++++++++++++++++++
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  47 +++++++
>  src/libcamera/property_ids.yaml               |  19 +++
>  12 files changed, 269 insertions(+), 3 deletions(-)

-- 
Regards,

Laurent Pinchart