[{"id":14034,"web_url":"https://patchwork.libcamera.org/comment/14034/","msgid":"<20201202171658.44d2tygvsxdo422g@uno.localdomain>","date":"2020-12-02T17:16:58","subject":"Re: [libcamera-devel] [RFC PATCH v2] android: camera_device:\n\tReorder configurations before request","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello Hiro,\n\nOn Tue, Dec 01, 2020 at 01:25:14PM +0900, Hirokazu Honda wrote:\n> This reorders configurations before calling\n> CameraConfiguration::validate() so that the streams requested\n> by an android HAL client can be achieved more likely.\n\nThis patch introduces a new intermediate type, which associate a\ncamera3_stream_t * with a libcamera::StreamConfiguration. I would\nrecord it in the commit message, something like:\n\nRe-order StreamConfiguration in the CameraConfiguration before\nvalidating it to make it easier for the Camera to satisfy the\nAndroid framework request.\n\nIntroduce a new Camera3StreamsToConfig type to associate\ncamera3_stream with StreamConfiguration and sort them before\nusing them to populate the CameraDevice::streams_ vector.\n\nTo be honest I would split this patch to make its consumption easier:\n1) Introduce the new type\n2) Collect the vector and create streams_ from it (this should be\n functionally equivalent to the existing 'unsorted' implementation\n afaict)\n3) Sort the vector before creating streams_\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 158 +++++++++++++++++++++++++++++-----\n> 1 file changed, 137 insertions(+), 21 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 4690346e..3de5a9f1 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,9 +9,11 @@\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n>\n> +#include \n> #include \n> #include \n> #include \n> +#include \n\n'o' comes before 's'\n\nActually utils/checkstyle.py reports several style issues (some false\npositives as well). I'll point out what I can spot but please re-check\nwith it.\n\n>\n> #include \n> #include \n> @@ -128,6 +130,107 @@ const std::map camera3FormatsMap = {\n> \t},\n> };\n>\n> +struct Camera3StreamsToConfig {\n\nCan you shorten names a bit ?\nI was about to suggest StreamConfig, but we have StreamConfiguration\nalready. Camera3Stream ? But we have CameraStream... Ok, keep the long\nname :) Maybe Camera3StreamConfig ?\n\n\n> +\tstd::vector streams; // Destination(s).\n\nWe usually use: \"typename *\" and \"typename &\"\n\n> +\tstd::vector types; // CameraStream::Type(s).\n> +\tStreamConfiguration config; // StreamConfiguration requested to a native camera.\n\nNo C++ comments please.\n\n> +};\n> +\n> +/*\n> + * Reorder the configurations so that CameraDevice can accept them as much as\n> + * possible.\n> + */\n\nNice! Can you provide a brief description of the sorting criteria as\nwell ?\n\n> +std::vector createdSortedCamera3StreamsToConfigs(\n\nmaybe just 'sortStreamConfigs' ?\n\nCan you move this function before its only user ?\n\n> +\tstd::vector unsortedStreamsToConfigs,\n\nconst & ?\nAh I see you move the vector here...\n\n> +\tconst camera3_stream_t *jpegStream) {\n> +\tconst size_t unsortedStreamsToConfigsSize = unsortedStreamsToConfigs.size();\n> +\tstd::optional streamsToConfigForJpeg = std::nullopt;\n> +\tif (jpegStream) {\n> +\t\tfor (auto it = unsortedStreamsToConfigs.begin();\n> +\t\t it != unsortedStreamsToConfigs.end(); it++) {\n\n for (const auto &it : unsortedStreamsToConfigs) ?\n\n> +\t\t\tconst auto& streams = it->streams;\n> +\t\t\tif (std::find(streams.begin(), streams.end(),\n> +\t\t\t\t jpegStream) != streams.end()) {\n> +\t\t\t\tstreamsToConfigForJpeg = *it;\n> +\t\t\t\tunsortedStreamsToConfigs.erase(it);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tassert(streamsToConfigForJpeg.has_value());\n\nLOG(Fatal) would make the error verbose\n\n> +\t}\n> +\n> +\tstd::map> formatToStreamsToConfigs;\n\nIt's internal stuff, just 'formatToConfig' or even 'formats' would\nmake this more readable.\n\n> +\tfor (const auto &streamsToConfig : unsortedStreamsToConfigs) {\n> +\t\tconst StreamConfiguration &config = streamsToConfig.config;\n> +\t\tformatToStreamsToConfigs[config.pixelFormat].push_back(streamsToConfig);\n> +\n> +\t}\n> +\tfor (auto& [format, streamsToConfigs] : formatToStreamsToConfigs) {\n\nauto &\nthe [] pair syntax is much nicer than having to use it.first, it.second!\n\n> +\t\t/* Sorted by resolution. Smaller is put first. */\n\nSize has an ordering defined, have you considered using it ?\n\n> +\t\tstd::sort(streamsToConfigs.begin(), streamsToConfigs.end(),\n> +\t\t\t [](const auto &streamsToConfigA, const auto &streamsToConfigB) {\n> +\t\t\t\t const Size &sizeA = streamsToConfigA.config.size;\n> +\t\t\t\t const Size &sizeB = streamsToConfigA.config.size;\n> +\t\t\t\t if (sizeA.width != sizeB.width)\n> +\t\t\t\t\t return sizeA.width < sizeB.width;\n> +\t\t\t\t return sizeA.height < sizeB.height;\n> +\t\t\t });\n> +\t}\n> +\n> +\tstd::vector sortedStreamsToConfigs;\n\nMaybe reserve space in all intermediate vectors to avoid relocations ?\nAlthough numbers should be small..\n\n> +\t/*\n> +\t * NV12 is the most prioritized format. Put the configuration with NV12\n> +\t * and the largest resolution first.\n> +\t */\n> +\tif (formatToStreamsToConfigs.find(formats::NV12) != formatToStreamsToConfigs.end()) {\n> +\t\tauto& nv12StreamsToConfigs = formatToStreamsToConfigs[formats::NV12];\n\n auto &nv12StreamsToConfigs\n\nCan the double lookup be avoided with:\n const auto nv12Stream = formatToStreamsToConfigs.find(formats::NV12)\n if (nv12Stream != formatToStreamsToConfigs.end())\n\n> +\t\tconst Size& nv12LargestSize = nv12StreamsToConfigs.back().config.size;\n\n const Size &nv12LargestSize\n\n> +\t\tif (streamsToConfigForJpeg &&\n> +\t\t streamsToConfigForJpeg->config.pixelFormat == formats::NV12) {\n> +\t\t\tconst Size& nv12SizeForJpeg = streamsToConfigForJpeg->config.size;\n\nCan you move it after the comment block ?\n\n> +\t\t\t/*\n> +\t\t\t * If JPEG will be created from NV12 and the size is\n> +\t\t\t * larger than the largest NV12 remained configurations,\n\nDidn't get 'remained'\n'configuration'\n> +\t\t\t * then put the NV12 configuration for JPEG first.\n ^ rogue space\n> +\t\t\t */\n\n> +\t\t\tif (nv12LargestSize.width < nv12SizeForJpeg.width &&\n> +\t\t\t nv12LargestSize.height < nv12SizeForJpeg.height) {\n> +\t\t\t\tsortedStreamsToConfigs.push_back(*streamsToConfigForJpeg);\n> +\t\t\t\tstreamsToConfigForJpeg = std::nullopt;\n> +\t\t\t}\n> +\t\t}\n> +\t\tsortedStreamsToConfigs.push_back(nv12StreamsToConfigs.back());\n> +\t\tnv12StreamsToConfigs.pop_back();\n> +\t}\n> +\n> +\t/*\n> +\t * If the configuration for JPEG is there, then put it.\n> +\t */\n\nFits on one line\n\n> +\tif (streamsToConfigForJpeg) {\n> +\t\tsortedStreamsToConfigs.push_back(*streamsToConfigForJpeg);\n> +\t\tstreamsToConfigForJpeg = std::nullopt;\n> +\t}\n> +\n> +\t/*\n> +\t * Put configurations with different formats and larger resolutions\n> +\t * earlier.\n> +\t */\n> +\twhile (!formatToStreamsToConfigs.empty()) {\n> +\t\tfor (auto it = formatToStreamsToConfigs.begin();\n> +\t\t it != formatToStreamsToConfigs.end();) {\n> +\t\t\tauto& streamsToConfigs = it->second;\n\n auto &streamsToConfigs\n\n> +\t\t\tif (streamsToConfigs.empty()) {\n> +\t\t\t\tit = formatToStreamsToConfigs.erase(it);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nCan't this loop be expressed as:\n\n for (const auto &format : formatToStreamsToConfigs) {\n for (const auto stream = format.second.rbegin();\n stream != format.second.rend(); ++stream)\n\t\t\tsortedStreamsToConfigs.push_back(stream);\n }\n\nFeels more readable to me (not compiled, just an idea)\n\nOr do you need to erase elements while you loop them ?\n\n> +\t\t\tsortedStreamsToConfigs.push_back(streamsToConfigs.back());\n> +\t\t\tstreamsToConfigs.pop_back();\n> +\t\t}\n> +\t}\n> +\tassert(sortedStreamsToConfigs.size() == unsortedStreamsToConfigsSize);\n\nOne empty line before return ?\n\n> +\treturn sortedStreamsToConfigs;\n> +}\n\nI think isolating the sorting algorithm in one patch would make it\neasier to evaluate it in isolation along with the validate()\nmodification you are working on!\n\n> +\n> } /* namespace */\n>\n> LOG_DECLARE_CATEGORY(HAL)\n> @@ -1225,6 +1328,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \tstreams_.clear();\n> \tstreams_.reserve(stream_list->num_streams);\n>\n> +\tstd::vector streamsToConfigs;\n> +\tstreamsToConfigs.reserve(stream_list->num_streams);\n> +\n> \t/* First handle all non-MJPEG streams. */\n> \tcamera3_stream_t *jpegStream = nullptr;\n> \tfor (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> @@ -1255,14 +1361,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\t\tcontinue;\n> \t\t}\n>\n> -\t\tStreamConfiguration streamConfiguration;\n> -\t\tstreamConfiguration.size = size;\n> -\t\tstreamConfiguration.pixelFormat = format;\n> -\n> -\t\tconfig_->addConfiguration(streamConfiguration);\n> -\t\tstreams_.emplace_back(this, CameraStream::Type::Direct,\n> -\t\t\t\t stream, config_->size() - 1);\n> -\t\tstream->priv = static_cast(&streams_.back());\n> +\t\tCamera3StreamsToConfig streamsToConfig;\n> +\t\tstreamsToConfig.streams = {stream};\n\n { stream };\n\n> +\t\tstreamsToConfig.types = {CameraStream::Type::Direct};\n\n { CameraStream::Type::Direct };\n\n> +\t\tstreamsToConfig.config.size = size;\n> +\t\tstreamsToConfig.config.pixelFormat = format;\n> +\t\tstreamsToConfigs.push_back(std::move(streamsToConfig));\n> \t}\n>\n> \t/* Now handle the MJPEG streams, adding a new stream if required. */\n> @@ -1271,9 +1375,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\tint index = -1;\n>\n> \t\t/* Search for a compatible stream in the non-JPEG ones. */\n> -\t\tfor (unsigned int i = 0; i < config_->size(); i++) {\n> -\t\t\tStreamConfiguration &cfg = config_->at(i);\n> -\n> +\t\tfor (size_t i = 0; i < streamsToConfigs.size(); ++i) {\n> +\t\t\tconst auto& cfg = streamsToConfigs[i].config;\n\n const auto &cfg\n\n> \t\t\t/*\n> \t\t\t * \\todo The PixelFormat must also be compatible with\n> \t\t\t * the encoder.\n> @@ -1295,28 +1398,41 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\t * introduce a new stream to satisfy the request requirements.\n> \t\t */\n> \t\tif (index < 0) {\n> -\t\t\tStreamConfiguration streamConfiguration;\n> -\n> +\t\t\tCamera3StreamsToConfig streamsToConfig;\n\nI would keep the empty line or move it below the comment block.\n\n> \t\t\t/*\n> \t\t\t * \\todo The pixelFormat should be a 'best-fit' choice\n> \t\t\t * and may require a validation cycle. This is not yet\n> \t\t\t * handled, and should be considered as part of any\n> \t\t\t * stream configuration reworks.\n> \t\t\t */\n> -\t\t\tstreamConfiguration.size.width = jpegStream->width;\n> -\t\t\tstreamConfiguration.size.height = jpegStream->height;\n> -\t\t\tstreamConfiguration.pixelFormat = formats::NV12;\n> +\t\t\tstreamsToConfig.config.size.width = jpegStream->width;\n> +\t\t\tstreamsToConfig.config.size.height = jpegStream->height;\n> +\t\t\tstreamsToConfig.config.pixelFormat = formats::NV12;\n> +\t\t\tstreamsToConfigs.push_back(std::move(streamsToConfig));\n>\n> -\t\t\tLOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> +\t\t\tLOG(HAL, Info) << \"Adding \" << streamsToConfig.config.toString()\n> \t\t\t\t << \" for MJPEG support\";\n>\n> \t\t\ttype = CameraStream::Type::Internal;\n> -\t\t\tconfig_->addConfiguration(streamConfiguration);\n> -\t\t\tindex = config_->size() - 1;\n> +\t\t\tindex = streamsToConfigs.size() - 1;\n> \t\t}\n>\n> -\t\tstreams_.emplace_back(this, type, jpegStream, index);\n> -\t\tjpegStream->priv = static_cast(&streams_.back());\n> +\t\tstreamsToConfigs[index].streams.push_back(jpegStream);\n> +\t\tstreamsToConfigs[index].types.push_back(type);\n> +\t}\n> +\n> +\tstreamsToConfigs =\n> +\t\tcreatedSortedCamera3StreamsToConfigs(std::move(streamsToConfigs),\n> +\t\t\t\t\t\t jpegStream);\n\nI'm sure we can find a shorter name for this function :)\n\n> +\tfor (auto& streamsToConfig : streamsToConfigs) {\n\n const auto &streamsToConfig\n\n> +\t\tconfig_->addConfiguration(streamsToConfig.config);\n> +\t\tfor (size_t i = 0; i < streamsToConfig.streams.size(); ++i) {\n> +\t\t\tauto* stream = streamsToConfig.streams[i];\n\n auto *stream\n\nWhen the type is 'trivial' please spell it in full\n\n> +\t\t\tconst CameraStream::Type type = streamsToConfig.types[i];\n> +\t\t\tstreams_.emplace_back(this, type,\n> +\t\t\t\t\t stream, config_->size() - 1);\n> +\t\t\tstream->priv = static_cast(&streams_.back());\n> +\t\t}\n\nStyle and a few minor issues apart, I think this is mostly ok. I tried\nto think how CameraStream can be modified not to introduce a new type,\nbut what we're actually sorting are the StreamConfiguration, so a new\ntype is probably the right thing to do.\n\nThanks\n j\n\n> \t}\n>\n> \tswitch (config_->validate()) {\n> --\n> 2.29.2.454.gaff20da3a2-goog","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 72593BE177\n\tfor ;\n\tWed, 2 Dec 2020 17:16:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2757635A5;\n\tWed, 2 Dec 2020 18:16:52 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63FFE634A2\n\tfor ;\n\tWed, 2 Dec 2020 18:16:52 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id D8743C0006;\n\tWed, 2 Dec 2020 17:16:51 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 2 Dec 2020 18:16:58 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20201202171658.44d2tygvsxdo422g@uno.localdomain>","References":"<20201201042514.3209146-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201201042514.3209146-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v2] android: camera_device:\n\tReorder configurations before request","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14094,"web_url":"https://patchwork.libcamera.org/comment/14094/","msgid":"<20201207100232.nf2gpdd6trdd3zij@uno.localdomain>","date":"2020-12-07T10:02:32","subject":"Re: [libcamera-devel] [RFC PATCH v2] android: camera_device:\n\tReorder configurations before request","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Dec 07, 2020 at 05:47:27PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo,\n>\n> On Thu, Dec 3, 2020 at 2:16 AM Jacopo Mondi wrote:\n> >\n> > Hello Hiro,\n> >\n> > On Tue, Dec 01, 2020 at 01:25:14PM +0900, Hirokazu Honda wrote:\n> > > This reorders configurations before calling\n> > > CameraConfiguration::validate() so that the streams requested\n> > > by an android HAL client can be achieved more likely.\n> >\n> > This patch introduces a new intermediate type, which associate a\n> > camera3_stream_t * with a libcamera::StreamConfiguration. I would\n> > record it in the commit message, something like:\n> >\n> > Re-order StreamConfiguration in the CameraConfiguration before\n> > validating it to make it easier for the Camera to satisfy the\n> > Android framework request.\n> >\n> > Introduce a new Camera3StreamsToConfig type to associate\n> > camera3_stream with StreamConfiguration and sort them before\n> > using them to populate the CameraDevice::streams_ vector.\n> >\n> > To be honest I would split this patch to make its consumption easier:\n> > 1) Introduce the new type\n> > 2) Collect the vector and create streams_ from it (this should be\n> > functionally equivalent to the existing 'unsorted' implementation\n> > afaict)\n> > 3) Sort the vector before creating streams_\n> >\n>\n> Sure, I splitted.\n>\n\nThanks I'll review soon\n\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 158 +++++++++++++++++++++++++++++-----\n> > > 1 file changed, 137 insertions(+), 21 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 4690346e..3de5a9f1 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -9,9 +9,11 @@\n> > > #include \"camera_ops.h\"\n> > > #include \"post_processor.h\"\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> >\n> > 'o' comes before 's'\n> >\n> > Actually utils/checkstyle.py reports several style issues (some false\n> > positives as well). I'll point out what I can spot but please re-check\n> > with it.\n> >\n> > >\n> > > #include \n> > > #include \n> > > @@ -128,6 +130,107 @@ const std::map camera3FormatsMap = {\n> > > },\n> > > };\n> > >\n> > > +struct Camera3StreamsToConfig {\n> >\n> > Can you shorten names a bit ?\n> > I was about to suggest StreamConfig, but we have StreamConfiguration\n> > already. Camera3Stream ? But we have CameraStream... Ok, keep the long\n> > name :) Maybe Camera3StreamConfig ?\n> >\n> >\n> > > + std::vector streams; // Destination(s).\n> >\n> > We usually use: \"typename *\" and \"typename &\"\n> >\n> > > + std::vector types; // CameraStream::Type(s).\n> > > + StreamConfiguration config; // StreamConfiguration requested to a native camera.\n> >\n> > No C++ comments please.\n> >\n> > > +};\n> > > +\n> > > +/*\n> > > + * Reorder the configurations so that CameraDevice can accept them as much as\n> > > + * possible.\n> > > + */\n> >\n> > Nice! Can you provide a brief description of the sorting criteria as\n> > well ?\n> >\n> > > +std::vector createdSortedCamera3StreamsToConfigs(\n> >\n> > maybe just 'sortStreamConfigs' ?\n> >\n> > Can you move this function before its only user ?\n> >\n> > > + std::vector unsortedStreamsToConfigs,\n> >\n> > const & ?\n> > Ah I see you move the vector here...\n> >\n> > > + const camera3_stream_t *jpegStream) {\n> > > + const size_t unsortedStreamsToConfigsSize = unsortedStreamsToConfigs.size();\n> > > + std::optional streamsToConfigForJpeg = std::nullopt;\n> > > + if (jpegStream) {\n> > > + for (auto it = unsortedStreamsToConfigs.begin();\n> > > + it != unsortedStreamsToConfigs.end(); it++) {\n> >\n> > for (const auto &it : unsortedStreamsToConfigs) ?\n> >\n> > > + const auto& streams = it->streams;\n> > > + if (std::find(streams.begin(), streams.end(),\n> > > + jpegStream) != streams.end()) {\n> > > + streamsToConfigForJpeg = *it;\n> > > + unsortedStreamsToConfigs.erase(it);\n> > > + break;\n> > > + }\n> > > + }\n> > > + assert(streamsToConfigForJpeg.has_value());\n> >\n> > LOG(Fatal) would make the error verbose\n> >\n> > > + }\n> > > +\n> > > + std::map> formatToStreamsToConfigs;\n> >\n> > It's internal stuff, just 'formatToConfig' or even 'formats' would\n> > make this more readable.\n> >\n> > > + for (const auto &streamsToConfig : unsortedStreamsToConfigs) {\n> > > + const StreamConfiguration &config = streamsToConfig.config;\n> > > + formatToStreamsToConfigs[config.pixelFormat].push_back(streamsToConfig);\n> > > +\n> > > + }\n> > > + for (auto& [format, streamsToConfigs] : formatToStreamsToConfigs) {\n> >\n> > auto &\n> > the [] pair syntax is much nicer than having to use it.first, it.second!\n> >\n> > > + /* Sorted by resolution. Smaller is put first. */\n> >\n> > Size has an ordering defined, have you considered using it ?\n> >\n> > > + std::sort(streamsToConfigs.begin(), streamsToConfigs.end(),\n> > > + [](const auto &streamsToConfigA, const auto &streamsToConfigB) {\n> > > + const Size &sizeA = streamsToConfigA.config.size;\n> > > + const Size &sizeB = streamsToConfigA.config.size;\n> > > + if (sizeA.width != sizeB.width)\n> > > + return sizeA.width < sizeB.width;\n> > > + return sizeA.height < sizeB.height;\n> > > + });\n> > > + }\n> > > +\n> > > + std::vector sortedStreamsToConfigs;\n> >\n> > Maybe reserve space in all intermediate vectors to avoid relocations ?\n> > Although numbers should be small..\n> >\n> > > + /*\n> > > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > > + * and the largest resolution first.\n> > > + */\n> > > + if (formatToStreamsToConfigs.find(formats::NV12) != formatToStreamsToConfigs.end()) {\n> > > + auto& nv12StreamsToConfigs = formatToStreamsToConfigs[formats::NV12];\n> >\n> > auto &nv12StreamsToConfigs\n> >\n> > Can the double lookup be avoided with:\n> > const auto nv12Stream = formatToStreamsToConfigs.find(formats::NV12)\n> > if (nv12Stream != formatToStreamsToConfigs.end())\n> >\n> > > + const Size& nv12LargestSize = nv12StreamsToConfigs.back().config.size;\n> >\n> > const Size &nv12LargestSize\n> >\n> > > + if (streamsToConfigForJpeg &&\n> > > + streamsToConfigForJpeg->config.pixelFormat == formats::NV12) {\n> > > + const Size& nv12SizeForJpeg = streamsToConfigForJpeg->config.size;\n> >\n> > Can you move it after the comment block ?\n> >\n> > > + /*\n> > > + * If JPEG will be created from NV12 and the size is\n> > > + * larger than the largest NV12 remained configurations,\n> >\n> > Didn't get 'remained'\n> > 'configuration'\n> > > + * then put the NV12 configuration for JPEG first.\n> > ^ rogue space\n> > > + */\n> >\n> > > + if (nv12LargestSize.width < nv12SizeForJpeg.width &&\n> > > + nv12LargestSize.height < nv12SizeForJpeg.height) {\n> > > + sortedStreamsToConfigs.push_back(*streamsToConfigForJpeg);\n> > > + streamsToConfigForJpeg = std::nullopt;\n> > > + }\n> > > + }\n> > > + sortedStreamsToConfigs.push_back(nv12StreamsToConfigs.back());\n> > > + nv12StreamsToConfigs.pop_back();\n> > > + }\n> > > +\n> > > + /*\n> > > + * If the configuration for JPEG is there, then put it.\n> > > + */\n> >\n> > Fits on one line\n> >\n> > > + if (streamsToConfigForJpeg) {\n> > > + sortedStreamsToConfigs.push_back(*streamsToConfigForJpeg);\n> > > + streamsToConfigForJpeg = std::nullopt;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Put configurations with different formats and larger resolutions\n> > > + * earlier.\n> > > + */\n> > > + while (!formatToStreamsToConfigs.empty()) {\n> > > + for (auto it = formatToStreamsToConfigs.begin();\n> > > + it != formatToStreamsToConfigs.end();) {\n> > > + auto& streamsToConfigs = it->second;\n> >\n> > auto &streamsToConfigs\n> >\n> > > + if (streamsToConfigs.empty()) {\n> > > + it = formatToStreamsToConfigs.erase(it);\n> > > + continue;\n> > > + }\n> >\n> > Can't this loop be expressed as:\n> >\n> > for (const auto &format : formatToStreamsToConfigs) {\n> > for (const auto stream = format.second.rbegin();\n> > stream != format.second.rend(); ++stream)\n> > sortedStreamsToConfigs.push_back(stream);\n> > }\n> >\n> > Feels more readable to me (not compiled, just an idea)\n> >\n> > Or do you need to erase elements while you loop them ?\n> >\n>\n> The while-loop is needed to assort formats.\n> In your way, the configurations with the same format are put earlier,\n> e.g., nv12, nv12, yv12, yv12,.. rather than nv12, yv12, nv12, yv12...\n>\n\nI'm sorry, I don't want to be insistent and I'm surely reading some\npiece wrong, but if I iterate your implementation on the following\nexample map (afaict the vector of configs is sorted by size, smaller\ngoes first))\n\nmap = { {NV12: {VGA, XGA},}, {YV12: {QVGA, VGA}} }\n\nThe pseudo-code of the implementation I read looks like:\n\n while (!map.empty()) {\n for (it = map.begin(); it != map.end()) {\n vector &v = it.second;\n\n if (v.empty())\n it = map.erase(it);\n continue;\n\n sorted.push_back(v.back());\n v.pop_back();\n }\n }\n\nAnd if I manually iterate it on the map I get:\n\nmap = { {NV12: {VGA, XGA},}, {YV12: {QVGA, VGA}} }\n0:\n it = { NV12, {VGA, XGA}}\n v = { VGA, XGA }\n sorted = { [NV12,XGA] }\n v = { VGA }\n\n1:\n it = { NV12, { VGA }}\n v = { VGA }\n sorted = { [NV12,XGA], [NV12, VGA] }\n v = {}\n\n2:\n it = {NV12, {}}\n v = {} {\n map = { {YV12: {QVGA, VGA}} }\n it = {YV12: {QVGA, VGA}}\n }\n\n3:\n it = {YV12: {QVGA, VGA}}\n v = { QVGA, VGA }\n sorted = { [NV12,XGA], [NV12, VGA], [YV12, VGA] }\n v = { QVGA }\n\n4:\n it = {YV12: {QVGA}}\n v = { QVGA }\n sorted = { [NV12,XGA], [NV12, VGA], [YV12, VGA], [YV12, VGA]}\n v = { }\n\n5:\n it = {Y12: {}}\n v = {} {\n map = {}\n it = map.end();\n }\n\n\nsorted = { [NV12,XGA], [NV12, VGA], [YV12, VGA], [YV12, VGA]}\n\nWhich seems to suggest the same result as the two nested for loops\nI've proposed. What am I missing ?\n\nThanks\n j\n\n> Best Regards,\n> -Hiro\n>\n>\n>\n> > > + sortedStreamsToConfigs.push_back(streamsToConfigs.back());\n> > > + streamsToConfigs.pop_back();\n> > > + }\n> > > + }\n> > > + assert(sortedStreamsToConfigs.size() == unsortedStreamsToConfigsSize);\n> >\n> > One empty line before return ?\n> >\n> > > + return sortedStreamsToConfigs;\n> > > +}\n> >\n> > I think isolating the sorting algorithm in one patch would make it\n> > easier to evaluate it in isolation along with the validate()\n> > modification you are working on!\n> >\n> > > +\n> > > } /* namespace */\n> > >\n> > > LOG_DECLARE_CATEGORY(HAL)\n> > > @@ -1225,6 +1328,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > streams_.clear();\n> > > streams_.reserve(stream_list->num_streams);\n> > >\n> > > + std::vector streamsToConfigs;\n> > > + streamsToConfigs.reserve(stream_list->num_streams);\n> > > +\n> > > /* First handle all non-MJPEG streams. */\n> > > camera3_stream_t *jpegStream = nullptr;\n> > > for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > > @@ -1255,14 +1361,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > continue;\n> > > }\n> > >\n> > > - StreamConfiguration streamConfiguration;\n> > > - streamConfiguration.size = size;\n> > > - streamConfiguration.pixelFormat = format;\n> > > -\n> > > - config_->addConfiguration(streamConfiguration);\n> > > - streams_.emplace_back(this, CameraStream::Type::Direct,\n> > > - stream, config_->size() - 1);\n> > > - stream->priv = static_cast(&streams_.back());\n> > > + Camera3StreamsToConfig streamsToConfig;\n> > > + streamsToConfig.streams = {stream};\n> >\n> > { stream };\n> >\n> > > + streamsToConfig.types = {CameraStream::Type::Direct};\n> >\n> > { CameraStream::Type::Direct };\n> >\n> > > + streamsToConfig.config.size = size;\n> > > + streamsToConfig.config.pixelFormat = format;\n> > > + streamsToConfigs.push_back(std::move(streamsToConfig));\n> > > }\n> > >\n> > > /* Now handle the MJPEG streams, adding a new stream if required. */\n> > > @@ -1271,9 +1375,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > int index = -1;\n> > >\n> > > /* Search for a compatible stream in the non-JPEG ones. */\n> > > - for (unsigned int i = 0; i < config_->size(); i++) {\n> > > - StreamConfiguration &cfg = config_->at(i);\n> > > -\n> > > + for (size_t i = 0; i < streamsToConfigs.size(); ++i) {\n> > > + const auto& cfg = streamsToConfigs[i].config;\n> >\n> > const auto &cfg\n> >\n> > > /*\n> > > * \\todo The PixelFormat must also be compatible with\n> > > * the encoder.\n> > > @@ -1295,28 +1398,41 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > * introduce a new stream to satisfy the request requirements.\n> > > */\n> > > if (index < 0) {\n> > > - StreamConfiguration streamConfiguration;\n> > > -\n> > > + Camera3StreamsToConfig streamsToConfig;\n> >\n> > I would keep the empty line or move it below the comment block.\n> >\n> > > /*\n> > > * \\todo The pixelFormat should be a 'best-fit' choice\n> > > * and may require a validation cycle. This is not yet\n> > > * handled, and should be considered as part of any\n> > > * stream configuration reworks.\n> > > */\n> > > - streamConfiguration.size.width = jpegStream->width;\n> > > - streamConfiguration.size.height = jpegStream->height;\n> > > - streamConfiguration.pixelFormat = formats::NV12;\n> > > + streamsToConfig.config.size.width = jpegStream->width;\n> > > + streamsToConfig.config.size.height = jpegStream->height;\n> > > + streamsToConfig.config.pixelFormat = formats::NV12;\n> > > + streamsToConfigs.push_back(std::move(streamsToConfig));\n> > >\n> > > - LOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> > > + LOG(HAL, Info) << \"Adding \" << streamsToConfig.config.toString()\n> > > << \" for MJPEG support\";\n> > >\n> > > type = CameraStream::Type::Internal;\n> > > - config_->addConfiguration(streamConfiguration);\n> > > - index = config_->size() - 1;\n> > > + index = streamsToConfigs.size() - 1;\n> > > }\n> > >\n> > > - streams_.emplace_back(this, type, jpegStream, index);\n> > > - jpegStream->priv = static_cast(&streams_.back());\n> > > + streamsToConfigs[index].streams.push_back(jpegStream);\n> > > + streamsToConfigs[index].types.push_back(type);\n> > > + }\n> > > +\n> > > + streamsToConfigs =\n> > > + createdSortedCamera3StreamsToConfigs(std::move(streamsToConfigs),\n> > > + jpegStream);\n> >\n> > I'm sure we can find a shorter name for this function :)\n> >\n> > > + for (auto& streamsToConfig : streamsToConfigs) {\n> >\n> > const auto &streamsToConfig\n> >\n> > > + config_->addConfiguration(streamsToConfig.config);\n> > > + for (size_t i = 0; i < streamsToConfig.streams.size(); ++i) {\n> > > + auto* stream = streamsToConfig.streams[i];\n> >\n> > auto *stream\n> >\n> > When the type is 'trivial' please spell it in full\n> >\n> > > + const CameraStream::Type type = streamsToConfig.types[i];\n> > > + streams_.emplace_back(this, type,\n> > > + stream, config_->size() - 1);\n> > > + stream->priv = static_cast(&streams_.back());\n> > > + }\n> >\n> > Style and a few minor issues apart, I think this is mostly ok. I tried\n> > to think how CameraStream can be modified not to introduce a new type,\n> > but what we're actually sorting are the StreamConfiguration, so a new\n> > type is probably the right thing to do.\n> >\n> > Thanks\n> > j\n> >\n> > > }\n> > >\n> > > switch (config_->validate()) {\n> > > --\n> > > 2.29.2.454.gaff20da3a2-goog","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 0500CBDB20\n\tfor ;\n\tMon, 7 Dec 2020 10:21:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 83E6C67E14;\n\tMon, 7 Dec 2020 11:21:09 +0100 (CET)","from mslow2.mail.gandi.net (mslow2.mail.gandi.net [217.70.178.242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CECB67E12\n\tfor ;\n\tMon, 7 Dec 2020 11:21:07 +0100 (CET)","from relay7-d.mail.gandi.net (unknown [217.70.183.200])\n\tby mslow2.mail.gandi.net (Postfix) with ESMTP id D112B3A1DFC\n\tfor ;\n\tMon, 7 Dec 2020 10:02:44 +0000 (UTC)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 5CD132000C;\n\tMon, 7 Dec 2020 10:02:24 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 7 Dec 2020 11:02:32 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20201207100232.nf2gpdd6trdd3zij@uno.localdomain>","References":"<20201201042514.3209146-1-hiroh@chromium.org>\n\t<20201202171658.44d2tygvsxdo422g@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2] android: camera_device:\n\tReorder configurations before request","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14095,"web_url":"https://patchwork.libcamera.org/comment/14095/","msgid":"","date":"2020-12-07T08:47:27","subject":"Re: [libcamera-devel] [RFC PATCH v2] android: camera_device:\n\tReorder configurations before request","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Dec 3, 2020 at 2:16 AM Jacopo Mondi wrote:\n>\n> Hello Hiro,\n>\n> On Tue, Dec 01, 2020 at 01:25:14PM +0900, Hirokazu Honda wrote:\n> > This reorders configurations before calling\n> > CameraConfiguration::validate() so that the streams requested\n> > by an android HAL client can be achieved more likely.\n>\n> This patch introduces a new intermediate type, which associate a\n> camera3_stream_t * with a libcamera::StreamConfiguration. I would\n> record it in the commit message, something like:\n>\n> Re-order StreamConfiguration in the CameraConfiguration before\n> validating it to make it easier for the Camera to satisfy the\n> Android framework request.\n>\n> Introduce a new Camera3StreamsToConfig type to associate\n> camera3_stream with StreamConfiguration and sort them before\n> using them to populate the CameraDevice::streams_ vector.\n>\n> To be honest I would split this patch to make its consumption easier:\n> 1) Introduce the new type\n> 2) Collect the vector and create streams_ from it (this should be\n> functionally equivalent to the existing 'unsorted' implementation\n> afaict)\n> 3) Sort the vector before creating streams_\n>\n\nSure, I splitted.\n\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 158 +++++++++++++++++++++++++++++-----\n> > 1 file changed, 137 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 4690346e..3de5a9f1 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -9,9 +9,11 @@\n> > #include \"camera_ops.h\"\n> > #include \"post_processor.h\"\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > +#include \n>\n> 'o' comes before 's'\n>\n> Actually utils/checkstyle.py reports several style issues (some false\n> positives as well). I'll point out what I can spot but please re-check\n> with it.\n>\n> >\n> > #include \n> > #include \n> > @@ -128,6 +130,107 @@ const std::map camera3FormatsMap = {\n> > },\n> > };\n> >\n> > +struct Camera3StreamsToConfig {\n>\n> Can you shorten names a bit ?\n> I was about to suggest StreamConfig, but we have StreamConfiguration\n> already. Camera3Stream ? But we have CameraStream... Ok, keep the long\n> name :) Maybe Camera3StreamConfig ?\n>\n>\n> > + std::vector streams; // Destination(s).\n>\n> We usually use: \"typename *\" and \"typename &\"\n>\n> > + std::vector types; // CameraStream::Type(s).\n> > + StreamConfiguration config; // StreamConfiguration requested to a native camera.\n>\n> No C++ comments please.\n>\n> > +};\n> > +\n> > +/*\n> > + * Reorder the configurations so that CameraDevice can accept them as much as\n> > + * possible.\n> > + */\n>\n> Nice! Can you provide a brief description of the sorting criteria as\n> well ?\n>\n> > +std::vector createdSortedCamera3StreamsToConfigs(\n>\n> maybe just 'sortStreamConfigs' ?\n>\n> Can you move this function before its only user ?\n>\n> > + std::vector unsortedStreamsToConfigs,\n>\n> const & ?\n> Ah I see you move the vector here...\n>\n> > + const camera3_stream_t *jpegStream) {\n> > + const size_t unsortedStreamsToConfigsSize = unsortedStreamsToConfigs.size();\n> > + std::optional streamsToConfigForJpeg = std::nullopt;\n> > + if (jpegStream) {\n> > + for (auto it = unsortedStreamsToConfigs.begin();\n> > + it != unsortedStreamsToConfigs.end(); it++) {\n>\n> for (const auto &it : unsortedStreamsToConfigs) ?\n>\n> > + const auto& streams = it->streams;\n> > + if (std::find(streams.begin(), streams.end(),\n> > + jpegStream) != streams.end()) {\n> > + streamsToConfigForJpeg = *it;\n> > + unsortedStreamsToConfigs.erase(it);\n> > + break;\n> > + }\n> > + }\n> > + assert(streamsToConfigForJpeg.has_value());\n>\n> LOG(Fatal) would make the error verbose\n>\n> > + }\n> > +\n> > + std::map> formatToStreamsToConfigs;\n>\n> It's internal stuff, just 'formatToConfig' or even 'formats' would\n> make this more readable.\n>\n> > + for (const auto &streamsToConfig : unsortedStreamsToConfigs) {\n> > + const StreamConfiguration &config = streamsToConfig.config;\n> > + formatToStreamsToConfigs[config.pixelFormat].push_back(streamsToConfig);\n> > +\n> > + }\n> > + for (auto& [format, streamsToConfigs] : formatToStreamsToConfigs) {\n>\n> auto &\n> the [] pair syntax is much nicer than having to use it.first, it.second!\n>\n> > + /* Sorted by resolution. Smaller is put first. */\n>\n> Size has an ordering defined, have you considered using it ?\n>\n> > + std::sort(streamsToConfigs.begin(), streamsToConfigs.end(),\n> > + [](const auto &streamsToConfigA, const auto &streamsToConfigB) {\n> > + const Size &sizeA = streamsToConfigA.config.size;\n> > + const Size &sizeB = streamsToConfigA.config.size;\n> > + if (sizeA.width != sizeB.width)\n> > + return sizeA.width < sizeB.width;\n> > + return sizeA.height < sizeB.height;\n> > + });\n> > + }\n> > +\n> > + std::vector sortedStreamsToConfigs;\n>\n> Maybe reserve space in all intermediate vectors to avoid relocations ?\n> Although numbers should be small..\n>\n> > + /*\n> > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > + * and the largest resolution first.\n> > + */\n> > + if (formatToStreamsToConfigs.find(formats::NV12) != formatToStreamsToConfigs.end()) {\n> > + auto& nv12StreamsToConfigs = formatToStreamsToConfigs[formats::NV12];\n>\n> auto &nv12StreamsToConfigs\n>\n> Can the double lookup be avoided with:\n> const auto nv12Stream = formatToStreamsToConfigs.find(formats::NV12)\n> if (nv12Stream != formatToStreamsToConfigs.end())\n>\n> > + const Size& nv12LargestSize = nv12StreamsToConfigs.back().config.size;\n>\n> const Size &nv12LargestSize\n>\n> > + if (streamsToConfigForJpeg &&\n> > + streamsToConfigForJpeg->config.pixelFormat == formats::NV12) {\n> > + const Size& nv12SizeForJpeg = streamsToConfigForJpeg->config.size;\n>\n> Can you move it after the comment block ?\n>\n> > + /*\n> > + * If JPEG will be created from NV12 and the size is\n> > + * larger than the largest NV12 remained configurations,\n>\n> Didn't get 'remained'\n> 'configuration'\n> > + * then put the NV12 configuration for JPEG first.\n> ^ rogue space\n> > + */\n>\n> > + if (nv12LargestSize.width < nv12SizeForJpeg.width &&\n> > + nv12LargestSize.height < nv12SizeForJpeg.height) {\n> > + sortedStreamsToConfigs.push_back(*streamsToConfigForJpeg);\n> > + streamsToConfigForJpeg = std::nullopt;\n> > + }\n> > + }\n> > + sortedStreamsToConfigs.push_back(nv12StreamsToConfigs.back());\n> > + nv12StreamsToConfigs.pop_back();\n> > + }\n> > +\n> > + /*\n> > + * If the configuration for JPEG is there, then put it.\n> > + */\n>\n> Fits on one line\n>\n> > + if (streamsToConfigForJpeg) {\n> > + sortedStreamsToConfigs.push_back(*streamsToConfigForJpeg);\n> > + streamsToConfigForJpeg = std::nullopt;\n> > + }\n> > +\n> > + /*\n> > + * Put configurations with different formats and larger resolutions\n> > + * earlier.\n> > + */\n> > + while (!formatToStreamsToConfigs.empty()) {\n> > + for (auto it = formatToStreamsToConfigs.begin();\n> > + it != formatToStreamsToConfigs.end();) {\n> > + auto& streamsToConfigs = it->second;\n>\n> auto &streamsToConfigs\n>\n> > + if (streamsToConfigs.empty()) {\n> > + it = formatToStreamsToConfigs.erase(it);\n> > + continue;\n> > + }\n>\n> Can't this loop be expressed as:\n>\n> for (const auto &format : formatToStreamsToConfigs) {\n> for (const auto stream = format.second.rbegin();\n> stream != format.second.rend(); ++stream)\n> sortedStreamsToConfigs.push_back(stream);\n> }\n>\n> Feels more readable to me (not compiled, just an idea)\n>\n> Or do you need to erase elements while you loop them ?\n>\n\nThe while-loop is needed to assort formats.\nIn your way, the configurations with the same format are put earlier,\ne.g., nv12, nv12, yv12, yv12,.. rather than nv12, yv12, nv12, yv12...\n\nBest Regards,\n-Hiro\n\n\n\n> > + sortedStreamsToConfigs.push_back(streamsToConfigs.back());\n> > + streamsToConfigs.pop_back();\n> > + }\n> > + }\n> > + assert(sortedStreamsToConfigs.size() == unsortedStreamsToConfigsSize);\n>\n> One empty line before return ?\n>\n> > + return sortedStreamsToConfigs;\n> > +}\n>\n> I think isolating the sorting algorithm in one patch would make it\n> easier to evaluate it in isolation along with the validate()\n> modification you are working on!\n>\n> > +\n> > } /* namespace */\n> >\n> > LOG_DECLARE_CATEGORY(HAL)\n> > @@ -1225,6 +1328,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > streams_.clear();\n> > streams_.reserve(stream_list->num_streams);\n> >\n> > + std::vector streamsToConfigs;\n> > + streamsToConfigs.reserve(stream_list->num_streams);\n> > +\n> > /* First handle all non-MJPEG streams. */\n> > camera3_stream_t *jpegStream = nullptr;\n> > for (unsigned int i = 0; i < stream_list->num_streams; ++i) {\n> > @@ -1255,14 +1361,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > continue;\n> > }\n> >\n> > - StreamConfiguration streamConfiguration;\n> > - streamConfiguration.size = size;\n> > - streamConfiguration.pixelFormat = format;\n> > -\n> > - config_->addConfiguration(streamConfiguration);\n> > - streams_.emplace_back(this, CameraStream::Type::Direct,\n> > - stream, config_->size() - 1);\n> > - stream->priv = static_cast(&streams_.back());\n> > + Camera3StreamsToConfig streamsToConfig;\n> > + streamsToConfig.streams = {stream};\n>\n> { stream };\n>\n> > + streamsToConfig.types = {CameraStream::Type::Direct};\n>\n> { CameraStream::Type::Direct };\n>\n> > + streamsToConfig.config.size = size;\n> > + streamsToConfig.config.pixelFormat = format;\n> > + streamsToConfigs.push_back(std::move(streamsToConfig));\n> > }\n> >\n> > /* Now handle the MJPEG streams, adding a new stream if required. */\n> > @@ -1271,9 +1375,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > int index = -1;\n> >\n> > /* Search for a compatible stream in the non-JPEG ones. */\n> > - for (unsigned int i = 0; i < config_->size(); i++) {\n> > - StreamConfiguration &cfg = config_->at(i);\n> > -\n> > + for (size_t i = 0; i < streamsToConfigs.size(); ++i) {\n> > + const auto& cfg = streamsToConfigs[i].config;\n>\n> const auto &cfg\n>\n> > /*\n> > * \\todo The PixelFormat must also be compatible with\n> > * the encoder.\n> > @@ -1295,28 +1398,41 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > * introduce a new stream to satisfy the request requirements.\n> > */\n> > if (index < 0) {\n> > - StreamConfiguration streamConfiguration;\n> > -\n> > + Camera3StreamsToConfig streamsToConfig;\n>\n> I would keep the empty line or move it below the comment block.\n>\n> > /*\n> > * \\todo The pixelFormat should be a 'best-fit' choice\n> > * and may require a validation cycle. This is not yet\n> > * handled, and should be considered as part of any\n> > * stream configuration reworks.\n> > */\n> > - streamConfiguration.size.width = jpegStream->width;\n> > - streamConfiguration.size.height = jpegStream->height;\n> > - streamConfiguration.pixelFormat = formats::NV12;\n> > + streamsToConfig.config.size.width = jpegStream->width;\n> > + streamsToConfig.config.size.height = jpegStream->height;\n> > + streamsToConfig.config.pixelFormat = formats::NV12;\n> > + streamsToConfigs.push_back(std::move(streamsToConfig));\n> >\n> > - LOG(HAL, Info) << \"Adding \" << streamConfiguration.toString()\n> > + LOG(HAL, Info) << \"Adding \" << streamsToConfig.config.toString()\n> > << \" for MJPEG support\";\n> >\n> > type = CameraStream::Type::Internal;\n> > - config_->addConfiguration(streamConfiguration);\n> > - index = config_->size() - 1;\n> > + index = streamsToConfigs.size() - 1;\n> > }\n> >\n> > - streams_.emplace_back(this, type, jpegStream, index);\n> > - jpegStream->priv = static_cast(&streams_.back());\n> > + streamsToConfigs[index].streams.push_back(jpegStream);\n> > + streamsToConfigs[index].types.push_back(type);\n> > + }\n> > +\n> > + streamsToConfigs =\n> > + createdSortedCamera3StreamsToConfigs(std::move(streamsToConfigs),\n> > + jpegStream);\n>\n> I'm sure we can find a shorter name for this function :)\n>\n> > + for (auto& streamsToConfig : streamsToConfigs) {\n>\n> const auto &streamsToConfig\n>\n> > + config_->addConfiguration(streamsToConfig.config);\n> > + for (size_t i = 0; i < streamsToConfig.streams.size(); ++i) {\n> > + auto* stream = streamsToConfig.streams[i];\n>\n> auto *stream\n>\n> When the type is 'trivial' please spell it in full\n>\n> > + const CameraStream::Type type = streamsToConfig.types[i];\n> > + streams_.emplace_back(this, type,\n> > + stream, config_->size() - 1);\n> > + stream->priv = static_cast(&streams_.back());\n> > + }\n>\n> Style and a few minor issues apart, I think this is mostly ok. I tried\n> to think how CameraStream can be modified not to introduce a new type,\n> but what we're actually sorting are the StreamConfiguration, so a new\n> type is probably the right thing to do.\n>\n> Thanks\n> j\n>\n> > }\n> >\n> > switch (config_->validate()) {\n> > --\n> > 2.29.2.454.gaff20da3a2-goog","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 48E92BDB20\n\tfor ;\n\tMon, 7 Dec 2020 10:27:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15A2567E14;\n\tMon, 7 Dec 2020 11:27:06 +0100 (CET)","from mail-ed1-x543.google.com (mail-ed1-x543.google.com\n\t[IPv6:2a00:1450:4864:20::543])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CE39367E12\n\tfor ;\n\tMon, 7 Dec 2020 11:27:03 +0100 (CET)","by mail-ed1-x543.google.com with SMTP id cw27so13128680edb.5\n\tfor ;\n\tMon, 07 Dec 2020 02:27:03 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"HxOPkqxR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=tBj4Qxm/PF/5sWUEH0FEftP4c5ZbX7X0/eVLwwp8qOw=;\n\tb=HxOPkqxRh8rLTh6fgHsQgbWz9i8nlJrDB9Y/QEr21G6WR0iO7IOBAfV3YrZEN2U6V2\n\t153+I4uMfigEcFaJ6gKEklOucLuD1nJHYvqhz5eGl9NEe+c5oUjkbfZhGMEAsjtCJ8Sf\n\tUii+q+xFn+Nt2b5DU/to/zQszbP13EBuCVQ88=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=tBj4Qxm/PF/5sWUEH0FEftP4c5ZbX7X0/eVLwwp8qOw=;\n\tb=Uc1ytULDoeaL7dhRa6aXZ8zizw0UhpnkdpDD/rkemo9M+X1RBMENksR5gwkz+rhh1z\n\tpB8Eyt1zob5S7BMzQIJNb+jJF893jzCKLPu+c78nQGnhd66y9rWJyjLeAzghMZ6yroZR\n\tzlYsQgPVJ0FyAP1b4zPO7fjRgDX/LmJuLkO1Nxyw9lL8WvrBK/NghHemw3uHVvDF4yTv\n\tKOzbjb0vzj3zUcZarqG2Susm9C/yORl4C+TYtxxVlI6vlUAf/FpN5eJO5lqGLF7fHlMM\n\tiGRakMaoY1P6oEnAKg8fOSTOv9y8PLwufFFSfwyuMZcXXc44GvEBFnKISgllp2+Bfta5\n\t1L4A==","X-Gm-Message-State":"AOAM5326eRZc8jcYgQIXS6RvQOGDLFWU7PCJMo0LR3hK1qeCj1xvwLte\n\tPVVMhC6IxxU+DTgQ6p0HlPIbQ4pxmlIZIm8L61NXFkEhQGx8Ng==","X-Google-Smtp-Source":"ABdhPJxRSfOkKPl6l3UDBqNhh/vr2r/ZBYTeqXOrweVZcROZ1BlJ3PJXJ4E+R4ADrPXYhstZHCAA9FnHXF7EROVUuOo=","X-Received":"by 2002:a17:906:af83:: with SMTP id\n\tmj3mr17495194ejb.243.1607330858919; \n\tMon, 07 Dec 2020 00:47:38 -0800 (PST)","MIME-Version":"1.0","References":"<20201201042514.3209146-1-hiroh@chromium.org>\n\t<20201202171658.44d2tygvsxdo422g@uno.localdomain>","In-Reply-To":"<20201202171658.44d2tygvsxdo422g@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 7 Dec 2020 17:47:27 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [RFC PATCH v2] android: camera_device:\n\tReorder configurations before request","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]