[{"id":15095,"web_url":"https://patchwork.libcamera.org/comment/15095/","msgid":"","date":"2021-02-11T15:34:51","subject":"Re: [libcamera-devel] [PATCH/RFC 2/2] libcamera: Use V4L2 Control\n\tinstances","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 08/02/2021 22:54, Laurent Pinchart wrote:\n> Use the V4L2 Control instances instead of the numerical V4L2_CID_*\n> identifiers where possible. This simplify usage of the ControlList API\n> in several places, and allows printing control names in the CameraSensor\n> class and the RaspberryPi IPA.\n> \n> There are still locations where the numerical identifiers are used:\n> \n> - In the IPU3 and RaspberryPi code, for custom controls not defined in\n> linux/v4l2-controls.h, which can be fixed by adding additional\n> device-specific Control instances.\n\nThat's easy enough...\n\n\n> - In the DelayedControls and V4L2Device controls APIs, which we could\n> move to ControlId if desired.\n\nAlso easy, I expect?\n\n> - In the uvcvideo and vimc pipeline handlers, when translating between\n> V4L2 and libcamera controls, for switch/case or for code that operate\n> on different controls in a generic way. This would be more difficult\n> to replace.\n\nOh :(\n\n\n\n> \n> Further improvements to the controls API may be possible on top of this,\n> such as dropping the ID-based ControlList::get() and ControlList::set()\n> functions (at possibly replacing them with a version that takes a\n> ControlId pointer). Another possibly interesting improvement would be to\n> add a lookup API to the ControlInfoMap class that would take a\n> Control reference, and return a type-aware wrapper around ControlInfo\n> to avoid having to deal manually with ControlValue.\n\nThis does also mean that a V4L2 control is now strongly typed, so we can\nbe explicit about when it is used rather than passing around integers. I\nfeel like that must bring some benefit or safety from the compiler in\nthe future, but I don't know yet.\n\n\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/ipu3.cpp | 10 +++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 25 ++++++-------\n> src/ipa/rkisp1/rkisp1.cpp | 10 +++---\n> src/libcamera/camera_sensor.cpp | 36 +++++++++----------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 16 ++++-----\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 29 +++++++--------\n> src/libcamera/pipeline/vimc/vimc.cpp | 10 +++---\n> 8 files changed, 68 insertions(+), 70 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index b11b03efa6ce..5ee3fd302e01 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -11,7 +11,6 @@\n> #include \n> \n> #include \n> -#include \n> \n> #include \n> #include \n> @@ -23,6 +22,7 @@\n> \n> #include \"libcamera/internal/buffer.h\"\n> #include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/v4l2_controls.h\"\n> \n> namespace libcamera {\n> \n> @@ -80,13 +80,13 @@ void IPAIPU3::configure([[maybe_unused]] const CameraSensorInfo &info,\n> \n> \tctrls_ = entityControls.at(0);\n> \n> -\tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> +\tconst auto itExp = ctrls_.find(&v4l2::EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> \t\tLOG(IPAIPU3, Error) << \"Can't find exposure control\";\n> \t\treturn;\n> \t}\n> \n> -\tconst auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> +\tconst auto itGain = ctrls_.find(&v4l2::ANALOGUE_GAIN);\n> \tif (itGain == ctrls_.end()) {\n> \t\tLOG(IPAIPU3, Error) << \"Can't find gain control\";\n> \t\treturn;\n> @@ -214,8 +214,8 @@ void IPAIPU3::setControls(unsigned int frame)\n> \top.operation = IPU3_IPA_ACTION_SET_SENSOR_CONTROLS;\n> \n> \tControlList ctrls(ctrls_);\n> -\tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> +\tctrls.set(v4l2::EXPOSURE, exposure_);\n> +\tctrls.set(v4l2::ANALOGUE_GAIN, gain_);\n\nHaving the types defined so they don't need casts looks like a good win\nhere.\n\n> \top.controls.push_back(ctrls);\n> \n> \tqueueFrameAction.emit(frame, op);\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index ff14cfc4b706..897f3f0b91f1 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -542,16 +542,16 @@ void IPARPi::reportMetadata()\n> \n> bool IPARPi::validateSensorControls()\n> {\n> -\tstatic const uint32_t ctrls[] = {\n> -\t\tV4L2_CID_ANALOGUE_GAIN,\n> -\t\tV4L2_CID_EXPOSURE,\n> -\t\tV4L2_CID_VBLANK,\n> +\tstatic const ControlId *const ctrls[] = {\n> +\t\t&v4l2::ANALOGUE_GAIN,\n> +\t\t&v4l2::EXPOSURE,\n> +\t\t&v4l2::VBLANK,\n> \t};\n> \n> \tfor (auto c : ctrls) {\n> \t\tif (sensorCtrls_.find(c) == sensorCtrls_.end()) {\n> \t\t\tLOG(IPARPI, Error) << \"Unable to find sensor control \"\n> -\t\t\t\t\t << utils::hex(c);\n> +\t\t\t\t\t << c->name();\n\nI'm sure you know that makes me quite happy ;-)\n\n> \t\t\treturn false;\n> \t\t}\n> \t}\n> @@ -1038,10 +1038,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> \tLOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gain_r << \" B: \"\n> \t\t\t << awbStatus->gain_b;\n> \n> -\tctrls.set(V4L2_CID_RED_BALANCE,\n> -\t\t static_cast(awbStatus->gain_r * 1000));\n> -\tctrls.set(V4L2_CID_BLUE_BALANCE,\n> -\t\t static_cast(awbStatus->gain_b * 1000));\n> +\tctrls.set(v4l2::RED_BALANCE, awbStatus->gain_r * 1000);\n> +\tctrls.set(v4l2::BLUE_BALANCE, awbStatus->gain_b * 1000);\n\nAgain, cleaner code without casts ....\n\n> }\n> \n> void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)\n> @@ -1100,15 +1098,14 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> \t * exposure time without us knowing. The next time though this function should\n> \t * clip exposure correctly.\n> \t */\n> -\tctrls.set(V4L2_CID_VBLANK, vblanking);\n> -\tctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> +\tctrls.set(v4l2::VBLANK, vblanking);\n> +\tctrls.set(v4l2::EXPOSURE, exposureLines);\n> +\tctrls.set(v4l2::ANALOGUE_GAIN, gainCode);\n> }\n> \n> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> {\n> -\tctrls.set(V4L2_CID_DIGITAL_GAIN,\n> -\t\t static_cast(dgStatus->digital_gain * 1000));\n> +\tctrls.set(v4l2::DIGITAL_GAIN, dgStatus->digital_gain * 1000);\n> }\n> \n> void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 39783abd731b..ce47ba8cb38a 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -13,7 +13,6 @@\n> #include \n> \n> #include \n> -#include \n> \n> #include \n> #include \n> @@ -25,6 +24,7 @@\n> #include \n> \n> #include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/v4l2_controls.h\"\n> \n> namespace libcamera {\n> \n> @@ -91,13 +91,13 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> \n> \tctrls_ = entityControls.at(0);\n> \n> -\tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> +\tconst auto itExp = ctrls_.find(&v4l2::EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> \t\tLOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> \t\treturn;\n> \t}\n> \n> -\tconst auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> +\tconst auto itGain = ctrls_.find(&v4l2::ANALOGUE_GAIN);\n> \tif (itGain == ctrls_.end()) {\n> \t\tLOG(IPARkISP1, Error) << \"Can't find gain control\";\n> \t\treturn;\n> @@ -261,8 +261,8 @@ void IPARkISP1::setControls(unsigned int frame)\n> \top.operation = RKISP1_IPA_ACTION_V4L2_SET;\n> \n> \tControlList ctrls(ctrls_);\n> -\tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> +\tctrls.set(v4l2::EXPOSURE, exposure_);\n> +\tctrls.set(v4l2::ANALOGUE_GAIN, gain_);\n> \top.controls.push_back(ctrls);\n> \n> \tqueueFrameAction.emit(frame, op);\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index c9e8d49b7935..eb96c4841708 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -280,16 +280,16 @@ int CameraSensor::validateSensorDriver()\n> \t * Optional controls are used to register optional sensor properties. If\n> \t * not present, some values will be defaulted.\n> \t */\n> -\tstatic constexpr uint32_t optionalControls[] = {\n> -\t\tV4L2_CID_CAMERA_ORIENTATION,\n> -\t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> +\tstatic const ControlId *const optionalControls[] = {\n> +\t\t&v4l2::CAMERA_ORIENTATION,\n> +\t\t&v4l2::CAMERA_SENSOR_ROTATION,\n> \t};\n> \n> -\tconst ControlIdMap &controls = subdev_->controls().idmap();\n> -\tfor (uint32_t ctrl : optionalControls) {\n> +\tconst ControlInfoMap &controls = subdev_->controls();\n> +\tfor (const ControlId *ctrl : optionalControls) {\n> \t\tif (!controls.count(ctrl))\n> \t\t\tLOG(CameraSensor, Debug)\n> -\t\t\t\t<< \"Optional V4L2 control \" << utils::hex(ctrl)\n> +\t\t\t\t<< \"Optional V4L2 control \" << ctrl->name()\n> \t\t\t\t<< \" not supported\";\n> \t}\n> \n> @@ -346,18 +346,18 @@ int CameraSensor::validateSensorDriver()\n> \t * For raw sensors, make sure the sensor driver supports the controls\n> \t * required by the CameraSensor class.\n> \t */\n> -\tstatic constexpr uint32_t mandatoryControls[] = {\n> -\t\tV4L2_CID_EXPOSURE,\n> -\t\tV4L2_CID_HBLANK,\n> -\t\tV4L2_CID_PIXEL_RATE,\n> -\t\tV4L2_CID_VBLANK,\n> +\tstatic const ControlId *const mandatoryControls[] = {\n> +\t\t&v4l2::EXPOSURE,\n> +\t\t&v4l2::HBLANK,\n> +\t\t&v4l2::PIXEL_RATE,\n> +\t\t&v4l2::VBLANK,\n> \t};\n> \n> \terr = 0;\n> -\tfor (uint32_t ctrl : mandatoryControls) {\n> +\tfor (const ControlId *ctrl : mandatoryControls) {\n> \t\tif (!controls.count(ctrl)) {\n> \t\t\tLOG(CameraSensor, Error)\n> -\t\t\t\t<< \"Mandatory V4L2 control \" << utils::hex(ctrl)\n> +\t\t\t\t<< \"Mandatory V4L2 control \" << ctrl->name()\n> \t\t\t\t<< \" not available\";\n\nAnd you've covered both optional and mandatory controls, (and more) ...\nso that's a big plus for me.\n\n\n> \t\t\terr = -EINVAL;\n> \t\t}\n> @@ -425,7 +425,7 @@ int CameraSensor::initProperties()\n> \tconst ControlInfoMap &controls = subdev_->controls();\n> \tint32_t propertyValue;\n> \n> -\tconst auto &orientation = controls.find(V4L2_CID_CAMERA_ORIENTATION);\n> +\tconst auto &orientation = controls.find(&v4l2::CAMERA_ORIENTATION);\n> \tif (orientation != controls.end()) {\n> \t\tint32_t v4l2Orientation = orientation->second.def().get();\n> \n> @@ -450,7 +450,7 @@ int CameraSensor::initProperties()\n> \t}\n> \tproperties_.set(properties::Location, propertyValue);\n> \n> -\tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> +\tconst auto &rotationControl = controls.find(&v4l2::CAMERA_SENSOR_ROTATION);\n> \tif (rotationControl != controls.end()) {\n> \t\tpropertyValue = rotationControl->second.def().get();\n> \t\tproperties_.set(properties::Rotation, propertyValue);\n> @@ -775,11 +775,11 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tint32_t hblank = ctrls.get(V4L2_CID_HBLANK).get();\n> +\tint32_t hblank = ctrls.get(v4l2::HBLANK);\n> \tinfo->lineLength = info->outputSize.width + hblank;\n> -\tinfo->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\n> +\tinfo->pixelRate = ctrls.get(v4l2::PIXEL_RATE);\n> \n> -\tconst ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);\n> +\tconst ControlInfo vblank = ctrls.infoMap()->at(&v4l2::VBLANK);\n> \tinfo->minFrameLength = info->outputSize.height + vblank.min().get();\n> \tinfo->maxFrameLength = info->outputSize.height + vblank.max().get();\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 9bc3df3352fd..d3fc51518598 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -845,7 +845,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \tdouble lineDuration = sensorInfo.lineLength\n> \t\t\t / (sensorInfo.pixelRate / 1e6);\n> \tconst ControlInfoMap &sensorControls = sensor->controls();\n> -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> +\tconst ControlInfo &v4l2Exposure = sensorControls.find(&v4l2::EXPOSURE)->second;\n> \tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> \tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> \tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0804a4393c19..90f846530346 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -992,8 +992,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \t\tdata->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT;\n> \n> \t\tControlList ctrls(dev->controls());\n> -\t\tctrls.set(V4L2_CID_HFLIP, 0);\n> -\t\tctrls.set(V4L2_CID_VFLIP, 0);\n> +\t\tctrls.set(v4l2::HFLIP, 0);\n> +\t\tctrls.set(v4l2::VFLIP, 0);\n\nIt's even a shorter prefix...\n\n> \t\tdev->setControls(&ctrls);\n> \t}\n> \n> @@ -1246,10 +1246,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t */\n> \tif (supportsFlips_) {\n> \t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> -\t\tctrls.set(V4L2_CID_HFLIP,\n> -\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));\n> -\t\tctrls.set(V4L2_CID_VFLIP,\n> -\t\t\t static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));\n> +\t\tctrls.set(v4l2::HFLIP,\n> +\t\t\t !!(rpiConfig->combinedTransform_ & Transform::HFlip));\n> +\t\tctrls.set(v4l2::VFLIP,\n> +\t\t\t !!(rpiConfig->combinedTransform_ & Transform::VFlip));\n\nDo those fit on one line (each) nicely now? Hrm... looks like 94 chars,\nstill good in my book ;-) But I know people are keen on their 80cols.\n\n\n> \t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> \t}\n> \n> @@ -1383,8 +1383,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \t\t\tauto it = mappedEmbeddedBuffers_.find(bufferId);\n> \t\t\tif (it != mappedEmbeddedBuffers_.end()) {\n> \t\t\t\tuint32_t *mem = reinterpret_cast(it->second.maps()[0].data());\n> -\t\t\t\tmem[0] = ctrl.get(V4L2_CID_EXPOSURE).get();\n> -\t\t\t\tmem[1] = ctrl.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\t\t\t\tmem[0] = ctrl.get(v4l2::EXPOSURE);\n> +\t\t\t\tmem[1] = ctrl.get(v4l2::ANALOGUE_GAIN);\n> \t\t\t} else {\n> \t\t\t\tLOG(RPI, Warning) << \"Failed to find embedded buffer \"\n> \t\t\t\t\t\t << bufferId;\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 08a594175091..a80b51f174a9 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -260,20 +260,20 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t\t\t\t const ControlValue &value)\n> {\n> -\tuint32_t cid;\n> +\tconst ControlId *cid;\n> \n> \tif (id == controls::Brightness)\n> -\t\tcid = V4L2_CID_BRIGHTNESS;\n> +\t\tcid = &v4l2::BRIGHTNESS;\n> \telse if (id == controls::Contrast)\n> -\t\tcid = V4L2_CID_CONTRAST;\n> +\t\tcid = &v4l2::CONTRAST;\n> \telse if (id == controls::Saturation)\n> -\t\tcid = V4L2_CID_SATURATION;\n> +\t\tcid = &v4l2::SATURATION;\n> \telse if (id == controls::AeEnable)\n> -\t\tcid = V4L2_CID_EXPOSURE_AUTO;\n> +\t\tcid = &v4l2::EXPOSURE_AUTO;\n> \telse if (id == controls::ExposureTime)\n> -\t\tcid = V4L2_CID_EXPOSURE_ABSOLUTE;\n> +\t\tcid = &v4l2::EXPOSURE_ABSOLUTE;\n> \telse if (id == controls::AnalogueGain)\n> -\t\tcid = V4L2_CID_GAIN;\n> +\t\tcid = &v4l2::GAIN;\n\nThat looks like we should have a common lookup table/map for known\nmatching controls...\n\nBut not a topic for this patch itself.\n\nExcept I bet that AnalogueGain == CID_GAIN would cause issues against\nANALOGUE_GAIN/DIGITAL_GAIN for any generic lookup :S ...\n\n> \telse\n> \t\treturn -EINVAL;\n> \n> @@ -286,18 +286,18 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t * See UVCCameraData::addControl() for explanations of the different\n> \t * value mappings.\n> \t */\n> -\tswitch (cid) {\n> +\tswitch (cid->id()) {\n> \tcase V4L2_CID_BRIGHTNESS: {\n\nI see, mixing and matching between v4l2:: and V4L2_CID here is a pain,\nbut it works at least.\n\n\nI had hoped we could give ControlID an operator unsigned int(), and be\nable to resolve this, but even with that - the switch statement needs a\nconstexpr which I can't see how to make them so.\n\nEven though as far as I can see they likely meet most of the\nrequirements, they are fully initialised at construtor, and not\nmodifyable...\n\n\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 3b7f3347761e..9938d3e6b2e9 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -219,6 +219,8 @@ public:\n> const std::string &name() const { return name_; }\n> ControlType type() const { return type_; }\n> \n> + operator unsigned int() const { return id_; }\n> +\n> private:\n> ControlId &operator=(const ControlId &) = delete;\n> ControlId(const ControlId &) = delete;\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index a80b51f174a9..c8647a1f708b 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -287,7 +287,7 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> * value mappings.\n> */\n> switch (cid->id()) {\n> - case V4L2_CID_BRIGHTNESS: {\n> + case v4l2::BRIGHTNESS: {\n> float scale = std::max(max - def, def - min);\n> float fvalue = value.get() * scale + def;\n> controls->set(v4l2::BRIGHTNESS, lroundf(fvalue));\n\n\nBut alas, I can't (quickly) fix this:\n\n> ../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp: In member function ‘int libcamera::PipelineHandlerUVC::processControl(libcamera::ControlList*, unsigned int, const libcamera::ControlValue&)’:\n> ../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:290:13: error: the value of ‘libcamera::v4l2::BRIGHTNESS’ is not usable in a constant expression\n> 290 | case v4l2::BRIGHTNESS: {\n> | ^~~~~~~~~~\n> In file included from ../include/libcamera/internal/v4l2_controls.h:15,\n> from ../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:28:\n> include/libcamera/internal/v4l2_control_ids.h:24:31: note: ‘libcamera::v4l2::BRIGHTNESS’ was not declared ‘constexpr’\n> 24 | extern const Control BRIGHTNESS;\n> | ^~~~~~~~~~\n> ../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:290:13: error: call to non-‘constexpr’ function ‘libcamera::ControlId::operator unsigned int() const’\n> 290 | case v4l2::BRIGHTNESS: {\n> | ^~~~~~~~~~\n\n\nI wonder if allowing to return as an unsigned int would break our type\nsafety anyway, but maybe there's a path to clean things up later.\n\n\n\n\n> \t\tfloat scale = std::max(max - def, def - min);\n> \t\tfloat fvalue = value.get() * scale + def;\n> -\t\tcontrols->set(cid, static_cast(lroundf(fvalue)));\n> +\t\tcontrols->set(v4l2::BRIGHTNESS, lroundf(fvalue));\n> \t\tbreak;\n> \t}\n> \n> \tcase V4L2_CID_SATURATION: {\n> \t\tfloat scale = def - min;\n> \t\tfloat fvalue = value.get() * scale + min;\n> -\t\tcontrols->set(cid, static_cast(lroundf(fvalue)));\n> +\t\tcontrols->set(v4l2::SATURATION, lroundf(fvalue));\n> \t\tbreak;\n> \t}\n> \n> @@ -305,12 +305,13 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t\tint32_t ivalue = value.get()\n> \t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> \t\t\t : V4L2_EXPOSURE_MANUAL;\n> -\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> +\t\tcontrols->set(v4l2::EXPOSURE_AUTO, ivalue);\n> \t\tbreak;\n> \t}\n> \n> \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> -\t\tcontrols->set(cid, value.get() / 100);\n> +\t\tcontrols->set(v4l2::EXPOSURE_ABSOLUTE,\n> +\t\t\t value.get() / 100);\n> \t\tbreak;\n> \n> \tcase V4L2_CID_CONTRAST:\n> @@ -324,13 +325,13 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t\t}\n> \n> \t\tfloat fvalue = (value.get() - p) / m;\n> -\t\tcontrols->set(cid, static_cast(lroundf(fvalue)));\n> +\t\tcontrols->set(cid->id(), static_cast(lroundf(fvalue)));\n> \t\tbreak;\n> \t}\n> \n> \tdefault: {\n> \t\tint32_t ivalue = value.get();\n> -\t\tcontrols->set(cid, ivalue);\n> +\t\tcontrols->set(cid->id(), ivalue);\n> \t\tbreak;\n> \t}\n> \t}\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 36325ffbbd7d..24b39b7dd879 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -342,24 +342,24 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> \n> \tfor (auto it : request->controls()) {\n> \t\tunsigned int id = it.first;\n> +\t\tconst ControlId *cid;\n> \t\tunsigned int offset;\n> -\t\tuint32_t cid;\n> \n> \t\tif (id == controls::Brightness) {\n> -\t\t\tcid = V4L2_CID_BRIGHTNESS;\n> +\t\t\tcid = &v4l2::BRIGHTNESS;\n> \t\t\toffset = 128;\n> \t\t} else if (id == controls::Contrast) {\n> -\t\t\tcid = V4L2_CID_CONTRAST;\n> +\t\t\tcid = &v4l2::CONTRAST;\n> \t\t\toffset = 0;\n> \t\t} else if (id == controls::Saturation) {\n> -\t\t\tcid = V4L2_CID_SATURATION;\n> +\t\t\tcid = &v4l2::SATURATION;\n> \t\t\toffset = 0;\n> \t\t} else {\n> \t\t\tcontinue;\n> \t\t}\n> \n> \t\tint32_t value = lroundf(it.second.get() * 128 + offset);\n> -\t\tcontrols.set(cid, std::clamp(value, 0, 255));\n> +\t\tcontrols.set(cid->id(), std::clamp(value, 0, 255));\n\nWell, at least adding an operator unsigned int() would let us use\ncontrols.set(cid...) directly. Not yet sure if the benefits outweigh the\n(not yet quantified) risks though.\n\nBut don't we have the ability to set a control on a ControlList by\nControlId anyway?\n\n\nAnyway, overall - having the V4L2s typed, seems to make the usage much\nnicer in the majority of cases, and only the switch statements are more\nawkward.\n\nMaybe there's a solution for those in the future, but generally I feel\nlike this would be a fairly good improvement.\n\n\nNot sure what's quite applicable here:\n\nAcked-by: Kieran Bingham \nor\nReviewed-by: Kieran Bingham \n\nas you wish, if you continue.\n\n--\nKieran\n\n\n> \t}\n> \n> \tfor (const auto &ctrl : controls)\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A354ABD160\n\tfor ;\n\tThu, 11 Feb 2021 15:34:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E93863737;\n\tThu, 11 Feb 2021 16:34:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45BD7601B5\n\tfor ;\n\tThu, 11 Feb 2021 16:34:54 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9E39A41;\n\tThu, 11 Feb 2021 16:34:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EvtWXkZq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613057693;\n\tbh=uKQtYhKCYZ5P2yu3oZHI0956mA5ToR8s19wAyFQebcE=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=EvtWXkZqJeUVq8G+LA9YQYAWmu3mUN9oR22n7v8pXLzRDFh+7s2tLajzshYIMkpsU\n\tm9hVb/0+J23rF3uv1RjE7HEG6RfxmWRRXUaB1yLUIPwXjeVnMtlWSGIOpDc+1rXALB\n\ttnpfEZeQ+72hgv16hsDdCsC0BkRAcSZucaUDLt4k=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210208225429.31627-1-laurent.pinchart@ideasonboard.com>\n\t<20210208225429.31627-3-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 11 Feb 2021 15:34:51 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210208225429.31627-3-laurent.pinchart@ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH/RFC 2/2] libcamera: Use V4L2 Control\n\tinstances","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]