[libcamera-devel] [PATCH v5 4/6] libcamera: CameraSensor: Enable retrieving supported test pattern modes

[Jacopo Mondi]

Hi Hiro

On Wed, May 19, 2021 at 04:59:39PM +0900, Hirokazu Honda wrote:
> This enables retrieving supported test pattern modes through
> CameraSensorInfo.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  7 +++++
>  src/libcamera/camera_sensor.cpp            | 34 ++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 2a5c51a1..59d1188f 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -58,6 +58,10 @@ public:
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
>  	int setFormat(V4L2SubdeviceFormat *format);
> +	const std::vector<uint8_t> &testPatternModes() const
> +	{
> +		return testPatternModes_;
> +	}

I would move this just below resolution().
It is not related to get/setFormat()

>
>  	const ControlInfoMap &controls() const;
>  	ControlList getControls(const std::vector<uint32_t> &ids);
> @@ -81,6 +85,8 @@ private:
>  	void initVimcDefaultProperties();
>  	void initStaticProperties();
>  	int initProperties();
> +	void initTestPatternModes(
> +		const std::map<uint8_t, uint8_t> &testPatternModeMap);
>
>  	const MediaEntity *entity_;
>  	std::unique_ptr<V4L2Subdevice> subdev_;
> @@ -92,6 +98,7 @@ private:
>  	V4L2Subdevice::Formats formats_;
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
> +	std::vector<uint8_t> testPatternModes_;
>
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index eb84d9eb..a6aed417 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()
>  	activeArea_ = Rectangle(pixelArraySize_);
>  }
>
> +void CameraSensor::initTestPatternModes(
> +	const std::map<uint8_t, uint8_t> &testPatternModeMap)
> +{
> +	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> +	if (v4l2TestPattern == controls().end()) {
> +		LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> +					 << model() << "\'";
> +		return;
> +	}
> +
> +	for (const ControlValue &value : v4l2TestPattern->second.values()) {

We have the index-control association in the sensor db, and the list of
indexes in the sensor's controls.

I would be fine with just using the map in the sensor db, without
going through indexes to be honest.

> +		const int32_t index = value.get<int32_t>();
> +
> +		const auto it = testPatternModeMap.find(index);
> +		if (it != testPatternModeMap.end()) {
> +			LOG(CameraSensor, Debug) << "Test pattern mode (index="
> +						 << index << ") ignored";

But if you feel it's safer to cross-check between the two, maybe it is
worth to report this issue ( I would s/\(index=\)// )
as it might indicate the sensor driver and the sensor db diverged

> +			continue;
> +		}
> +
> +		LOG(CameraSensor, Debug) << "Test pattern mode (index="
> +					 << index << ") added";

But this one has no real value at run-time

> +		testPatternModes_.push_back(it->second);
> +	}
> +}
> +
>  void CameraSensor::initStaticProperties()
>  {
>  	const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()
>
>  	/* Register the properties retrieved from the sensor database. */
>  	properties_.set(properties::UnitCellSize, props->unitCellSize);
> +
> +	initTestPatternModes(props->testPatternModeMap);
>  }
>
>  int CameraSensor::initProperties()
> @@ -767,6 +795,12 @@ int CameraSensor::setControls(ControlList *ctrls)
>   * \return The list of camera sensor properties
>   */
>
> +/**
> + * \fn CameraSensor::testPatternModes()
> + * \brief Retrieve all the supported test pattern modes of the camera sensor
> + * \return The list of test pattern modes.

No full stop please.

Mostly minors
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j


> + */
> +
>  /**
>   * \brief Assemble and return the camera sensor info
>   * \param[out] info The camera sensor info
> --
> 2.31.1.751.gd2f1c929bd-goog
>