[libcamera-devel,2/5] libcamera: raspberrypi: Apply transform and pass through to IPA

9061 diff mbox series

The user-supplied transform is applied to the camera, taking account
of any pre-existing rotation. It is then also plumbed through to the
IPA so that it will (in a further commit) be able to make use of
it.
---
 include/libcamera/ipa/raspberrypi.h           |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp           | 13 ++++++++-
 .../pipeline/raspberrypi/raspberrypi.cpp      | 27 +++++++++++++++++--
 3 files changed, 38 insertions(+), 3 deletions(-)

Hi David,

Thank you for the patch.

On Wed, Jul 29, 2020 at 10:31:51AM +0100, David Plowman wrote:
> The user-supplied transform is applied to the camera, taking account
> of any pre-existing rotation. It is then also plumbed through to the
> IPA so that it will (in a further commit) be able to make use of
> it.
> ---
>  include/libcamera/ipa/raspberrypi.h           |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           | 13 ++++++++-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 27 +++++++++++++++++--
>  3 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index ca62990..3905a4a 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -14,6 +14,7 @@ enum RPiConfigParameters {
>  	RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
>  	RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
>  	RPI_IPA_CONFIG_SENSOR = (1 << 2),
> +	RPI_IPA_CONFIG_TRANSFORM = (1 << 3),

I would drop this flag and always pass the transform value. You could
also store it first to avoid the next_element variable below.

>  };
>  
>  enum RPiOperations {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3747208..2809521 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -21,6 +21,7 @@
>  #include <libcamera/ipa/raspberrypi.h>
>  #include <libcamera/request.h>
>  #include <libcamera/span.h>
> +#include <libcamera/transform.h>
>  
>  #include <libipa/ipa_interface_wrapper.h>
>  
> @@ -144,6 +145,9 @@ private:
>  	/* LS table allocation passed in from the pipeline handler. */
>  	FileDescriptor lsTableHandle_;
>  	void *lsTable_;
> +
> +	/* This is the transform requested by the user/application driving libcamera. */
> +	Transform userTransform_;
>  };
>  
>  int IPARPi::init(const IPASettings &settings)
> @@ -282,6 +286,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	lastMode_ = mode_;
>  
>  	/* Store the lens shading table pointer and handle if available. */
> +	unsigned int next_element = 0;
>  	if (ipaConfig.operation & RPI_IPA_CONFIG_LS_TABLE) {
>  		/* Remove any previous table, if there was one. */
>  		if (lsTable_) {
> @@ -290,7 +295,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		}
>  
>  		/* Map the LS table buffer into user space. */
> -		lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
> +		lsTableHandle_ = FileDescriptor(ipaConfig.data[next_element++]);
>  		if (lsTableHandle_.isValid()) {
>  			lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
>  					MAP_SHARED, lsTableHandle_.fd(), 0);
> @@ -301,6 +306,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  			}
>  		}
>  	}
> +
> +	/* Fish out any transform set by the user/application. */
> +	if (ipaConfig.operation & RPI_IPA_CONFIG_TRANSFORM) {
> +		uint32_t transformType = ipaConfig.data[next_element++];
> +		userTransform_ = reinterpret_cast<Transform &>(transformType);

I think it would be best to just make the constructor that takes a
Transform::Type public.

> +	}
>  }
>  
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 82a0a4d..5fb427a 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -324,6 +324,9 @@ public:
>  	uint32_t expectedSequence_;
>  	bool sensorMetadata_;
>  
> +	/* This is the transform requested by the user/application driving libcamera. */

Could you wrap this line at 80 columns ?

