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

[Kieran Bingham]

Hi David,

On 02/09/2020 11:44, 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. We make a note of the "native"
> (untransformed) Bayer order when the system starts, so that we can
> deduce transformed Bayer orders more easily.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 146 ++++++++++++++++--
>  1 file changed, 134 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 1087257..29112da 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -23,6 +23,7 @@
>  
>  #include <linux/videodev2.h>
>  
> +#include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/ipa_manager.h"
> @@ -287,6 +288,7 @@ class RPiCameraData : public CameraData
>  public:
>  	RPiCameraData(PipelineHandler *pipe)
>  		: CameraData(pipe), sensor_(nullptr), state_(State::Stopped),
> +		  supportsFlips_(false), flipsAlterBayerOrder_(false),
>  		  dropFrame_(false), ispOutputCount_(0)
>  	{
>  	}
> @@ -335,6 +337,17 @@ public:
>  	std::queue<FrameBuffer *> embeddedQueue_;
>  	std::deque<Request *> requestQueue_;
>  
> +	/*
> +	 * The remaining field are for handling horizontal and vertical flips

"The remaining fields"?
What would get confusing/ambiguous when new fields are added below... ?

Perhaps just
	'Manage horizontal and vertical flips supported by the sensor."

> +	 * that sensors may support.
> +	 */
> +	bool supportsFlips_;
> +	bool flipsAlterBayerOrder_;
> +	BayerFormat::Order nativeBayerOrder_;
> +
> +	Transform combinedTransform_;
> +	Transform userTransform_;

Shouldn't the combinedTransform_ and userTransform_ go in the
RPiCameraConfiguration ?

> +
>  private:
>  	void checkRequestCompleted();
>  	void tryRunPipeline();
> @@ -348,12 +361,12 @@ private:
>  class RPiCameraConfiguration : public CameraConfiguration
>  {
>  public:
> -	RPiCameraConfiguration(const RPiCameraData *data);
> +	RPiCameraConfiguration(RPiCameraData *data);
>  
>  	Status validate() override;
>  
>  private:
> -	const RPiCameraData *data_;
> +	RPiCameraData *data_;
>  };
>  
>  class PipelineHandlerRPi : public PipelineHandler
> @@ -388,7 +401,7 @@ private:
>  	MediaDevice *isp_;
>  };
>  
> -RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
> +RPiCameraConfiguration::RPiCameraConfiguration(RPiCameraData *data)
>  	: CameraConfiguration(), data_(data)
>  {
>  }
> @@ -400,11 +413,56 @@ 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.
> +	 */

It sounds like non-90 degree rotations are not permitted.

I don't mind keeping that comment, but you could also simplify it if you
wish to just:

 /*
  * Non-90 degree rotations are not permitted, and no rotation will be
  * reported(/used) for unsupported values.
  */

> +	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

'flips it in the'

> +		 * combined result too (as it's the last thing that happens),
> +		 * which is of course clearing it.
> +		 */
> +		transform ^= Transform::Transpose;
> +		combined &= ~Transform::Transpose;
>  		status = Adjusted;
>  	}
>  
> +	/*
> +	 * We also check if the sensor doesn't do h/vflips at all, in which
> +	 * case we clear them, and the application will have to do everything.
> +	 */
> +	if (!data_->supportsFlips_ && !!combined) {
> +		transform &= Transform::Transpose;
> +		combined = Transform::Identity;
> +		status = Adjusted;
> +	}
> +
> +	/*
> +	 * Store the final combined transform that configure() will need to
> +	 * apply to the sensor, and the associated user transform that gives
> +	 * rise to it (which is what the IPAs will see).
> +	 */
> +	data_->combinedTransform_ = combined;
> +	data_->userTransform_ = transform;
> +

This looks like it's modifying the state of the CameraData in validate.

I think that means that combinedTransform and userTransform belong in
the RPiCameraConfiguration,


