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

[Kieran Bingham]

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?


>  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?


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?




> + *
> + * \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...


> +}
> +
>  /**
>   * \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