> +	Transform userTransform_;
> +
>  	/*
>  	 * All the functions in this class are called from a single calling
>  	 * thread. So, we do not need to have any mutex to protect access to any
> @@ -400,6 +403,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  	if (config_.empty())
>  		return Invalid;
>  
> +	if (userTransform.contains(Transform::transpose()))
> +		return Invalid;

Shouldn't you adjust it instead ?

> +
>  	unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
>  	std::pair<int, Size> outSize[2];
>  	Size maxSize;
> @@ -609,6 +615,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	unsigned int maxIndex = 0;
>  	bool rawStream = false;
>  
> +	/* Record the transform requested by the application. */
> +	data->userTransform_ = config->userTransform;
> +
>  	/*
>  	 * Look for the RAW stream (if given) size as well as the largest
>  	 * ISP output size.
> @@ -1140,6 +1149,11 @@ int RPiCameraData::configureIPA()
>  		ipaConfig.data = { static_cast<unsigned int>(lsTable_.fd()) };
>  	}
>  
> +	/* We must pass the user transform to the IPA too. */
> +	uint32_t transformType = reinterpret_cast<uint32_t &>(userTransform_);
> +	ipaConfig.operation |= RPI_IPA_CONFIG_TRANSFORM;
> +	ipaConfig.data.push_back(transformType);
> +
>  	CameraSensorInfo sensorInfo = {};
>  	int ret = sensor_->sensorInfo(&sensorInfo);
>  	if (ret) {
> @@ -1168,8 +1182,17 @@ int RPiCameraData::configureIPA()
>  		/* 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));
> +
> +		/* The camera needs to compose the user transform with the rotation. */
> +		Transform rotationTransform;
> +		if (Transform::rotation(rotation, rotationTransform) == false)
> +			return -EINVAL;
> +		Transform transform = userTransform_ * rotationTransform;
> +
> +		ctrls.set(V4L2_CID_HFLIP,
> +			  static_cast<int32_t>(transform.contains(Transform::hflip())));
> +		ctrls.set(V4L2_CID_VFLIP,
> +			  static_cast<int32_t>(transform.contains(Transform::vflip())));
>  		unicam_[Unicam::Image].dev()->setControls(&ctrls);
>  	}
>

Hi Laurent

Thanks for the comments.

