[libcamera-devel] [PATCH v5 6/8] libcamera: raspberrypi: Set camera flips correctly from user transform

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Sat, Aug 29, 2020 at 12:54:27PM +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.
> 
> Note that the validate() method has to work out what the final Bayer
> order of any raw streams will be, before configure() actually applies
> the transform to the sensor.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 82 ++++++++++++++++---
>  1 file changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index c554532..333aa94 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -294,7 +294,7 @@ public:
>  	void frameStarted(uint32_t sequence);
>  
>  	int loadIPA();
> -	int configureIPA();
> +	int configureIPA(CameraConfiguration *config);
>  
>  	void queueFrameAction(unsigned int frame, const IPAOperationData &action);
>  
> @@ -400,8 +400,34 @@ 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.
> +	 * In this case, flipping only the transpose bit is helpful to
> +	 * applications - they either get the transform they requested, or have
> +	 * to do a simple transpose themselves (they don't have to worry about
> +	 * the other possible cases).
> +	 */
> +	if (!!(combined & Transform::Transpose)) {
> +		/*
> +		 * Flipping the transpose bit in "transform" flips it in
> +		 * combined result too (as it's the last thing that happens).
> +		 */
> +		transform ^= Transform::Transpose;
> +		combined ^= Transform::Transpose;
>  		status = Adjusted;
>  	}

Shouldn't thus logic handle the case where the sensor can't flip ? I
think we can keep it simple that considering that either both or neither
flips are supported.

>  
> @@ -414,13 +440,42 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  			 * Calculate the best sensor mode we can use based on
>  			 * the user request.
>  			 */
> -			V4L2VideoDevice::Formats fmts = data_->unicam_[Unicam::Image].dev()->formats();
> +			V4L2VideoDevice *dev = data_->unicam_[Unicam::Image].dev();
> +			V4L2VideoDevice::Formats fmts = dev->formats();
>  			V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);
> -			int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);
> +			int ret = dev->tryFormat(&sensorFormat);
>  			if (ret)
>  				return Invalid;
>  
> -			PixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat();
> +			/*
> +			 * Some sensors change their Bayer order when they are
> +			 * h-flipped or v-flipped, according to the transform.
> +			 * If the controls own up to "modifying the layout" we
> +			 * will assume that's what is going on and advertise
> +			 * the transformed Bayer order in the stream.
> +			 */
> +			V4L2PixelFormat fourcc = sensorFormat.fourcc;
> +
> +			/*
> +			 * The camera may already be transformed, so we must
> +			 * only transform the fourcc if the new transform is
> +			 * different.
> +			 */
> +			ControlList ctrls = dev->getControls({ V4L2_CID_HFLIP, V4L2_CID_VFLIP });
> +
> +			const struct v4l2_query_ext_ctrl *hflipCtrl = dev->queryControl(V4L2_CID_HFLIP);
> +			if (hflipCtrl &&
> +			    (hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) &&
> +			    ctrls.get(V4L2_CID_HFLIP).get<int32_t>() != !!(combined & Transform::HFlip))
> +				fourcc = fourcc.transform(Transform::HFlip);
> +
> +			const struct v4l2_query_ext_ctrl *vflipCtrl = dev->queryControl(V4L2_CID_VFLIP);
> +			if (vflipCtrl &&
> +			    (vflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) &&
> +			    ctrls.get(V4L2_CID_VFLIP).get<int32_t>() != !!(combined & Transform::VFlip))
> +				fourcc = fourcc.transform(Transform::VFlip);

To simplify this, would it make sense to reset the h/v flip controls at
init time, and then read and store the bayer format ? That would be the
native bayer format, and we would only need to care about the
V4L2_CTRL_FLAG_MODIFY_LAYOUT flags here.

If we consider it safe to assume that V4L2_CTRL_FLAG_MODIFY_LAYOUT will
be set for either both or neither flip controls, the code would be
simplified to

			V4L2PixelFormat fourcc = data_->sensorFormat_;

			const struct v4l2_query_ext_ctrl *hflipCtrl = dev->queryControl(V4L2_CID_HFLIP);
			const struct v4l2_query_ext_ctrl *vflipCtrl = dev->queryControl(V4L2_CID_VFLIP);

			if (hflipCtrl && vflipCtrl &&
			    (hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) &&
			    (vflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))
				fourcc = fourcc.transform(combined);

> +
> +			PixelFormat sensorPixFormat = fourcc.toPixelFormat();
>  			if (cfg.size != sensorFormat.size ||
>  			    cfg.pixelFormat != sensorPixFormat) {
>  				cfg.size = sensorFormat.size;
> @@ -756,7 +811,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	crop.y = (sensorFormat.size.height - crop.height) >> 1;
>  	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
>  
> -	ret = data->configureIPA();
> +	ret = data->configureIPA(config);
>  	if (ret)
>  		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
>  
> @@ -1112,7 +1167,7 @@ int RPiCameraData::loadIPA()
>  	return ipa_->init(settings);
>  }
>  
> -int RPiCameraData::configureIPA()
> +int RPiCameraData::configureIPA(CameraConfiguration *config)
>  {
>  	std::map<unsigned int, IPAStream> streamConfig;
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> @@ -1170,11 +1225,16 @@ int RPiCameraData::configureIPA()
>  			sensorMetadata_ = result.data[2];
>  		}
>  
> -		/* Configure the H/V flip controls based on the sensor rotation. */
> +		/* 
> +		 * Configure the H/V flip controls based on the sensor rotation
> +		 * and user transform.
> +		 */
>  		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 angle was already checked in validate(). */
> +		Transform combined = config->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