[{"id":2904,"web_url":"https://patchwork.libcamera.org/comment/2904/","msgid":"<20191015004246.GJ5976@bigcity.dyn.berto.se>","date":"2019-10-15T00:42:46","subject":"Re: [libcamera-devel] [PATCH 10/10] libcamera: v4l2_controls:\n\tRemove V4L2ControlList class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-10-14 02:33:14 +0300, Laurent Pinchart wrote:\n> The V4L2ControlList class only provides a convenience constructor for\n> the ControlList, which can easily be moved to the ControlList class and\n> may benefit it later (to construct a ControlList from controls supported\n> by a camera). Move the constructor and remove V4L2ControlList.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> include/libcamera/controls.h | 1 +\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/controls.cpp | 10 ++++++++++\n> src/libcamera/include/v4l2_controls.h | 14 --------------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> src/libcamera/v4l2_controls.cpp | 20 --------------------\n> test/v4l2_videodevice/controls.cpp | 2 +-\n> 9 files changed, 16 insertions(+), 39 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 80414c6f0748..0d279d508dcc 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -170,6 +170,7 @@ private:\n> \n> public:\n> \tControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n> +\tControlList(const ControlInfoMap &info, ControlValidator *validator = nullptr);\n> \n> \tusing iterator = ControlListMap::iterator;\n> \tusing const_iterator = ControlListMap::const_iterator;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index bd703898c523..570145ce71b2 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -212,7 +212,7 @@ void IPARkISP1::setControls(unsigned int frame)\n> \tIPAOperationData op;\n> \top.operation = RKISP1_IPA_ACTION_V4L2_SET;\n> \n> -\tV4L2ControlList ctrls(ctrls_);\n> +\tControlList ctrls(ctrls_);\n> \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> \top.controls.push_back(ctrls);\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index bf7634aab470..93ad2fc6a276 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -561,6 +561,16 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> {\n> }\n> \n> +/**\n> + * \\brief Construct a ControlList with the idmap of a control info map\n> + * \\param[in] info The ControlInfoMap for the control list target object\n> + * \\param[in] validator The validator (may be null)\n> + */\n> +ControlList::ControlList(const ControlInfoMap &info, ControlValidator *validator)\n> +\t: validator_(validator), idmap_(&info.idmap())\n> +{\n> +}\n> +\n> /**\n> * \\typedef ControlList::iterator\n> * \\brief Iterator for the controls contained within the list\n> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> index e16c4957f9d1..882546a89340 100644\n> --- a/src/libcamera/include/v4l2_controls.h\n> +++ b/src/libcamera/include/v4l2_controls.h\n> @@ -8,11 +8,6 @@\n> #ifndef __LIBCAMERA_V4L2_CONTROLS_H__\n> #define __LIBCAMERA_V4L2_CONTROLS_H__\n> \n> -#include \n> -#include \n> -#include \n> -#include \n> -\n> #include \n> \n> #include \n> @@ -31,15 +26,6 @@ public:\n> \tV4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl);\n> };\n> \n> -class V4L2ControlList : public ControlList\n> -{\n> -public:\n> -\tV4L2ControlList(const ControlInfoMap &info)\n> -\t\t: ControlList(info.idmap())\n> -\t{\n> -\t}\n> -};\n> -\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 9776b36b88cd..8d3ad568d16e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -604,7 +604,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn ret;\n> \n> \t/* Apply the \"pipe_mode\" control to the ImgU subdevice. */\n> -\tV4L2ControlList ctrls(imgu->imgu_->controls());\n> +\tControlList ctrls(imgu->imgu_->controls());\n> \tctrls.set(V4L2_CID_IPU3_PIPE_MODE,\n> \t\t static_cast(vfStream->active_ ? IPU3PipeModeVideo :\n> \t\t\t\t IPU3PipeModeStillCapture));\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index a64e1af4c550..fae0ffc4de30 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -228,7 +228,7 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> \n> int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> {\n> -\tV4L2ControlList controls(data->video_->controls());\n> +\tControlList controls(data->video_->controls());\n> \n> \tfor (auto it : request->controls()) {\n> \t\tconst ControlId &id = *it.first;\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 6a4244119dc5..dcdaef120ad1 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -279,7 +279,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n> \n> int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> {\n> -\tV4L2ControlList controls(data->sensor_->controls());\n> +\tControlList controls(data->sensor_->controls());\n> \n> \tfor (auto it : request->controls()) {\n> \t\tconst ControlId &id = *it.first;\n> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> index bd5e18590b76..83ac7b0dc666 100644\n> --- a/src/libcamera/v4l2_controls.cpp\n> +++ b/src/libcamera/v4l2_controls.cpp\n> @@ -134,24 +134,4 @@ V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)\n> \t\t\t\t\t\t static_cast(ctrl.maximum)));\n> }\n> \n> -/**\n> - * \\class V4L2ControlList\n> - * \\brief A list of controls for a V4L2 device\n> - *\n> - * This class specialises the ControList class for use with V4L2 devices. It\n> - * offers a convenience API to create a ControlList from a ControlInfoMap.\n> - *\n> - * V4L2ControlList allows easy construction of a ControlList containing V4L2\n> - * controls for a device. It can be used to construct the list of controls\n> - * passed to the V4L2Device::getControls() and V4L2Device::setControls()\n> - * methods. The class should however not be used in place of ControlList in\n> - * APIs.\n> - */\n> -\n> -/**\n> - * \\fn V4L2ControlList::V4L2ControlList(const ControlInfoMap &info)\n> - * \\brief Construct a V4L2ControlList associated with a V4L2 device\n> - * \\param[in] info The V4L2 device control info map\n> - */\n> -\n> } /* namespace libcamera */\n> diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp\n> index 31879b794ed0..975c852b8cbf 100644\n> --- a/test/v4l2_videodevice/controls.cpp\n> +++ b/test/v4l2_videodevice/controls.cpp\n> @@ -46,7 +46,7 @@ protected:\n> \t\tconst ControlRange &saturation = info.find(V4L2_CID_SATURATION)->second;\n> \n> \t\t/* Test getting controls. */\n> -\t\tV4L2ControlList ctrls(info);\n> +\t\tControlList ctrls(info);\n> \t\tctrls.set(V4L2_CID_BRIGHTNESS, -1);\n> \t\tctrls.set(V4L2_CID_CONTRAST, -1);\n> \t\tctrls.set(V4L2_CID_SATURATION, -1);\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 mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D329600F9\n\tfor ;\n\tTue, 15 Oct 2019 02:42:48 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id q64so18337057ljb.12\n\tfor ;\n\tMon, 14 Oct 2019 17:42:48 -0700 (PDT)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tj7sm4612927lfc.16.2019.10.14.17.42.47\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Oct 2019 17:42:47 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=QXgSZeM/lX5jwiT1YvnHtBdjbA5Rx2LIcnZm7Oau0/A=;\n\tb=BXS4ItNMaJt6T5SVDywJz1W3ODqOUk0vVgWFS9cMXn7wx0Upf0V87MqZpA4s8xHCeX\n\tpUpjS57fE22vqRVsHgPm0DmslCRYjAT9SDPqvvGQLDcnUegMtDNzBdROFdL9tDnYXsKX\n\tmG6u+Sr370Q3RWSIfcLhH6nhvGyQg5h3e8oKNDi1Lxs0nB5Q9Zo0PCIU+FGF/1xTcVDl\n\twZOrTRAm8e9F0br6oX15V9pI5h93FpJTHOtW68TFIXf2qBagbmv5DhqaZIQQWP9tJlt8\n\te6JG2IHJCQEhbWYLM1kKb0Fve89GeXFM6eznJzHduOMCKIWUl4dF+QuDPGgpKDY7AVI6\n\tJTlg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=QXgSZeM/lX5jwiT1YvnHtBdjbA5Rx2LIcnZm7Oau0/A=;\n\tb=TzreyeSmQymMVs2OJWg66MNcfYHSKheQPah51OASqbqihPwKnYWDct/XZh6aYZJDae\n\t3b8Q9tuemtnc/GEOEwHQBUdDfoyUrsy/X5azFyh9e0uxRLBuytseziYOC3QaTw8QQ2dx\n\tC2F1wMGyTxtIzZXOrBxTfpw6jGgDcSPmgzbUBPya8xAPzSHYfNgYfJTFKPKErE0srefS\n\tbbI8a5sLfUr7SWGvXKcdLy8UUkXq31vF6pGvc8F0dLk3T6SkzBpMtVz/VHAKtvTBn4QX\n\teD79011QjgjNrikPXzMWu7dmloqzTGFKp/nhfvdxZYSHLCZnOkubJixBBNEXijuUPCGG\n\tmE4g==","X-Gm-Message-State":"APjAAAXhLjYXV4tKChDSEhvYgIFUj3DeHV02/Z5PZs2bCAYFAxiL0VAk\n\tXEeBSymlrswD3HSW7pdhTfQx0f7w/DE=","X-Google-Smtp-Source":"APXvYqyPMFfEADtyhryWCI8t/XY1Q2K/tJCoyyC4Ah6tvQEcNoe6bPvxzDP5UJNAUa02vUC5q9IvBg==","X-Received":"by 2002:a2e:8654:: with SMTP id\n\ti20mr20611919ljj.238.1571100167929; \n\tMon, 14 Oct 2019 17:42:47 -0700 (PDT)","Date":"Tue, 15 Oct 2019 02:42:46 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191015004246.GJ5976@bigcity.dyn.berto.se>","References":"<20191013232755.3292-1-laurent.pinchart@ideasonboard.com>\n\t<20191013233314.4434-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191013233314.4434-1-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH 10/10] libcamera: v4l2_controls:\n\tRemove V4L2ControlList class","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":"Tue, 15 Oct 2019 00:42:48 -0000"}},{"id":2924,"web_url":"https://patchwork.libcamera.org/comment/2924/","msgid":"<20191015185235.ui4am7xbb7squbuv@uno.localdomain>","date":"2019-10-15T18:52:35","subject":"Re: [libcamera-devel] [PATCH 10/10] libcamera: v4l2_controls:\n\tRemove V4L2ControlList class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Oct 14, 2019 at 02:33:14AM +0300, Laurent Pinchart wrote:\n> The V4L2ControlList class only provides a convenience constructor for\n> the ControlList, which can easily be moved to the ControlList class and\n> may benefit it later (to construct a ControlList from controls supported\n> by a camera). Move the constructor and remove V4L2ControlList.\n\nAh scratch my previous comment then :)\nI think we should make this the only available constructor\n\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/controls.h | 1 +\n> src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> src/libcamera/controls.cpp | 10 ++++++++++\n> src/libcamera/include/v4l2_controls.h | 14 --------------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> src/libcamera/v4l2_controls.cpp | 20 --------------------\n> test/v4l2_videodevice/controls.cpp | 2 +-\n> 9 files changed, 16 insertions(+), 39 deletions(-)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 80414c6f0748..0d279d508dcc 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -170,6 +170,7 @@ private:\n>\n> public:\n> \tControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n> +\tControlList(const ControlInfoMap &info, ControlValidator *validator = nullptr);\n>\n> \tusing iterator = ControlListMap::iterator;\n> \tusing const_iterator = ControlListMap::const_iterator;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index bd703898c523..570145ce71b2 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -212,7 +212,7 @@ void IPARkISP1::setControls(unsigned int frame)\n> \tIPAOperationData op;\n> \top.operation = RKISP1_IPA_ACTION_V4L2_SET;\n>\n> -\tV4L2ControlList ctrls(ctrls_);\n> +\tControlList ctrls(ctrls_);\n> \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> \top.controls.push_back(ctrls);\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index bf7634aab470..93ad2fc6a276 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -561,6 +561,16 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> {\n> }\n>\n> +/**\n> + * \\brief Construct a ControlList with the idmap of a control info map\n> + * \\param[in] info The ControlInfoMap for the control list target object\n> + * \\param[in] validator The validator (may be null)\n> + */\n> +ControlList::ControlList(const ControlInfoMap &info, ControlValidator *validator)\n> +\t: validator_(validator), idmap_(&info.idmap())\n\nMaybe this is a bit to much to ask, but the different order of\nparameters and field declaration bothers me a bit.\n\nAnyway, this as the whole series look good and I hope we could merge\nit soon\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +{\n> +}\n> +\n> /**\n> * \\typedef ControlList::iterator\n> * \\brief Iterator for the controls contained within the list\n> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> index e16c4957f9d1..882546a89340 100644\n> --- a/src/libcamera/include/v4l2_controls.h\n> +++ b/src/libcamera/include/v4l2_controls.h\n> @@ -8,11 +8,6 @@\n> #ifndef __LIBCAMERA_V4L2_CONTROLS_H__\n> #define __LIBCAMERA_V4L2_CONTROLS_H__\n>\n> -#include \n> -#include \n> -#include \n> -#include \n> -\n> #include \n>\n> #include \n> @@ -31,15 +26,6 @@ public:\n> \tV4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl);\n> };\n>\n> -class V4L2ControlList : public ControlList\n> -{\n> -public:\n> -\tV4L2ControlList(const ControlInfoMap &info)\n> -\t\t: ControlList(info.idmap())\n> -\t{\n> -\t}\n> -};\n> -\n> } /* namespace libcamera */\n>\n> #endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 9776b36b88cd..8d3ad568d16e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -604,7 +604,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn ret;\n>\n> \t/* Apply the \"pipe_mode\" control to the ImgU subdevice. */\n> -\tV4L2ControlList ctrls(imgu->imgu_->controls());\n> +\tControlList ctrls(imgu->imgu_->controls());\n> \tctrls.set(V4L2_CID_IPU3_PIPE_MODE,\n> \t\t static_cast(vfStream->active_ ? IPU3PipeModeVideo :\n> \t\t\t\t IPU3PipeModeStillCapture));\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index a64e1af4c550..fae0ffc4de30 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -228,7 +228,7 @@ void PipelineHandlerUVC::stop(Camera *camera)\n>\n> int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> {\n> -\tV4L2ControlList controls(data->video_->controls());\n> +\tControlList controls(data->video_->controls());\n>\n> \tfor (auto it : request->controls()) {\n> \t\tconst ControlId &id = *it.first;\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 6a4244119dc5..dcdaef120ad1 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -279,7 +279,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n>\n> int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> {\n> -\tV4L2ControlList controls(data->sensor_->controls());\n> +\tControlList controls(data->sensor_->controls());\n>\n> \tfor (auto it : request->controls()) {\n> \t\tconst ControlId &id = *it.first;\n> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> index bd5e18590b76..83ac7b0dc666 100644\n> --- a/src/libcamera/v4l2_controls.cpp\n> +++ b/src/libcamera/v4l2_controls.cpp\n> @@ -134,24 +134,4 @@ V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)\n> \t\t\t\t\t\t static_cast(ctrl.maximum)));\n> }\n>\n> -/**\n> - * \\class V4L2ControlList\n> - * \\brief A list of controls for a V4L2 device\n> - *\n> - * This class specialises the ControList class for use with V4L2 devices. It\n> - * offers a convenience API to create a ControlList from a ControlInfoMap.\n> - *\n> - * V4L2ControlList allows easy construction of a ControlList containing V4L2\n> - * controls for a device. It can be used to construct the list of controls\n> - * passed to the V4L2Device::getControls() and V4L2Device::setControls()\n> - * methods. The class should however not be used in place of ControlList in\n> - * APIs.\n> - */\n> -\n> -/**\n> - * \\fn V4L2ControlList::V4L2ControlList(const ControlInfoMap &info)\n> - * \\brief Construct a V4L2ControlList associated with a V4L2 device\n> - * \\param[in] info The V4L2 device control info map\n> - */\n> -\n> } /* namespace libcamera */\n> diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp\n> index 31879b794ed0..975c852b8cbf 100644\n> --- a/test/v4l2_videodevice/controls.cpp\n> +++ b/test/v4l2_videodevice/controls.cpp\n> @@ -46,7 +46,7 @@ protected:\n> \t\tconst ControlRange &saturation = info.find(V4L2_CID_SATURATION)->second;\n>\n> \t\t/* Test getting controls. */\n> -\t\tV4L2ControlList ctrls(info);\n> +\t\tControlList ctrls(info);\n> \t\tctrls.set(V4L2_CID_BRIGHTNESS, -1);\n> \t\tctrls.set(V4L2_CID_CONTRAST, -1);\n> \t\tctrls.set(V4L2_CID_SATURATION, -1);\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 relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E22EE61562\n\tfor ;\n\tTue, 15 Oct 2019 20:50:47 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 82513240004;\n\tTue, 15 Oct 2019 18:50:47 +0000 (UTC)"],"Date":"Tue, 15 Oct 2019 20:52:35 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191015185235.ui4am7xbb7squbuv@uno.localdomain>","References":"<20191013232755.3292-1-laurent.pinchart@ideasonboard.com>\n\t<20191013233314.4434-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"xdqc6w2o52w67yzb\"","Content-Disposition":"inline","In-Reply-To":"<20191013233314.4434-1-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 10/10] libcamera: v4l2_controls:\n\tRemove V4L2ControlList class","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":"Tue, 15 Oct 2019 18:50:48 -0000"}},{"id":2928,"web_url":"https://patchwork.libcamera.org/comment/2928/","msgid":"<20191015194607.GB19403@pendragon.ideasonboard.com>","date":"2019-10-15T19:46:07","subject":"Re: [libcamera-devel] [PATCH 10/10] libcamera: v4l2_controls:\n\tRemove V4L2ControlList class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Oct 15, 2019 at 08:52:35PM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 14, 2019 at 02:33:14AM +0300, Laurent Pinchart wrote:\n> > The V4L2ControlList class only provides a convenience constructor for\n> > the ControlList, which can easily be moved to the ControlList class and\n> > may benefit it later (to construct a ControlList from controls supported\n> > by a camera). Move the constructor and remove V4L2ControlList.\n> \n> Ah scratch my previous comment then :)\n> I think we should make this the only available constructor\n\n:-) I'm fine experimenting with this.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/controls.h | 1 +\n> > src/ipa/rkisp1/rkisp1.cpp | 2 +-\n> > src/libcamera/controls.cpp | 10 ++++++++++\n> > src/libcamera/include/v4l2_controls.h | 14 --------------\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> > src/libcamera/pipeline/vimc.cpp | 2 +-\n> > src/libcamera/v4l2_controls.cpp | 20 --------------------\n> > test/v4l2_videodevice/controls.cpp | 2 +-\n> > 9 files changed, 16 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 80414c6f0748..0d279d508dcc 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -170,6 +170,7 @@ private:\n> >\n> > public:\n> > \tControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n> > +\tControlList(const ControlInfoMap &info, ControlValidator *validator = nullptr);\n> >\n> > \tusing iterator = ControlListMap::iterator;\n> > \tusing const_iterator = ControlListMap::const_iterator;\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index bd703898c523..570145ce71b2 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -212,7 +212,7 @@ void IPARkISP1::setControls(unsigned int frame)\n> > \tIPAOperationData op;\n> > \top.operation = RKISP1_IPA_ACTION_V4L2_SET;\n> >\n> > -\tV4L2ControlList ctrls(ctrls_);\n> > +\tControlList ctrls(ctrls_);\n> > \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > \tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > \top.controls.push_back(ctrls);\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index bf7634aab470..93ad2fc6a276 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -561,6 +561,16 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> > {\n> > }\n> >\n> > +/**\n> > + * \\brief Construct a ControlList with the idmap of a control info map\n> > + * \\param[in] info The ControlInfoMap for the control list target object\n> > + * \\param[in] validator The validator (may be null)\n> > + */\n> > +ControlList::ControlList(const ControlInfoMap &info, ControlValidator *validator)\n> > +\t: validator_(validator), idmap_(&info.idmap())\n> \n> Maybe this is a bit to much to ask, but the different order of\n> parameters and field declaration bothers me a bit.\n\ninfo has to be the first argument otherwise validator can't have a\ndefault value. We can reorder idmap_ and validator_, I'll ack a patch\nthat does so if you want to submit one.\n\n> Anyway, this as the whole series look good and I hope we could merge\n> it soon\n\nDone :-) Thank you for the review.\n\n> Reviewed-by: Jacopo Mondi \n> \n> > +{\n> > +}\n> > +\n> > /**\n> > * \\typedef ControlList::iterator\n> > * \\brief Iterator for the controls contained within the list\n> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > index e16c4957f9d1..882546a89340 100644\n> > --- a/src/libcamera/include/v4l2_controls.h\n> > +++ b/src/libcamera/include/v4l2_controls.h\n> > @@ -8,11 +8,6 @@\n> > #ifndef __LIBCAMERA_V4L2_CONTROLS_H__\n> > #define __LIBCAMERA_V4L2_CONTROLS_H__\n> >\n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -\n> > #include \n> >\n> > #include \n> > @@ -31,15 +26,6 @@ public:\n> > \tV4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl);\n> > };\n> >\n> > -class V4L2ControlList : public ControlList\n> > -{\n> > -public:\n> > -\tV4L2ControlList(const ControlInfoMap &info)\n> > -\t\t: ControlList(info.idmap())\n> > -\t{\n> > -\t}\n> > -};\n> > -\n> > } /* namespace libcamera */\n> >\n> > #endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 9776b36b88cd..8d3ad568d16e 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -604,7 +604,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \t\treturn ret;\n> >\n> > \t/* Apply the \"pipe_mode\" control to the ImgU subdevice. */\n> > -\tV4L2ControlList ctrls(imgu->imgu_->controls());\n> > +\tControlList ctrls(imgu->imgu_->controls());\n> > \tctrls.set(V4L2_CID_IPU3_PIPE_MODE,\n> > \t\t static_cast(vfStream->active_ ? IPU3PipeModeVideo :\n> > \t\t\t\t IPU3PipeModeStillCapture));\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index a64e1af4c550..fae0ffc4de30 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -228,7 +228,7 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> >\n> > int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > {\n> > -\tV4L2ControlList controls(data->video_->controls());\n> > +\tControlList controls(data->video_->controls());\n> >\n> > \tfor (auto it : request->controls()) {\n> > \t\tconst ControlId &id = *it.first;\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 6a4244119dc5..dcdaef120ad1 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -279,7 +279,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n> >\n> > int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> > {\n> > -\tV4L2ControlList controls(data->sensor_->controls());\n> > +\tControlList controls(data->sensor_->controls());\n> >\n> > \tfor (auto it : request->controls()) {\n> > \t\tconst ControlId &id = *it.first;\n> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > index bd5e18590b76..83ac7b0dc666 100644\n> > --- a/src/libcamera/v4l2_controls.cpp\n> > +++ b/src/libcamera/v4l2_controls.cpp\n> > @@ -134,24 +134,4 @@ V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)\n> > \t\t\t\t\t\t static_cast(ctrl.maximum)));\n> > }\n> >\n> > -/**\n> > - * \\class V4L2ControlList\n> > - * \\brief A list of controls for a V4L2 device\n> > - *\n> > - * This class specialises the ControList class for use with V4L2 devices. It\n> > - * offers a convenience API to create a ControlList from a ControlInfoMap.\n> > - *\n> > - * V4L2ControlList allows easy construction of a ControlList containing V4L2\n> > - * controls for a device. It can be used to construct the list of controls\n> > - * passed to the V4L2Device::getControls() and V4L2Device::setControls()\n> > - * methods. The class should however not be used in place of ControlList in\n> > - * APIs.\n> > - */\n> > -\n> > -/**\n> > - * \\fn V4L2ControlList::V4L2ControlList(const ControlInfoMap &info)\n> > - * \\brief Construct a V4L2ControlList associated with a V4L2 device\n> > - * \\param[in] info The V4L2 device control info map\n> > - */\n> > -\n> > } /* namespace libcamera */\n> > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp\n> > index 31879b794ed0..975c852b8cbf 100644\n> > --- a/test/v4l2_videodevice/controls.cpp\n> > +++ b/test/v4l2_videodevice/controls.cpp\n> > @@ -46,7 +46,7 @@ protected:\n> > \t\tconst ControlRange &saturation = info.find(V4L2_CID_SATURATION)->second;\n> >\n> > \t\t/* Test getting controls. */\n> > -\t\tV4L2ControlList ctrls(info);\n> > +\t\tControlList ctrls(info);\n> > \t\tctrls.set(V4L2_CID_BRIGHTNESS, -1);\n> > \t\tctrls.set(V4L2_CID_CONTRAST, -1);\n> > \t\tctrls.set(V4L2_CID_SATURATION, -1);","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19DBC60E1B\n\tfor ;\n\tTue, 15 Oct 2019 21:46:11 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84B0A324;\n\tTue, 15 Oct 2019 21:46:10 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1571168770;\n\tbh=abgcAfDi4GUgx/zUVXRgr1o4jdumhHR+nkG79CQS1kE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m0Itm2nnAFyoWw/m0oAjb2fpj3bAtgfXdZxoiO5dhdT+M1ZGTl8Gz+cQZgaqHVfF6\n\taj3ZaHoGhgE4u/3kBOU+TsQH9LZCws+SGDvHX74ucHt1B/fDeYRl0bUuUnEYzg4qE/\n\trBWvGPEJqQEt7nuRk2TvugcQVD3aKRCQEugds3tY=","Date":"Tue, 15 Oct 2019 22:46:07 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191015194607.GB19403@pendragon.ideasonboard.com>","References":"<20191013232755.3292-1-laurent.pinchart@ideasonboard.com>\n\t<20191013233314.4434-1-laurent.pinchart@ideasonboard.com>\n\t<20191015185235.ui4am7xbb7squbuv@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191015185235.ui4am7xbb7squbuv@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 10/10] libcamera: v4l2_controls:\n\tRemove V4L2ControlList class","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":"Tue, 15 Oct 2019 19:46:11 -0000"}}]