On Wed, 29 Jul 2020 at 16:19, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Wed, Jul 29, 2020 at 10:31:51AM +0100, David Plowman wrote:
> > The user-supplied transform is applied to the camera, taking account
> > of any pre-existing rotation. It is then also plumbed through to the
> > IPA so that it will (in a further commit) be able to make use of
> > it.
> > ---
> >  include/libcamera/ipa/raspberrypi.h           |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 13 ++++++++-
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 27 +++++++++++++++++--
> >  3 files changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index ca62990..3905a4a 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -14,6 +14,7 @@ enum RPiConfigParameters {
> >       RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> >       RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> >       RPI_IPA_CONFIG_SENSOR = (1 << 2),
> > +     RPI_IPA_CONFIG_TRANSFORM = (1 << 3),
>
> I would drop this flag and always pass the transform value. You could
> also store it first to avoid the next_element variable below.
>
> >  };
> >
> >  enum RPiOperations {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 3747208..2809521 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -21,6 +21,7 @@
> >  #include <libcamera/ipa/raspberrypi.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/span.h>
> > +#include <libcamera/transform.h>
> >
> >  #include <libipa/ipa_interface_wrapper.h>
> >
> > @@ -144,6 +145,9 @@ private:
> >       /* LS table allocation passed in from the pipeline handler. */
> >       FileDescriptor lsTableHandle_;
> >       void *lsTable_;
> > +
> > +     /* This is the transform requested by the user/application driving libcamera. */
> > +     Transform userTransform_;
> >  };
> >
> >  int IPARPi::init(const IPASettings &settings)
> > @@ -282,6 +286,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >       lastMode_ = mode_;
> >
> >       /* Store the lens shading table pointer and handle if available. */
> > +     unsigned int next_element = 0;
> >       if (ipaConfig.operation & RPI_IPA_CONFIG_LS_TABLE) {
> >               /* Remove any previous table, if there was one. */
> >               if (lsTable_) {
> > @@ -290,7 +295,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >               }
> >
> >               /* Map the LS table buffer into user space. */
> > -             lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
> > +             lsTableHandle_ = FileDescriptor(ipaConfig.data[next_element++]);
> >               if (lsTableHandle_.isValid()) {
> >                       lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
> >                                       MAP_SHARED, lsTableHandle_.fd(), 0);
> > @@ -301,6 +306,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >                       }
> >               }
> >       }
> > +
> > +     /* Fish out any transform set by the user/application. */
> > +     if (ipaConfig.operation & RPI_IPA_CONFIG_TRANSFORM) {
> > +             uint32_t transformType = ipaConfig.data[next_element++];
> > +             userTransform_ = reinterpret_cast<Transform &>(transformType);
>
> I think it would be best to just make the constructor that takes a
> Transform::Type public.

Indeed, except that I didn't like the public Type field...  oh dear...!

>
> > +     }
> >  }
> >
> >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 82a0a4d..5fb427a 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -324,6 +324,9 @@ public:
> >       uint32_t expectedSequence_;
> >       bool sensorMetadata_;
> >
> > +     /* This is the transform requested by the user/application driving libcamera. */
>
> Could you wrap this line at 80 columns ?
>
> > +     Transform userTransform_;
> > +
> >       /*
> >        * All the functions in this class are called from a single calling
> >        * thread. So, we do not need to have any mutex to protect access to any
> > @@ -400,6 +403,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >       if (config_.empty())
> >               return Invalid;
> >
> > +     if (userTransform.contains(Transform::transpose()))
> > +             return Invalid;
>
> Shouldn't you adjust it instead ?

As I said earlier, it wasn't clear to me that "adjusting" felt right.
But maybe it makes sense, I wonder what we adjust it to - perhaps we
just set it to "identity" if we can't do it, and then it becomes the
application's problem to do it in another way?

Thanks!
David

>
> > +
> >       unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
> >       std::pair<int, Size> outSize[2];
> >       Size maxSize;
> > @@ -609,6 +615,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >       unsigned int maxIndex = 0;
> >       bool rawStream = false;
> >
> > +     /* Record the transform requested by the application. */
> > +     data->userTransform_ = config->userTransform;
> > +
> >       /*
> >        * Look for the RAW stream (if given) size as well as the largest
> >        * ISP output size.
> > @@ -1140,6 +1149,11 @@ int RPiCameraData::configureIPA()
> >               ipaConfig.data = { static_cast<unsigned int>(lsTable_.fd()) };
> >       }
> >
> > +     /* We must pass the user transform to the IPA too. */
> > +     uint32_t transformType = reinterpret_cast<uint32_t &>(userTransform_);
> > +     ipaConfig.operation |= RPI_IPA_CONFIG_TRANSFORM;
> > +     ipaConfig.data.push_back(transformType);
> > +
> >       CameraSensorInfo sensorInfo = {};
> >       int ret = sensor_->sensorInfo(&sensorInfo);
> >       if (ret) {
> > @@ -1168,8 +1182,17 @@ int RPiCameraData::configureIPA()
> >               /* 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));
> > +
> > +             /* The camera needs to compose the user transform with the rotation. */
> > +             Transform rotationTransform;
> > +             if (Transform::rotation(rotation, rotationTransform) == false)
> > +                     return -EINVAL;
> > +             Transform transform = userTransform_ * rotationTransform;
> > +
> > +             ctrls.set(V4L2_CID_HFLIP,
> > +                       static_cast<int32_t>(transform.contains(Transform::hflip())));
> > +             ctrls.set(V4L2_CID_VFLIP,
> > +                       static_cast<int32_t>(transform.contains(Transform::vflip())));
> >               unicam_[Unicam::Image].dev()->setControls(&ctrls);
> >       }
> >
>
> --
> Regards,
>
> Laurent Pinchart