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

[Laurent Pinchart]

Hi David,

Another small comment.

On Tue, Sep 01, 2020 at 03:42:14AM +0300, Laurent Pinchart wrote:
> 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. */
> > +		/* 

There's a trailing white space here.

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