[{"id":2680,"web_url":"https://patchwork.libcamera.org/comment/2680/","msgid":"<4d3b147b-8e3b-0d1d-d6ff-51d602909049@ideasonboard.com>","date":"2019-09-20T11:32:32","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: controls: De-couple\n\tControls from Camera","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 18/09/2019 11:34, Jacopo Mondi wrote:\n> ControlList requires a Camera class instance at construction time,\n> preventing it from being re-constructed from serialized binary data.\n> \n> De-couple ControlList from Camera by internally storing a reference to\n> the Camera's ControlInfoList.\n\nInteresting, I guess this allows us to just serialise the\nControlInfoList, and not the whole camera.\n\n\nNo major concern, and only really some discussion points below.\n\nI'm not fond of std::pair type obfuscation; the \".first .second\" inline\nsyntax is far too easy to misinterpret (at least for me).\n\nBut I'll not object to that. I don't think we have any coding style\nruling on it.\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  include/libcamera/controls.h   |  4 +--\n>  src/libcamera/controls.cpp     | 54 +++++++++++++++++-----------------\n>  src/libcamera/request.cpp      |  4 +--\n>  test/controls/control_list.cpp |  4 +--\n>  4 files changed, 33 insertions(+), 33 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index e46cd8a78679..d3065c0f3f16 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -48,7 +48,7 @@ private:\n>  \tusing ControlListMap = std::unordered_map<const ControlInfo *, DataValue>;\n> \n>  public:\n> -\tControlList(Camera *camera);\n> +\tControlList(const ControlInfoMap &infoMap);\n> \n>  \tusing iterator = ControlListMap::iterator;\n>  \tusing const_iterator = ControlListMap::const_iterator;\n> @@ -70,7 +70,7 @@ public:\n>  \tvoid update(const ControlList &list);\n> \n>  private:\n> -\tCamera *camera_;\n> +\tconst ControlInfoMap &infoMap_;\n>  \tControlListMap controls_;\n>  };\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 2d8adde5688e..184d544c05bc 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -175,10 +175,10 @@ std::string ControlInfo::toString() const\n> \n>  /**\n>   * \\brief Construct a ControlList with a reference to the Camera it applies on\n> - * \\param[in] camera The camera\n> + * \\param[in] infoMap The ControlInfoMap of the camera the control list refers to\n>   */\n> -ControlList::ControlList(Camera *camera)\n> -\t: camera_(camera)\n> +ControlList::ControlList(const ControlInfoMap &infoMap)\n> +\t: infoMap_(infoMap)\n>  {\n>  }\n> \n> @@ -229,17 +229,14 @@ ControlList::ControlList(Camera *camera)\n>   */\n>  bool ControlList::contains(ControlId id) const\n>  {\n> -\tconst ControlInfoMap &controls = camera_->controls();\n> -\tconst auto iter = controls.find(id);\n> -\tif (iter == controls.end()) {\n> -\t\tLOG(Controls, Error)\n> -\t\t\t<< \"Camera \" << camera_->name()\n> -\t\t\t<< \" does not support control \" << id;\n> +\tconst auto info = infoMap_.find(id);\n> +\tif (info == infoMap_.end()) {\n> +\t\tLOG(Controls, Error) << \"Control \" << id << \" not supported\";\n> \n>  \t\treturn false;\n>  \t}\n> \n> -\treturn controls_.find(&iter->second) != controls_.end();\n> +\treturn controls_.find(&info->second) != controls_.end();\n>  }\n> \n>  /**\n> @@ -283,18 +280,15 @@ bool ControlList::contains(const ControlInfo *info) const\n>   */\n>  DataValue &ControlList::operator[](ControlId id)\n>  {\n> -\tconst ControlInfoMap &controls = camera_->controls();\n> -\tconst auto iter = controls.find(id);\n> -\tif (iter == controls.end()) {\n> -\t\tLOG(Controls, Error)\n> -\t\t\t<< \"Camera \" << camera_->name()\n> -\t\t\t<< \" does not support control \" << id;\n> +\tconst auto info = infoMap_.find(id);\n> +\tif (info == infoMap_.end()) {\n> +\t\tLOG(Controls, Error) << \"Control \" << id << \" not supported\";\n> \n>  \t\tstatic DataValue empty;\n>  \t\treturn empty;\n>  \t}\n> \n> -\treturn controls_[&iter->second];\n> +\treturn controls_[&info->second];\n>  }\n> \n>  /**\n> @@ -322,18 +316,24 @@ DataValue &ControlList::operator[](ControlId id)\n>   */\n>  void ControlList::update(const ControlList &other)\n>  {\n> -\tif (other.camera_ != camera_) {\n> -\t\tLOG(Controls, Error)\n> -\t\t\t<< \"Can't update ControlList from a different camera\";\n> -\t\treturn;\n> +\t/*\n> +\t * Make sure if all controls in other's list are supported by this\n\n\"Make sure that all controls in the other list are \"\n\n> +\t * Camera. This is guaranteed to be true if the two lists refer to\n> +\t * the same Camera.\n> +\t */\n> +\tfor (auto &control : other) {\n> +\t\tControlId id = control.first->id();\n> +\n> +\t\tif (infoMap_.find(id) == infoMap_.end()) {\n> +\t\t\tLOG(Controls, Error)\n> +\t\t\t\t<< \"Failed to update control list with control: \"\n> +\t\t\t\t<< id;\n> +\t\t\treturn;\n> +\t\t}\n\nWe get a performance penalty here, as now we must search the list,\nrather than make sure the camera pointer is identical.\n\nWould there be any value in storing an opaque handle for the camera\n(essentially just the pointer, which could be serialised easily) to\noptimise this out again?\n\nI don't (yet) expect this to be a hot-path - so we could look at this later.\n\nBut if you think it might be worth considering, could you add a todo: to\ndocument the possibility?\n\n\n\n>  \t}\n> \n> -\tfor (auto it : other) {\n> -\t\tconst ControlInfo *info = it.first;\n> -\t\tconst DataValue &value = it.second;\n> -\n> -\t\tcontrols_[info] = value;\n> -\t}\n> +\tfor (auto &control : other)\n> +\t\tcontrols_[control.first] = control.second;\n\nUgh, I really do dislike std::pair obfuscation through .first/.second.\n\nI hope one day we can move to C++17, and have the structured bindings,\nbut until then I'm going to close my eyes.\n\nWould it be overkill to specify the pair types explicitly?\nNo requirement I don't think though.\n\n>  }\n> \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index ee2158fc7a9c..2b3e1870094e 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -55,8 +55,8 @@ LOG_DEFINE_CATEGORY(Request)\n>   *\n>   */\n>  Request::Request(Camera *camera, uint64_t cookie)\n> -\t: camera_(camera), controls_(camera), cookie_(cookie),\n> -\t  status_(RequestPending), cancelled_(false)\n> +\t: camera_(camera), controls_(camera->controls()),\n> +\t  cookie_(cookie), status_(RequestPending), cancelled_(false)\n>  {\n>  }\n> \n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index f1d79ff8fcfd..3c6eb40c091b 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -39,7 +39,7 @@ protected:\n> \n>  \tint run()\n>  \t{\n> -\t\tControlList list(camera_.get());\n> +\t\tControlList list(camera_->controls());\n> \n>  \t\t/* Test that the list is initially empty. */\n>  \t\tif (!list.empty()) {\n> @@ -155,7 +155,7 @@ protected:\n>  \t\t * the old list. Verify that the new list is empty and that the\n>  \t\t * new list contains the expected items and values.\n>  \t\t */\n> -\t\tControlList newList(camera_.get());\n> +\t\tControlList newList(camera_->controls());\n> \n>  \t\tnewList[Brightness] = 128;\n>  \t\tnewList[Saturation] = 255;\n> --\n> 2.23.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","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 2D57260BED\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Sep 2019 13:32:36 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 896504FE;\n\tFri, 20 Sep 2019 13:32:35 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568979155;\n\tbh=mPcx4H7UAfw0p0Y+cEv7MHtPQCGHu4qWg4gj5ZXezOA=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=oAszfBPxZxqs8q+vCJq9/7PV3xv2xPwdfIb/eyy5pG5oVfYxW2yh2WL/MHHvkFgaZ\n\tuuLph+RPkLQxhtMgMlah0+qtDTQLcw9YO7+4V5H5fLcVis38fdwCQEUCkao6nOELcg\n\ti1ZrFvk9rww38wpF9TWItfoi6XDSTfNz/Am11BQM=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","References":"<20190918103424.14536-1-jacopo@jmondi.org>\n\t<20190918103424.14536-5-jacopo@jmondi.org>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","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+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCJQQYAQoADwIbDAUCWcOUawUJ\n\tB4D+AgAKCRChHkZyEKRh/XJhEACr5iidt/0MZ0rWRMCbZFMWD7D2g6nZeOp+F2zY8CEUW+sd\n\tCDVd9BH9QX9KN5SZo6YtJzMzSzpcx45VwTvtQW0n/6Eujg9EUqblfU9xqvqDmbjEapr5d/OL\n\t21GTALb0owKhA5qDUGEcKGCphpQffKhTNo/BP99jvmJUj7IPSKH97qPypi8/ym8bAxB+uY31\n\tgHTMHf1jMJJ1pRo2tYYPeIIHGDqXBI4sp5GHHF+JcIhgR/e/A6w/dgzHYmQPl2ix5eZYEZbV\n\tTRP+gkX4NV8oHqa/lR+xPOlWElGB57viOSOoWriqxQbFy8XbG1GR8cWlkNtGBGVWaJaSoORP\n\tiowD7irXL91bCyFIqL+7BVk3Jy4uzP744PzE80KwxOp5SQAp9sPzFbgsJrLev90PZySjFHG0\n\twP144DK7nBjOj/J0g9OHVASP1JjK+nw7SDoKnETDIdRC0XmiHXk7TXzPdkvO0UkpHdEPjZUp\n\tWyuc0MqehjR/hTTPt4m/Y14XzEcy6JREIjOrFfUZVho2QpOdv9CNryGdieRTNjUtz463CIaZ\n\tdPBiw9mOMBoNffkn9FIoCjLnAaj9gUAnEHWBZOEviQ5NuyqpeP0YtzI4iaRbSUkYZHej99X3\n\tVmHrdLlMqd/ZgYYbPGSL4AN3FVACb5CxuxEHwo029VcE5U3CSjzqtCoX12tm7A==","Organization":"Ideas on Board","Message-ID":"<4d3b147b-8e3b-0d1d-d6ff-51d602909049@ideasonboard.com>","Date":"Fri, 20 Sep 2019 12:32:32 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190918103424.14536-5-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: controls: De-couple\n\tControls from Camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 20 Sep 2019 11:32:36 -0000"}},{"id":2685,"web_url":"https://patchwork.libcamera.org/comment/2685/","msgid":"<20190923111240.3wwqqm7oa24kfohd@uno.localdomain>","date":"2019-09-23T11:12:40","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: controls: De-couple\n\tControls from Camera","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Fri, Sep 20, 2019 at 12:32:32PM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 18/09/2019 11:34, Jacopo Mondi wrote:\n> > ControlList requires a Camera class instance at construction time,\n> > preventing it from being re-constructed from serialized binary data.\n> >\n> > De-couple ControlList from Camera by internally storing a reference to\n> > the Camera's ControlInfoList.\n>\n> Interesting, I guess this allows us to just serialise the\n> ControlInfoList, and not the whole camera.\n>\n\nYes, serializing the whole camera means transporting a ton of data,\npointers and complext stuff.\n\nWe'll serialize the ControlInfoMap and the ControlList, to be\nre-constructed by the IPA modules separately.\n\n>\n> No major concern, and only really some discussion points below.\n>\n> I'm not fond of std::pair type obfuscation; the \".first .second\" inline\n> syntax is far too easy to misinterpret (at least for me).\n\nI'm not a fan either, but, well, C++\n\n>\n> But I'll not object to that. I don't think we have any coding style\n> ruling on it.\n>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n>\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  include/libcamera/controls.h   |  4 +--\n> >  src/libcamera/controls.cpp     | 54 +++++++++++++++++-----------------\n> >  src/libcamera/request.cpp      |  4 +--\n> >  test/controls/control_list.cpp |  4 +--\n> >  4 files changed, 33 insertions(+), 33 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index e46cd8a78679..d3065c0f3f16 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -48,7 +48,7 @@ private:\n> >  \tusing ControlListMap = std::unordered_map<const ControlInfo *, DataValue>;\n> >\n> >  public:\n> > -\tControlList(Camera *camera);\n> > +\tControlList(const ControlInfoMap &infoMap);\n> >\n> >  \tusing iterator = ControlListMap::iterator;\n> >  \tusing const_iterator = ControlListMap::const_iterator;\n> > @@ -70,7 +70,7 @@ public:\n> >  \tvoid update(const ControlList &list);\n> >\n> >  private:\n> > -\tCamera *camera_;\n> > +\tconst ControlInfoMap &infoMap_;\n> >  \tControlListMap controls_;\n> >  };\n> >\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 2d8adde5688e..184d544c05bc 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -175,10 +175,10 @@ std::string ControlInfo::toString() const\n> >\n> >  /**\n> >   * \\brief Construct a ControlList with a reference to the Camera it applies on\n> > - * \\param[in] camera The camera\n> > + * \\param[in] infoMap The ControlInfoMap of the camera the control list refers to\n> >   */\n> > -ControlList::ControlList(Camera *camera)\n> > -\t: camera_(camera)\n> > +ControlList::ControlList(const ControlInfoMap &infoMap)\n> > +\t: infoMap_(infoMap)\n> >  {\n> >  }\n> >\n> > @@ -229,17 +229,14 @@ ControlList::ControlList(Camera *camera)\n> >   */\n> >  bool ControlList::contains(ControlId id) const\n> >  {\n> > -\tconst ControlInfoMap &controls = camera_->controls();\n> > -\tconst auto iter = controls.find(id);\n> > -\tif (iter == controls.end()) {\n> > -\t\tLOG(Controls, Error)\n> > -\t\t\t<< \"Camera \" << camera_->name()\n> > -\t\t\t<< \" does not support control \" << id;\n> > +\tconst auto info = infoMap_.find(id);\n> > +\tif (info == infoMap_.end()) {\n> > +\t\tLOG(Controls, Error) << \"Control \" << id << \" not supported\";\n> >\n> >  \t\treturn false;\n> >  \t}\n> >\n> > -\treturn controls_.find(&iter->second) != controls_.end();\n> > +\treturn controls_.find(&info->second) != controls_.end();\n> >  }\n> >\n> >  /**\n> > @@ -283,18 +280,15 @@ bool ControlList::contains(const ControlInfo *info) const\n> >   */\n> >  DataValue &ControlList::operator[](ControlId id)\n> >  {\n> > -\tconst ControlInfoMap &controls = camera_->controls();\n> > -\tconst auto iter = controls.find(id);\n> > -\tif (iter == controls.end()) {\n> > -\t\tLOG(Controls, Error)\n> > -\t\t\t<< \"Camera \" << camera_->name()\n> > -\t\t\t<< \" does not support control \" << id;\n> > +\tconst auto info = infoMap_.find(id);\n> > +\tif (info == infoMap_.end()) {\n> > +\t\tLOG(Controls, Error) << \"Control \" << id << \" not supported\";\n> >\n> >  \t\tstatic DataValue empty;\n> >  \t\treturn empty;\n> >  \t}\n> >\n> > -\treturn controls_[&iter->second];\n> > +\treturn controls_[&info->second];\n> >  }\n> >\n> >  /**\n> > @@ -322,18 +316,24 @@ DataValue &ControlList::operator[](ControlId id)\n> >   */\n> >  void ControlList::update(const ControlList &other)\n> >  {\n> > -\tif (other.camera_ != camera_) {\n> > -\t\tLOG(Controls, Error)\n> > -\t\t\t<< \"Can't update ControlList from a different camera\";\n> > -\t\treturn;\n> > +\t/*\n> > +\t * Make sure if all controls in other's list are supported by this\n>\n> \"Make sure that all controls in the other list are \"\n>\n> > +\t * Camera. This is guaranteed to be true if the two lists refer to\n> > +\t * the same Camera.\n> > +\t */\n> > +\tfor (auto &control : other) {\n> > +\t\tControlId id = control.first->id();\n> > +\n> > +\t\tif (infoMap_.find(id) == infoMap_.end()) {\n> > +\t\t\tLOG(Controls, Error)\n> > +\t\t\t\t<< \"Failed to update control list with control: \"\n> > +\t\t\t\t<< id;\n> > +\t\t\treturn;\n> > +\t\t}\n>\n> We get a performance penalty here, as now we must search the list,\n> rather than make sure the camera pointer is identical.\n\nThis will really only happens when merging two control lists...\n\n>\n> Would there be any value in storing an opaque handle for the camera\n> (essentially just the pointer, which could be serialised easily) to\n> optimise this out again?\n\nAs we serialize raw values and info, without any class specific\nheader, adding one value for a field would require specializing the\nserialization format for this class. Not a huge deal, but I would\nrather pay this small penalty\n\n>\n> I don't (yet) expect this to be a hot-path - so we could look at this later.\n>\n> But if you think it might be worth considering, could you add a todo: to\n> document the possibility?\n>\n>\n>\n> >  \t}\n> >\n> > -\tfor (auto it : other) {\n> > -\t\tconst ControlInfo *info = it.first;\n> > -\t\tconst DataValue &value = it.second;\n> > -\n> > -\t\tcontrols_[info] = value;\n> > -\t}\n> > +\tfor (auto &control : other)\n> > +\t\tcontrols_[control.first] = control.second;\n>\n> Ugh, I really do dislike std::pair obfuscation through .first/.second.\n>\n> I hope one day we can move to C++17, and have the structured bindings,\n> but until then I'm going to close my eyes.\n>\n> Would it be overkill to specify the pair types explicitly?\n\nYes :)\nI tried, it's quite long, but yes, first/second is obfuscating.\n\n> No requirement I don't think though.\n>\n> >  }\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index ee2158fc7a9c..2b3e1870094e 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -55,8 +55,8 @@ LOG_DEFINE_CATEGORY(Request)\n> >   *\n> >   */\n> >  Request::Request(Camera *camera, uint64_t cookie)\n> > -\t: camera_(camera), controls_(camera), cookie_(cookie),\n> > -\t  status_(RequestPending), cancelled_(false)\n> > +\t: camera_(camera), controls_(camera->controls()),\n> > +\t  cookie_(cookie), status_(RequestPending), cancelled_(false)\n> >  {\n> >  }\n> >\n> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > index f1d79ff8fcfd..3c6eb40c091b 100644\n> > --- a/test/controls/control_list.cpp\n> > +++ b/test/controls/control_list.cpp\n> > @@ -39,7 +39,7 @@ protected:\n> >\n> >  \tint run()\n> >  \t{\n> > -\t\tControlList list(camera_.get());\n> > +\t\tControlList list(camera_->controls());\n> >\n> >  \t\t/* Test that the list is initially empty. */\n> >  \t\tif (!list.empty()) {\n> > @@ -155,7 +155,7 @@ protected:\n> >  \t\t * the old list. Verify that the new list is empty and that the\n> >  \t\t * new list contains the expected items and values.\n> >  \t\t */\n> > -\t\tControlList newList(camera_.get());\n> > +\t\tControlList newList(camera_->controls());\n> >\n> >  \t\tnewList[Brightness] = 128;\n> >  \t\tnewList[Saturation] = 255;\n> > --\n> > 2.23.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B33AF60BCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Sep 2019 13:11:01 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 1213D4000F;\n\tMon, 23 Sep 2019 11:11:00 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 23 Sep 2019 13:12:40 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190923111240.3wwqqm7oa24kfohd@uno.localdomain>","References":"<20190918103424.14536-1-jacopo@jmondi.org>\n\t<20190918103424.14536-5-jacopo@jmondi.org>\n\t<4d3b147b-8e3b-0d1d-d6ff-51d602909049@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"leroe2g5cg2anh5j\"","Content-Disposition":"inline","In-Reply-To":"<4d3b147b-8e3b-0d1d-d6ff-51d602909049@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: controls: De-couple\n\tControls from Camera","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 23 Sep 2019 11:11:01 -0000"}}]