[libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do not clear camera flips when listing formats

[Jacopo Mondi]

Hi David

On Thu, Nov 03, 2022 at 10:40:27AM +0000, David Plowman via libcamera-devel wrote:
> Previously the code used to clear the camnera's h and v flip bits when
> enumerating the supported formats so as to obtain any Bayer formats in
> the sensor's native (untransformed) orientation. However this fails
> when the camera is already in use elsewhere.
>
> Instead, we query the current state of the flip bits and transform the
> formats - which we obtain in their flipped orientation - back into
> their native orientation to be stored.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 572a313a..6670dfb9 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -19,6 +19,8 @@
>
>  #include <libcamera/base/utils.h>
>
> +#include <libcamera/transform.h>
> +
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/camera_lens.h"
>  #include "libcamera/internal/camera_sensor_properties.h"
> @@ -108,18 +110,51 @@ int CameraSensor::init()
>  		return ret;
>
>  	/*
> -	 * Clear any flips to be sure we get the "native" Bayer order. This is
> -	 * harmless for sensors where the flips don't affect the Bayer order.
> +	 * We want to get the native mbus codes for the sensor, without any flips.
> +	 * We can't clear any flips here, so we have to read the current values
> +	 * (if the flip controls exist), decide whether they actually modify any
> +	 * output Bayer pattern, and finally undo their effect on the formats.
> +	 *
> +	 * First, check if the flip controls exist and if so read them.
>  	 */
>  	ControlList ctrls(subdev_->controls());
> -	if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())
> -		ctrls.set(V4L2_CID_HFLIP, 0);
> -	if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())
> -		ctrls.set(V4L2_CID_VFLIP, 0);
> -	subdev_->setControls(&ctrls);
> +	std::vector<uint32_t> flipCtrlIds;
> +	bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();
> +	bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();
> +	if (hasHflip)
> +		flipCtrlIds.push_back(V4L2_CID_HFLIP);
> +	if (hasVflip)
> +		flipCtrlIds.push_back(V4L2_CID_VFLIP);
> +	ControlList flipCtrls = subdev_->getControls(flipCtrlIds);
> +
> +	/* Now construct a transform that would undo any flips. */
> +	Transform transform = Transform::Identity;
> +	if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get<int>()) {
> +		const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);
> +		if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)
> +			transform |= Transform::HFlip;
> +	}
> +	if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get<int>()) {
> +		const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);
> +		if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)
> +			transform |= Transform::VFlip;
> +	}

Might avoid a few lookups with:

	const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP);
	const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP);
	if (hflipInfo)
		flipCtrlIds.push_back(V4L2_CID_HFLIP);
	if (vflipInfo)
		flipCtrlIds.push_back(V4L2_CID_VFLIP);
	ControlList flipCtrls = subdev_->getControls(flipCtrlIds);

	/* Now construct a transform that would undo any flips. */
	Transform transform = Transform::Identity;
	if (hflipInfo && flipCtrls.get(V4L2_CID_HFLIP).get<int>() &&
	    (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))
		transform |= Transform::HFlip;
	if (vflipInfo && flipCtrls.get(V4L2_CID_VFLIP).get<int>() &&
	    (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))
		transform |= Transform::VFlip;


Tested with a sensor without flips and with flips but not flag


> +
> +	/* Finally get the formats, and apply the transform to the mbus codes. */
> +	auto formats = subdev_->formats(pad_);
> +	for (const auto &format : formats) {
> +		unsigned int mbusCode = format.first;
> +		BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> +		bool valid = true;
> +
> +		if (bayerFormat.isValid())
> +			mbusCode = bayerFormat.transform(transform).toMbusCode(valid);
> +
> +		if (valid)
> +			formats_[mbusCode] = std::move(format.second);
> +	}
>
>  	/* Enumerate, sort and cache media bus codes and sizes. */
> -	formats_ = subdev_->formats(pad_);
>  	if (formats_.empty()) {
>  		LOG(CameraSensor, Error) << "No image format found";
>  		return -EINVAL;
> --
> 2.30.2
>