[libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital zoom

[David Plowman]

Hi Kieran

Thanks for your feedback. Apologies for the delay in replying - I've
been away for the past week!

I agree there's still some stuff to figure out here! Let me try and
answer some of the questions (and probably pose some more!):

On Fri, 10 Jul 2020 at 14:12, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi David,
>
> On 09/07/2020 10:15, David Plowman wrote:
> > These changes add a Digital Zoom control, tacking a rectangle as its
>
> s/tacking/taking/
>
> > argument, indicating the region of the sensor output that will be
> > zoomed up to the final output size.
> >
> > Additionally, we need have a method returning the "sensorCrop" which
>
> s/need have/need to have/
>
> > gives the dimensions of the sensor output within which we can pan and
> > zoom.
>
>
> This commit message describes implementing digital zoom, but *this*
> commit is only dealing with the representaion of a crop region in the
> (CameraSensor) which can be used to implement digital zoom.
>
> Perhaps this commit should be titled:
>  "libcamera: camera_sensor: Support cropping regions"
>
> (with that being the assumption that my comments below about moving this
> to the CameraSensor class are correct)
>
>
>
> > ---
> >  include/libcamera/camera.h                    |  2 ++
> >  include/libcamera/internal/pipeline_handler.h |  4 +++
> >  src/libcamera/camera.cpp                      | 26 +++++++++++++++++++
> >  src/libcamera/control_ids.yaml                | 10 +++++++
> >  4 files changed, 42 insertions(+)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 4d1a4a9..dd07f7a 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -92,6 +92,8 @@ public:
> >       std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> >       int configure(CameraConfiguration *config);
> >
> > +     Size const &getSensorCrop() const;
> > +
> >       Request *createRequest(uint64_t cookie = 0);
> >       int queueRequest(Request *request);
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 22e629a..37e069a 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -89,6 +89,8 @@ public:
> >
> >       const char *name() const { return name_; }
> >
> > +     Size const &getSensorCrop() const { return sensorCrop_; }
> > +
> >  protected:
> >       void registerCamera(std::shared_ptr<Camera> camera,
> >                           std::unique_ptr<CameraData> data);
> > @@ -100,6 +102,8 @@ protected:
> >
> >       CameraManager *manager_;
> >
> > +     Size sensorCrop_;
> > +
>
> I think this needs to go in the CameraSensor class...
> It's 'per sensor' not 'per pipeline handler' right?

Maybe this is the billion dollar question. See below...!

>
>
> >  private:
> >       void mediaDeviceDisconnected(MediaDevice *media);
> >       virtual void disconnect();
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 69a1b44..6614c93 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -793,6 +793,32 @@ int Camera::configure(CameraConfiguration *config)
> >       return 0;
> >  }
> >
> > +/**
> > + * \brief Return the size of the sensor image being used to form the output
> > + *
> > + * This method returns the size, in pixels, of the raw image read from the
> > + * sensor and which is used to form the output image(s). Note that these
> > + * values take account of any cropping performed on the sensor output so
> > + * as to produce the correct aspect ratio. It would normally be necessary
> > + * to retrieve these values in order to calculate correct parameters for
> > + * digital zoom.
> > + *
> > + * Example: a sensor mode may produce a 1920x1440 output image. But if an
> > + * application has requested a 16:9 image, the values returned here would
> > + * be 1920x1080 - the largest portion of the sensor output that provides
> > + * the correct aspect ratio.
>
> Should this be a Rectangle? Can the crop region be positioned
> arbitrarily or is it always centered?

As I've implemented this, it is just a pair of dimensions. The
pipeline handler may position it anywhere it likes within the array of
pixels output by the sensor (and takes care of any offset within
that), but the application doesn't need to know. The idea being that
the application gets to choose a *rectangle* from within the
dimensions it is given and doesn't have to bother itself with how it's
offset in the pixel array beyond that.

>
>
> What is the relationship between this and the analogCrop in
> CameraSensorInfo? I 'think' that one is more about what the actual valid
> data region is from the camera, so I presume the analogCrop is sort of
> the 'maximum' image size?

Yes, I think the analogCrop is the maximum usable pixel area from the
sensor. The crop here is what the pipeline handler decides it wants to
use. It's adjusted for aspect ratio (of the requested output), the
actual size that it wants to use (and hence the scaling). I certainly
agree that it might want putting somewhere else... yet it's only the
pipeline handler that can calculate it. It depends on the sensor, but
it also depends on the mode selected, the output size requested, and
"arbitrary" decisions made in the pipeline handler (for example, they
may wish to avoid marginal scaling and prefer 1:1 input to output
pixels).

>
>
>
>
> > + *
> > + * \context This function is \threadsafe. It will only return valid
> > + * (non-zero) values when the camera has been configured.
> > + *
> > + * \return The dimensions of the sensor image in use.
> > + */
> > +
> > +Size const &Camera::getSensorCrop() const
> > +{
> > +     return p_->pipe_->getSensorCrop();
>
> Aha, more indication to me that this should go in to the Camera class,
> and it would save the indirection required here...

Indeed. But given that the pipeline handler calculates it, can we have
it poke its value into the Camera class? Or is that weird if the
number depends on rather more than just the camera? (And will change
every time we start the sensor in a different mode.)

Another solution to this that I've talked about previously is simply
to go use ratios, and avoid pixel coordinates altogether. Then you
just don't need this function. The counter argument is that an
application can't control digital pan/zoom down to the precise pixel -
which sounds appealing. Yet I think real applications will have in
mind a _rate_ at which they wish to work (e.g. pan across half the
field of view in 5 seconds) at which point ratios are quite natural.

Best regards
David

>
>
> > +}
> > +
> >  /**
> >   * \brief Create a request object for the camera
> >   * \param[in] cookie Opaque cookie for application use
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 988b501..5a099d5 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -262,4 +262,14 @@ controls:
> >          In this respect, it is not necessarily aimed at providing a way to
> >          implement a focus algorithm by the application, rather an indication of
> >          how in-focus a frame is.
> > +
> > +  - DigitalZoom:
> > +      type: Rectangle
> > +      description: |
> > +        Sets the portion of the full sensor image, in pixels, that will be
> > +        used for digital zoom. That is, this part of the sensor output will
> > +        be scaled up to make the full size output image (and everything else
> > +        discarded). To obtain the "full sensor image" that is available, the
> > +        method Camera::getOutputCrop() should be called once the camera is
> > +        configured. An application may pan and zoom within this rectangle.
> >  ...
> >
>
> --
> Regards
> --
> Kieran