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

[Laurent Pinchart]

Hi Kieran,

On Fri, Aug 28, 2020 at 04:56:17PM +0100, Kieran Bingham wrote:
> On 28/08/2020 15:41, 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      | 42 +++++++++++++++----
> >  1 file changed, 34 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index dc36f53..6ea1432 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -400,20 +400,46 @@ 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.
> > +	 */
> 
> Should we clamp to the 'nearest' value? I don't imagine it's helpful
> either way ...

I don't think we support non-90° rotations anywhere, so I wouldn't worry
about them. If we had to support that, I think we'd need a different
control to report a 3D pose (translation and rotation) of the sensor.

> > +	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)) {
> 
> Are the !! operators required here, does the underlying value not
> represent something that can be interpreted as boolean otherwise?

Unfortunately not, it's an enum class, not an enum.

> I don't think I object to the double ! operator, just curious about the
> 'requirement' of it.
> 
> > +		/*
> > +		 * 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;
> >  	}
> >  
> >  	/*
> > -	 * Configure the H/V flip controls based on the sensor rotation. We do
> > -	 * this here so that the sensor has the correct Bayer format that will
> > -	 * get advertised in the configuration of any raw streams.
> > +	 * Configure the H/V flip controls based on the combination of the
> > +	 * sensor rotation and the user transform. We do this here so that the
> > +	 * sensor has the correct Bayer format that will get advertised in the
> > +	 * configuration of any raw streams.
> >  	 */
> >  	ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls());
> > -	int32_t rotation = data_->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));
> > +	ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(combined & Transform::HFlip)));
> > +	ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(combined & Transform::VFlip)));
> >  	data_->unicam_[Unicam::Image].dev()->setControls(&ctrls);
> 
> Ah, that hflip/vflip is much more readable now :-)
> 
> As highlighted from Laurent though, setting this in validate feels quite
> wrong to me.
> 
> I'm not sure if a stream could be actively running while we validate a
> configuration, but in theory I think that's the aim - you should be able
> to have an active stream - and validate the next configuration without
> interfering with a currently active stream...
> 
> (Perhaps for example, while determining if a reconfiguration is
> possible, without tearing down the stream until you have a valid set of
> parameters to apply).
> 
> >  
> >  	unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
> > 

-- 
Regards,

Laurent Pinchart