[libcamera-devel,PATCH/RFC,2/2] libcamera: Use V4L2 Control instances

11199 diff mbox series

Use the V4L2 Control instances instead of the numerical V4L2_CID_*
identifiers where possible. This simplify usage of the ControlList API
in several places, and allows printing control names in the CameraSensor
class and the RaspberryPi IPA.

There are still locations where the numerical identifiers are used:

- In the IPU3 and RaspberryPi code, for custom controls not defined in
  linux/v4l2-controls.h, which can be fixed by adding additional
  device-specific Control instances.

- In the DelayedControls and V4L2Device controls APIs, which we could
  move to ControlId if desired.

- In the uvcvideo and vimc pipeline handlers, when translating between
  V4L2 and libcamera controls, for switch/case or for code that operate
  on different controls in a generic way. This would be more difficult
  to replace.

Further improvements to the controls API may be possible on top of this,
such as dropping the ID-based ControlList::get() and ControlList::set()
functions (at possibly replacing them with a version that takes a
ControlId pointer). Another possibly interesting improvement would be to
add a lookup API to the ControlInfoMap class that would take a
Control<T> reference, and return a type-aware wrapper around ControlInfo
to avoid having to deal manually with ControlValue.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp                         | 10 +++---
 src/ipa/raspberrypi/raspberrypi.cpp           | 25 ++++++-------
 src/ipa/rkisp1/rkisp1.cpp                     | 10 +++---
 src/libcamera/camera_sensor.cpp               | 36 +++++++++----------
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
 .../pipeline/raspberrypi/raspberrypi.cpp      | 16 ++++-----
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  | 29 +++++++--------
 src/libcamera/pipeline/vimc/vimc.cpp          | 10 +++---
 8 files changed, 68 insertions(+), 70 deletions(-)

Hi Laurent,

On 08/02/2021 22:54, Laurent Pinchart wrote:
> Use the V4L2 Control instances instead of the numerical V4L2_CID_*
> identifiers where possible. This simplify usage of the ControlList API
> in several places, and allows printing control names in the CameraSensor
> class and the RaspberryPi IPA.
> 
> There are still locations where the numerical identifiers are used:
> 
> - In the IPU3 and RaspberryPi code, for custom controls not defined in
>   linux/v4l2-controls.h, which can be fixed by adding additional
>   device-specific Control instances.

That's easy enough...


> - In the DelayedControls and V4L2Device controls APIs, which we could
>   move to ControlId if desired.

Also easy, I expect?

> - In the uvcvideo and vimc pipeline handlers, when translating between
>   V4L2 and libcamera controls, for switch/case or for code that operate
>   on different controls in a generic way. This would be more difficult
>   to replace.

