[libcamera-devel] [PATCH v2 08/12] libcamera: pipeline: raspberrypi: Set sensor flip based on rotation

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jul 06, 2020 at 11:16:11AM +0200, Jacopo Mondi wrote:
> On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:
> > Instead of receiving sensor orientation configuration from the IPA,
> > retrieve it from the CameraSensor Rotation property, and configure the
> > HFLIP and VFLIP controls accordingly.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 74bee6895dcd..fda6831e6a7e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -16,6 +16,7 @@
> >  #include <libcamera/formats.h>
> >  #include <libcamera/ipa/raspberrypi.h>
> >  #include <libcamera/logging.h>
> > +#include <libcamera/property_ids.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >
> > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()
> >  	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> >  			nullptr);
> >
> > +	/* 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));
> 
> mmm, I see in the IPA this, which is where I assume you got this from
> 
> 		ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));
> 		ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));
> 
> Where the 'orientation' flag is retrieved from
> 		RPi::CamTransform orientation = helper_->GetOrientation();
> 
> and expressed through members of this enumeration
>                 static constexpr CamTransform CamTransform_IDENTITY = 0;
>                 static constexpr CamTransform CamTransform_HFLIP    = 1;
>                 static constexpr CamTransform CamTransform_VFLIP    = 2;
> 
> Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[
> interval, shouldn't we:
>         if (rotation == 180)
>                 HFLIP
>         if (rotation == 90 || roation == 270)
>                 VFLIP
>  ?

90° or 270° rotations can't be achieved through flipping, they require a
real rotation engine. Only 180° can be achieved with H+V flip. Note that
the RPi IPA currently sets either both or neither of the flip controls,
based on hard-coded per-sensor values.

> > +	unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > +
> >  	return 0;
> >  }
> >
> > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >  					      { V4L2_CID_EXPOSURE, action.data[1] } });
> >  			sensorMetadata_ = action.data[2];
> >  		}
> > -
> > -		/* Set the sensor orientation here as well. */
> > -		ControlList controls = action.controls[0];
> > -		unicam_[Unicam::Image].dev()->setControls(&controls);
> 
> Do you think we can now remove the custom rotation handling from the
> IPA helpers ?

See other patches in this series :-)

> >  		return;
> >  	}
> >

-- 
Regards,

Laurent Pinchart