[libcamera-devel] pipeline: simple: Validate transform
diff mbox series

Message ID 20230427105356.16869-1-robert.mader@collabora.com
State Accepted
Headers show
Series
  • [libcamera-devel] pipeline: simple: Validate transform
Related show

Commit Message

Robert Mader April 27, 2023, 10:53 a.m. UTC 
Just like we do for other pipeline handlers already.
This ensures we corretly pass on transforms that are not handled by the
sensor - e.g. rotations - back to the app via the config, which is
required on devices like the Pinephone.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
Tested-by: Arnav Singh <me@arnavion.dev>
---
 src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi April 27, 2023, 11:36 a.m. UTC | #1 
Hi Robert,

On Thu, Apr 27, 2023 at 12:53:56PM +0200, Robert Mader via libcamera-devel wrote:
> Just like we do for other pipeline handlers already.
> This ensures we corretly pass on transforms that are not handled by the
> sensor - e.g. rotations - back to the app via the config, which is
> required on devices like the Pinephone.
>
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Tested-by: Arnav Singh <me@arnavion.dev>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 2423ec10..abfb4c87 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -211,7 +211,8 @@ public:
>  	int init();
>  	int setupLinks();
>  	int setupFormats(V4L2SubdeviceFormat *format,
> -			 V4L2Subdevice::Whence whence);
> +			 V4L2Subdevice::Whence whence,
> +			 Transform transform = Transform::Identity);
>  	void bufferReady(FrameBuffer *buffer);
>
>  	unsigned int streamIndex(const Stream *stream) const
> @@ -292,6 +293,7 @@ public:
>  	}
>
>  	bool needConversion() const { return needConversion_; }
> +	const Transform &combinedTransform() { return combinedTransform_; }

	const Transform &combinedTransform() const { return combinedTransform_; }

>
>  private:
>  	/*
> @@ -304,6 +306,7 @@ private:
>
>  	const SimpleCameraData::Configuration *pipeConfig_;
>  	bool needConversion_;
> +	Transform combinedTransform_;
>  };
>
>  class SimplePipelineHandler : public PipelineHandler
> @@ -664,7 +667,8 @@ int SimpleCameraData::setupLinks()
>  }
>
>  int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
> -				   V4L2Subdevice::Whence whence)
> +				   V4L2Subdevice::Whence whence,
> +				   Transform transform)
>  {
>  	SimplePipelineHandler *pipe = SimpleCameraData::pipe();
>  	int ret;
> @@ -673,7 +677,7 @@ int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
>  	 * Configure the format on the sensor output and propagate it through
>  	 * the pipeline.
>  	 */
> -	ret = sensor_->setFormat(format);
> +	ret = sensor_->setFormat(format, transform);
>  	if (ret < 0)
>  		return ret;
>
> @@ -877,15 +881,16 @@ SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
>
>  CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  {
> +	const CameraSensor *sensor = data_->sensor_.get();
>  	Status status = Valid;
>
>  	if (config_.empty())
>  		return Invalid;
>
> -	if (transform != Transform::Identity) {
> -		transform = Transform::Identity;
> +	Transform requestedTransform = transform;
> +	combinedTransform_ = sensor->validateTransform(&transform);
> +	if (transform != requestedTransform)
>  		status = Adjusted;
> -	}
>
>  	/* Cap the number of entries to the available streams. */
>  	if (config_.size() > data_->streams_.size()) {
> @@ -1116,7 +1121,9 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	format.mbus_code = pipeConfig->code;
>  	format.size = pipeConfig->sensorSize;
>
> -	ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
> +	ret = data->setupFormats(&format,
> +				 V4L2Subdevice::ActiveFormat,
> +				 config->combinedTransform());

Probably fits in 2 lines only

Minor nits, can be fixed when applying
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  	if (ret < 0)
>  		return ret;
>
> --
> 2.40.0
>
Kieran Bingham April 29, 2023, 9:16 p.m. UTC | #2 
Quoting Jacopo Mondi via libcamera-devel (2023-04-27 12:36:21)
> Hi Robert,
> 
> On Thu, Apr 27, 2023 at 12:53:56PM +0200, Robert Mader via libcamera-devel wrote:
> > Just like we do for other pipeline handlers already.
> > This ensures we corretly pass on transforms that are not handled by the
> > sensor - e.g. rotations - back to the app via the config, which is
> > required on devices like the Pinephone.
> >
> > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > Tested-by: Arnav Singh <me@arnavion.dev>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 2423ec10..abfb4c87 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -211,7 +211,8 @@ public:
> >       int init();
> >       int setupLinks();
> >       int setupFormats(V4L2SubdeviceFormat *format,
> > -                      V4L2Subdevice::Whence whence);
> > +                      V4L2Subdevice::Whence whence,
> > +                      Transform transform = Transform::Identity);
> >       void bufferReady(FrameBuffer *buffer);
> >
> >       unsigned int streamIndex(const Stream *stream) const
> > @@ -292,6 +293,7 @@ public:
> >       }
> >
> >       bool needConversion() const { return needConversion_; }
> > +     const Transform &combinedTransform() { return combinedTransform_; }
> 
>         const Transform &combinedTransform() const { return combinedTransform_; }
> 
> >
> >  private:
> >       /*
> > @@ -304,6 +306,7 @@ private:
> >
> >       const SimpleCameraData::Configuration *pipeConfig_;
> >       bool needConversion_;
> > +     Transform combinedTransform_;
> >  };
> >
> >  class SimplePipelineHandler : public PipelineHandler
> > @@ -664,7 +667,8 @@ int SimpleCameraData::setupLinks()
> >  }
> >
> >  int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
> > -                                V4L2Subdevice::Whence whence)
> > +                                V4L2Subdevice::Whence whence,
> > +                                Transform transform)
> >  {
> >       SimplePipelineHandler *pipe = SimpleCameraData::pipe();
> >       int ret;
> > @@ -673,7 +677,7 @@ int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
> >        * Configure the format on the sensor output and propagate it through
> >        * the pipeline.
> >        */
> > -     ret = sensor_->setFormat(format);
> > +     ret = sensor_->setFormat(format, transform);
> >       if (ret < 0)
> >               return ret;
> >
> > @@ -877,15 +881,16 @@ SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
> >
> >  CameraConfiguration::Status SimpleCameraConfiguration::validate()
> >  {
> > +     const CameraSensor *sensor = data_->sensor_.get();
> >       Status status = Valid;
> >
> >       if (config_.empty())
> >               return Invalid;
> >
> > -     if (transform != Transform::Identity) {
> > -             transform = Transform::Identity;
> > +     Transform requestedTransform = transform;
> > +     combinedTransform_ = sensor->validateTransform(&transform);
> > +     if (transform != requestedTransform)
> >               status = Adjusted;
> > -     }
> >
> >       /* Cap the number of entries to the available streams. */
> >       if (config_.size() > data_->streams_.size()) {
> > @@ -1116,7 +1121,9 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> >       format.mbus_code = pipeConfig->code;
> >       format.size = pipeConfig->sensorSize;
> >
> > -     ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
> > +     ret = data->setupFormats(&format,
> > +                              V4L2Subdevice::ActiveFormat,
> > +                              config->combinedTransform());
> 
> Probably fits in 2 lines only
> 
> Minor nits, can be fixed when applying
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>


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

Minors addressed and applied.
--
Kieran

> 
> >       if (ret < 0)
> >               return ret;
> >
> > --
> > 2.40.0
> >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 2423ec10..abfb4c87 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -211,7 +211,8 @@  public:
 	int init();
 	int setupLinks();
 	int setupFormats(V4L2SubdeviceFormat *format,
-			 V4L2Subdevice::Whence whence);
+			 V4L2Subdevice::Whence whence,
+			 Transform transform = Transform::Identity);
 	void bufferReady(FrameBuffer *buffer);
 
 	unsigned int streamIndex(const Stream *stream) const
@@ -292,6 +293,7 @@  public:
 	}
 
 	bool needConversion() const { return needConversion_; }
