[libcamera-devel,RFC,1/4] libcamera: Add SensorOutputSize property

9512 diff mbox series

The SensorOutputSize camera property changes with the selected camera
mode, so it must be updated when a new mode is chosen.
---
 src/libcamera/camera_sensor.cpp                    | 3 +++
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++
 src/libcamera/property_ids.yaml                    | 6 ++++++
 3 files changed, 11 insertions(+)

Hi David,

On Mon, Sep 07, 2020 at 05:44:47PM +0100, David Plowman wrote:
> The SensorOutputSize camera property changes with the selected camera
> mode, so it must be updated when a new mode is chosen.

nitpicking: I would make a separate patch that adds the property
definition.

> ---
>  src/libcamera/camera_sensor.cpp                    | 3 +++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++
>  src/libcamera/property_ids.yaml                    | 6 ++++++
>  3 files changed, 11 insertions(+)
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index d2679a4..b1c218e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -249,6 +249,9 @@ int CameraSensor::init()
>  		propertyValue = 0;
>  	properties_.set(properties::Rotation, propertyValue);
>
> +	/* The SensorOutputSize is known once the camera mode is chosen. */
> +	properties_.set(properties::SensorOutputSize, Size(0, 0));
> +

Wouldn't it be better to initialize the property reading the format applied
to the sensor instead of reporting (0,0) ?

True that before any configuration have been applied, it has a limited
value knowing it, but I feel it's still safer.


>  	/* Enumerate, sort and cache media bus codes and sizes. */
>  	formats_ = subdev_->formats(pad_);
>  	if (formats_.empty()) {
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index ce43af3..7f151cb 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -646,6 +646,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	LOG(RPI, Info) << "Sensor: " << camera->id()
>  		       << " - Selected mode: " << sensorFormat.toString();
>
> +	data->properties_.set(properties::SensorOutputSize, sensorFormat.size);
> +

I guess this is fine for now. We left the option of augmenting
CameraConfiguration with properties dandling for quite some time now.

It's my personal opinion this is acceptable at the moment and we
should not block this feature, but once we have augmented CameraConfiguration
with properties this one shall be set at validate() time
it should be moved there. Could you add:

        \todo Move this property to CameraConfiguration when that
        feature will be made available and set it validate() time
?


>  	/*
>  	 * This format may be reset on start() if the bayer order has changed
>  	 * because of flips in the sensor.
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 74ad019..ed1df19 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -640,4 +640,10 @@ controls:
>          \todo Rename this property to ActiveAreas once we will have property
>                categories (i.e. Properties::PixelArray::ActiveAreas)
>
> +  - SensorOutputSize:
> +      type: Size
> +      description: |
> +        The size, in pixels, of the image being output by the sensor in this
> +        camera mode.
> +

A Size or a rectangle reporting the offset from the active pixel
array top-left corner ?

Reporting a Rectangle might help calculating the FOV, but it's
probably not enough wihtout reporting information on the sensor
processing pipeline (analog crop, scaling/binning etc) all information
we currently report to IPAs through CameraSensorInfo. I guess for the
time being it's fine just reporting the Size to keep things simple...

Another question is, for the purpose of this property, the here
reported size might be obtained by cropping a larger frame in the
CSI-2 receiver so 'Sensor' might be slightly mis-leading in the name.
Expanding the documentation might help clarify this.

        The size, in pixels of the image being used to produce the
        desired output streams. The image size might correspond to the
        size of the frames produced by the image sensor or might take
        into account additional cropping (or even re-scaling)
        performed by the CSI-2 receiver to adjust the sensor frame
        size to conform to the output images ratio. This property
        might be used to implement digital zoom.

Take anything you consider meaningful and fix anything that sounds
weird in English :)

Thanks
  j


>  ...
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel