[libcamera-devel,v4,1/3] camera_sensor: Deal test pattern mode control values with its enum
diff mbox series

Message ID 20211104064252.2091330-2-hiroh@chromium.org
State Superseded
Headers show
Series
  • Apply a sensor test pattern mode
Related show

Commit Message

Hirokazu Honda Nov. 4, 2021, 6:42 a.m. UTC 
This changes the type of test pattern mode control values to
controls::draft::TestPatternModeEnum from int32_t

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/camera_sensor.h            | 10 +++++++---
 include/libcamera/internal/camera_sensor_properties.h |  3 ++-
 src/libcamera/camera_sensor.cpp                       |  4 ++--
 src/libcamera/pipeline/ipu3/ipu3.cpp                  |  7 ++++---
 4 files changed, 15 insertions(+), 9 deletions(-)

Comments

Jacopo Mondi Nov. 4, 2021, 9:11 a.m. UTC | #1 
Hi Hiro

   s/deal/handle. in subject

On Thu, Nov 04, 2021 at 03:42:50PM +0900, Hirokazu Honda wrote:
> This changes the type of test pattern mode control values to
> controls::draft::TestPatternModeEnum from int32_t
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  include/libcamera/internal/camera_sensor.h            | 10 +++++++---
>  include/libcamera/internal/camera_sensor_properties.h |  3 ++-
>  src/libcamera/camera_sensor.cpp                       |  4 ++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp                  |  7 ++++---
>  4 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index d25a1165..edef2220 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -14,8 +14,10 @@
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
> +
>  #include <libcamera/ipa/core_ipa_interface.h>
>
>  #include "libcamera/internal/formats.h"
> @@ -40,7 +42,8 @@ public:
>  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
>  	const std::vector<Size> sizes(unsigned int mbusCode) const;
>  	Size resolution() const;
> -	const std::vector<int32_t> &testPatternModes() const
> +	const std::vector<controls::draft::TestPatternModeEnum>
> +		&testPatternModes() const
>  	{
>  		return testPatternModes_;
>  	}
> @@ -71,7 +74,8 @@ private:
>  	void initVimcDefaultProperties();
>  	void initStaticProperties();
>  	void initTestPatternModes(
> -		const std::map<int32_t, int32_t> &testPatternModeMap);
> +		const std::map<controls::draft::TestPatternModeEnum, int32_t>
> +			&testPatternModeMap);
>  	int initProperties();
>
>  	const MediaEntity *entity_;
> @@ -84,7 +88,7 @@ private:
>  	V4L2Subdevice::Formats formats_;
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
> -	std::vector<int32_t> testPatternModes_;
> +	std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
>
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
> index 67c77920..5c7e5e87 100644
> --- a/include/libcamera/internal/camera_sensor_properties.h
> +++ b/include/libcamera/internal/camera_sensor_properties.h
> @@ -10,6 +10,7 @@
>  #include <map>
>  #include <string>
>
> +#include <libcamera/control_ids.h>
>  #include <libcamera/geometry.h>
>
>  namespace libcamera {
> @@ -18,7 +19,7 @@ struct CameraSensorProperties {
>  	static const CameraSensorProperties *get(const std::string &sensor);
>
>  	Size unitCellSize;
> -	std::map<int32_t, int32_t> testPatternModes;
> +	std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
>  };
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9fdb8c09..f0aa9f24 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -311,7 +311,7 @@ void CameraSensor::initStaticProperties()
>  }
>
>  void CameraSensor::initTestPatternModes(
> -	const std::map<int32_t, int32_t> &testPatternModes)
> +	const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
>  {
>  	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
>  	if (v4l2TestPattern == controls().end()) {
> @@ -327,7 +327,7 @@ void CameraSensor::initTestPatternModes(
>  	 * control index is supported in the below for loop that creates the
>  	 * list of supported test patterns.
>  	 */
> -	std::map<int32_t, int32_t> indexToTestPatternMode;
> +	std::map<int32_t, controls::draft::TestPatternModeEnum> indexToTestPatternMode;
>  	for (const auto &it : testPatternModes)
>  		indexToTestPatternMode[it.second] = it.first;
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index eb714aa6..63cb7f11 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -981,13 +981,14 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
>  		return ret;
>
>  	ControlInfoMap::Map controls = IPU3Controls;
> -	const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();
> +	const std::vector<controls::draft::TestPatternModeEnum>
> +		&testPatternModes = sensor->testPatternModes();
>  	if (!testPatternModes.empty()) {
>  		std::vector<ControlValue> values;
>  		values.reserve(testPatternModes.size());
>
> -		for (int32_t pattern : testPatternModes)
> -			values.emplace_back(pattern);
> +		for (auto pattern : testPatternModes)
> +			values.emplace_back(static_cast<int32_t>(pattern));
>
>  		controls[&controls::draft::TestPatternMode] = ControlInfo(values);
>  	}
> --
> 2.33.1.1089.g2158813163f-goog
>
Kieran Bingham Nov. 8, 2021, 10:51 p.m. UTC | #2 
The $SUBJECT doesn't have libcamera: before camera_sensor: while patch
2/3 in this series does.

Making it consistent, and simplifying I'd write the following: Feel free
to take or adapt as you like.


"""
libcamera: camera_sensor: Reference test pattern modes by enum type

The CameraSensor stores TestPatternModes as an int32_t. This prevents
the compiler from verifying the usage against the defined enum types.

Fix references to the TestPatternMode to store the value as the
TestPatternModeEnum type which is defined by the control generator.
"""

Quoting Hirokazu Honda (2021-11-04 06:42:50)
> This changes the type of test pattern mode control values to
> controls::draft::TestPatternModeEnum from int32_t
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  include/libcamera/internal/camera_sensor.h            | 10 +++++++---
>  include/libcamera/internal/camera_sensor_properties.h |  3 ++-
>  src/libcamera/camera_sensor.cpp                       |  4 ++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp                  |  7 ++++---
>  4 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index d25a1165..edef2220 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -14,8 +14,10 @@
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/log.h>
>  
> +#include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
> +
>  #include <libcamera/ipa/core_ipa_interface.h>
>  
>  #include "libcamera/internal/formats.h"
> @@ -40,7 +42,8 @@ public:
>         const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
>         const std::vector<Size> sizes(unsigned int mbusCode) const;
>         Size resolution() const;
> -       const std::vector<int32_t> &testPatternModes() const
> +       const std::vector<controls::draft::TestPatternModeEnum>

It's a shame the generated Enum type is so long. But that's not a fault
of this patch.

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

> +               &testPatternModes() const
>         {
>                 return testPatternModes_;
>         }
> @@ -71,7 +74,8 @@ private:
>         void initVimcDefaultProperties();
>         void initStaticProperties();
>         void initTestPatternModes(
> -               const std::map<int32_t, int32_t> &testPatternModeMap);
> +               const std::map<controls::draft::TestPatternModeEnum, int32_t>
> +                       &testPatternModeMap);
>         int initProperties();
>  
>         const MediaEntity *entity_;
> @@ -84,7 +88,7 @@ private:
>         V4L2Subdevice::Formats formats_;
>         std::vector<unsigned int> mbusCodes_;
>         std::vector<Size> sizes_;
> -       std::vector<int32_t> testPatternModes_;
> +       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
>  
>         Size pixelArraySize_;
>         Rectangle activeArea_;
> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
> index 67c77920..5c7e5e87 100644
> --- a/include/libcamera/internal/camera_sensor_properties.h
> +++ b/include/libcamera/internal/camera_sensor_properties.h
> @@ -10,6 +10,7 @@
>  #include <map>
>  #include <string>
>  
> +#include <libcamera/control_ids.h>
>  #include <libcamera/geometry.h>
>  
>  namespace libcamera {
> @@ -18,7 +19,7 @@ struct CameraSensorProperties {
>         static const CameraSensorProperties *get(const std::string &sensor);
>  
>         Size unitCellSize;
> -       std::map<int32_t, int32_t> testPatternModes;
> +       std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9fdb8c09..f0aa9f24 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -311,7 +311,7 @@ void CameraSensor::initStaticProperties()
>  }
>  
>  void CameraSensor::initTestPatternModes(
> -       const std::map<int32_t, int32_t> &testPatternModes)
> +       const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
>  {
>         const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
>         if (v4l2TestPattern == controls().end()) {
> @@ -327,7 +327,7 @@ void CameraSensor::initTestPatternModes(
>          * control index is supported in the below for loop that creates the
>          * list of supported test patterns.
>          */
> -       std::map<int32_t, int32_t> indexToTestPatternMode;
> +       std::map<int32_t, controls::draft::TestPatternModeEnum> indexToTestPatternMode;
>         for (const auto &it : testPatternModes)
>                 indexToTestPatternMode[it.second] = it.first;
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index eb714aa6..63cb7f11 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -981,13 +981,14 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
>                 return ret;
>  
>         ControlInfoMap::Map controls = IPU3Controls;
> -       const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();
> +       const std::vector<controls::draft::TestPatternModeEnum>
> +               &testPatternModes = sensor->testPatternModes();
>         if (!testPatternModes.empty()) {
>                 std::vector<ControlValue> values;
>                 values.reserve(testPatternModes.size());
>  
> -               for (int32_t pattern : testPatternModes)
> -                       values.emplace_back(pattern);
> +               for (auto pattern : testPatternModes)
> +                       values.emplace_back(static_cast<int32_t>(pattern));
>  
>                 controls[&controls::draft::TestPatternMode] = ControlInfo(values);
>         }
> -- 
> 2.33.1.1089.g2158813163f-goog
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index d25a1165..edef2220 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -14,8 +14,10 @@ 
 #include <libcamera/base/class.h>
 #include <libcamera/base/log.h>
 
+#include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
 #include <libcamera/geometry.h>
+
 #include <libcamera/ipa/core_ipa_interface.h>
 
 #include "libcamera/internal/formats.h"
@@ -40,7 +42,8 @@  public:
 	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
 	const std::vector<Size> sizes(unsigned int mbusCode) const;
 	Size resolution() const;
-	const std::vector<int32_t> &testPatternModes() const
+	const std::vector<controls::draft::TestPatternModeEnum>
+		&testPatternModes() const
 	{
 		return testPatternModes_;
 	}
@@ -71,7 +74,8 @@  private:
 	void initVimcDefaultProperties();
 	void initStaticProperties();
 	void initTestPatternModes(
-		const std::map<int32_t, int32_t> &testPatternModeMap);
+		const std::map<controls::draft::TestPatternModeEnum, int32_t>
+			&testPatternModeMap);
 	int initProperties();
 
 	const MediaEntity *entity_;
@@ -84,7 +88,7 @@  private:
 	V4L2Subdevice::Formats formats_;
 	std::vector<unsigned int> mbusCodes_;
 	std::vector<Size> sizes_;
-	std::vector<int32_t> testPatternModes_;
+	std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
 
 	Size pixelArraySize_;
 	Rectangle activeArea_;
diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
index 67c77920..5c7e5e87 100644
--- a/include/libcamera/internal/camera_sensor_properties.h
+++ b/include/libcamera/internal/camera_sensor_properties.h
@@ -10,6 +10,7 @@ 
 #include <map>
 #include <string>
 
+#include <libcamera/control_ids.h>
 #include <libcamera/geometry.h>
 
 namespace libcamera {
@@ -18,7 +19,7 @@  struct CameraSensorProperties {
 	static const CameraSensorProperties *get(const std::string &sensor);
 
 	Size unitCellSize;
-	std::map<int32_t, int32_t> testPatternModes;
+	std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index 9fdb8c09..f0aa9f24 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -311,7 +311,7 @@  void CameraSensor::initStaticProperties()
 }
 
 void CameraSensor::initTestPatternModes(
-	const std::map<int32_t, int32_t> &testPatternModes)
+	const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
 {
 	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
 	if (v4l2TestPattern == controls().end()) {
@@ -327,7 +327,7 @@  void CameraSensor::initTestPatternModes(
 	 * control index is supported in the below for loop that creates the
 	 * list of supported test patterns.
 	 */
-	std::map<int32_t, int32_t> indexToTestPatternMode;
+	std::map<int32_t, controls::draft::TestPatternModeEnum> indexToTestPatternMode;
 	for (const auto &it : testPatternModes)
 		indexToTestPatternMode[it.second] = it.first;
 
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index eb714aa6..63cb7f11 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -981,13 +981,14 @@  int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
 		return ret;
 
 	ControlInfoMap::Map controls = IPU3Controls;
-	const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();
+	const std::vector<controls::draft::TestPatternModeEnum>
+		&testPatternModes = sensor->testPatternModes();
 	if (!testPatternModes.empty()) {
 		std::vector<ControlValue> values;
 		values.reserve(testPatternModes.size());
 
-		for (int32_t pattern : testPatternModes)
-			values.emplace_back(pattern);
+		for (auto pattern : testPatternModes)
+			values.emplace_back(static_cast<int32_t>(pattern));
 
 		controls[&controls::draft::TestPatternMode] = ControlInfo(values);
 	}