>  	unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
>  	std::pair<int, Size> outSize[2];
>  	Size maxSize;
> @@ -420,7 +478,23 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  			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 this one does, we must advertise the transformed
> +			 * Bayer order in the raw stream. Note how we must
> +			 * fetch the "native" (i.e. untransformed) Bayer order,
> +			 * because the sensor may currently be flipped!
> +			 */
> +			V4L2PixelFormat fourcc = sensorFormat.fourcc;
> +			if (data_->flipsAlterBayerOrder_) {
> +				BayerFormat bayer(fourcc);
> +				bayer.order = data_->nativeBayerOrder_;
> +				bayer = bayer.transform(combined);
> +				fourcc = bayer.toV4L2PixelFormat();
> +			}
> +
> +			PixelFormat sensorPixFormat = fourcc.toPixelFormat();
>  			if (cfg.size != sensorFormat.size ||
>  			    cfg.pixelFormat != sensorPixFormat) {
>  				cfg.size = sensorFormat.size;
> @@ -967,6 +1041,48 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>  
> +	/*
> +	 * We cache three things about the sensor in relation to transforms
> +	 * (meaning horizontal and vertical flips).
> +	 *
> +	 * Firstly, does it support them?
> +	 * Secondly, if you use them does it affect the Bayer ordering?
> +	 * Thirdly, what is the "native" Bayer order, when no transforms are
> +	 * applied?
> +	 *
> +	 * As part of answering the final question, we reset the camera to
> +	 * no transform at all.
> +	 */
> +
> +	V4L2VideoDevice *dev = data->unicam_[Unicam::Image].dev();
> +	const struct v4l2_query_ext_ctrl *hflipCtrl = dev->controlInfo(V4L2_CID_HFLIP);
> +	if (hflipCtrl) {
> +		/* We assume it will support vflips too... */
> +		data->supportsFlips_ = true;
> +		data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +		ControlList ctrls(dev->controls());
> +		ctrls.set(V4L2_CID_HFLIP, 0);
> +		ctrls.set(V4L2_CID_VFLIP, 0);
> +		dev->setControls(&ctrls);
> +	}
> +
> +	/* Look for a valid Bayer format. */
> +	V4L2VideoDevice::Formats fmts = dev->formats();
> +	BayerFormat bayerFormat;
> +	for (const auto &iter : fmts) {
> +		V4L2PixelFormat v4l2Format = iter.first;
> +		bayerFormat = BayerFormat(v4l2Format);
> +		if (bayerFormat.isValid())
> +			break;
> +	}
> +
> +	if (!bayerFormat.isValid()) {
> +		LOG(RPI, Error) << "No Bayer format found";
> +		return false;
> +	}
> +	data->nativeBayerOrder_ = bayerFormat.order;
> +
>  	/*
>  	 * List the available output streams.
>  	 * Currently cannot do Unicam streams!
> @@ -1172,12 +1288,18 @@ int RPiCameraData::configureIPA()
>  			sensorMetadata_ = result.data[2];
>  		}
>  
> -		/* 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));
> -		unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +		/*
> +		 * Configure the H/V flip controls based on the combination of
> +		 * the sensor and user transform.
> +		 */
> +		if (supportsFlips_) {
> +			ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> +			ctrls.set(V4L2_CID_HFLIP,
> +				  static_cast<int32_t>(!!(combinedTransform_ & Transform::HFlip)));
> +			ctrls.set(V4L2_CID_VFLIP,
> +				  static_cast<int32_t>(!!(combinedTransform_ & Transform::VFlip)));
> +			unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +		}
>  	}
>  
>  	if (result.operation & RPI_IPA_CONFIG_SENSOR) {
> 

I think the bayer/flip/transform configuration state really needs to be
in the configuration - not the camera data objects, but other than
that.., the rest is trivial, so with that fixed (or if there's a good
reason why it can't...)

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


-- 
Regards
--
Kieran