[{"id":5381,"web_url":"https://patchwork.libcamera.org/comment/5381/","msgid":"<20200624235722.GO5980@pendragon.ideasonboard.com>","date":"2020-06-24T23:57:22","subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo and Niklas,\n\nOn Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:\n> > Instead of spreading the mapping between media bus codes and V4L2 FourCC\n> > all over the CIO2 code collect it in a single map and extract the data\n> > from it. This is done in preparation of adding PixelFormat information\n> > to the mix.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------\n> > 1 file changed, 24 insertions(+), 26 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -18,6 +18,17 @@ namespace libcamera {\n> >\n> > LOG_DECLARE_CATEGORY(IPU3)\n> >\n> > +namespace {\n> > +\n> > +static const std::map mbusCodesToInfo = {\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> > +};\n> > +\n> > +} /* namespace */\n> > +\n> \n> Ok, I see where this is going with the next patch, but I'm not super\n> excited by having to iterate the map just to extract keys (and anyway,\n> why doesn't std::map have a keys() function ??).\n\nPossibly because its implementation would be O(n), and STL generally\ntries to avoid hiding expensive operations behind apparently simple\nfunctions. Just a speculation.\n\n> > CIO2Device::CIO2Device()\n> > \t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> > {\n> > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> > \t * Make sure the sensor produces at least one format compatible with\n> > \t * the CIO2 requirements.\n> > \t */\n> > -\tconst std::set cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > -\t\t\t\t\t\tMEDIA_BUS_FMT_SRGGB10_1X10 };\n> > +\tstd::set cio2Codes;\n> \n> why a set ?\n\nTo keep this efficient, given that std::map is sorted by key, this could\nbe a vector, and we could call\n\n\tcio2Codes.reserve(mbusCodesToInfo.size());\n\nbefore the std::transform(). I think a set is indeed overkill.\n\n> > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > +\t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n> > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> \n> you could\n> \t\t[](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });\n> or since C++14\n> \t\t[](auto &pair){ return pair.first; });\n\nMuch nicer indeed.\n\n> > \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> > \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> > \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > \t * Apply the selected format to the sensor, the CSI-2 receiver and\n> > \t * the CIO2 output device.\n> > \t */\n> > -\tsensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > -\t\t\t\t\t size);\n> > +\tstd::vector mbusCodes;\n> \n> You could reserve space in the vector knowing the map size\n\nSeems we agree :-)\n\n> > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > +\t\tstd::back_inserter(mbusCodes),\n> > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > +\n> > +\tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> > \tret = sensor_->setFormat(&sensorFormat);\n> > \tif (ret)\n> > \t\treturn ret;\n> > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > -\tV4L2PixelFormat v4l2Format;\n> > -\tswitch (sensorFormat.mbus_code) {\n> > -\tcase MEDIA_BUS_FMT_SBGGR10_1X10:\n> > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);\n> > -\t\tbreak;\n> > -\tcase MEDIA_BUS_FMT_SGBRG10_1X10:\n> > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);\n> > -\t\tbreak;\n> > -\tcase MEDIA_BUS_FMT_SGRBG10_1X10:\n> > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);\n> > -\t\tbreak;\n> > -\tcase MEDIA_BUS_FMT_SRGGB10_1X10:\n> > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);\n> > -\t\tbreak;\n> > -\tdefault:\n> > +\tconst auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);\n> > +\tif (itInfo == mbusCodesToInfo.end())\n> > \t\treturn -EINVAL;\n> > -\t}\n> >\n> > -\toutputFormat->fourcc = v4l2Format;\n> > +\toutputFormat->fourcc = itInfo->second;\n> \n> I would be tempted to suggest you to add helper functions around that\n> map, not sure it's worth the effort, but those transform to just\n> retreive keys are not nice imho.\n\nI was about to mention that, thinking that the vector of keys could then\nbe cached, but given that the code is only used in init paths, it may\nnot be worth it.\n\nReviewed-by: Laurent Pinchart \n\n> That apart\n> Reviewed-by: Jacopo Mondi \n> \n> > \toutputFormat->size = sensorFormat.size;\n> > \toutputFormat->planesCount = 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 B0510C0101\n\tfor ;\n\tWed, 24 Jun 2020 23:57:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 235E5609A9;\n\tThu, 25 Jun 2020 01:57:25 +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 229BD603BE\n\tfor ;\n\tThu, 25 Jun 2020 01:57:24 +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 89CA02A8;\n\tThu, 25 Jun 2020 01:57:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cQLtxix+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593043043;\n\tbh=QeC1aEaDWizeoLcb+mHYoNAk7kLU7FboMvjMrMVqBGE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cQLtxix+LAHjtpAX4wli7IfTZ+0NzoASGHj4dvt9g7TdndsIjFqRncjw4QTHB2BTM\n\tO9lv6NOgLkDvdHq1qHeW4EyhHR5ZoJEb/Oc/RGUx1C8ywnXf5dxGyXhvjxvh7jOy2Z\n\tQ0laEtLbj/3eIObo60NV9dAZv1UnDGa0H6Jx4fLQ=","Date":"Thu, 25 Jun 2020 02:57:22 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200624235722.GO5980@pendragon.ideasonboard.com>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-7-niklas.soderlund@ragnatech.se>\n\t<20200624100114.bnh7jxfhenfayd5o@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200624100114.bnh7jxfhenfayd5o@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":5382,"web_url":"https://patchwork.libcamera.org/comment/5382/","msgid":"<20200625001259.GB10256@oden.dyn.berto.se>","date":"2020-06-25T00:12:59","subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent and Jacopo,\n\nOn 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote:\n> Hi Jacopo and Niklas,\n> \n> On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote:\n> > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:\n> > > Instead of spreading the mapping between media bus codes and V4L2 FourCC\n> > > all over the CIO2 code collect it in a single map and extract the data\n> > > from it. This is done in preparation of adding PixelFormat information\n> > > to the mix.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------\n> > > 1 file changed, 24 insertions(+), 26 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644\n> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > @@ -18,6 +18,17 @@ namespace libcamera {\n> > >\n> > > LOG_DECLARE_CATEGORY(IPU3)\n> > >\n> > > +namespace {\n> > > +\n> > > +static const std::map mbusCodesToInfo = {\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> > > +};\n> > > +\n> > > +} /* namespace */\n> > > +\n> > \n> > Ok, I see where this is going with the next patch, but I'm not super\n> > excited by having to iterate the map just to extract keys (and anyway,\n> > why doesn't std::map have a keys() function ??).\n> \n> Possibly because its implementation would be O(n), and STL generally\n> tries to avoid hiding expensive operations behind apparently simple\n> functions. Just a speculation.\n> \n> > > CIO2Device::CIO2Device()\n> > > \t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> > > {\n> > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> > > \t * Make sure the sensor produces at least one format compatible with\n> > > \t * the CIO2 requirements.\n> > > \t */\n> > > -\tconst std::set cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SRGGB10_1X10 };\n> > > +\tstd::set cio2Codes;\n> > \n> > why a set ?\n> \n> To keep this efficient, given that std::map is sorted by key, this could\n> be a vector, and we could call\n> \n> \tcio2Codes.reserve(mbusCodesToInfo.size());\n\nWe would then also need to remember to sort the vector as that is a \nrequirement for utils::set_overlap() where this is used. I think it's \nmuch nicer to use a set from the start to make sure nothing goes wrong.\n\nI'm feeling a bit of resistance against set in this series. Is there a \ndownside to using it I don't see? I think they are neat as a \nspecialization of vector where we get guarantees there is only one \noccurrence of each value and the readout/sorting order will be the same \nno mater where the container where created. Am I missing something?\n\n> \n> before the std::transform(). I think a set is indeed overkill.\n> \n> > > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > > +\t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n> > > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > \n> > you could\n> > \t\t[](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });\n> > or since C++14\n> > \t\t[](auto &pair){ return pair.first; });\n> \n> Much nicer indeed.\n\nNeat.\n\n> \n> > > \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> > > \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> > > \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > \t * Apply the selected format to the sensor, the CSI-2 receiver and\n> > > \t * the CIO2 output device.\n> > > \t */\n> > > -\tsensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> > > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> > > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > > -\t\t\t\t\t size);\n> > > +\tstd::vector mbusCodes;\n> > \n> > You could reserve space in the vector knowing the map size\n> \n> Seems we agree :-)\n> \n> > > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > > +\t\tstd::back_inserter(mbusCodes),\n> > > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > > +\n> > > +\tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> > > \tret = sensor_->setFormat(&sensorFormat);\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > >\n> > > -\tV4L2PixelFormat v4l2Format;\n> > > -\tswitch (sensorFormat.mbus_code) {\n> > > -\tcase MEDIA_BUS_FMT_SBGGR10_1X10:\n> > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);\n> > > -\t\tbreak;\n> > > -\tcase MEDIA_BUS_FMT_SGBRG10_1X10:\n> > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);\n> > > -\t\tbreak;\n> > > -\tcase MEDIA_BUS_FMT_SGRBG10_1X10:\n> > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);\n> > > -\t\tbreak;\n> > > -\tcase MEDIA_BUS_FMT_SRGGB10_1X10:\n> > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);\n> > > -\t\tbreak;\n> > > -\tdefault:\n> > > +\tconst auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);\n> > > +\tif (itInfo == mbusCodesToInfo.end())\n> > > \t\treturn -EINVAL;\n> > > -\t}\n> > >\n> > > -\toutputFormat->fourcc = v4l2Format;\n> > > +\toutputFormat->fourcc = itInfo->second;\n> > \n> > I would be tempted to suggest you to add helper functions around that\n> > map, not sure it's worth the effort, but those transform to just\n> > retreive keys are not nice imho.\n> \n> I was about to mention that, thinking that the vector of keys could then\n> be cached, but given that the code is only used in init paths, it may\n> not be worth it.\n\nI see two options for this problem, the one used in this patch or \ncreating multiple data structures in the anonyms namespace, one for each \nway to lookup something. The later consumes less CPU but consumes more \nmemory, I'm not sure which on is the best option here. I went with this \noption as it prevents possible differences between the multiple data \nstructures for lookup. I'm open to switching to that option if that is \nthe consensus.\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > That apart\n> > Reviewed-by: Jacopo Mondi \n> > \n> > > \toutputFormat->size = sensorFormat.size;\n> > > \toutputFormat->planesCount = 1;\n> > >\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 49AE4C0100\n\tfor ;\n\tThu, 25 Jun 2020 00:13:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAF4D603BE;\n\tThu, 25 Jun 2020 02:13:03 +0200 (CEST)","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 2F088603BB\n\tfor ;\n\tThu, 25 Jun 2020 02:13:02 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id n24so4517883lji.10\n\tfor ;\n\tWed, 24 Jun 2020 17:13:02 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv24sm5592647lfo.4.2020.06.24.17.13.00\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 24 Jun 2020 17:13:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"j0Oz2AAx\"; dkim-atps=neutral","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\tbh=WsT68JxVBo9m5A1dZ5dthAWos/xSRWP1lhfzz3wqOH8=;\n\tb=j0Oz2AAxSUuvAES+kZbsIdDHVyhVN0pT40wWlV/Ff0OK0SEI3t4V8bYI94iOq/kpua\n\tAHUMkHajhqVuuyxH+8oo+gpYz4mgD85byczgPsoL/Ovv2WeRpx/kvRkv3o6pLVJyn3WM\n\tgG0mSRHjzT36nWBBRYaWc7J3+fNNe5MTHNuC+HnFamyOyMnJg3p9FdE2izwpSUx/mIY+\n\tWmH2Cd61eJOcbFsmModCwPQsAYKgRhl2qzstjfVNUe0mBbP0YJ0BUfd30U30ve0eoLA8\n\t8oqrBWhkq+FO4Dcfw3AqgjCMkO0Tf5syIb2QWRVdGNdUIMg1KrUN+HEk9v/X8ViCk5Cv\n\tfuCA==","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;\n\tbh=WsT68JxVBo9m5A1dZ5dthAWos/xSRWP1lhfzz3wqOH8=;\n\tb=BNeMJFakVgkD+puqwwkzM8FHJXdAgHSPQ46PmRtDXiOdcPB/NJia0KYdIGfqGHGN59\n\t8mDHC3wczz30a/S95lMVOA1uUayKQWeF9jOesEyvEfrZrtCcSJUlUQi2lBmsKI5OX5dx\n\tTNtWMx0Rhxu3sEN6Sx71QAYuw5dfoZOU1xvOWhiCHxdrCGwcT/jsbFkwaLdKq/dlcStT\n\to3ofLCPPEjA563eewfBR4drxx8pdmMiPQwz+Wbg3JTNnrJzlBC8Q46gWLnIe9tEpHQWg\n\tuBGMjySLV/weZtvZeHo28ztPQsYDJ1yxPWYiS5NzNRt1+OHjc7IW5PNivIUp840cShn1\n\tm0SQ==","X-Gm-Message-State":"AOAM5310C5P5i2uIaxNPaPylA84MWZ3Nh5Pw85+ugOiDCrNbJgMB3xJx\n\tdoMIIqcgX8HmE6ITqM046J7z+L2SjZo=","X-Google-Smtp-Source":"ABdhPJwzWSPIzpDVSu+qIW25KLPUdsfeYAfLIME3tFX4pcZHncVH4Aj/w0DrtF95y+ZyYy3M8U/EQg==","X-Received":"by 2002:a05:651c:c9:: with SMTP id\n\t9mr16494008ljr.365.1593043981362; \n\tWed, 24 Jun 2020 17:13:01 -0700 (PDT)","Date":"Thu, 25 Jun 2020 02:12:59 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200625001259.GB10256@oden.dyn.berto.se>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-7-niklas.soderlund@ragnatech.se>\n\t<20200624100114.bnh7jxfhenfayd5o@uno.localdomain>\n\t<20200624235722.GO5980@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200624235722.GO5980@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":5384,"web_url":"https://patchwork.libcamera.org/comment/5384/","msgid":"<20200625012346.GA1875@pendragon.ideasonboard.com>","date":"2020-06-25T01:23:46","subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Thu, Jun 25, 2020 at 02:12:59AM +0200, Niklas Söderlund wrote:\n> On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote:\n> > On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote:\n> > > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:\n> > > > Instead of spreading the mapping between media bus codes and V4L2 FourCC\n> > > > all over the CIO2 code collect it in a single map and extract the data\n> > > > from it. This is done in preparation of adding PixelFormat information\n> > > > to the mix.\n> > > >\n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------\n> > > > 1 file changed, 24 insertions(+), 26 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > @@ -18,6 +18,17 @@ namespace libcamera {\n> > > >\n> > > > LOG_DECLARE_CATEGORY(IPU3)\n> > > >\n> > > > +namespace {\n> > > > +\n> > > > +static const std::map mbusCodesToInfo = {\n> > > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> > > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> > > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> > > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> > > > +};\n> > > > +\n> > > > +} /* namespace */\n> > > > +\n> > > \n> > > Ok, I see where this is going with the next patch, but I'm not super\n> > > excited by having to iterate the map just to extract keys (and anyway,\n> > > why doesn't std::map have a keys() function ??).\n> > \n> > Possibly because its implementation would be O(n), and STL generally\n> > tries to avoid hiding expensive operations behind apparently simple\n> > functions. Just a speculation.\n> > \n> > > > CIO2Device::CIO2Device()\n> > > > \t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> > > > {\n> > > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> > > > \t * Make sure the sensor produces at least one format compatible with\n> > > > \t * the CIO2 requirements.\n> > > > \t */\n> > > > -\tconst std::set cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SRGGB10_1X10 };\n> > > > +\tstd::set cio2Codes;\n> > > \n> > > why a set ?\n> > \n> > To keep this efficient, given that std::map is sorted by key, this could\n> > be a vector, and we could call\n> > \n> > \tcio2Codes.reserve(mbusCodesToInfo.size());\n> \n> We would then also need to remember to sort the vector as that is a \n> requirement for utils::set_overlap() where this is used. I think it's \n> much nicer to use a set from the start to make sure nothing goes wrong.\n\nI agree std::set has that benefit. Note that here we wouldn't need to\nsort the vector explicitly. I thus don't really see a reason to use a\nset in this specific case. However, maybe adding\n\n\t/*\n\t * utils::set_overlap() requires the containers to be sorted,\n\t * this is guaranteed for cio2Codes as it gets generated from a\n\t * sorted map with a loop that preserves the order.\n\t */\n\ncould avoid surprises in the future.\n\n> I'm feeling a bit of resistance against set in this series. Is there a \n> downside to using it I don't see? I think they are neat as a \n> specialization of vector where we get guarantees there is only one \n> occurrence of each value and the readout/sorting order will be the same \n> no mater where the container where created. Am I missing something?\n\nstd::set is (usually) implemented as a red-black tree. That will consume\nmore memory than a vector, and for small sets, will likely be slower\nthan creating a vector and sorting it (if needed). I'm not necessarily\nopposed to usage of std::set in non-hot paths, but as noted in another\nreply in this series, we would need to make that consistent then,\npassing a set to CameraSensor::getFormats(), and possibly more\nfunctions. I fear std::set would quickly spread through lots of places.\n\n> > before the std::transform(). I think a set is indeed overkill.\n> > \n> > > > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > > > +\t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n\nbtw shouldn't this be end() and not begin() ?\n\n> > > > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > > \n> > > you could\n> > > \t\t[](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });\n> > > or since C++14\n> > > \t\t[](auto &pair){ return pair.first; });\n> > \n> > Much nicer indeed.\n> \n> Neat.\n> \n> > > > \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> > > > \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> > > > \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> > > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > > \t * Apply the selected format to the sensor, the CSI-2 receiver and\n> > > > \t * the CIO2 output device.\n> > > > \t */\n> > > > -\tsensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> > > > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> > > > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > > > -\t\t\t\t\t size);\n> > > > +\tstd::vector mbusCodes;\n> > > \n> > > You could reserve space in the vector knowing the map size\n> > \n> > Seems we agree :-)\n> > \n> > > > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > > > +\t\tstd::back_inserter(mbusCodes),\n> > > > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > > > +\n> > > > +\tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> > > > \tret = sensor_->setFormat(&sensorFormat);\n> > > > \tif (ret)\n> > > > \t\treturn ret;\n> > > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > > \tif (ret)\n> > > > \t\treturn ret;\n> > > >\n> > > > -\tV4L2PixelFormat v4l2Format;\n> > > > -\tswitch (sensorFormat.mbus_code) {\n> > > > -\tcase MEDIA_BUS_FMT_SBGGR10_1X10:\n> > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);\n> > > > -\t\tbreak;\n> > > > -\tcase MEDIA_BUS_FMT_SGBRG10_1X10:\n> > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);\n> > > > -\t\tbreak;\n> > > > -\tcase MEDIA_BUS_FMT_SGRBG10_1X10:\n> > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);\n> > > > -\t\tbreak;\n> > > > -\tcase MEDIA_BUS_FMT_SRGGB10_1X10:\n> > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);\n> > > > -\t\tbreak;\n> > > > -\tdefault:\n> > > > +\tconst auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);\n> > > > +\tif (itInfo == mbusCodesToInfo.end())\n> > > > \t\treturn -EINVAL;\n> > > > -\t}\n> > > >\n> > > > -\toutputFormat->fourcc = v4l2Format;\n> > > > +\toutputFormat->fourcc = itInfo->second;\n> > > \n> > > I would be tempted to suggest you to add helper functions around that\n> > > map, not sure it's worth the effort, but those transform to just\n> > > retreive keys are not nice imho.\n> > \n> > I was about to mention that, thinking that the vector of keys could then\n> > be cached, but given that the code is only used in init paths, it may\n> > not be worth it.\n> \n> I see two options for this problem, the one used in this patch or \n> creating multiple data structures in the anonyms namespace, one for each \n> way to lookup something. The later consumes less CPU but consumes more \n> memory, I'm not sure which on is the best option here. I went with this \n> option as it prevents possible differences between the multiple data \n> structures for lookup. I'm open to switching to that option if that is \n> the consensus.\n\nIf it was used in more places I'd cache the keys the first time they're\nused, but it's not worth it here. Same for reverse lookups, I'd create\nand cache a reverse map on the first call to the helper, but only if it\nreally makes a difference.\n\n> > Reviewed-by: Laurent Pinchart \n> > \n> > > That apart\n> > > Reviewed-by: Jacopo Mondi \n> > > \n> > > > \toutputFormat->size = sensorFormat.size;\n> > > > \toutputFormat->planesCount = 1;","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 33A80C0104\n\tfor ;\n\tThu, 25 Jun 2020 01:23:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 010B8609A3;\n\tThu, 25 Jun 2020 03:23:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8426A603BB\n\tfor ;\n\tThu, 25 Jun 2020 03:23:47 +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 0CA7772E;\n\tThu, 25 Jun 2020 03:23:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dqJzGPcM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593048227;\n\tbh=LLf2yPjJp100UuaRB54sr8h21rkKutzJZsIJzHD0/f8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dqJzGPcMk5HK/xTFrr15oTctvjoLJU58lNleUsBbh+kkizpZOJhS+CN+WTltKGizN\n\tMEtaxVTnH+PnidOiCUN+DhrPBDgFJTiq6ddJyT1PAXwUN+srDKiKl/D5PpqUNbuH7r\n\t2T1ZtBLhfbTvK6wOe1YgkFDarooynslJicdXtEsc=","Date":"Thu, 25 Jun 2020 04:23:46 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200625012346.GA1875@pendragon.ideasonboard.com>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-7-niklas.soderlund@ragnatech.se>\n\t<20200624100114.bnh7jxfhenfayd5o@uno.localdomain>\n\t<20200624235722.GO5980@pendragon.ideasonboard.com>\n\t<20200625001259.GB10256@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200625001259.GB10256@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":5409,"web_url":"https://patchwork.libcamera.org/comment/5409/","msgid":"<20200625075605.o6ggeb6cyb7snjko@uno.localdomain>","date":"2020-06-25T07:56:05","subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Thu, Jun 25, 2020 at 04:23:46AM +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n>\n> On Thu, Jun 25, 2020 at 02:12:59AM +0200, Niklas Söderlund wrote:\n> > On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote:\n> > > On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote:\n> > > > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:\n> > > > > Instead of spreading the mapping between media bus codes and V4L2 FourCC\n> > > > > all over the CIO2 code collect it in a single map and extract the data\n> > > > > from it. This is done in preparation of adding PixelFormat information\n> > > > > to the mix.\n> > > > >\n> > > > > Signed-off-by: Niklas Söderlund \n> > > > > ---\n> > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------\n> > > > > 1 file changed, 24 insertions(+), 26 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644\n> > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > > @@ -18,6 +18,17 @@ namespace libcamera {\n> > > > >\n> > > > > LOG_DECLARE_CATEGORY(IPU3)\n> > > > >\n> > > > > +namespace {\n> > > > > +\n> > > > > +static const std::map mbusCodesToInfo = {\n> > > > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> > > > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> > > > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> > > > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> > > > > +};\n> > > > > +\n> > > > > +} /* namespace */\n> > > > > +\n> > > >\n> > > > Ok, I see where this is going with the next patch, but I'm not super\n> > > > excited by having to iterate the map just to extract keys (and anyway,\n> > > > why doesn't std::map have a keys() function ??).\n> > >\n> > > Possibly because its implementation would be O(n), and STL generally\n> > > tries to avoid hiding expensive operations behind apparently simple\n> > > functions. Just a speculation.\n> > >\n\nCould be!\n\n> > > > > CIO2Device::CIO2Device()\n> > > > > \t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> > > > > {\n> > > > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> > > > > \t * Make sure the sensor produces at least one format compatible with\n> > > > > \t * the CIO2 requirements.\n> > > > > \t */\n> > > > > -\tconst std::set cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SRGGB10_1X10 };\n> > > > > +\tstd::set cio2Codes;\n> > > >\n> > > > why a set ?\n> > >\n> > > To keep this efficient, given that std::map is sorted by key, this could\n> > > be a vector, and we could call\n> > >\n> > > \tcio2Codes.reserve(mbusCodesToInfo.size());\n> >\n> > We would then also need to remember to sort the vector as that is a\n> > requirement for utils::set_overlap() where this is used. I think it's\n> > much nicer to use a set from the start to make sure nothing goes wrong.\n>\n\nSorry but, we're extracting keys from a sorted map with unique\nentries. The resulting vector of keys will be sorted as well, isn't it ?\n\nAnd the requirement of being sorted depends on what you will use the\nvector for anway, it's not something an \"extract keys\" function should\ncare about (and again, if' I'm not mistaken, the vector of keys will\nbe anyhow sorted and unique, being the result of iterating an\nstd::map)\n\n> I agree std::set has that benefit. Note that here we wouldn't need to\n> sort the vector explicitly. I thus don't really see a reason to use a\n> set in this specific case. However, maybe adding\n>\n> \t/*\n> \t * utils::set_overlap() requires the containers to be sorted,\n> \t * this is guaranteed for cio2Codes as it gets generated from a\n> \t * sorted map with a loop that preserves the order.\n> \t */\n>\n> could avoid surprises in the future.\n>\n> > I'm feeling a bit of resistance against set in this series. Is there a\n> > downside to using it I don't see? I think they are neat as a\n> > specialization of vector where we get guarantees there is only one\n> > occurrence of each value and the readout/sorting order will be the same\n> > no mater where the container where created. Am I missing something?\n>\n> std::set is (usually) implemented as a red-black tree. That will consume\n> more memory than a vector, and for small sets, will likely be slower\n> than creating a vector and sorting it (if needed). I'm not necessarily\n> opposed to usage of std::set in non-hot paths, but as noted in another\n> reply in this series, we would need to make that consistent then,\n> passing a set to CameraSensor::getFormats(), and possibly more\n> functions. I fear std::set would quickly spread through lots of places.\n>\n\nThat's my fear as well. If we have utility functions returning set,\ncallers will use set too, and indeed they're not efficient as vectors.\n\n> > > before the std::transform(). I think a set is indeed overkill.\n> > >\n> > > > > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > > > > +\t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n>\n> btw shouldn't this be end() and not begin() ?\n>\n\nAs commented on Naush's patch, can this be handled with an\nstd::back_inserter(cio2Codes) ?\n\n> > > > > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > > >\n> > > > you could\n> > > > \t\t[](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });\n> > > > or since C++14\n> > > > \t\t[](auto &pair){ return pair.first; });\n> > >\n> > > Much nicer indeed.\n> >\n> > Neat.\n> >\n> > > > > \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> > > > > \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> > > > > \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> > > > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > > > \t * Apply the selected format to the sensor, the CSI-2 receiver and\n> > > > > \t * the CIO2 output device.\n> > > > > \t */\n> > > > > -\tsensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > > > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> > > > > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> > > > > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > > > > -\t\t\t\t\t size);\n> > > > > +\tstd::vector mbusCodes;\n> > > >\n> > > > You could reserve space in the vector knowing the map size\n> > >\n> > > Seems we agree :-)\n> > >\n> > > > > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > > > > +\t\tstd::back_inserter(mbusCodes),\n> > > > > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > > > > +\n> > > > > +\tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> > > > > \tret = sensor_->setFormat(&sensorFormat);\n> > > > > \tif (ret)\n> > > > > \t\treturn ret;\n> > > > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > > > \tif (ret)\n> > > > > \t\treturn ret;\n> > > > >\n> > > > > -\tV4L2PixelFormat v4l2Format;\n> > > > > -\tswitch (sensorFormat.mbus_code) {\n> > > > > -\tcase MEDIA_BUS_FMT_SBGGR10_1X10:\n> > > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);\n> > > > > -\t\tbreak;\n> > > > > -\tcase MEDIA_BUS_FMT_SGBRG10_1X10:\n> > > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);\n> > > > > -\t\tbreak;\n> > > > > -\tcase MEDIA_BUS_FMT_SGRBG10_1X10:\n> > > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);\n> > > > > -\t\tbreak;\n> > > > > -\tcase MEDIA_BUS_FMT_SRGGB10_1X10:\n> > > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);\n> > > > > -\t\tbreak;\n> > > > > -\tdefault:\n> > > > > +\tconst auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);\n> > > > > +\tif (itInfo == mbusCodesToInfo.end())\n> > > > > \t\treturn -EINVAL;\n> > > > > -\t}\n> > > > >\n> > > > > -\toutputFormat->fourcc = v4l2Format;\n> > > > > +\toutputFormat->fourcc = itInfo->second;\n> > > >\n> > > > I would be tempted to suggest you to add helper functions around that\n> > > > map, not sure it's worth the effort, but those transform to just\n> > > > retreive keys are not nice imho.\n> > >\n> > > I was about to mention that, thinking that the vector of keys could then\n> > > be cached, but given that the code is only used in init paths, it may\n> > > not be worth it.\n> >\n> > I see two options for this problem, the one used in this patch or\n> > creating multiple data structures in the anonyms namespace, one for each\n> > way to lookup something. The later consumes less CPU but consumes more\n> > memory, I'm not sure which on is the best option here. I went with this\n> > option as it prevents possible differences between the multiple data\n> > structures for lookup. I'm open to switching to that option if that is\n> > the consensus.\n>\n> If it was used in more places I'd cache the keys the first time they're\n> used, but it's not worth it here. Same for reverse lookups, I'd create\n> and cache a reverse map on the first call to the helper, but only if it\n> really makes a difference.\n>\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > > That apart\n> > > > Reviewed-by: Jacopo Mondi \n> > > >\n> > > > > \toutputFormat->size = sensorFormat.size;\n> > > > > \toutputFormat->planesCount = 1;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 4995BC0100\n\tfor ;\n\tThu, 25 Jun 2020 07:52:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2D0C609A9;\n\tThu, 25 Jun 2020 09:52:39 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E1B3603BE\n\tfor ;\n\tThu, 25 Jun 2020 09:52:38 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 44BAC200007;\n\tThu, 25 Jun 2020 07:52:37 +0000 (UTC)"],"Date":"Thu, 25 Jun 2020 09:56:05 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200625075605.o6ggeb6cyb7snjko@uno.localdomain>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-7-niklas.soderlund@ragnatech.se>\n\t<20200624100114.bnh7jxfhenfayd5o@uno.localdomain>\n\t<20200624235722.GO5980@pendragon.ideasonboard.com>\n\t<20200625001259.GB10256@oden.dyn.berto.se>\n\t<20200625012346.GA1875@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200625012346.GA1875@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":10797,"web_url":"https://patchwork.libcamera.org/comment/10797/","msgid":"<20200624100114.bnh7jxfhenfayd5o@uno.localdomain>","date":"2020-06-24T10:01:14","subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:\n> Instead of spreading the mapping between media bus codes and V4L2 FourCC\n> all over the CIO2 code collect it in a single map and extract the data\n> from it. This is done in preparation of adding PixelFormat information\n> to the mix.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------\n> 1 file changed, 24 insertions(+), 26 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -18,6 +18,17 @@ namespace libcamera {\n>\n> LOG_DECLARE_CATEGORY(IPU3)\n>\n> +namespace {\n> +\n> +static const std::map mbusCodesToInfo = {\n> +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> +};\n> +\n> +} /* namespace */\n> +\n\nOk, I see where this is going with the next patch, but I'm not super\nexcited by having to iterate the map just to extract keys (and anyway,\nwhy doesn't std::map have a keys() function ??).\n\n> CIO2Device::CIO2Device()\n> \t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> {\n> @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> \t * Make sure the sensor produces at least one format compatible with\n> \t * the CIO2 requirements.\n> \t */\n> -\tconst std::set cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> -\t\t\t\t\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> -\t\t\t\t\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> -\t\t\t\t\t\tMEDIA_BUS_FMT_SRGGB10_1X10 };\n> +\tstd::set cio2Codes;\n\nwhy a set ?\n\n> +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> +\t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n> +\t\t[](const std::map::value_type &pair){ return pair.first; });\n\nyou could\n\t\t[](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });\nor since C++14\n\t\t[](auto &pair){ return pair.first; });\n\n\n\n> \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \t * Apply the selected format to the sensor, the CSI-2 receiver and\n> \t * the CIO2 output device.\n> \t */\n> -\tsensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> -\t\t\t\t\t size);\n> +\tstd::vector mbusCodes;\n\nYou could reserve space in the vector knowing the map size\n\n> +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> +\t\tstd::back_inserter(mbusCodes),\n> +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> +\n> +\tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> \tret = sensor_->setFormat(&sensorFormat);\n> \tif (ret)\n> \t\treturn ret;\n> @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\tV4L2PixelFormat v4l2Format;\n> -\tswitch (sensorFormat.mbus_code) {\n> -\tcase MEDIA_BUS_FMT_SBGGR10_1X10:\n> -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);\n> -\t\tbreak;\n> -\tcase MEDIA_BUS_FMT_SGBRG10_1X10:\n> -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);\n> -\t\tbreak;\n> -\tcase MEDIA_BUS_FMT_SGRBG10_1X10:\n> -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);\n> -\t\tbreak;\n> -\tcase MEDIA_BUS_FMT_SRGGB10_1X10:\n> -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);\n> -\t\tbreak;\n> -\tdefault:\n> +\tconst auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);\n> +\tif (itInfo == mbusCodesToInfo.end())\n> \t\treturn -EINVAL;\n> -\t}\n>\n> -\toutputFormat->fourcc = v4l2Format;\n> +\toutputFormat->fourcc = itInfo->second;\n\nI would be tempted to suggest you to add helper functions around that\nmap, not sure it's worth the effort, but those transform to just\nretreive keys are not nice imho.\n\nThat apart\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n> \toutputFormat->size = sensorFormat.size;\n> \toutputFormat->planesCount = 1;\n>\n> --\n> 2.27.0\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 relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 586FE609A7\n\tfor ;\n\tWed, 24 Jun 2020 11:57:47 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id D3BCE1C0012;\n\tWed, 24 Jun 2020 09:57:46 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 24 Jun 2020 12:01:14 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200624100114.bnh7jxfhenfayd5o@uno.localdomain>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-7-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200623020930.1781469-7-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","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":"Wed, 24 Jun 2020 09:57:47 -0000"}},{"id":10864,"web_url":"https://patchwork.libcamera.org/comment/10864/","msgid":"<20200625213318.GA439338@oden.dyn.berto.se>","date":"2020-06-25T21:33:18","subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2020-06-25 09:56:05 +0200, Jacopo Mondi wrote:\n> Hello,\n> \n> On Thu, Jun 25, 2020 at 04:23:46AM +0300, Laurent Pinchart wrote:\n> > Hi Niklas,\n> >\n> > On Thu, Jun 25, 2020 at 02:12:59AM +0200, Niklas Söderlund wrote:\n> > > On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote:\n> > > > On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote:\n> > > > > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote:\n> > > > > > Instead of spreading the mapping between media bus codes and V4L2 FourCC\n> > > > > > all over the CIO2 code collect it in a single map and extract the data\n> > > > > > from it. This is done in preparation of adding PixelFormat information\n> > > > > > to the mix.\n> > > > > >\n> > > > > > Signed-off-by: Niklas Söderlund \n> > > > > > ---\n> > > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++---------------\n> > > > > > 1 file changed, 24 insertions(+), 26 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644\n> > > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > > > @@ -18,6 +18,17 @@ namespace libcamera {\n> > > > > >\n> > > > > > LOG_DECLARE_CATEGORY(IPU3)\n> > > > > >\n> > > > > > +namespace {\n> > > > > > +\n> > > > > > +static const std::map mbusCodesToInfo = {\n> > > > > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> > > > > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> > > > > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> > > > > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> > > > > > +};\n> > > > > > +\n> > > > > > +} /* namespace */\n> > > > > > +\n> > > > >\n> > > > > Ok, I see where this is going with the next patch, but I'm not super\n> > > > > excited by having to iterate the map just to extract keys (and anyway,\n> > > > > why doesn't std::map have a keys() function ??).\n> > > >\n> > > > Possibly because its implementation would be O(n), and STL generally\n> > > > tries to avoid hiding expensive operations behind apparently simple\n> > > > functions. Just a speculation.\n> > > >\n> \n> Could be!\n> \n> > > > > > CIO2Device::CIO2Device()\n> > > > > > \t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> > > > > > {\n> > > > > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> > > > > > \t * Make sure the sensor produces at least one format compatible with\n> > > > > > \t * the CIO2 requirements.\n> > > > > > \t */\n> > > > > > -\tconst std::set cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > > > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > > > > > -\t\t\t\t\t\tMEDIA_BUS_FMT_SRGGB10_1X10 };\n> > > > > > +\tstd::set cio2Codes;\n> > > > >\n> > > > > why a set ?\n> > > >\n> > > > To keep this efficient, given that std::map is sorted by key, this could\n> > > > be a vector, and we could call\n> > > >\n> > > > \tcio2Codes.reserve(mbusCodesToInfo.size());\n> > >\n> > > We would then also need to remember to sort the vector as that is a\n> > > requirement for utils::set_overlap() where this is used. I think it's\n> > > much nicer to use a set from the start to make sure nothing goes wrong.\n> >\n> \n> Sorry but, we're extracting keys from a sorted map with unique\n> entries. The resulting vector of keys will be sorted as well, isn't it ?\n> \n> And the requirement of being sorted depends on what you will use the\n> vector for anway, it's not something an \"extract keys\" function should\n> care about (and again, if' I'm not mistaken, the vector of keys will\n> be anyhow sorted and unique, being the result of iterating an\n> std::map)\n> \n> > I agree std::set has that benefit. Note that here we wouldn't need to\n> > sort the vector explicitly. I thus don't really see a reason to use a\n> > set in this specific case. However, maybe adding\n> >\n> > \t/*\n> > \t * utils::set_overlap() requires the containers to be sorted,\n> > \t * this is guaranteed for cio2Codes as it gets generated from a\n> > \t * sorted map with a loop that preserves the order.\n> > \t */\n> >\n> > could avoid surprises in the future.\n> >\n> > > I'm feeling a bit of resistance against set in this series. Is there a\n> > > downside to using it I don't see? I think they are neat as a\n> > > specialization of vector where we get guarantees there is only one\n> > > occurrence of each value and the readout/sorting order will be the same\n> > > no mater where the container where created. Am I missing something?\n> >\n> > std::set is (usually) implemented as a red-black tree. That will consume\n> > more memory than a vector, and for small sets, will likely be slower\n> > than creating a vector and sorting it (if needed). I'm not necessarily\n> > opposed to usage of std::set in non-hot paths, but as noted in another\n> > reply in this series, we would need to make that consistent then,\n> > passing a set to CameraSensor::getFormats(), and possibly more\n> > functions. I fear std::set would quickly spread through lots of places.\n> >\n> \n> That's my fear as well. If we have utility functions returning set,\n> callers will use set too, and indeed they're not efficient as vectors.\n> \n> > > > before the std::transform(). I think a set is indeed overkill.\n> > > >\n> > > > > > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > > > > > +\t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n> >\n> > btw shouldn't this be end() and not begin() ?\n> >\n> \n> As commented on Naush's patch, can this be handled with an\n> std::back_inserter(cio2Codes) ?\n\nIf its a vector yes, if its a set no :-) Sets does not implement \npush_back().\n\nSince the goal of this series is not to create a (in my view) better API \nfor the CameraSensor I will will use vectors for this interface in the \nnext incarnation and will thus use the back_inserter here.\n\n> \n> > > > > > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > > > >\n> > > > > you could\n> > > > > \t\t[](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; });\n> > > > > or since C++14\n> > > > > \t\t[](auto &pair){ return pair.first; });\n> > > >\n> > > > Much nicer indeed.\n> > >\n> > > Neat.\n> > >\n> > > > > > \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> > > > > > \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> > > > > > \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> > > > > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > > > > \t * Apply the selected format to the sensor, the CSI-2 receiver and\n> > > > > > \t * the CIO2 output device.\n> > > > > > \t */\n> > > > > > -\tsensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > > > > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> > > > > > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> > > > > > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > > > > > -\t\t\t\t\t size);\n> > > > > > +\tstd::vector mbusCodes;\n> > > > >\n> > > > > You could reserve space in the vector knowing the map size\n> > > >\n> > > > Seems we agree :-)\n> > > >\n> > > > > > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > > > > > +\t\tstd::back_inserter(mbusCodes),\n> > > > > > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > > > > > +\n> > > > > > +\tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> > > > > > \tret = sensor_->setFormat(&sensorFormat);\n> > > > > > \tif (ret)\n> > > > > > \t\treturn ret;\n> > > > > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > > > > \tif (ret)\n> > > > > > \t\treturn ret;\n> > > > > >\n> > > > > > -\tV4L2PixelFormat v4l2Format;\n> > > > > > -\tswitch (sensorFormat.mbus_code) {\n> > > > > > -\tcase MEDIA_BUS_FMT_SBGGR10_1X10:\n> > > > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);\n> > > > > > -\t\tbreak;\n> > > > > > -\tcase MEDIA_BUS_FMT_SGBRG10_1X10:\n> > > > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);\n> > > > > > -\t\tbreak;\n> > > > > > -\tcase MEDIA_BUS_FMT_SGRBG10_1X10:\n> > > > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);\n> > > > > > -\t\tbreak;\n> > > > > > -\tcase MEDIA_BUS_FMT_SRGGB10_1X10:\n> > > > > > -\t\tv4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);\n> > > > > > -\t\tbreak;\n> > > > > > -\tdefault:\n> > > > > > +\tconst auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code);\n> > > > > > +\tif (itInfo == mbusCodesToInfo.end())\n> > > > > > \t\treturn -EINVAL;\n> > > > > > -\t}\n> > > > > >\n> > > > > > -\toutputFormat->fourcc = v4l2Format;\n> > > > > > +\toutputFormat->fourcc = itInfo->second;\n> > > > >\n> > > > > I would be tempted to suggest you to add helper functions around that\n> > > > > map, not sure it's worth the effort, but those transform to just\n> > > > > retreive keys are not nice imho.\n> > > >\n> > > > I was about to mention that, thinking that the vector of keys could then\n> > > > be cached, but given that the code is only used in init paths, it may\n> > > > not be worth it.\n> > >\n> > > I see two options for this problem, the one used in this patch or\n> > > creating multiple data structures in the anonyms namespace, one for each\n> > > way to lookup something. The later consumes less CPU but consumes more\n> > > memory, I'm not sure which on is the best option here. I went with this\n> > > option as it prevents possible differences between the multiple data\n> > > structures for lookup. I'm open to switching to that option if that is\n> > > the consensus.\n> >\n> > If it was used in more places I'd cache the keys the first time they're\n> > used, but it's not worth it here. Same for reverse lookups, I'd create\n> > and cache a reverse map on the first call to the helper, but only if it\n> > really makes a difference.\n> >\n> > > > Reviewed-by: Laurent Pinchart \n> > > >\n> > > > > That apart\n> > > > > Reviewed-by: Jacopo Mondi \n> > > > >\n> > > > > > \toutputFormat->size = sensorFormat.size;\n> > > > > > \toutputFormat->planesCount = 1;\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 C32E1C0109\n\tfor ;\n\tThu, 25 Jun 2020 21:33:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B20D609C7;\n\tThu, 25 Jun 2020 23:33:22 +0200 (CEST)","from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C127609A5\n\tfor ;\n\tThu, 25 Jun 2020 23:33:21 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id e4so8168013ljn.4\n\tfor ;\n\tThu, 25 Jun 2020 14:33:21 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ty69sm5716883lfa.86.2020.06.25.14.33.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 25 Jun 2020 14:33:19 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Mtbylj4g\"; dkim-atps=neutral","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\tbh=p5LTFpnUm4yR8erawhuvnAl7q9+CDzOwdR8xNlr96Mg=;\n\tb=Mtbylj4g+MOS4k97uHWz6HQsJTkW1gcggWD8uQ8KXAU5+VXmdI7LIAOKiy0Hv+Xt9U\n\t9MFlDmWaG9o3gFG8TPbUtT9+fXnZlpem/KIHAvUiN1CFwW7NFNOEuZapTmWNARYt5G5A\n\tGSLADT44QejB/CjINBDgi52FZfsFAlOx2TdhwHLJuK3Or1a+xaeODqtY3xZOjEwvs2HO\n\t8asP6/eNpj+lIxnx6M9C/iOHKeoxIXWEQoCMh3qdwBo6Rfm37OK7Mbu53A+S3/aFMHyU\n\t0wDQCBpHhtFGY9TIY5oflj5X2dgZobB3UQaNMTuGqQ08fgsjtL1Lx3AMBF4SMfmFyzKS\n\twJXg==","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;\n\tbh=p5LTFpnUm4yR8erawhuvnAl7q9+CDzOwdR8xNlr96Mg=;\n\tb=rIGIXLNJDmAMIexCR4/psnAyqDZaCbGjg5dxbfn4u06y5zuOzrpWba28cENvXekIE+\n\td39FP8G6U/LllzrlpG47oQe8yZxU3YolRIOeNS0UwhHfdBeGgWprQIATUnt0j9MBcqpX\n\tt7uEVgqy8ZVpjY8wGhgOD0y9XGo9YpuFwYXKbdZGMkDiGK8MQwfrdFUlMg4HWg8M3oS8\n\tJVdDDOOKqZj1Zu+PTJTAT8bPjTbdA+3VDyC8+pDZHth52vwZDgloJezOGrioJlVV0Lb+\n\tx/MMnayeRnufmjHWA/db+CN1br8zPoYnMDId7CgKEPAGFWGcjv1m+zzDZYiSjl0D1KFF\n\tJt/Q==","X-Gm-Message-State":"AOAM531+Jz3Bkz511QRjJrjXvARXeG4aGSzPHZ+isAS59X3ciGLGrZgR\n\tGVOUnH/hGE2uZRp9gbs1+nExRQ==","X-Google-Smtp-Source":"ABdhPJz6wJrR6sO9ahi8c0HrLRuerlpwpqXufj7AcT/vQVeTAZx6cEQzabWeitb7EYPE12GOCrjY4Q==","X-Received":"by 2002:a2e:7219:: with SMTP id\n\tn25mr18537903ljc.164.1593120800313; \n\tThu, 25 Jun 2020 14:33:20 -0700 (PDT)","Date":"Thu, 25 Jun 2020 23:33:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200625213318.GA439338@oden.dyn.berto.se>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-7-niklas.soderlund@ragnatech.se>\n\t<20200624100114.bnh7jxfhenfayd5o@uno.localdomain>\n\t<20200624235722.GO5980@pendragon.ideasonboard.com>\n\t<20200625001259.GB10256@oden.dyn.berto.se>\n\t<20200625012346.GA1875@pendragon.ideasonboard.com>\n\t<20200625075605.o6ggeb6cyb7snjko@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200625075605.o6ggeb6cyb7snjko@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 06/10] libcamera: ipu3: cio2:\n\tConsolidate information about formats","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]