[libcamera-devel] pipeline: raspberrypi: Sensor flips should be applied unconditionally
diff mbox series

Message ID 20201002090807.108859-1-naush@raspberrypi.com
State Accepted
Commit 0b427b8ecffdb24a5e133cf7d3b4d89fa4bad22f
Headers show
Series
  • [libcamera-devel] pipeline: raspberrypi: Sensor flips should be applied unconditionally
Related show

Commit Message

Naushir Patuck Oct. 2, 2020, 9:08 a.m. UTC 
The code in pipeline_handler::start() that applies the flips were in a
block that was conditional on the RPi::IPA_CONFIG_STAGGERED_WRITE return
result. This should be applied unconditionally.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

Comments

David Plowman Oct. 2, 2020, 10:45 a.m. UTC | #1 
Hi Naush

Thanks for this patch.

On Fri, 2 Oct 2020 at 10:08, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> The code in pipeline_handler::start() that applies the flips were in a
> block that was conditional on the RPi::IPA_CONFIG_STAGGERED_WRITE return
> result. This should be applied unconditionally.

Agree. This seems correct, though I guess it wasn't actually making
any difference in practice.

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Best regards
David

>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 26 +++++++++----------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index d4d04c0d..1052bdce 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1172,19 +1172,6 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>                                               { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
>                         sensorMetadata_ = result.data[resultIdx++];
>                 }
> -
> -               /*
> -                * Configure the H/V flip controls based on the combination of
> -                * the sensor and user transform.
> -                */
> -               if (supportsFlips_) {
> -                       ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> -                       ctrls.set(V4L2_CID_HFLIP,
> -                                 static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> -                       ctrls.set(V4L2_CID_VFLIP,
> -                                 static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> -                       unicam_[Unicam::Image].dev()->setControls(&ctrls);
> -               }
>         }
>
>         if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> @@ -1198,6 +1185,19 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>                 dropFrameCount_ = result.data[resultIdx++];
>         }
>
> +       /*
> +        * Configure the H/V flip controls based on the combination of
> +        * the sensor and user transform.
> +        */
> +       if (supportsFlips_) {
> +               ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> +               ctrls.set(V4L2_CID_HFLIP,
> +                         static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> +               ctrls.set(V4L2_CID_VFLIP,
> +                         static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> +               unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +       }
> +
>         return 0;
>  }
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Oct. 2, 2020, 3 p.m. UTC | #2 
Hi Naush,

On 02/10/2020 10:08, Naushir Patuck wrote:
> The code in pipeline_handler::start() that applies the flips were in a
> block that was conditional on the RPi::IPA_CONFIG_STAGGERED_WRITE return
> result. This should be applied unconditionally.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

This sounds and looks fine to me.

I see David has also given it a tag, so I'll merge this now for you.

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



> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 26 +++++++++----------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index d4d04c0d..1052bdce 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1172,19 +1172,6 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  					      { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
>  			sensorMetadata_ = result.data[resultIdx++];
>  		}
> -
> -		/*
> -		 * Configure the H/V flip controls based on the combination of
> -		 * the sensor and user transform.
> -		 */
> -		if (supportsFlips_) {
> -			ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> -			ctrls.set(V4L2_CID_HFLIP,
> -				  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> -			ctrls.set(V4L2_CID_VFLIP,
> -				  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> -			unicam_[Unicam::Image].dev()->setControls(&ctrls);
> -		}
>  	}
>  
>  	if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> @@ -1198,6 +1185,19 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  		dropFrameCount_ = result.data[resultIdx++];
>  	}
>  
> +	/*
> +	 * Configure the H/V flip controls based on the combination of
> +	 * the sensor and user transform.
> +	 */
> +	if (supportsFlips_) {
> +		ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> +		ctrls.set(V4L2_CID_HFLIP,
> +			  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> +		ctrls.set(V4L2_CID_VFLIP,
> +			  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> +		unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +	}
> +
>  	return 0;
>  }
>  
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index d4d04c0d..1052bdce 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1172,19 +1172,6 @@  int RPiCameraData::configureIPA(const CameraConfiguration *config)
 					      { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
 			sensorMetadata_ = result.data[resultIdx++];
 		}
-
-		/*
-		 * Configure the H/V flip controls based on the combination of
-		 * the sensor and user transform.
-		 */
-		if (supportsFlips_) {
-			ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
-			ctrls.set(V4L2_CID_HFLIP,
-				  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
-			ctrls.set(V4L2_CID_VFLIP,
-				  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
-			unicam_[Unicam::Image].dev()->setControls(&ctrls);
-		}
 	}
 
 	if (result.operation & RPi::IPA_CONFIG_SENSOR) {
@@ -1198,6 +1185,19 @@  int RPiCameraData::configureIPA(const CameraConfiguration *config)
 		dropFrameCount_ = result.data[resultIdx++];
 	}
 
+	/*
+	 * Configure the H/V flip controls based on the combination of
+	 * the sensor and user transform.
+	 */
+	if (supportsFlips_) {
+		ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
+		ctrls.set(V4L2_CID_HFLIP,
+			  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
+		ctrls.set(V4L2_CID_VFLIP,
+			  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
+		unicam_[Unicam::Image].dev()->setControls(&ctrls);
+	}
+
 	return 0;
 }