+	const Transform &combinedTransform() { return combinedTransform_; }
 
 private:
 	/*
@@ -304,6 +306,7 @@  private:
 
 	const SimpleCameraData::Configuration *pipeConfig_;
 	bool needConversion_;
+	Transform combinedTransform_;
 };
 
 class SimplePipelineHandler : public PipelineHandler
@@ -664,7 +667,8 @@  int SimpleCameraData::setupLinks()
 }
 
 int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
-				   V4L2Subdevice::Whence whence)
+				   V4L2Subdevice::Whence whence,
+				   Transform transform)
 {
 	SimplePipelineHandler *pipe = SimpleCameraData::pipe();
 	int ret;
@@ -673,7 +677,7 @@  int SimpleCameraData::setupFormats(V4L2SubdeviceFormat *format,
 	 * Configure the format on the sensor output and propagate it through
 	 * the pipeline.
 	 */
-	ret = sensor_->setFormat(format);
+	ret = sensor_->setFormat(format, transform);
 	if (ret < 0)
 		return ret;
 
@@ -877,15 +881,16 @@  SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
 
 CameraConfiguration::Status SimpleCameraConfiguration::validate()
 {
+	const CameraSensor *sensor = data_->sensor_.get();
 	Status status = Valid;
 
 	if (config_.empty())
 		return Invalid;
 
-	if (transform != Transform::Identity) {
-		transform = Transform::Identity;
+	Transform requestedTransform = transform;
+	combinedTransform_ = sensor->validateTransform(&transform);
+	if (transform != requestedTransform)
 		status = Adjusted;
-	}
 
 	/* Cap the number of entries to the available streams. */
 	if (config_.size() > data_->streams_.size()) {
@@ -1116,7 +1121,9 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 	format.mbus_code = pipeConfig->code;
 	format.size = pipeConfig->sensorSize;
 
-	ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
+	ret = data->setupFormats(&format,
+				 V4L2Subdevice::ActiveFormat,
+				 config->combinedTransform());
 	if (ret < 0)
 		return ret;