[{"id":4581,"web_url":"https://patchwork.libcamera.org/comment/4581/","msgid":"<20200427144334.wpxya4f4cnkcj2g3@uno.localdomain>","date":"2020-04-27T14:43:34","subject":"Re: [libcamera-devel] [PATCH v5.1 5/7] libcamera: controls: Reorder\n\tand update description of existing controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Apr 26, 2020 at 03:25:30AM +0300, Laurent Pinchart wrote:\n> From: Naushir Patuck \n>\n> Group AE, AWB, etc. controls together for accessibility.\n>\n> Update descriptions for Contrast, Brightness, and Saturation controls.\n>\n> Update the uvcvideo and vimc pipeline handlers to use the new\n> brightness, contrast and saturation units. UVC has no explicit units for\n> those controls, so map them with a best effort heuristic. For VIMC,\n> hardcode the control range based on knowledge of the driver\n> implementation for simplicity.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Laurent Pinchart \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> Changes since v5:\n>\n> - Include math.h in vimc.cpp\n> ---\n> src/libcamera/control_ids.yaml | 42 +++++++-----\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 67 +++++++++++++++++---\n> src/libcamera/pipeline/vimc/vimc.cpp | 30 ++++++---\n> 3 files changed, 104 insertions(+), 35 deletions(-)\n>\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index d8bdb3829be4..f2ac052b3d3e 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -25,23 +25,6 @@ controls:\n>\n> \\sa AeEnable\n>\n> - - AwbEnable:\n> - type: bool\n> - description: |\n> - Enable or disable the AWB.\n> -\n> - - Brightness:\n> - type: int32_t\n> - description: Specify a fixed brightness parameter\n> -\n> - - Contrast:\n> - type: int32_t\n> - description: Specify a fixed contrast parameter\n> -\n> - - Saturation:\n> - type: int32_t\n> - description: Specify a fixed saturation parameter\n> -\n> - ExposureTime:\n> type: int32_t\n> description: |\n> @@ -58,4 +41,29 @@ controls:\n> colour channels. This value cannot be lower than 1.0.\n>\n> \\sa ExposureTime AeEnable\n> +\n> + - Brightness:\n> + type: float\n> + description: |\n> + Specify a fixed brightness parameter. Positive values (up to 1.0)\n> + produce brighter images; negative values (up to -1.0) produce darker\n> + images and 0.0 leaves pixels unchanged.\n> +\n> + - Contrast:\n> + type: float\n> + description: |\n> + Specify a fixed contrast parameter. Normal contrast is given by the\n> + value 1.0; larger values produce images with more contrast.\n> +\n> + - AwbEnable:\n> + type: bool\n> + description: |\n> + Enable or disable the AWB.\n> +\n> + - Saturation:\n> + type: float\n> + description: |\n> + Specify a fixed saturation parameter. Normal saturation is given by\n> + the value 1.0; larger values produce more saturated colours; 0.0\n> + produces a greyscale image.\n> ...\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 08462542a79b..030ac6864752 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -263,12 +263,29 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t\treturn -EINVAL;\n>\n> \tconst ControlInfo &v4l2Info = controls->infoMap()->at(cid);\n> +\tint32_t min = v4l2Info.min().get();\n> +\tint32_t def = v4l2Info.def().get();\n> +\tint32_t max = v4l2Info.max().get();\n>\n> \t/*\n> \t * See UVCCameraData::addControl() for explanations of the different\n> \t * value mappings.\n> \t */\n> \tswitch (cid) {\n> +\tcase V4L2_CID_BRIGHTNESS: {\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\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\tbreak;\n> +\t}\n> +\n> \tcase V4L2_CID_EXPOSURE_AUTO: {\n> \t\tint32_t ivalue = value.get()\n> \t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> @@ -281,11 +298,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t\tcontrols->set(cid, value.get() / 100);\n> \t\tbreak;\n>\n> +\tcase V4L2_CID_CONTRAST:\n> \tcase V4L2_CID_GAIN: {\n> -\t\tint32_t min = v4l2Info.min().get();\n> -\t\tint32_t max = v4l2Info.max().get();\n> -\t\tint32_t def = v4l2Info.def().get();\n> -\n> \t\tfloat m = (4.0f - 1.0f) / (max - def);\n> \t\tfloat p = 1.0f - m * def;\n>\n> @@ -457,6 +471,38 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> \tint32_t def = v4l2Info.def().get();\n>\n> \tswitch (cid) {\n> +\tcase V4L2_CID_BRIGHTNESS: {\n> +\t\t/*\n> +\t\t * The Brightness control is a float, with 0.0 mapped to the\n> +\t\t * default value. The control range is [-1.0, 1.0], but the V4L2\n> +\t\t * default may not be in the middle of the V4L2 range.\n> +\t\t * Accommodate this by restricting the range of the libcamera\n> +\t\t * control, but always within the maximum limits.\n> +\t\t */\n> +\t\tfloat scale = std::max(max - def, def - min);\n> +\n> +\t\tinfo = ControlInfo{\n> +\t\t\t{ static_cast(min - def) / scale },\n> +\t\t\t{ static_cast(max - def) / scale },\n> +\t\t\t{ 0.0f }\n> +\t\t};\n> +\t\tbreak;\n> +\t}\n> +\n> +\tcase V4L2_CID_SATURATION:\n> +\t\t/*\n> +\t\t * The Saturation control is a float, with 0.0 mapped to the\n> +\t\t * minimum value (corresponding to a fully desaturated image)\n> +\t\t * and 1.0 mapped to the default value. Calculate the maximum\n> +\t\t * value accordingly.\n> +\t\t */\n> +\t\tinfo = ControlInfo{\n> +\t\t\t{ 0.0f },\n> +\t\t\t{ static_cast(max - min) / (def - min) },\n> +\t\t\t{ 1.0f }\n> +\t\t};\n> +\t\tbreak;\n> +\n> \tcase V4L2_CID_EXPOSURE_AUTO:\n> \t\tinfo = ControlInfo{ false, true, true };\n> \t\tbreak;\n> @@ -473,14 +519,15 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> \t\t};\n> \t\tbreak;\n>\n> +\tcase V4L2_CID_CONTRAST:\n> \tcase V4L2_CID_GAIN: {\n> \t\t/*\n> -\t\t * The AnalogueGain control is a float, with 1.0 mapped to the\n> -\t\t * default value. UVC doesn't specify units, and cameras have\n> -\t\t * been seen to expose very different ranges for the gain\n> -\t\t * control. Arbitrarily assume that the minimum and maximum\n> -\t\t * values are respectively no lower than 0.5 and no higher than\n> -\t\t * 4.0.\n> +\t\t * The Contrast and AnalogueGain controls are floats, with 1.0\n> +\t\t * mapped to the default value. UVC doesn't specify units, and\n> +\t\t * cameras have been seen to expose very different ranges for\n> +\t\t * the controls. Arbitrarily assume that the minimum and\n> +\t\t * maximum values are respectively no lower than 0.5 and no\n> +\t\t * higher than 4.0.\n> \t\t */\n> \t\tfloat m = (4.0f - 1.0f) / (max - def);\n> \t\tfloat p = 1.0f - m * def;\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index c5eea3a01b0e..77de4c38bf01 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -304,14 +305,24 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n>\n> \tfor (auto it : request->controls()) {\n> \t\tunsigned int id = it.first;\n> -\t\tControlValue &value = it.second;\n> +\t\tunsigned int offset;\n> +\t\tuint32_t cid;\n>\n> -\t\tif (id == controls::Brightness)\n> -\t\t\tcontrols.set(V4L2_CID_BRIGHTNESS, value);\n> -\t\telse if (id == controls::Contrast)\n> -\t\t\tcontrols.set(V4L2_CID_CONTRAST, value);\n> -\t\telse if (id == controls::Saturation)\n> -\t\t\tcontrols.set(V4L2_CID_SATURATION, value);\n> +\t\tif (id == controls::Brightness) {\n> +\t\t\tcid = V4L2_CID_BRIGHTNESS;\n> +\t\t\toffset = 128;\n> +\t\t} else if (id == controls::Contrast) {\n> +\t\t\tcid = V4L2_CID_CONTRAST;\n> +\t\t\toffset = 0;\n> +\t\t} else if (id == controls::Saturation) {\n> +\t\t\tcid = V4L2_CID_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, utils::clamp(value, 0, 255));\n> \t}\n>\n> \tfor (const auto &ctrl : controls)\n> @@ -434,18 +445,21 @@ int VimcCameraData::init(MediaDevice *media)\n> \tControlInfoMap::Map ctrls;\n>\n> \tfor (const auto &ctrl : controls) {\n> -\t\tconst ControlInfo &info = ctrl.second;\n> \t\tconst ControlId *id;\n> +\t\tControlInfo info;\n>\n> \t\tswitch (ctrl.first->id()) {\n> \t\tcase V4L2_CID_BRIGHTNESS:\n> \t\t\tid = &controls::Brightness;\n> +\t\t\tinfo = ControlInfo{ { -1.0f }, { 1.0f }, { 0.0f } };\n> \t\t\tbreak;\n> \t\tcase V4L2_CID_CONTRAST:\n> \t\t\tid = &controls::Contrast;\n> +\t\t\tinfo = ControlInfo{ { 0.0f }, { 2.0f }, { 1.0f } };\n> \t\t\tbreak;\n> \t\tcase V4L2_CID_SATURATION:\n> \t\t\tid = &controls::Saturation;\n> +\t\t\tinfo = ControlInfo{ { 0.0f }, { 2.0f }, { 1.0f } };\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\tcontinue;\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14A45603F9\n\tfor ;\n\tMon, 27 Apr 2020 16:40:25 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 1630F60003;\n\tMon, 27 Apr 2020 14:40:23 +0000 (UTC)"],"X-Originating-IP":"212.216.150.148","Date":"Mon, 27 Apr 2020 16:43:34 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200427144334.wpxya4f4cnkcj2g3@uno.localdomain>","References":"<20200425004533.26907-6-laurent.pinchart@ideasonboard.com>\n\t<20200426002530.24747-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200426002530.24747-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5.1 5/7] libcamera: controls: Reorder\n\tand update description of existing controls","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","X-List-Received-Date":"Mon, 27 Apr 2020 14:40:25 -0000"}}]