Oh :(



> 
> Further improvements to the controls API may be possible on top of this,
> such as dropping the ID-based ControlList::get() and ControlList::set()
> functions (at possibly replacing them with a version that takes a
> ControlId pointer). Another possibly interesting improvement would be to
> add a lookup API to the ControlInfoMap class that would take a
> Control<T> reference, and return a type-aware wrapper around ControlInfo
> to avoid having to deal manually with ControlValue.

This does also mean that a V4L2 control is now strongly typed, so we can
be explicit about when it is used rather than passing around integers. I
feel like that must bring some benefit or safety from the compiler in
the future, but I don't know yet.


> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp                         | 10 +++---
>  src/ipa/raspberrypi/raspberrypi.cpp           | 25 ++++++-------
>  src/ipa/rkisp1/rkisp1.cpp                     | 10 +++---
>  src/libcamera/camera_sensor.cpp               | 36 +++++++++----------
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 16 ++++-----
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  | 29 +++++++--------
>  src/libcamera/pipeline/vimc/vimc.cpp          | 10 +++---
>  8 files changed, 68 insertions(+), 70 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index b11b03efa6ce..5ee3fd302e01 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -11,7 +11,6 @@
>  #include <sys/mman.h>
>  
>  #include <linux/intel-ipu3.h>
> -#include <linux/v4l2-controls.h>
>  
>  #include <libcamera/buffer.h>
>  #include <libcamera/control_ids.h>
> @@ -23,6 +22,7 @@
>  
>  #include "libcamera/internal/buffer.h"
>  #include "libcamera/internal/log.h"
> +#include "libcamera/internal/v4l2_controls.h"
>  
>  namespace libcamera {
>  
> @@ -80,13 +80,13 @@ void IPAIPU3::configure([[maybe_unused]] const CameraSensorInfo &info,
>  
>  	ctrls_ = entityControls.at(0);
>  
> -	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> +	const auto itExp = ctrls_.find(&v4l2::EXPOSURE);
>  	if (itExp == ctrls_.end()) {
>  		LOG(IPAIPU3, Error) << "Can't find exposure control";
>  		return;
>  	}
>  
> -	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> +	const auto itGain = ctrls_.find(&v4l2::ANALOGUE_GAIN);
>  	if (itGain == ctrls_.end()) {
>  		LOG(IPAIPU3, Error) << "Can't find gain control";
>  		return;
> @@ -214,8 +214,8 @@ void IPAIPU3::setControls(unsigned int frame)
>  	op.operation = IPU3_IPA_ACTION_SET_SENSOR_CONTROLS;
>  
>  	ControlList ctrls(ctrls_);
> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> +	ctrls.set(v4l2::EXPOSURE, exposure_);
> +	ctrls.set(v4l2::ANALOGUE_GAIN, gain_);

Having the types defined so they don't need casts looks like a good win
here.

>  	op.controls.push_back(ctrls);
>  
>  	queueFrameAction.emit(frame, op);
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index ff14cfc4b706..897f3f0b91f1 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -542,16 +542,16 @@ void IPARPi::reportMetadata()
>  
>  bool IPARPi::validateSensorControls()
>  {
> -	static const uint32_t ctrls[] = {
> -		V4L2_CID_ANALOGUE_GAIN,
> -		V4L2_CID_EXPOSURE,
> -		V4L2_CID_VBLANK,
> +	static const ControlId *const ctrls[] = {
> +		&v4l2::ANALOGUE_GAIN,
> +		&v4l2::EXPOSURE,
> +		&v4l2::VBLANK,
>  	};
>  
>  	for (auto c : ctrls) {
>  		if (sensorCtrls_.find(c) == sensorCtrls_.end()) {
>  			LOG(IPARPI, Error) << "Unable to find sensor control "
> -					   << utils::hex(c);
> +					   << c->name();

I'm sure you know that makes me quite happy ;-)

>  			return false;
>  		}
>  	}
> @@ -1038,10 +1038,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>  	LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gain_r << " B: "
>  			   << awbStatus->gain_b;
>  
> -	ctrls.set(V4L2_CID_RED_BALANCE,
> -		  static_cast<int32_t>(awbStatus->gain_r * 1000));
> -	ctrls.set(V4L2_CID_BLUE_BALANCE,
> -		  static_cast<int32_t>(awbStatus->gain_b * 1000));
> +	ctrls.set(v4l2::RED_BALANCE, awbStatus->gain_r * 1000);
> +	ctrls.set(v4l2::BLUE_BALANCE, awbStatus->gain_b * 1000);

Again, cleaner code without casts ....

>  }
>  
>  void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)
> @@ -1100,15 +1098,14 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  	 * exposure time without us knowing. The next time though this function should
>  	 * clip exposure correctly.
>  	 */
> -	ctrls.set(V4L2_CID_VBLANK, vblanking);
> -	ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> +	ctrls.set(v4l2::VBLANK, vblanking);
> +	ctrls.set(v4l2::EXPOSURE, exposureLines);
> +	ctrls.set(v4l2::ANALOGUE_GAIN, gainCode);
>  }
>  
>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
>  {
> -	ctrls.set(V4L2_CID_DIGITAL_GAIN,
> -		  static_cast<int32_t>(dgStatus->digital_gain * 1000));
> +	ctrls.set(v4l2::DIGITAL_GAIN, dgStatus->digital_gain * 1000);
>  }
>  
>  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 39783abd731b..ce47ba8cb38a 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -13,7 +13,6 @@
>  #include <sys/mman.h>
>  
>  #include <linux/rkisp1-config.h>
> -#include <linux/v4l2-controls.h>
>  
>  #include <libcamera/buffer.h>
>  #include <libcamera/control_ids.h>
> @@ -25,6 +24,7 @@
>  #include <libipa/ipa_interface_wrapper.h>
>  
>  #include "libcamera/internal/log.h"
> +#include "libcamera/internal/v4l2_controls.h"
>  
>  namespace libcamera {
>  
> @@ -91,13 +91,13 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
>  
>  	ctrls_ = entityControls.at(0);
>  
> -	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> +	const auto itExp = ctrls_.find(&v4l2::EXPOSURE);
>  	if (itExp == ctrls_.end()) {
>  		LOG(IPARkISP1, Error) << "Can't find exposure control";
>  		return;
>  	}
>  
> -	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> +	const auto itGain = ctrls_.find(&v4l2::ANALOGUE_GAIN);
>  	if (itGain == ctrls_.end()) {
>  		LOG(IPARkISP1, Error) << "Can't find gain control";
>  		return;
> @@ -261,8 +261,8 @@ void IPARkISP1::setControls(unsigned int frame)
>  	op.operation = RKISP1_IPA_ACTION_V4L2_SET;
>  
>  	ControlList ctrls(ctrls_);
> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> +	ctrls.set(v4l2::EXPOSURE, exposure_);
> +	ctrls.set(v4l2::ANALOGUE_GAIN, gain_);
>  	op.controls.push_back(ctrls);
>  
>  	queueFrameAction.emit(frame, op);
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index c9e8d49b7935..eb96c4841708 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -280,16 +280,16 @@ int CameraSensor::validateSensorDriver()
>  	 * Optional controls are used to register optional sensor properties. If
>  	 * not present, some values will be defaulted.
>  	 */
> -	static constexpr uint32_t optionalControls[] = {
> -		V4L2_CID_CAMERA_ORIENTATION,
> -		V4L2_CID_CAMERA_SENSOR_ROTATION,
> +	static const ControlId *const optionalControls[] = {
> +		&v4l2::CAMERA_ORIENTATION,
> +		&v4l2::CAMERA_SENSOR_ROTATION,
>  	};
>  
> -	const ControlIdMap &controls = subdev_->controls().idmap();
> -	for (uint32_t ctrl : optionalControls) {
> +	const ControlInfoMap &controls = subdev_->controls();
> +	for (const ControlId *ctrl : optionalControls) {
>  		if (!controls.count(ctrl))
>  			LOG(CameraSensor, Debug)
> -				<< "Optional V4L2 control " << utils::hex(ctrl)
> +				<< "Optional V4L2 control " << ctrl->name()
>  				<< " not supported";
>  	}
>  
> @@ -346,18 +346,18 @@ int CameraSensor::validateSensorDriver()
>  	 * For raw sensors, make sure the sensor driver supports the controls
>  	 * required by the CameraSensor class.
>  	 */
> -	static constexpr uint32_t mandatoryControls[] = {
> -		V4L2_CID_EXPOSURE,
> -		V4L2_CID_HBLANK,
> -		V4L2_CID_PIXEL_RATE,
> -		V4L2_CID_VBLANK,
> +	static const ControlId *const mandatoryControls[] = {
> +		&v4l2::EXPOSURE,
> +		&v4l2::HBLANK,
> +		&v4l2::PIXEL_RATE,
> +		&v4l2::VBLANK,
>  	};
>  
>  	err = 0;
> -	for (uint32_t ctrl : mandatoryControls) {
> +	for (const ControlId *ctrl : mandatoryControls) {
>  		if (!controls.count(ctrl)) {
>  			LOG(CameraSensor, Error)
> -				<< "Mandatory V4L2 control " << utils::hex(ctrl)
> +				<< "Mandatory V4L2 control " << ctrl->name()
>  				<< " not available";

And you've covered both optional and mandatory controls, (and more) ...
so that's a big plus for me.


>  			err = -EINVAL;
>  		}
> @@ -425,7 +425,7 @@ int CameraSensor::initProperties()
>  	const ControlInfoMap &controls = subdev_->controls();
>  	int32_t propertyValue;
>  
> -	const auto &orientation = controls.find(V4L2_CID_CAMERA_ORIENTATION);
> +	const auto &orientation = controls.find(&v4l2::CAMERA_ORIENTATION);
>  	if (orientation != controls.end()) {
>  		int32_t v4l2Orientation = orientation->second.def().get<int32_t>();
>  
> @@ -450,7 +450,7 @@ int CameraSensor::initProperties()
>  	}
>  	properties_.set(properties::Location, propertyValue);
>  
> -	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> +	const auto &rotationControl = controls.find(&v4l2::CAMERA_SENSOR_ROTATION);
>  	if (rotationControl != controls.end()) {
>  		propertyValue = rotationControl->second.def().get<int32_t>();
>  		properties_.set(properties::Rotation, propertyValue);
> @@ -775,11 +775,11 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  		return -EINVAL;
>  	}
>  
> -	int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>();
> +	int32_t hblank = ctrls.get(v4l2::HBLANK);
>  	info->lineLength = info->outputSize.width + hblank;
> -	info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();
> +	info->pixelRate = ctrls.get(v4l2::PIXEL_RATE);
>  
> -	const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);
> +	const ControlInfo vblank = ctrls.infoMap()->at(&v4l2::VBLANK);
>  	info->minFrameLength = info->outputSize.height + vblank.min().get<int32_t>();
>  	info->maxFrameLength = info->outputSize.height + vblank.max().get<int32_t>();
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 9bc3df3352fd..d3fc51518598 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -845,7 +845,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	double lineDuration = sensorInfo.lineLength
>  			    / (sensorInfo.pixelRate / 1e6);
>  	const ControlInfoMap &sensorControls = sensor->controls();
> -	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> +	const ControlInfo &v4l2Exposure = sensorControls.find(&v4l2::EXPOSURE)->second;
>  	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
>  	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
>  	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0804a4393c19..90f846530346 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -992,8 +992,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  		data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>  
>  		ControlList ctrls(dev->controls());
> -		ctrls.set(V4L2_CID_HFLIP, 0);
> -		ctrls.set(V4L2_CID_VFLIP, 0);
> +		ctrls.set(v4l2::HFLIP, 0);
> +		ctrls.set(v4l2::VFLIP, 0);

It's even a shorter prefix...

>  		dev->setControls(&ctrls);
>  	}
>  
> @@ -1246,10 +1246,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	 */
>  	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)));
> +		ctrls.set(v4l2::HFLIP,
> +			  !!(rpiConfig->combinedTransform_ & Transform::HFlip));
> +		ctrls.set(v4l2::VFLIP,
> +			  !!(rpiConfig->combinedTransform_ & Transform::VFlip));

Do those fit on one line (each) nicely now? Hrm... looks like 94 chars,
still good in my book ;-) But I know people are keen on their 80cols.


>  		unicam_[Unicam::Image].dev()->setControls(&ctrls);
>  	}
>  
> @@ -1383,8 +1383,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  			auto it = mappedEmbeddedBuffers_.find(bufferId);
>  			if (it != mappedEmbeddedBuffers_.end()) {
>  				uint32_t *mem = reinterpret_cast<uint32_t *>(it->second.maps()[0].data());
> -				mem[0] = ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -				mem[1] = ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> +				mem[0] = ctrl.get(v4l2::EXPOSURE);
> +				mem[1] = ctrl.get(v4l2::ANALOGUE_GAIN);
>  			} else {
>  				LOG(RPI, Warning) << "Failed to find embedded buffer "
>  						  << bufferId;
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 08a594175091..a80b51f174a9 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -260,20 +260,20 @@ void PipelineHandlerUVC::stop(Camera *camera)
>  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  				       const ControlValue &value)
>  {
> -	uint32_t cid;
> +	const ControlId *cid;
>  
>  	if (id == controls::Brightness)
> -		cid = V4L2_CID_BRIGHTNESS;
> +		cid = &v4l2::BRIGHTNESS;
>  	else if (id == controls::Contrast)
> -		cid = V4L2_CID_CONTRAST;
> +		cid = &v4l2::CONTRAST;
>  	else if (id == controls::Saturation)
> -		cid = V4L2_CID_SATURATION;
> +		cid = &v4l2::SATURATION;
>  	else if (id == controls::AeEnable)
> -		cid = V4L2_CID_EXPOSURE_AUTO;
> +		cid = &v4l2::EXPOSURE_AUTO;
>  	else if (id == controls::ExposureTime)
> -		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> +		cid = &v4l2::EXPOSURE_ABSOLUTE;
>  	else if (id == controls::AnalogueGain)
> -		cid = V4L2_CID_GAIN;
> +		cid = &v4l2::GAIN;

That looks like we should have a common lookup table/map for known
matching controls...

But not a topic for this patch itself.

Except I bet that AnalogueGain == CID_GAIN would cause issues against
ANALOGUE_GAIN/DIGITAL_GAIN for any generic lookup :S ...

>  	else
>  		return -EINVAL;
>  
> @@ -286,18 +286,18 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  	 * See UVCCameraData::addControl() for explanations of the different
>  	 * value mappings.
>  	 */
> -	switch (cid) {
> +	switch (cid->id()) {
>  	case V4L2_CID_BRIGHTNESS: {

I see, mixing and matching between v4l2:: and V4L2_CID here is a pain,
but it works at least.


I had hoped we could give ControlID an operator unsigned int(), and be
able to resolve this, but even with that - the switch statement needs a
constexpr which I can't see how to make them so.

Even though as far as I can see they likely meet most of the
requirements, they are fully initialised at construtor, and not
modifyable...


> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 3b7f3347761e..9938d3e6b2e9 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -219,6 +219,8 @@ public:
>         const std::string &name() const { return name_; }
>         ControlType type() const { return type_; }
>  
> +       operator unsigned int() const { return id_; }
> +
>  private:
>         ControlId &operator=(const ControlId &) = delete;
>         ControlId(const ControlId &) = delete;
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index a80b51f174a9..c8647a1f708b 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -287,7 +287,7 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>          * value mappings.
>          */
>         switch (cid->id()) {
> -       case V4L2_CID_BRIGHTNESS: {
> +       case v4l2::BRIGHTNESS: {
>                 float scale = std::max(max - def, def - min);
>                 float fvalue = value.get<float>() * scale + def;
>                 controls->set(v4l2::BRIGHTNESS, lroundf(fvalue));


But alas, I can't (quickly) fix this:

> ../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp: In member function ‘int libcamera::PipelineHandlerUVC::processControl(libcamera::ControlList*, unsigned int, const libcamera::ControlValue&)’:
> ../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:290:13: error: the value of ‘libcamera::v4l2::BRIGHTNESS’ is not usable in a constant expression
>   290 |  case v4l2::BRIGHTNESS: {
>       |             ^~~~~~~~~~
> In file included from ../include/libcamera/internal/v4l2_controls.h:15,
>                  from ../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:28:
> include/libcamera/internal/v4l2_control_ids.h:24:31: note: ‘libcamera::v4l2::BRIGHTNESS’ was not declared ‘constexpr’
>    24 | extern const Control<int32_t> BRIGHTNESS;
>       |                               ^~~~~~~~~~
> ../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:290:13: error: call to non-‘constexpr’ function ‘libcamera::ControlId::operator unsigned int() const’
>   290 |  case v4l2::BRIGHTNESS: {
>       |             ^~~~~~~~~~


I wonder if allowing to return as an unsigned int would break our type
safety anyway, but maybe there's a path to clean things up later.




>  		float scale = std::max(max - def, def - min);
>  		float fvalue = value.get<float>() * scale + def;
> -		controls->set(cid, static_cast<int32_t>(lroundf(fvalue)));
> +		controls->set(v4l2::BRIGHTNESS, lroundf(fvalue));
>  		break;
>  	}
>  
>  	case V4L2_CID_SATURATION: {
>  		float scale = def - min;
>  		float fvalue = value.get<float>() * scale + min;
> -		controls->set(cid, static_cast<int32_t>(lroundf(fvalue)));
> +		controls->set(v4l2::SATURATION, lroundf(fvalue));
>  		break;
>  	}
>  
> @@ -305,12 +305,13 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  		int32_t ivalue = value.get<bool>()
>  			       ? V4L2_EXPOSURE_APERTURE_PRIORITY
>  			       : V4L2_EXPOSURE_MANUAL;
> -		controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> +		controls->set(v4l2::EXPOSURE_AUTO, ivalue);
>  		break;
>  	}
>  
>  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> -		controls->set(cid, value.get<int32_t>() / 100);
> +		controls->set(v4l2::EXPOSURE_ABSOLUTE,
> +			      value.get<int32_t>() / 100);
>  		break;
>  
>  	case V4L2_CID_CONTRAST:
> @@ -324,13 +325,13 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  		}
>  
>  		float fvalue = (value.get<float>() - p) / m;
> -		controls->set(cid, static_cast<int32_t>(lroundf(fvalue)));
> +		controls->set(cid->id(), static_cast<int32_t>(lroundf(fvalue)));
>  		break;
>  	}
>  
>  	default: {
>  		int32_t ivalue = value.get<int32_t>();
> -		controls->set(cid, ivalue);
> +		controls->set(cid->id(), ivalue);
>  		break;
>  	}
>  	}
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 36325ffbbd7d..24b39b7dd879 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -342,24 +342,24 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
>  
>  	for (auto it : request->controls()) {
>  		unsigned int id = it.first;
> +		const ControlId *cid;
>  		unsigned int offset;
> -		uint32_t cid;
>  
>  		if (id == controls::Brightness) {
> -			cid = V4L2_CID_BRIGHTNESS;
> +			cid = &v4l2::BRIGHTNESS;
>  			offset = 128;
>  		} else if (id == controls::Contrast) {
> -			cid = V4L2_CID_CONTRAST;
> +			cid = &v4l2::CONTRAST;
>  			offset = 0;
>  		} else if (id == controls::Saturation) {
> -			cid = V4L2_CID_SATURATION;
> +			cid = &v4l2::SATURATION;
>  			offset = 0;
>  		} else {
>  			continue;
>  		}
>  
>  		int32_t value = lroundf(it.second.get<float>() * 128 + offset);
> -		controls.set(cid, std::clamp(value, 0, 255));
> +		controls.set(cid->id(), std::clamp(value, 0, 255));

Well, at least adding an operator unsigned int() would let us use
controls.set(cid...) directly. Not yet sure if the benefits outweigh the
(not yet quantified) risks though.

But don't we have the ability to set a control on a ControlList by
ControlId anyway?


Anyway, overall - having the V4L2s typed, seems to make the usage much
nicer in the majority of cases, and only the switch statements are more
awkward.

Maybe there's a solution for those in the future, but generally I feel
like this would be a fairly good improvement.


Not sure what's quite applicable here:

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

as you wish, if you continue.

--
Kieran


>  	}
>  
>  	for (const auto &ctrl : controls)
>