[{"id":4525,"web_url":"https://patchwork.libcamera.org/comment/4525/","msgid":"<20200426125057.kcpft54tiv73can5@uno.localdomain>","date":"2020-04-26T12:50:57","subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: pipeline: uvcvideo:\n\tRefactor control handling","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Apr 25, 2020 at 03:45:27AM +0300, Laurent Pinchart wrote:\n> Move addition and processing of individual controls to separate\n> functions, to prepare for more complex mappings of control values.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 103 ++++++++++++-------\n> 1 file changed, 67 insertions(+), 36 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index ffbddf27ae2f..92777b9f5fe4 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -42,6 +42,8 @@ public:\n> \t}\n>\n> \tint init(MediaEntity *entity);\n> +\tvoid addControl(uint32_t cid, const ControlInfo &v4l2info,\n> +\t\t\tControlInfoMap::Map *ctrls);\n> \tvoid bufferReady(FrameBuffer *buffer);\n>\n> \tV4L2VideoDevice *video_;\n> @@ -76,6 +78,8 @@ public:\n> \tbool match(DeviceEnumerator *enumerator) override;\n>\n> private:\n> +\tint processControl(ControlList *controls, unsigned int id,\n> +\t\t\t const ControlValue &value);\n> \tint processControls(UVCCameraData *data, Request *request);\n>\n> \tUVCCameraData *cameraData(const Camera *camera)\n> @@ -237,6 +241,37 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> \tdata->video_->releaseBuffers();\n> }\n>\n> +int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> +\t\t\t\t const ControlValue &value)\n> +{\n> +\tif (value.type() != ControlTypeInteger32)\n> +\t\treturn -EINVAL;\n\nIs this needed ? All supported controls are int32, you will fail later\nanyway if the control id is not supported, and this check will have to\nbe removed once (and if) we support non int32_t controls\n\n> +\n> +\tuint32_t cid;\n> +\n> +\tif (id == controls::Brightness)\n> +\t\tcid = V4L2_CID_BRIGHTNESS;\n> +\telse if (id == controls::Contrast)\n> +\t\tcid = V4L2_CID_CONTRAST;\n> +\telse if (id == controls::Saturation)\n> +\t\tcid = V4L2_CID_SATURATION;\n> +\telse if (id == controls::ManualExposure)\n> +\t\tcid = V4L2_CID_EXPOSURE_ABSOLUTE;\n> +\telse if (id == controls::ManualGain)\n> +\t\tcid = V4L2_CID_GAIN;\n> +\telse\n> +\t\treturn -EINVAL;\n> +\n> +\tint32_t ivalue = value.get();\n\ncould you move this one after the next if condition ?\n\n> +\n> +\tif (cid == V4L2_CID_EXPOSURE_ABSOLUTE)\n> +\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, static_cast(1));\n> +\n> +\tcontrols->set(cid, ivalue);\n> +\n> +\treturn 0;\n> +}\n> +\n> int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> {\n> \tControlList controls(data->video_->controls());\n> @@ -245,18 +280,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> \t\tunsigned int id = it.first;\n> \t\tControlValue &value = it.second;\n>\n> -\t\tif (id == controls::Brightness) {\n> -\t\t\tcontrols.set(V4L2_CID_BRIGHTNESS, value);\n> -\t\t} else if (id == controls::Contrast) {\n> -\t\t\tcontrols.set(V4L2_CID_CONTRAST, value);\n> -\t\t} else if (id == controls::Saturation) {\n> -\t\t\tcontrols.set(V4L2_CID_SATURATION, value);\n> -\t\t} else if (id == controls::ManualExposure) {\n> -\t\t\tcontrols.set(V4L2_CID_EXPOSURE_AUTO, static_cast(1));\n> -\t\t\tcontrols.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);\n> -\t\t} else if (id == controls::ManualGain) {\n> -\t\t\tcontrols.set(V4L2_CID_GAIN, value);\n> -\t\t}\n> +\t\tprocessControl(&controls, id, value);\n> \t}\n>\n> \tfor (const auto &ctrl : controls)\n> @@ -346,34 +370,13 @@ int UVCCameraData::init(MediaEntity *entity)\n> \tvideo_->bufferReady.connect(this, &UVCCameraData::bufferReady);\n>\n> \t/* Initialise the supported controls. */\n> -\tconst ControlInfoMap &controls = video_->controls();\n> \tControlInfoMap::Map ctrls;\n>\n> -\tfor (const auto &ctrl : controls) {\n> +\tfor (const auto &ctrl : video_->controls()) {\n> +\t\tuint32_t cid = ctrl.first->id();\n> \t\tconst ControlInfo &info = ctrl.second;\n> -\t\tconst ControlId *id;\n>\n> -\t\tswitch (ctrl.first->id()) {\n> -\t\tcase V4L2_CID_BRIGHTNESS:\n> -\t\t\tid = &controls::Brightness;\n> -\t\t\tbreak;\n> -\t\tcase V4L2_CID_CONTRAST:\n> -\t\t\tid = &controls::Contrast;\n> -\t\t\tbreak;\n> -\t\tcase V4L2_CID_SATURATION:\n> -\t\t\tid = &controls::Saturation;\n> -\t\t\tbreak;\n> -\t\tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> -\t\t\tid = &controls::ManualExposure;\n> -\t\t\tbreak;\n> -\t\tcase V4L2_CID_GAIN:\n> -\t\t\tid = &controls::ManualGain;\n> -\t\t\tbreak;\n> -\t\tdefault:\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\tctrls.emplace(id, info);\n> +\t\taddControl(cid, info, &ctrls);\n> \t}\n>\n> \tcontrolInfo_ = std::move(ctrls);\n> @@ -381,6 +384,34 @@ int UVCCameraData::init(MediaEntity *entity)\n> \treturn 0;\n> }\n>\n> +void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> +\t\t\t ControlInfoMap::Map *ctrls)\n> +{\n> +\tconst ControlId *id;\n> +\n> +\tswitch (cid) {\n> +\tcase V4L2_CID_BRIGHTNESS:\n> +\t\tid = &controls::Brightness;\n> +\t\tbreak;\n> +\tcase V4L2_CID_CONTRAST:\n> +\t\tid = &controls::Contrast;\n> +\t\tbreak;\n> +\tcase V4L2_CID_SATURATION:\n> +\t\tid = &controls::Saturation;\n> +\t\tbreak;\n> +\tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> +\t\tid = &controls::ManualExposure;\n> +\t\tbreak;\n> +\tcase V4L2_CID_GAIN:\n> +\t\tid = &controls::ManualGain;\n\nI'm a bit concerned all pipeline handlers will have to implement this\nlibcamera-control<->v4l2-control translation, but that's not on this\npatch\n\nAnyway\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treturn;\n> +\t}\n> +\n> +\tctrls->emplace(id, v4l2Info);\n> +}\n> +\n> void UVCCameraData::bufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\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 relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E2DD603FA\n\tfor ;\n\tSun, 26 Apr 2020 14:47:55 +0200 (CEST)","from uno.localdomain\n\t(host170-48-dynamic.3-87-r.retail.telecomitalia.it [87.3.48.170])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 17D2F200006;\n\tSun, 26 Apr 2020 12:47:51 +0000 (UTC)"],"Date":"Sun, 26 Apr 2020 14:50:57 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426125057.kcpft54tiv73can5@uno.localdomain>","References":"<20200425004533.26907-1-laurent.pinchart@ideasonboard.com>\n\t<20200425004533.26907-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200425004533.26907-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: pipeline: uvcvideo:\n\tRefactor control handling","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":"Sun, 26 Apr 2020 12:47:55 -0000"}},{"id":4535,"web_url":"https://patchwork.libcamera.org/comment/4535/","msgid":"<20200426170005.GC5950@pendragon.ideasonboard.com>","date":"2020-04-26T17:00:05","subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: pipeline: uvcvideo:\n\tRefactor control handling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Apr 26, 2020 at 02:50:57PM +0200, Jacopo Mondi wrote:\n> On Sat, Apr 25, 2020 at 03:45:27AM +0300, Laurent Pinchart wrote:\n> > Move addition and processing of individual controls to separate\n> > functions, to prepare for more complex mappings of control values.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 103 ++++++++++++-------\n> > 1 file changed, 67 insertions(+), 36 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index ffbddf27ae2f..92777b9f5fe4 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -42,6 +42,8 @@ public:\n> > \t}\n> >\n> > \tint init(MediaEntity *entity);\n> > +\tvoid addControl(uint32_t cid, const ControlInfo &v4l2info,\n> > +\t\t\tControlInfoMap::Map *ctrls);\n> > \tvoid bufferReady(FrameBuffer *buffer);\n> >\n> > \tV4L2VideoDevice *video_;\n> > @@ -76,6 +78,8 @@ public:\n> > \tbool match(DeviceEnumerator *enumerator) override;\n> >\n> > private:\n> > +\tint processControl(ControlList *controls, unsigned int id,\n> > +\t\t\t const ControlValue &value);\n> > \tint processControls(UVCCameraData *data, Request *request);\n> >\n> > \tUVCCameraData *cameraData(const Camera *camera)\n> > @@ -237,6 +241,37 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> > \tdata->video_->releaseBuffers();\n> > }\n> >\n> > +int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > +\t\t\t\t const ControlValue &value)\n> > +{\n> > +\tif (value.type() != ControlTypeInteger32)\n> > +\t\treturn -EINVAL;\n> \n> Is this needed ? All supported controls are int32, you will fail later\n> anyway if the control id is not supported, and this check will have to\n> be removed once (and if) we support non int32_t controls\n\nIt's actually removed in later patches within this series. I've added it\nhere for testing purpose, I will drop it.\n\n> > +\n> > +\tuint32_t cid;\n> > +\n> > +\tif (id == controls::Brightness)\n> > +\t\tcid = V4L2_CID_BRIGHTNESS;\n> > +\telse if (id == controls::Contrast)\n> > +\t\tcid = V4L2_CID_CONTRAST;\n> > +\telse if (id == controls::Saturation)\n> > +\t\tcid = V4L2_CID_SATURATION;\n> > +\telse if (id == controls::ManualExposure)\n> > +\t\tcid = V4L2_CID_EXPOSURE_ABSOLUTE;\n> > +\telse if (id == controls::ManualGain)\n> > +\t\tcid = V4L2_CID_GAIN;\n> > +\telse\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tint32_t ivalue = value.get();\n> \n> could you move this one after the next if condition ?\n\nSure. The code is reworked later anyway.\n\n> > +\n> > +\tif (cid == V4L2_CID_EXPOSURE_ABSOLUTE)\n> > +\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, static_cast(1));\n> > +\n> > +\tcontrols->set(cid, ivalue);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > {\n> > \tControlList controls(data->video_->controls());\n> > @@ -245,18 +280,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > \t\tunsigned int id = it.first;\n> > \t\tControlValue &value = it.second;\n> >\n> > -\t\tif (id == controls::Brightness) {\n> > -\t\t\tcontrols.set(V4L2_CID_BRIGHTNESS, value);\n> > -\t\t} else if (id == controls::Contrast) {\n> > -\t\t\tcontrols.set(V4L2_CID_CONTRAST, value);\n> > -\t\t} else if (id == controls::Saturation) {\n> > -\t\t\tcontrols.set(V4L2_CID_SATURATION, value);\n> > -\t\t} else if (id == controls::ManualExposure) {\n> > -\t\t\tcontrols.set(V4L2_CID_EXPOSURE_AUTO, static_cast(1));\n> > -\t\t\tcontrols.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);\n> > -\t\t} else if (id == controls::ManualGain) {\n> > -\t\t\tcontrols.set(V4L2_CID_GAIN, value);\n> > -\t\t}\n> > +\t\tprocessControl(&controls, id, value);\n> > \t}\n> >\n> > \tfor (const auto &ctrl : controls)\n> > @@ -346,34 +370,13 @@ int UVCCameraData::init(MediaEntity *entity)\n> > \tvideo_->bufferReady.connect(this, &UVCCameraData::bufferReady);\n> >\n> > \t/* Initialise the supported controls. */\n> > -\tconst ControlInfoMap &controls = video_->controls();\n> > \tControlInfoMap::Map ctrls;\n> >\n> > -\tfor (const auto &ctrl : controls) {\n> > +\tfor (const auto &ctrl : video_->controls()) {\n> > +\t\tuint32_t cid = ctrl.first->id();\n> > \t\tconst ControlInfo &info = ctrl.second;\n> > -\t\tconst ControlId *id;\n> >\n> > -\t\tswitch (ctrl.first->id()) {\n> > -\t\tcase V4L2_CID_BRIGHTNESS:\n> > -\t\t\tid = &controls::Brightness;\n> > -\t\t\tbreak;\n> > -\t\tcase V4L2_CID_CONTRAST:\n> > -\t\t\tid = &controls::Contrast;\n> > -\t\t\tbreak;\n> > -\t\tcase V4L2_CID_SATURATION:\n> > -\t\t\tid = &controls::Saturation;\n> > -\t\t\tbreak;\n> > -\t\tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > -\t\t\tid = &controls::ManualExposure;\n> > -\t\t\tbreak;\n> > -\t\tcase V4L2_CID_GAIN:\n> > -\t\t\tid = &controls::ManualGain;\n> > -\t\t\tbreak;\n> > -\t\tdefault:\n> > -\t\t\tcontinue;\n> > -\t\t}\n> > -\n> > -\t\tctrls.emplace(id, info);\n> > +\t\taddControl(cid, info, &ctrls);\n> > \t}\n> >\n> > \tcontrolInfo_ = std::move(ctrls);\n> > @@ -381,6 +384,34 @@ int UVCCameraData::init(MediaEntity *entity)\n> > \treturn 0;\n> > }\n> >\n> > +void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> > +\t\t\t ControlInfoMap::Map *ctrls)\n> > +{\n> > +\tconst ControlId *id;\n> > +\n> > +\tswitch (cid) {\n> > +\tcase V4L2_CID_BRIGHTNESS:\n> > +\t\tid = &controls::Brightness;\n> > +\t\tbreak;\n> > +\tcase V4L2_CID_CONTRAST:\n> > +\t\tid = &controls::Contrast;\n> > +\t\tbreak;\n> > +\tcase V4L2_CID_SATURATION:\n> > +\t\tid = &controls::Saturation;\n> > +\t\tbreak;\n> > +\tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > +\t\tid = &controls::ManualExposure;\n> > +\t\tbreak;\n> > +\tcase V4L2_CID_GAIN:\n> > +\t\tid = &controls::ManualGain;\n> \n> I'm a bit concerned all pipeline handlers will have to implement this\n> libcamera-control<->v4l2-control translation, but that's not on this\n> patch\n\nWe could possibly do better indeed. Don't forget that, in practice, few\npipeline handlers will translate libcamera controls to V4L2 controls,\nmost controls will be handled by the IPA. For an ISP-based pipeline\nhandlers, the saturation control, for instance, will likely be handled\nthrough an RGB-to-RGB conversion matrix. So will the contrast. Something\nalong the lines of\n\n[R' [ YUV [1 0 0 [ RGB [R\n G' = to x 0 S 0 x to x G\n B'] RGB ] 0 0 S] YUV ] B]\n\nWith the hardware programmed with a RGB-to-RGB matrix equal to\n\n[ YUV [1 0 0 [ RGB\n to x 0 S 0 x to\n RGB ] 0 0 S] YUV ]\n\nSo it won't be a direct mapping.\n\n> Anyway\n> Reviewed-by: Jacopo Mondi \n> \n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tctrls->emplace(id, v4l2Info);\n> > +}\n> > +\n> > void UVCCameraData::bufferReady(FrameBuffer *buffer)\n> > {\n> > \tRequest *request = buffer->request();","headers":{"Return-Path":"","Received":["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 B4D9D62E55\n\tfor ;\n\tSun, 26 Apr 2020 19:00:20 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1CE1F4F7;\n\tSun, 26 Apr 2020 19:00:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"E8SF3/p7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587920420;\n\tbh=lank11gQjSQCRC+T+/Ws8j1rtP+KtKZKt2rBQ/r19MU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E8SF3/p7ksK5Wpbmp9vBwt1NhlEPCarWvay46T9WUa77loy+xHiGI3kY3Ozdv8o9y\n\tf7WL0bq2dw7GEuyFpfSBw44IlnktSN1P5VuSrjmR7zf5ys/v3ob7j1xDzJmufrqqDQ\n\tg+ZJQn4qT1JJSVZoflPdu5VblxA24s3/1/8aL+dI=","Date":"Sun, 26 Apr 2020 20:00:05 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426170005.GC5950@pendragon.ideasonboard.com>","References":"<20200425004533.26907-1-laurent.pinchart@ideasonboard.com>\n\t<20200425004533.26907-2-laurent.pinchart@ideasonboard.com>\n\t<20200426125057.kcpft54tiv73can5@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200426125057.kcpft54tiv73can5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 1/7] libcamera: pipeline: uvcvideo:\n\tRefactor control handling","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":"Sun, 26 Apr 2020 17:00:20 -0000"}}]