[{"id":23502,"web_url":"https://patchwork.libcamera.org/comment/23502/","msgid":"<165581421836.1149771.4927497988659868556@Monstersaurus>","date":"2022-06-21T12:23:38","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise\n\tScalerCrop from the pipeline handler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:33)\n> The ScalerCrop control is handled directly by the pipeline handler. Remove the\n> control from the IPA's static ControlInfoMap, and let the pipeline handler add\n> it to the ControlInfoMap advertised to the application, ensuring the limits\n> are set appropriately based on the current sensor mode.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 1 -\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++++-\n> 2 files changed, 9 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 295f6b735dc0..f46fccdd4177 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -86,7 +86,6 @@ static const ControlInfoMap::Map ipaControls{\n> { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> };\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 4596f2babcea..66a84b1dfb97 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -941,7 +941,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);\n> \n> /* Update the controls that the Raspberry Pi IPA can handle. */\n> - data->controlInfo_ = result.controlInfo;\n> + ControlInfoMap::Map ctrlMap;\n> + for (auto const &c : result.controlInfo)\n> + ctrlMap.emplace(c.first, c.second);\n> +\n> + /* Add the ScalerCrop control limits based on the current mode. */\n> + ctrlMap.emplace(&controls::ScalerCrop,\n> + ControlInfo(Rectangle(data->ispMinCropSize_), Rectangle(data->sensorInfo_.outputSize)));\n\nI don't think this works with emplace.\n\nReading: https://en.cppreference.com/w/cpp/container/map/emplace\n\n \"The element may be constructed even if there already is an element\n with the key in the container, in which case the newly constructed\n element will be destroyed immediately.\"\n\n\nSo taking their example code, and extending with an emplace after\nalready emplaced (which is the same as this ctrlMap being updated on any\nsecond or consecutive call to PipelineHandlerRPi::configure() ...\n\n$ cat snippets/map-emplace.cpp\n\n#include \n#include \n#include \n#include \n\nint main()\n{\n std::map m;\n\n // uses pair's move constructor\n m.emplace(std::make_pair(std::string(\"a\"), std::string(\"a\")));\n\n // uses pair's converting move constructor\n m.emplace(std::make_pair(\"b\", \"abcd\"));\n\n // uses pair's template constructor\n m.emplace(\"d\", \"ddd\");\n\n m.emplace(\"d\", \"a new ddd\");\n\n // uses pair's piecewise constructor\n m.emplace(std::piecewise_construct,\n std::forward_as_tuple(\"c\"),\n std::forward_as_tuple(10, 'c'));\n // as of C++17, m.try_emplace(\"c\", 10, 'c'); can be used\n\n for (const auto &p : m) {\n std::cout << p.first << \" => \" << p.second << '\\n';\n }\n}\n\n\nkbingham@Monstersaurus:~/iob/libcamera/libcamera$ make snippets/map-emplace\ng++ snippets/map-emplace.cpp -o snippets/map-emplace\nkbingham@Monstersaurus:~/iob/libcamera/libcamera$ ./snippets/map-emplace \na => a\nb => abcd\nc => cccccccccc\nd => ddd\n\n\nPerhaps we need a helper on the ControlInfoMap to support updates.\n\n\n> +\n> + data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> \n> /* Setup the Video Mux/Bridge entities. */\n> for (auto &[device, link] : data->bridgeDevices_) {\n> -- \n> 2.25.1\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 1B8BFBD808\n\tfor ;\n\tTue, 21 Jun 2022 12:23:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69F1565638;\n\tTue, 21 Jun 2022 14:23:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B5B061FB2\n\tfor ;\n\tTue, 21 Jun 2022 14:23:41 +0200 (CEST)","from pendragon.ideasonboard.com\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 2F458104;\n\tTue, 21 Jun 2022 14:23:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655814223;\n\tbh=YdyHbyVKyH66H4mt3XCF7ctTe8qiZk6XtLCZghEFSUc=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=rr+UzlzhMdvzWdRBQR8S0eKDyK8s+OhPgIHsAgvS08+FPn1CiHUov3oJ+fq60B+vU\n\tzh0nJ58sl3pUQIgRWVdWevCX05PbcMbASG/1sCnroy4URodpJ2YsNmcek7f6bN0KvK\n\tXgtkGISqlwqovlVG0WZBND4ke5t2M2Xn8oOVafEpsgU4I7UOEhhOZflFJwqCMRvMFs\n\tlGSOwso9zc8TtgSMwvdG/FpVcrp4K1/Tkf7QCJjoPDDM1xt8E6Djh2SrCNx805xWBA\n\tqFcg0dvw5Aj6502mmg4KjsN93UOaJyQ1DpyROc6b4r/CrZgZlRoCn7rdLXo8AX0y/c\n\tKYyfLA1hKgaDg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655814221;\n\tbh=YdyHbyVKyH66H4mt3XCF7ctTe8qiZk6XtLCZghEFSUc=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Xcmu2mIGfLGm8/pv82WilCC0FsDEzi1rgujRqn0Kx+n5msvrGm2kworAAhfn7FtRU\n\tEOUqyExCimcSqdnSK9hvKC0KzpnUC6r0tP8hSXhPZyssIlcpQ8ukub3b5izkkB89ex\n\tJYFfi7eHAqHGbvjMXcoGO/JufrkXsXN+qz263Fp0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Xcmu2mIG\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220610122533.11888-3-naush@raspberrypi.com>","References":"<20220610122533.11888-1-naush@raspberrypi.com>\n\t<20220610122533.11888-3-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 21 Jun 2022 13:23:38 +0100","Message-ID":"<165581421836.1149771.4927497988659868556@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise\n\tScalerCrop from the pipeline handler","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23505,"web_url":"https://patchwork.libcamera.org/comment/23505/","msgid":"","date":"2022-06-21T12:45:29","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise\n\tScalerCrop from the pipeline handler","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for your feedback.\n\nOn Tue, 21 Jun 2022 at 13:23, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:33)\n> > The ScalerCrop control is handled directly by the pipeline handler.\n> Remove the\n> > control from the IPA's static ControlInfoMap, and let the pipeline\n> handler add\n> > it to the ControlInfoMap advertised to the application, ensuring the\n> limits\n> > are set appropriately based on the current sensor mode.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 1 -\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++++-\n> > 2 files changed, 9 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 295f6b735dc0..f46fccdd4177 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -86,7 +86,6 @@ static const ControlInfoMap::Map ipaControls{\n> > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f)\n> },\n> > - { &controls::ScalerCrop, ControlInfo(Rectangle{},\n> Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > { &controls::draft::NoiseReductionMode,\n> ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > };\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 4596f2babcea..66a84b1dfb97 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -941,7 +941,15 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > data->properties_.set(properties::SensorSensitivity,\n> result.modeSensitivity);\n> >\n> > /* Update the controls that the Raspberry Pi IPA can handle. */\n> > - data->controlInfo_ = result.controlInfo;\n> > + ControlInfoMap::Map ctrlMap;\n> > + for (auto const &c : result.controlInfo)\n> > + ctrlMap.emplace(c.first, c.second);\n> > +\n> > + /* Add the ScalerCrop control limits based on the current mode.\n> */\n> > + ctrlMap.emplace(&controls::ScalerCrop,\n> > + ControlInfo(Rectangle(data->ispMinCropSize_),\n> Rectangle(data->sensorInfo_.outputSize)));\n>\n> I don't think this works with emplace.\n>\n> Reading: https://en.cppreference.com/w/cpp/container/map/emplace\n>\n> \"The element may be constructed even if there already is an element\n> with the key in the container, in which case the newly constructed\n> element will be destroyed immediately.\"\n>\n\nI see what you are getting at (I think), but the code is correct by an\nunfortunate\naccident. I cannot see a way to add a single element to an existing\nControlInfoMap\n(it is privately inherited from an unordered_map). So I have to to the\nfollowing\n(slightly hideous) sequence:\n\n1) Populate a ControlInfoMap::Map from the IPA's ControlInfo\n2) emplace the \"controls::ScalerCrop\" to this new ControlInfoMap::Map\n3) Convert the new ControlInfoMap::Map back to a new ControlInfo for\nsubsequent use.\n\nBecause 1) is creating a new ControlInfoMap::Map, the\nemplace(controls::ScalerCrop)\nwill not fail as the key will not be present in the map. Does that make\nsense?\n\nI would dearly love to have a ControlInfo::add() or ControlINfo::emplace()\nso I can\navoid doing the above sequence to add a new key/value to the map!\n\nRegards,\nNaush\n\n\n>\n>\n> So taking their example code, and extending with an emplace after\n> already emplaced (which is the same as this ctrlMap being updated on any\n> second or consecutive call to PipelineHandlerRPi::configure() ...\n>\n> $ cat snippets/map-emplace.cpp\n>\n> #include \n> #include \n> #include \n> #include \n>\n> int main()\n> {\n> std::map m;\n>\n> // uses pair's move constructor\n> m.emplace(std::make_pair(std::string(\"a\"), std::string(\"a\")));\n>\n> // uses pair's converting move constructor\n> m.emplace(std::make_pair(\"b\", \"abcd\"));\n>\n> // uses pair's template constructor\n> m.emplace(\"d\", \"ddd\");\n>\n> m.emplace(\"d\", \"a new ddd\");\n>\n> // uses pair's piecewise constructor\n> m.emplace(std::piecewise_construct,\n> std::forward_as_tuple(\"c\"),\n> std::forward_as_tuple(10, 'c'));\n> // as of C++17, m.try_emplace(\"c\", 10, 'c'); can be used\n>\n> for (const auto &p : m) {\n> std::cout << p.first << \" => \" << p.second << '\\n';\n> }\n> }\n>\n>\n> kbingham@Monstersaurus:~/iob/libcamera/libcamera$ make\n> snippets/map-emplace\n> g++ snippets/map-emplace.cpp -o snippets/map-emplace\n> kbingham@Monstersaurus:~/iob/libcamera/libcamera$ ./snippets/map-emplace\n> a => a\n> b => abcd\n> c => cccccccccc\n> d => ddd\n>\n>\n> Perhaps we need a helper on the ControlInfoMap to support updates.\n>\n>\n> > +\n> > + data->controlInfo_ = ControlInfoMap(std::move(ctrlMap),\n> result.controlInfo.idmap());\n> >\n> > /* Setup the Video Mux/Bridge entities. */\n> > for (auto &[device, link] : data->bridgeDevices_) {\n> > --\n> > 2.25.1\n> >\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 01FD1BD808\n\tfor ;\n\tTue, 21 Jun 2022 12:45:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BA8E61FB2;\n\tTue, 21 Jun 2022 14:45:36 +0200 (CEST)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6703C61FB2\n\tfor ;\n\tTue, 21 Jun 2022 14:45:34 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id s21so11617456lfs.13\n\tfor ;\n\tTue, 21 Jun 2022 05:45:34 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655815536;\n\tbh=reAsUno6HY9I/F6cTtsexOEIgpiRexdeQ2/wJK4HJFk=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ph8flLiJhXS6AbAzoKOI1lpP9TpgSFaspPNc9djxYWNKYreiQqLACManklbRsL7cq\n\tBlxu1olPvdqhcConPVeBDO22H3RVzv+uJNRRIlcBTIqB0Mu90qd+/6jvlgDDyIIcKI\n\tE0Ur4A/WgOTTfIxqFse3eBu+XlMQM3zpWUGOras254z89ufGWVbzrqt1NnhQt9CDQR\n\t5euWqOwylvdFP9s65/X2+Vs4A9moChAuquR150JcAUOhkWkh31FJpIGzmNDp+zqFhS\n\tobWEc3GseyRs4eF4GLX9mbwMvnYye6ghGib/itDjptK6scK6y8c5f7fh94nrqDAH3/\n\tFGpDTTlTT6u2g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=4QaW3AQbXWRIL4dHdHSBeVHAkFj4kg6QXoK+iYgiW5w=;\n\tb=YI2gZo0FWAYLbarpfDCoLTihd9QCC9S3OIFWK9l8wTRc/7Gy989Byx94WF6WT3FpSL\n\tZ4kDetgOKltv8kcW/2DXvDTCQlP21DN9FDfarfTHmRiG4XxFvgm4XFt3N7SeqrIMNsN/\n\tTasQWDwplkRWvF4O05gzot1PHmobxgXGSvSF8t7ctvN8r9PQ44q/OXd0+Xfqf1KmOy8+\n\t3Du8AoXMyiaRhhNgP8fuXle54F27h4XkXoW00cgjUkKGcMxOMspbnZJB+2Ad6EE4M1gW\n\t0RcyQjMg8peISvQTMlabZjObpqDsD3VE10wnOcVZKwAUBGVWau/glbYD3nplAimsBDwo\n\txCVQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"YI2gZo0F\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=4QaW3AQbXWRIL4dHdHSBeVHAkFj4kg6QXoK+iYgiW5w=;\n\tb=Oo2pHK62G75GZaTtBSc14nOygd5zhJK/8q2Y2ueyuS6p9iOqIwdJGDxobPXPIYr3SB\n\tIy1xPwibwG7ARLPSk4CgPwDN2tCbDxaGUpE2ejP56+hHQLOzrNOYU8iRec7K6+fdXQtH\n\tILuUcimrkT7gkl2W4MgKma/m9pVRJ0MWVxYqZ/et1f7IIBQjEuPLZnlFQ5Ea4VAByCxA\n\tPAmfsAESlvB0mv185bR0e8Cub1u7Uc1oMDrpFoxCayy7l1SZ1tb2Kx2o97qmjUqdqVon\n\tA6M0WZ6O0mSuW83yMPq4ENNiVcQvzBenXZFdxBBwydVWzqwRzDZhv+utREn3UaAEFiPm\n\taZDg==","X-Gm-Message-State":"AJIora8K2exzCkczuSWa2mAyRJYFtzRwWRN40Ttbbuh8HQ0QhSB5jGeY\n\tJGuazZNNUWlgQE2D9qjJEZmAuF6w1Ndh16rF/vF+mhJDv9Fyhzmk","X-Google-Smtp-Source":"AGRyM1uPPcqayBa2oaJc4CpTf1Ht/fhtq7TrS3lurvMovh4w2uA4isoJ85UZtxc63fKxajwUiQ0l+gkQlgQTMfRps9k=","X-Received":"by 2002:a05:6512:3581:b0:47f:6e4d:bf6a with SMTP id\n\tm1-20020a056512358100b0047f6e4dbf6amr6451082lfr.63.1655815533683;\n\tTue, 21 Jun 2022 05:45:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20220610122533.11888-1-naush@raspberrypi.com>\n\t<20220610122533.11888-3-naush@raspberrypi.com>\n\t<165581421836.1149771.4927497988659868556@Monstersaurus>","In-Reply-To":"<165581421836.1149771.4927497988659868556@Monstersaurus>","Date":"Tue, 21 Jun 2022 13:45:29 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000b3111005e1f49836\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise\n\tScalerCrop from the pipeline handler","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23507,"web_url":"https://patchwork.libcamera.org/comment/23507/","msgid":"","date":"2022-06-21T13:03:36","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise\n\tScalerCrop from the pipeline handler","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Jun 21, 2022 at 01:45:29PM +0100, Naushir Patuck via libcamera-devel wrote:\n> On Tue, 21 Jun 2022 at 13:23, Kieran Bingham wrote:\n> > Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:33)\n> > > The ScalerCrop control is handled directly by the pipeline handler. Remove the\n> > > control from the IPA's static ControlInfoMap, and let the pipeline handler add\n> > > it to the ControlInfoMap advertised to the application, ensuring the limits\n> > > are set appropriately based on the current sensor mode.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 1 -\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++++-\n> > > 2 files changed, 9 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 295f6b735dc0..f46fccdd4177 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -86,7 +86,6 @@ static const ControlInfoMap::Map ipaControls{\n> > > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > > - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > > };\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 4596f2babcea..66a84b1dfb97 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -941,7 +941,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);\n> > >\n> > > /* Update the controls that the Raspberry Pi IPA can handle. */\n> > > - data->controlInfo_ = result.controlInfo;\n> > > + ControlInfoMap::Map ctrlMap;\n> > > + for (auto const &c : result.controlInfo)\n> > > + ctrlMap.emplace(c.first, c.second);\n> > > +\n> > > + /* Add the ScalerCrop control limits based on the current mode. */\n> > > + ctrlMap.emplace(&controls::ScalerCrop,\n> > > + ControlInfo(Rectangle(data->ispMinCropSize_), Rectangle(data->sensorInfo_.outputSize)));\n> >\n> > I don't think this works with emplace.\n> >\n> > Reading: https://en.cppreference.com/w/cpp/container/map/emplace\n> >\n> > \"The element may be constructed even if there already is an element\n> > with the key in the container, in which case the newly constructed\n> > element will be destroyed immediately.\"\n> \n> I see what you are getting at (I think), but the code is correct by an unfortunate\n> accident. I cannot see a way to add a single element to an existing ControlInfoMap\n> (it is privately inherited from an unordered_map). So I have to to the following\n> (slightly hideous) sequence:\n> \n> 1) Populate a ControlInfoMap::Map from the IPA's ControlInfo\n> 2) emplace the \"controls::ScalerCrop\" to this new ControlInfoMap::Map\n> 3) Convert the new ControlInfoMap::Map back to a new ControlInfo for\n> subsequent use.\n> \n> Because 1) is creating a new ControlInfoMap::Map, the emplace(controls::ScalerCrop)\n> will not fail as the key will not be present in the map. Does that make sense?\n> \n> I would dearly love to have a ControlInfo::add() or ControlINfo::emplace() so I can\n> avoid doing the above sequence to add a new key/value to the map!\n\nControlInfoMap was meant to be immutable, as controls exposed by a\ncamera were not meant to change, but as we've blown up that design\nprinciple, I'm fine with adding a new function to the ControlInfoMap\nclass to add/update an element until we redesign all this.\n\n> > So taking their example code, and extending with an emplace after\n> > already emplaced (which is the same as this ctrlMap being updated on any\n> > second or consecutive call to PipelineHandlerRPi::configure() ...\n> >\n> > $ cat snippets/map-emplace.cpp\n> >\n> > #include \n> > #include \n> > #include \n> > #include \n> >\n> > int main()\n> > {\n> > std::map m;\n> >\n> > // uses pair's move constructor\n> > m.emplace(std::make_pair(std::string(\"a\"), std::string(\"a\")));\n> >\n> > // uses pair's converting move constructor\n> > m.emplace(std::make_pair(\"b\", \"abcd\"));\n> >\n> > // uses pair's template constructor\n> > m.emplace(\"d\", \"ddd\");\n> >\n> > m.emplace(\"d\", \"a new ddd\");\n> >\n> > // uses pair's piecewise constructor\n> > m.emplace(std::piecewise_construct,\n> > std::forward_as_tuple(\"c\"),\n> > std::forward_as_tuple(10, 'c'));\n> > // as of C++17, m.try_emplace(\"c\", 10, 'c'); can be used\n> >\n> > for (const auto &p : m) {\n> > std::cout << p.first << \" => \" << p.second << '\\n';\n> > }\n> > }\n> >\n> >\n> > kbingham@Monstersaurus:~/iob/libcamera/libcamera$ make\n> > snippets/map-emplace\n> > g++ snippets/map-emplace.cpp -o snippets/map-emplace\n> > kbingham@Monstersaurus:~/iob/libcamera/libcamera$ ./snippets/map-emplace\n> > a => a\n> > b => abcd\n> > c => cccccccccc\n> > d => ddd\n> >\n> >\n> > Perhaps we need a helper on the ControlInfoMap to support updates.\n> >\n> > > +\n> > > + data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> > >\n> > > /* Setup the Video Mux/Bridge entities. */\n> > > for (auto &[device, link] : data->bridgeDevices_) {","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 7362EBD808\n\tfor ;\n\tTue, 21 Jun 2022 13:03:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE40F65638;\n\tTue, 21 Jun 2022 15:03:54 +0200 (CEST)","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 EF1EE61FB2\n\tfor ;\n\tTue, 21 Jun 2022 15:03:52 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 70FCB104;\n\tTue, 21 Jun 2022 15:03:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655816634;\n\tbh=qYfiIuhUbasPABktQHjYSvUf204pnTjLdzLN7tHdq2g=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=gOjEtpoHf97gogsNzAYOWaqftKgekcLUqY1X2C4CikEs/CFqXrhbAxupjPOJOe0Hy\n\tAymCLIkCEYylp8H5Fh0863rBA6eE0cF9Fu1fD+PV/59Yd9XP3ewR+r0jUPjNaBxStO\n\tnuqZidTjF7q0lORiJiSs1IAEKikFpAGrZyhMFP+PWPPYhf+yLg8I8QAR/OdEsZpZg+\n\twCjbJneKqk2iThOby0NYGYMW/SrCm6mimDphUV7IUZbhyGhEry5PAwG/eHExMQKARg\n\tZ9Ge9+rOazoqrr0sUxzQZgH3YSjHBXY6a3OZQ33MyJ8wqjPaMIuDiVkzTZ+bt6m1rW\n\ty/Lz5Xj0ZDERg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655816632;\n\tbh=qYfiIuhUbasPABktQHjYSvUf204pnTjLdzLN7tHdq2g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V0pTMFkPiuAVoCNZ6YcpBFIigSXyFahT6ZgSJh9z0FB8R3sDKa1z/IouCgcro4yOy\n\t1HaNcA2mac0351Kdp4fkvYJ9gl5lcNWomkJ6NZ/6wTxp5YazW3rsHj3HOYnpGUyQZL\n\tdkCEFVYfaIp3kYQTg9MR2iKVaK6SAP0Sl8JdAySc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"V0pTMFkP\"; dkim-atps=neutral","Date":"Tue, 21 Jun 2022 16:03:36 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220610122533.11888-1-naush@raspberrypi.com>\n\t<20220610122533.11888-3-naush@raspberrypi.com>\n\t<165581421836.1149771.4927497988659868556@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise\n\tScalerCrop from the pipeline handler","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23509,"web_url":"https://patchwork.libcamera.org/comment/23509/","msgid":"<165581860913.1149771.10879673691583405050@Monstersaurus>","date":"2022-06-21T13:36:49","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise\n\tScalerCrop from the pipeline handler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2022-06-21 13:45:29)\n> Hi Kieran,\n> \n> Thank you for your feedback.\n> \n> On Tue, 21 Jun 2022 at 13:23, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:33)\n> > > The ScalerCrop control is handled directly by the pipeline handler.\n> > Remove the\n> > > control from the IPA's static ControlInfoMap, and let the pipeline\n> > handler add\n> > > it to the ControlInfoMap advertised to the application, ensuring the\n> > limits\n> > > are set appropriately based on the current sensor mode.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 1 -\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++++-\n> > > 2 files changed, 9 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 295f6b735dc0..f46fccdd4177 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -86,7 +86,6 @@ static const ControlInfoMap::Map ipaControls{\n> > > { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f)\n> > },\n> > > - { &controls::ScalerCrop, ControlInfo(Rectangle{},\n> > Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > > { &controls::draft::NoiseReductionMode,\n> > ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > > };\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 4596f2babcea..66a84b1dfb97 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -941,7 +941,15 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> > CameraConfiguration *config)\n> > > data->properties_.set(properties::SensorSensitivity,\n> > result.modeSensitivity);\n> > >\n> > > /* Update the controls that the Raspberry Pi IPA can handle. */\n> > > - data->controlInfo_ = result.controlInfo;\n> > > + ControlInfoMap::Map ctrlMap;\n> > > + for (auto const &c : result.controlInfo)\n> > > + ctrlMap.emplace(c.first, c.second);\n> > > +\n> > > + /* Add the ScalerCrop control limits based on the current mode.\n> > */\n> > > + ctrlMap.emplace(&controls::ScalerCrop,\n> > > + ControlInfo(Rectangle(data->ispMinCropSize_),\n> > Rectangle(data->sensorInfo_.outputSize)));\n> >\n> > I don't think this works with emplace.\n> >\n> > Reading: https://en.cppreference.com/w/cpp/container/map/emplace\n> >\n> > \"The element may be constructed even if there already is an element\n> > with the key in the container, in which case the newly constructed\n> > element will be destroyed immediately.\"\n> >\n> \n> I see what you are getting at (I think), but the code is correct by an\n> unfortunate\n> accident. I cannot see a way to add a single element to an existing\n> ControlInfoMap\n> (it is privately inherited from an unordered_map). So I have to to the\n> following\n> (slightly hideous) sequence:\n> \n> 1) Populate a ControlInfoMap::Map from the IPA's ControlInfo\n> 2) emplace the \"controls::ScalerCrop\" to this new ControlInfoMap::Map\n> 3) Convert the new ControlInfoMap::Map back to a new ControlInfo for\n> subsequent use.\n> \n> Because 1) is creating a new ControlInfoMap::Map, the\n> emplace(controls::ScalerCrop)\n> will not fail as the key will not be present in the map. Does that make\n> sense?\n> \n> I would dearly love to have a ControlInfo::add() or ControlINfo::emplace()\n> so I can\n> avoid doing the above sequence to add a new key/value to the map!\n\nAha, yes, ok - so I see it 'currently' works, but I agree it could be\nbetter if we had a nicer helper to update an existing one.\n\nI haven't looked yet, but I suspect that can't be too difficult to\nimplement...\n\n--\nKieran\n\n\n\n> \n> Regards,\n> Naush\n> \n> \n> >\n> >\n> > So taking their example code, and extending with an emplace after\n> > already emplaced (which is the same as this ctrlMap being updated on any\n> > second or consecutive call to PipelineHandlerRPi::configure() ...\n> >\n> > $ cat snippets/map-emplace.cpp\n> >\n> > #include \n> > #include \n> > #include \n> > #include \n> >\n> > int main()\n> > {\n> > std::map m;\n> >\n> > // uses pair's move constructor\n> > m.emplace(std::make_pair(std::string(\"a\"), std::string(\"a\")));\n> >\n> > // uses pair's converting move constructor\n> > m.emplace(std::make_pair(\"b\", \"abcd\"));\n> >\n> > // uses pair's template constructor\n> > m.emplace(\"d\", \"ddd\");\n> >\n> > m.emplace(\"d\", \"a new ddd\");\n> >\n> > // uses pair's piecewise constructor\n> > m.emplace(std::piecewise_construct,\n> > std::forward_as_tuple(\"c\"),\n> > std::forward_as_tuple(10, 'c'));\n> > // as of C++17, m.try_emplace(\"c\", 10, 'c'); can be used\n> >\n> > for (const auto &p : m) {\n> > std::cout << p.first << \" => \" << p.second << '\\n';\n> > }\n> > }\n> >\n> >\n> > kbingham@Monstersaurus:~/iob/libcamera/libcamera$ make\n> > snippets/map-emplace\n> > g++ snippets/map-emplace.cpp -o snippets/map-emplace\n> > kbingham@Monstersaurus:~/iob/libcamera/libcamera$ ./snippets/map-emplace\n> > a => a\n> > b => abcd\n> > c => cccccccccc\n> > d => ddd\n> >\n> >\n> > Perhaps we need a helper on the ControlInfoMap to support updates.\n> >\n> >\n> > > +\n> > > + data->controlInfo_ = ControlInfoMap(std::move(ctrlMap),\n> > result.controlInfo.idmap());\n> > >\n> > > /* Setup the Video Mux/Bridge entities. */\n> > > for (auto &[device, link] : data->bridgeDevices_) {\n> > > --\n> > > 2.25.1\n> > >\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 853E2BD808\n\tfor ;\n\tTue, 21 Jun 2022 13:36:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0A4360473;\n\tTue, 21 Jun 2022 15:36:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4E6AF60473\n\tfor ;\n\tTue, 21 Jun 2022 15:36:52 +0200 (CEST)","from pendragon.ideasonboard.com\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 C90CA576;\n\tTue, 21 Jun 2022 15:36:51 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655818614;\n\tbh=ZSzn1myAIVV03rSyZeHr3u+o6okv978yZ6f6SWR52cg=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=tT9QOmC56/yIllJDYYM6qya4W/5llNZoxD4khRj4QRTXv/FRbtPT76vNbzkywK09g\n\tgCmKvpvAi1wzn5P1p+GGRCeK2QaIepv+c3jWwxdycaiU0KByE9usgDPXJRGCftOVzN\n\tSx9mEoXnei4AS3L2vRauBeHo4FKK8SFek+qWTCojd0QbwZHhnB2P1i2KTP0jNZeNWF\n\t/l66ug5wIGo4ZtA4YH4yeOzunhozyO4mFO0KXuVyQgAwe3PuzuW8VKz+HUPJNuPrNo\n\tsKXmFB4rqOQFsjyA2xUmnOYDqoyBLB+1GvOytX1VqPXzX3PCqHqGZpMlIqLTkc3nYi\n\tYTOrKj+atbiwQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655818611;\n\tbh=ZSzn1myAIVV03rSyZeHr3u+o6okv978yZ6f6SWR52cg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=v3xGSh8hL2YBpRM/EzoazGDlibFrLhUC6tAqDNgNB+NTVcz6WcgKrJOuUEcZzJ2PU\n\tnIiFcyNkS9MK596ALhz6ws7rzbHC71my5X2MOEbHxgEpDRKW2IOl2kDDxmPt1l+Rht\n\tOcPLckhANExeWkQgc4yskXc/3+prlg2IKb1pJwWU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"v3xGSh8h\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220610122533.11888-1-naush@raspberrypi.com>\n\t<20220610122533.11888-3-naush@raspberrypi.com>\n\t<165581421836.1149771.4927497988659868556@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Tue, 21 Jun 2022 14:36:49 +0100","Message-ID":"<165581860913.1149771.10879673691583405050@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: raspberrypi: Advertise\n\tScalerCrop from the pipeline handler","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]