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

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Fri, Aug 21, 2020 at 04:56:39PM +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.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 34 ++++++++++++++++---
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 236aa5c..a3f8438 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,27 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  	if (config_.empty())
>  		return Invalid;
>  
> -	if (transform != Transform::Identity) {
> -		transform = Transform::Identity;
> +	/*
> +	 * What if the platform has a non-90 degree rotation? We can't even
> +	 * "adjust" the configuration and carry on. Alternatively, raising an
> +	 * error means the platform can never run. Let's just print a warning
> +	 * and continue regardless; the rotation is effectively set to zero.
> +	 */
> +	int32_t rotation = data_->sensor_->properties().get(properties::Rotation);
> +	bool success;
> +	Transform combined = transform * transformFromRotation(rotation, &success);
> +	if (!success)
> +		LOG(RPI, Warning) << "Invalid rotation of " << rotation
> +				  << " degrees - ignoring";
> +
> +	/*
> +	 * We combine the platform and user transform, but must "adjust away"
> +	 * any combined result that includes a transform, as we can't do those.
> +	 * Flipping the transpose bit in either input transform causes the
> +	 * corresponding bit in the combined result to flip.
> +	 */
> +	if (!!(combined & Transform::Transpose)) {
> +		transform ^= Transform::Transpose;
>  		status = Adjusted;
>  	}

I wonder if this wouldn't be confusing for the application. Imagine the
following use case. We have a device with a screen, meant to operate in
portrait mode. The camera sensor will typically be mounted with a 90°
(or 270°) rotation, in order to match the aspect ratio of the scene and
the screen (otherwise the scene would be captured in landscape mode and
couldn't be displayed full-screen). Assuming the ISP can't transpose, as
in the Raspberry Pi case the application will have to rotate the image
by 90° before displaying it. Let's further assume the user doesn't need
to apply any h/v flip on the camera side.

transformFromRotation() returns Rot90 or Rot270. As transform is set to
Identity by the application, combined is equal to Rot90 or Rot270, which
has the Transpose bit set. The code above will XOR the Transpose bit
out, leaving transform set to HFlip or VFlip. This seems an unexpect
side effect to me.

We could of course argue that the application should look at the
Rotation property and compensate for the 90° rotation by requesting
Rot90 or Rot270, but is that the best option, especially given that we
don't give a way to applications to enumerate what transformations are
supported. Maybe this is good enough for now as we don't really claim to
support 90° or 270° rotations yet, but I feel this will need to be
revisited sooner than later.

>  
> @@ -610,6 +631,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 +1198,10 @@ 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));
> +		/* The rotation was already checked in RPiCameraConfiguration::validate. */
> +		Transform combined = transform_ * transformFromRotation(rotation);
> +		ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(combined & Transform::HFlip)));
> +		ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(combined & Transform::VFlip)));
>  		unicam_[Unicam::Image].dev()->setControls(&ctrls);
>  	}
>  

-- 
Regards,

Laurent Pinchart