[libcamera-devel] [PATCH v2 3/5] libcamera: raspberrypi: Set camera flips correctly from user transform

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Thu, Aug 06, 2020 at 05:36:37PM +0100, David Plowman wrote:
> The Raspberry Pi pipeline handler allows all transforms except those
> involving a transpose. The user transform is combined with any
> inherent rotation of the camera, and the camera's H and V flip bits
> are set accordingly.
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 20 +++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 236aa5c..9d183e3 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -324,6 +324,8 @@ public:
>  	uint32_t expectedSequence_;
>  	bool sensorMetadata_;
>  
> +	Transform transform_;
> +
>  	/*
>  	 * All the functions in this class are called from a single calling
>  	 * thread. So, we do not need to have any mutex to protect access to any
> @@ -400,8 +402,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  	if (config_.empty())
>  		return Invalid;
>  
> -	if (transform != Transform::Identity) {
> -		transform = Transform::Identity;
> +	/* We cannot do Transforms with a transpose in them. */
> +	if (!!(transform & Transform::Transpose)) {
> +		transform = transform ^ Transform::Transpose;

I wonder if we should define a ~ operator to be able to write

		transform = transform & ~Transform::Transpose;

And maybe also making the following possible ?

		transform &= ~Transform::Transpose;

>  		status = Adjusted;
>  	}
>  
> @@ -610,6 +613,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	for (auto const stream : data->streams_)
>  		stream->reset();
>  
> +	/* We will want to know the transform requested by the application. */
> +	data->transform_ = config->transform;
> +
>  	Size maxSize, sensorSize;
>  	unsigned int maxIndex = 0;
>  	bool rawStream = false;
> @@ -1174,8 +1180,14 @@ int RPiCameraData::configureIPA()
>  		/* Configure the H/V flip controls based on the sensor rotation. */
>  		ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
>  		int32_t rotation = sensor_->properties().get(properties::Rotation);
> -		ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> -		ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> +		bool success;
> +		Transform combined = transform_ * transformFromRotation(rotation, &success);
> +		if (!success) {
> +			LOG(RPI, Error) << "Invalid rotation: " << rotation;
> +			return -EINVAL;
> +		}
> +		ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(combined & Transform::HFlip)));
> +		ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(combined & Transform::VFlip)));

Shouldn't this code guard against a 90° or 270° rotation ? Actually,
thinking about it, what if the sensor is mounted with a 90° rotation,
and the user is fine with that ? How should we combine that with the
user transform to make it possible to capture images ?

>  		unicam_[Unicam::Image].dev()->setControls(&ctrls);
>  	}
>  

-- 
Regards,

Laurent Pinchart