[libcamera-devel] [PATCH v9 1/3] libcamera: camera_sensor: Reference test pattern modes by enum type

[Jacopo Mondi]

Hi Hiro,

  while running CTS with this series applied I have noticed that once
a test pattern is set in one of the test, the rest of the session is
run with a test mode applied. I noticed as the displayed images are
actually the sensor's test patterns. Running the Camera app after CTS
has completed displays test pattern modes.

It's pretty obvious that the test pattern mode is reset to Off during
CameraSensor::init() which is called only at pipeline handler
initialization time, so if the library is not tore down the test
pattern mode is never reset.

Could you check if the same happens on your side by running CTS please ?

On Mon, Nov 29, 2021 at 05:34:22PM +0900, Hirokazu Honda wrote:
> 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.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  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 2facbf3c..310571ca 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 af381a12..1ee3cb99 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 c65afdb2..25490dcf 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -982,13 +982,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.34.0.rc2.393.gf8c9666880-goog
>