[{"id":14219,"web_url":"https://patchwork.libcamera.org/comment/14219/","msgid":"","date":"2020-12-11T13:12:30","subject":"Re: [libcamera-devel] [PATCH v7 3/3] android: camera_device:\n\tReorder configurations before requesting","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote:\n> This reorders Camera3Configs before executing\n> CameraConfiguration::validate() to make it easier for the Camera\n> to satisfy the Android framework request.\n> \n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++-\n> 1 file changed, 108 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 7e8b2818..0c58c1c5 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -27,6 +27,8 @@\n> \n> using namespace libcamera;\n> \n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> namespace {\n> \n> /*\n> @@ -144,9 +146,112 @@ struct Camera3StreamConfig {\n> \tstd::vector streams;\n> \tStreamConfiguration config;\n> };\n> -} /* namespace */\n> \n> -LOG_DECLARE_CATEGORY(HAL)\n> +/*\n> + * Reorder the configurations so that libcamera::Camera can accept them as much\n> + * as possible. The sort rule is as follows.\n> + * 1.) The configuration for NV12 request whose resolution is the largest.\n> + * 2.) The configuration for JPEG request.\n> + * 3.) Others. Larger resolutions and different formats are put earlier.\n> + */\n> +void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> +\t\t\t const camera3_stream_t *jpegStream)\n> +{\n> +\tconst Camera3StreamConfig *jpegConfig = nullptr;\n> +\n> +\tstd::map> formatToConfigs;\n> +\tfor (const auto &streamConfig : unsortedConfigs) {\n> +\t\tif (jpegStream && !jpegConfig) {\n> +\t\t\tconst auto &streams = streamConfig.streams;\n> +\t\t\tif (std::find_if(streams.begin(), streams.end(),\n> +\t\t\t\t\t [jpegStream](const auto &stream) {\n> +\t\t\t\t\t\t return stream.stream == jpegStream;\n> +\t\t\t\t\t }) != streams.end()) {\n> +\t\t\t\tjpegConfig = &streamConfig;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\t\tformatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig);\n> +\t}\n> +\tif (jpegStream && !jpegConfig)\n> +\t\tLOG(HAL, Fatal) << \"No Camera3StreamConfig is found for JPEG\";\n> +\n> +\tfor (auto &[format, streamConfigs] : formatToConfigs) {\n\nThis breaks compilation on gcc 7 :-(\n\n../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’:\n../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable]\n for (auto &[format, streamConfigs] : formatToConfigs) {\n\ngcc 7 doesn't seem to support [[maybe_unused]] for structured bindings,\nso the only option that seem to work is\n\n\tfor (auto &fmt : formatToConfigs) {\n\t\tauto &streamConfigs = fmt.second;\n\n> +\t\t/* Sorted by resolution. Smaller is put first. */\n> +\t\tstd::sort(streamConfigs.begin(), streamConfigs.end(),\n> +\t\t\t [](const auto *streamConfigA, const auto *streamConfigB) {\n> +\t\t\t\t const Size &sizeA = streamConfigA->config.size;\n> +\t\t\t\t const Size &sizeB = streamConfigB->config.size;\n> +\t\t\t\t return sizeA < sizeB;\n> +\t\t\t });\n> +\t}\n> +\n> +\tstd::vector sortedConfigs;\n> +\tsortedConfigs.reserve(unsortedConfigs.size());\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> +\tconst auto nv12It = formatToConfigs.find(formats::NV12);\n> +\tif (nv12It != formatToConfigs.end()) {\n> +\t\tauto &nv12Configs = nv12It->second;\n> +\t\tconst auto &nv12Largest = nv12Configs.back();\n\nThe use of auto makes it more difficult to read the code, as one has to\nlook up the variables types. auto has its uses, as some types are just\ntoo long to type out explicitly (like for nv12It for instance, which\nwould be std::map>::iterator),\nbut for nv12Largest typing out\n\n\t\tconst Camera3StreamConfig *nv12Largest = nv12Configs.back();\n\nisn't too bad, and improves readability I think. This is nothing that\nhas to be addressed now, but could we keep this in mind for the future ?\n\n> +\n> +\t\t/*\n> +\t\t * If JPEG will be created from NV12 and the size is larger than\n> +\t\t * the largest NV12 configurations, then put the NV12\n> +\t\t * configuration for JPEG first.\n> +\t\t */\n> +\t\tif (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) {\n> +\t\t\tconst Size &nv12SizeForJpeg = jpegConfig->config.size;\n> +\t\t\tconst Size &nv12LargestSize = nv12Largest->config.size;\n> +\n> +\t\t\tif (nv12LargestSize < nv12SizeForJpeg) {\n> +\t\t\t\tLOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> +\t\t\t\tsortedConfigs.push_back(std::move(*jpegConfig));\n> +\t\t\t\tjpegConfig = nullptr;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tLOG(HAL, Debug) << \"Insert \" << nv12Largest->config.toString();\n> +\t\tsortedConfigs.push_back(*nv12Largest);\n> +\t\tnv12Configs.pop_back();\n> +\n> +\t\tif (nv12Configs.empty())\n> +\t\t\tformatToConfigs.erase(nv12It);\n> +\t}\n> +\n> +\t/* If the configuration for JPEG is there, then put it. */\n> +\tif (jpegConfig) {\n> +\t\tLOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> +\t\tsortedConfigs.push_back(std::move(*jpegConfig));\n> +\t\tjpegConfig = nullptr;\n> +\t}\n> +\n> +\t/*\n> +\t * Put configurations with different formats and larger resolutions\n> +\t * earlier.\n> +\t */\n> +\twhile (!formatToConfigs.empty()) {\n> +\t\tfor (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) {\n> +\t\t\tauto &configs = it->second;\n> +\t\t\tLOG(HAL, Debug) << \"Insert \" << configs.back()->config.toString();\n> +\t\t\tsortedConfigs.push_back(*configs.back());\n> +\t\t\tconfigs.pop_back();\n> +\n> +\t\t\tif (configs.empty())\n> +\t\t\t\tit = formatToConfigs.erase(it);\n> +\t\t\telse\n> +\t\t\t\tit++;\n> +\t\t}\n> +\t}\n> +\n> +\tASSERT(sortedConfigs.size() == unsortedConfigs.size());\n> +\n> +\tunsortedConfigs = sortedConfigs;\n> +}\n\nI've added a blank line here.\n\nReviewed-by: Laurent Pinchart \n\nAnd pushed with the compilation fix and the blank line. Thanks for\nbearing with us and the lengthy review. I'll do my best to review your\nfuture patches faster.\n\n> +} /* namespace */\n> \n> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> \t\t\t\t\t int flags)\n> @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\tstreamConfigs[index].streams.push_back({ jpegStream, type });\n> \t}\n> \n> +\tsortCamera3StreamConfigs(streamConfigs, jpegStream);\n> \tfor (const auto &streamConfig : streamConfigs) {\n> \t\tconfig_->addConfiguration(streamConfig.config);\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 6CA86BD80A\n\tfor ;\n\tFri, 11 Dec 2020 13:12:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE19E67FA0;\n\tFri, 11 Dec 2020 14:12:36 +0100 (CET)","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 8477C67F06\n\tfor ;\n\tFri, 11 Dec 2020 14:12:35 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB85F99;\n\tFri, 11 Dec 2020 14:12:34 +0100 (CET)"],"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=\"Yjb5ydqb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607692355;\n\tbh=RB1EnoEcjSkm4FTLIh28Wf9j/HoB/to8Lo1M23GGXlI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Yjb5ydqb3c4KFV47szIruilqq+2MtprBgNrpj9ClA/PCGieak5mDgpAJG+vypVUDi\n\tc7tYJaFp/JEPXUrzZTmTGEpjSM1hkXsxHxPDcscfy5gJ0pGWPit1n8erUBFLtVOvaV\n\tdfu244sACmoMS/oQjst300UHiRKWyuOhDnSGiLqQ=","Date":"Fri, 11 Dec 2020 15:12:30 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20201211095336.40500-1-hiroh@chromium.org>\n\t<20201211095336.40500-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201211095336.40500-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v7 3/3] android: camera_device:\n\tReorder configurations before requesting","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":14230,"web_url":"https://patchwork.libcamera.org/comment/14230/","msgid":"","date":"2020-12-12T00:50:36","subject":"Re: [libcamera-devel] [PATCH v7 3/3] android: camera_device:\n\tReorder configurations before requesting","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Sat, Dec 12, 2020 at 9:46 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Sat, Dec 12, 2020 at 09:42:28AM +0900, Hirokazu Honda wrote:\n> > Hi Laurent,\n> >\n> > Thanks for reviewing.\n> > I submitted new patches addressing your comments.\n>\n> Thank you, but I've already addressed the small issues and pushed the\n> series. I apologize if that wasn't clear from my last e-mail.\n>\n\nI thought so but I didn't see the patches after fetching the master now.\nLooking https://git.linuxtv.org/libcamera.git/log/, you're correct.\nI think my local git setting was wrong.\n\n> There's one change missing in the code I've pushed though, compared to\n> your v8:\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 38689bdc40b1..0b68a92764ba 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -199,7 +198,7 @@ void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> const auto nv12It = formatToConfigs.find(formats::NV12);\n> if (nv12It != formatToConfigs.end()) {\n> auto &nv12Configs = nv12It->second;\n> - const auto &nv12Largest = nv12Configs.back();\n> + const Camera3StreamConfig *nv12Largest = nv12Configs.back();\n>\n> /*\n> * If JPEG will be created from NV12 and the size is larger than\n>\n> Would you like to send a patch for that, or would you like me to handle\n> it ?\n\nI don't mind it. I would not push it.\n\nRegards,\n-Hiro\n>\n> > On Fri, Dec 11, 2020 at 10:12 PM Laurent Pinchart wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote:\n> > > > This reorders Camera3Configs before executing\n> > > > CameraConfiguration::validate() to make it easier for the Camera\n> > > > to satisfy the Android framework request.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Reviewed-by: Jacopo Mondi \n> > > > Reviewed-by: Kieran Bingham \n> > > > Reviewed-by: Umang Jain \n> > > > ---\n> > > > src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++-\n> > > > 1 file changed, 108 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 7e8b2818..0c58c1c5 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -27,6 +27,8 @@\n> > > >\n> > > > using namespace libcamera;\n> > > >\n> > > > +LOG_DECLARE_CATEGORY(HAL)\n> > > > +\n> > > > namespace {\n> > > >\n> > > > /*\n> > > > @@ -144,9 +146,112 @@ struct Camera3StreamConfig {\n> > > > std::vector streams;\n> > > > StreamConfiguration config;\n> > > > };\n> > > > -} /* namespace */\n> > > >\n> > > > -LOG_DECLARE_CATEGORY(HAL)\n> > > > +/*\n> > > > + * Reorder the configurations so that libcamera::Camera can accept them as much\n> > > > + * as possible. The sort rule is as follows.\n> > > > + * 1.) The configuration for NV12 request whose resolution is the largest.\n> > > > + * 2.) The configuration for JPEG request.\n> > > > + * 3.) Others. Larger resolutions and different formats are put earlier.\n> > > > + */\n> > > > +void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> > > > + const camera3_stream_t *jpegStream)\n> > > > +{\n> > > > + const Camera3StreamConfig *jpegConfig = nullptr;\n> > > > +\n> > > > + std::map> formatToConfigs;\n> > > > + for (const auto &streamConfig : unsortedConfigs) {\n> > > > + if (jpegStream && !jpegConfig) {\n> > > > + const auto &streams = streamConfig.streams;\n> > > > + if (std::find_if(streams.begin(), streams.end(),\n> > > > + [jpegStream](const auto &stream) {\n> > > > + return stream.stream == jpegStream;\n> > > > + }) != streams.end()) {\n> > > > + jpegConfig = &streamConfig;\n> > > > + continue;\n> > > > + }\n> > > > + }\n> > > > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig);\n> > > > + }\n> > > > + if (jpegStream && !jpegConfig)\n> > > > + LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for JPEG\";\n> > > > +\n> > > > + for (auto &[format, streamConfigs] : formatToConfigs) {\n> > >\n> > > This breaks compilation on gcc 7 :-(\n> > >\n> > > ../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’:\n> > > ../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable]\n> > > for (auto &[format, streamConfigs] : formatToConfigs) {\n> > >\n> > > gcc 7 doesn't seem to support [[maybe_unused]] for structured bindings,\n> > > so the only option that seem to work is\n> > >\n> > > for (auto &fmt : formatToConfigs) {\n> > > auto &streamConfigs = fmt.second;\n> > >\n> > > > + /* Sorted by resolution. Smaller is put first. */\n> > > > + std::sort(streamConfigs.begin(), streamConfigs.end(),\n> > > > + [](const auto *streamConfigA, const auto *streamConfigB) {\n> > > > + const Size &sizeA = streamConfigA->config.size;\n> > > > + const Size &sizeB = streamConfigB->config.size;\n> > > > + return sizeA < sizeB;\n> > > > + });\n> > > > + }\n> > > > +\n> > > > + std::vector sortedConfigs;\n> > > > + sortedConfigs.reserve(unsortedConfigs.size());\n> > > > +\n> > > > + /*\n> > > > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > > > + * and the largest resolution first.\n> > > > + */\n> > > > + const auto nv12It = formatToConfigs.find(formats::NV12);\n> > > > + if (nv12It != formatToConfigs.end()) {\n> > > > + auto &nv12Configs = nv12It->second;\n> > > > + const auto &nv12Largest = nv12Configs.back();\n> > >\n> > > The use of auto makes it more difficult to read the code, as one has to\n> > > look up the variables types. auto has its uses, as some types are just\n> > > too long to type out explicitly (like for nv12It for instance, which\n> > > would be std::map>::iterator),\n> > > but for nv12Largest typing out\n> > >\n> > > const Camera3StreamConfig *nv12Largest = nv12Configs.back();\n> > >\n> > > isn't too bad, and improves readability I think. This is nothing that\n> > > has to be addressed now, but could we keep this in mind for the future ?\n> > >\n> > > > +\n> > > > + /*\n> > > > + * If JPEG will be created from NV12 and the size is larger than\n> > > > + * the largest NV12 configurations, then put the NV12\n> > > > + * configuration for JPEG first.\n> > > > + */\n> > > > + if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) {\n> > > > + const Size &nv12SizeForJpeg = jpegConfig->config.size;\n> > > > + const Size &nv12LargestSize = nv12Largest->config.size;\n> > > > +\n> > > > + if (nv12LargestSize < nv12SizeForJpeg) {\n> > > > + LOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> > > > + sortedConfigs.push_back(std::move(*jpegConfig));\n> > > > + jpegConfig = nullptr;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + LOG(HAL, Debug) << \"Insert \" << nv12Largest->config.toString();\n> > > > + sortedConfigs.push_back(*nv12Largest);\n> > > > + nv12Configs.pop_back();\n> > > > +\n> > > > + if (nv12Configs.empty())\n> > > > + formatToConfigs.erase(nv12It);\n> > > > + }\n> > > > +\n> > > > + /* If the configuration for JPEG is there, then put it. */\n> > > > + if (jpegConfig) {\n> > > > + LOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> > > > + sortedConfigs.push_back(std::move(*jpegConfig));\n> > > > + jpegConfig = nullptr;\n> > > > + }\n> > > > +\n> > > > + /*\n> > > > + * Put configurations with different formats and larger resolutions\n> > > > + * earlier.\n> > > > + */\n> > > > + while (!formatToConfigs.empty()) {\n> > > > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) {\n> > > > + auto &configs = it->second;\n> > > > + LOG(HAL, Debug) << \"Insert \" << configs.back()->config.toString();\n> > > > + sortedConfigs.push_back(*configs.back());\n> > > > + configs.pop_back();\n> > > > +\n> > > > + if (configs.empty())\n> > > > + it = formatToConfigs.erase(it);\n> > > > + else\n> > > > + it++;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + ASSERT(sortedConfigs.size() == unsortedConfigs.size());\n> > > > +\n> > > > + unsortedConfigs = sortedConfigs;\n> > > > +}\n> > >\n> > > I've added a blank line here.\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > And pushed with the compilation fix and the blank line. Thanks for\n> > > bearing with us and the lengthy review. I'll do my best to review your\n> > > future patches faster.\n> > >\n> > > > +} /* namespace */\n> > > >\n> > > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > > > int flags)\n> > > > @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > streamConfigs[index].streams.push_back({ jpegStream, type });\n> > > > }\n> > > >\n> > > > + sortCamera3StreamConfigs(streamConfigs, jpegStream);\n> > > > for (const auto &streamConfig : streamConfigs) {\n> > > > config_->addConfiguration(streamConfig.config);\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 B7C9FBD808\n\tfor ;\n\tSat, 12 Dec 2020 01:21:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 48857746F0;\n\tSat, 12 Dec 2020 02:21:31 +0100 (CET)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A8CA68053\n\tfor ;\n\tSat, 12 Dec 2020 02:21:30 +0100 (CET)","by mail-ej1-x633.google.com with SMTP id jx16so14827598ejb.10\n\tfor ;\n\tFri, 11 Dec 2020 17:21:30 -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=\"WNae3y8j\"; 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:content-transfer-encoding;\n\tbh=jCZ6zRa09SXmUrOkTCR2CgNLNodV5pRvPJhUcWElz5k=;\n\tb=WNae3y8jg958UDUR4HSxnJXGzCfdxh4oCq272ZAm5EIqm/DoxA4trqUjniU6rfNECk\n\tbVJb38TNV9HkVJpUXxdm39KDYe+FxFTSnuDdrmgDTIlX8OSiDcz67P8xl8XPo1hC6eQp\n\tu3zAB5956lfcS624CGTHdPu6jvI2CrvUsXQhI=","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:content-transfer-encoding;\n\tbh=jCZ6zRa09SXmUrOkTCR2CgNLNodV5pRvPJhUcWElz5k=;\n\tb=H4K6IK7fNPoRO20n+0GMfxmFVASMQBgHA7o6CbdSeiU0VRSzhOHvRpM4l9AJHF1yKF\n\t4iERoYCGHfbu524BDtV1H2MFlgWjOY6d4zqF0k/CYooefCAYwJZxnIpYm6Lul6sf+0sU\n\tfYE9d7Iq8jr1FVW8qsAyrGzSHVm16K0m9sEz5PUFiWp9qtVEXsAWEs2BK3b7TMYbOqIJ\n\t1K08ZZycqPjJ4vBUy4+WXjiLL+QvKFH5aKjAZKW90a1JtVr7pbuRzH4muy/xhPIF9sqZ\n\tAPyqEsW5xQ6XKfn8rOehL4NiuhiLz8hPEm8Tf7U9Ssh0Zw+NukaYLNkYfvAbrKhg5BSO\n\trx/g==","X-Gm-Message-State":"AOAM531rhnBWab26YIFFVEPZC9rIFQLii+qAxSaQ0l5k0xtAvV2xgvKJ\n\t++ZCOkw3g/NHQI7ZTLQFeUrH5s9+JN/sJet1TzFHxBahkieOwQh6","X-Google-Smtp-Source":"ABdhPJyOcfsuTQ9Ayi8oYcNCUTFj8p/ujCVakpq/8qWkvg1NSd6TRpql5zkzuWjojrwwksS3GhR7Scj4DG7JNi41L3E=","X-Received":"by 2002:a50:d757:: with SMTP id\n\ti23mr14607085edj.116.1607734246207; \n\tFri, 11 Dec 2020 16:50:46 -0800 (PST)","MIME-Version":"1.0","References":"<20201211095336.40500-1-hiroh@chromium.org>\n\t<20201211095336.40500-3-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Sat, 12 Dec 2020 09:50:36 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v7 3/3] android: camera_device:\n\tReorder configurations before requesting","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":14231,"web_url":"https://patchwork.libcamera.org/comment/14231/","msgid":"","date":"2020-12-12T00:45:58","subject":"Re: [libcamera-devel] [PATCH v7 3/3] android: camera_device:\n\tReorder configurations before requesting","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Sat, Dec 12, 2020 at 09:42:28AM +0900, Hirokazu Honda wrote:\n> Hi Laurent,\n> \n> Thanks for reviewing.\n> I submitted new patches addressing your comments.\n\nThank you, but I've already addressed the small issues and pushed the\nseries. I apologize if that wasn't clear from my last e-mail.\n\nThere's one change missing in the code I've pushed though, compared to\nyour v8:\n\ndiff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 38689bdc40b1..0b68a92764ba 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -199,7 +198,7 @@ void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n \tconst auto nv12It = formatToConfigs.find(formats::NV12);\n \tif (nv12It != formatToConfigs.end()) {\n \t\tauto &nv12Configs = nv12It->second;\n-\t\tconst auto &nv12Largest = nv12Configs.back();\n+\t\tconst Camera3StreamConfig *nv12Largest = nv12Configs.back();\n\n \t\t/*\n \t\t * If JPEG will be created from NV12 and the size is larger than\n\nWould you like to send a patch for that, or would you like me to handle\nit ?\n\n> On Fri, Dec 11, 2020 at 10:12 PM Laurent Pinchart wrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote:\n> > > This reorders Camera3Configs before executing\n> > > CameraConfiguration::validate() to make it easier for the Camera\n> > > to satisfy the Android framework request.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Jacopo Mondi \n> > > Reviewed-by: Kieran Bingham \n> > > Reviewed-by: Umang Jain \n> > > ---\n> > > src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++-\n> > > 1 file changed, 108 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 7e8b2818..0c58c1c5 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -27,6 +27,8 @@\n> > >\n> > > using namespace libcamera;\n> > >\n> > > +LOG_DECLARE_CATEGORY(HAL)\n> > > +\n> > > namespace {\n> > >\n> > > /*\n> > > @@ -144,9 +146,112 @@ struct Camera3StreamConfig {\n> > > std::vector streams;\n> > > StreamConfiguration config;\n> > > };\n> > > -} /* namespace */\n> > >\n> > > -LOG_DECLARE_CATEGORY(HAL)\n> > > +/*\n> > > + * Reorder the configurations so that libcamera::Camera can accept them as much\n> > > + * as possible. The sort rule is as follows.\n> > > + * 1.) The configuration for NV12 request whose resolution is the largest.\n> > > + * 2.) The configuration for JPEG request.\n> > > + * 3.) Others. Larger resolutions and different formats are put earlier.\n> > > + */\n> > > +void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> > > + const camera3_stream_t *jpegStream)\n> > > +{\n> > > + const Camera3StreamConfig *jpegConfig = nullptr;\n> > > +\n> > > + std::map> formatToConfigs;\n> > > + for (const auto &streamConfig : unsortedConfigs) {\n> > > + if (jpegStream && !jpegConfig) {\n> > > + const auto &streams = streamConfig.streams;\n> > > + if (std::find_if(streams.begin(), streams.end(),\n> > > + [jpegStream](const auto &stream) {\n> > > + return stream.stream == jpegStream;\n> > > + }) != streams.end()) {\n> > > + jpegConfig = &streamConfig;\n> > > + continue;\n> > > + }\n> > > + }\n> > > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig);\n> > > + }\n> > > + if (jpegStream && !jpegConfig)\n> > > + LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for JPEG\";\n> > > +\n> > > + for (auto &[format, streamConfigs] : formatToConfigs) {\n> >\n> > This breaks compilation on gcc 7 :-(\n> >\n> > ../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’:\n> > ../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable]\n> > for (auto &[format, streamConfigs] : formatToConfigs) {\n> >\n> > gcc 7 doesn't seem to support [[maybe_unused]] for structured bindings,\n> > so the only option that seem to work is\n> >\n> > for (auto &fmt : formatToConfigs) {\n> > auto &streamConfigs = fmt.second;\n> >\n> > > + /* Sorted by resolution. Smaller is put first. */\n> > > + std::sort(streamConfigs.begin(), streamConfigs.end(),\n> > > + [](const auto *streamConfigA, const auto *streamConfigB) {\n> > > + const Size &sizeA = streamConfigA->config.size;\n> > > + const Size &sizeB = streamConfigB->config.size;\n> > > + return sizeA < sizeB;\n> > > + });\n> > > + }\n> > > +\n> > > + std::vector sortedConfigs;\n> > > + sortedConfigs.reserve(unsortedConfigs.size());\n> > > +\n> > > + /*\n> > > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > > + * and the largest resolution first.\n> > > + */\n> > > + const auto nv12It = formatToConfigs.find(formats::NV12);\n> > > + if (nv12It != formatToConfigs.end()) {\n> > > + auto &nv12Configs = nv12It->second;\n> > > + const auto &nv12Largest = nv12Configs.back();\n> >\n> > The use of auto makes it more difficult to read the code, as one has to\n> > look up the variables types. auto has its uses, as some types are just\n> > too long to type out explicitly (like for nv12It for instance, which\n> > would be std::map>::iterator),\n> > but for nv12Largest typing out\n> >\n> > const Camera3StreamConfig *nv12Largest = nv12Configs.back();\n> >\n> > isn't too bad, and improves readability I think. This is nothing that\n> > has to be addressed now, but could we keep this in mind for the future ?\n> >\n> > > +\n> > > + /*\n> > > + * If JPEG will be created from NV12 and the size is larger than\n> > > + * the largest NV12 configurations, then put the NV12\n> > > + * configuration for JPEG first.\n> > > + */\n> > > + if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) {\n> > > + const Size &nv12SizeForJpeg = jpegConfig->config.size;\n> > > + const Size &nv12LargestSize = nv12Largest->config.size;\n> > > +\n> > > + if (nv12LargestSize < nv12SizeForJpeg) {\n> > > + LOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> > > + sortedConfigs.push_back(std::move(*jpegConfig));\n> > > + jpegConfig = nullptr;\n> > > + }\n> > > + }\n> > > +\n> > > + LOG(HAL, Debug) << \"Insert \" << nv12Largest->config.toString();\n> > > + sortedConfigs.push_back(*nv12Largest);\n> > > + nv12Configs.pop_back();\n> > > +\n> > > + if (nv12Configs.empty())\n> > > + formatToConfigs.erase(nv12It);\n> > > + }\n> > > +\n> > > + /* If the configuration for JPEG is there, then put it. */\n> > > + if (jpegConfig) {\n> > > + LOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> > > + sortedConfigs.push_back(std::move(*jpegConfig));\n> > > + jpegConfig = nullptr;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Put configurations with different formats and larger resolutions\n> > > + * earlier.\n> > > + */\n> > > + while (!formatToConfigs.empty()) {\n> > > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) {\n> > > + auto &configs = it->second;\n> > > + LOG(HAL, Debug) << \"Insert \" << configs.back()->config.toString();\n> > > + sortedConfigs.push_back(*configs.back());\n> > > + configs.pop_back();\n> > > +\n> > > + if (configs.empty())\n> > > + it = formatToConfigs.erase(it);\n> > > + else\n> > > + it++;\n> > > + }\n> > > + }\n> > > +\n> > > + ASSERT(sortedConfigs.size() == unsortedConfigs.size());\n> > > +\n> > > + unsortedConfigs = sortedConfigs;\n> > > +}\n> >\n> > I've added a blank line here.\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > And pushed with the compilation fix and the blank line. Thanks for\n> > bearing with us and the lengthy review. I'll do my best to review your\n> > future patches faster.\n> >\n> > > +} /* namespace */\n> > >\n> > > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > > int flags)\n> > > @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > streamConfigs[index].streams.push_back({ jpegStream, type });\n> > > }\n> > >\n> > > + sortCamera3StreamConfigs(streamConfigs, jpegStream);\n> > > for (const auto &streamConfig : streamConfigs) {\n> > > config_->addConfiguration(streamConfig.config);\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 F048FBD808\n\tfor ;\n\tSat, 12 Dec 2020 01:22:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BCC97746F2;\n\tSat, 12 Dec 2020 02:22:46 +0100 (CET)","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 DAEA668053\n\tfor ;\n\tSat, 12 Dec 2020 02:22:44 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C9EF04FD;\n\tSat, 12 Dec 2020 01:46:03 +0100 (CET)"],"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=\"cK5CS11l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607733964;\n\tbh=5+NPcIfFY90Zks4PxLCAMaCO9M0st47xVB4uVnko5tI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cK5CS11ld8PSTswJfqa1zAQ9EWt7MRanJ/JzgGAAnvA7/rbbChOtKU8Hz4wZDyZgR\n\tUn30yh48vpE/XXZAhRo/GjPCIPNzSmXazps3TTaU5PkzFK2rXx9yNmnXD5x3u5VVbu\n\tF8razTikGiqswV+OW14eulgqS4X3lbUG8CFrf/xQ=","Date":"Sat, 12 Dec 2020 02:45:58 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20201211095336.40500-1-hiroh@chromium.org>\n\t<20201211095336.40500-3-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 3/3] android: camera_device:\n\tReorder configurations before requesting","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":14232,"web_url":"https://patchwork.libcamera.org/comment/14232/","msgid":"","date":"2020-12-12T00:42:28","subject":"Re: [libcamera-devel] [PATCH v7 3/3] android: camera_device:\n\tReorder configurations before requesting","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nThanks for reviewing.\nI submitted new patches addressing your comments.\n\nRegards,\n-Hiro\n\nOn Fri, Dec 11, 2020 at 10:12 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote:\n> > This reorders Camera3Configs before executing\n> > CameraConfiguration::validate() to make it easier for the Camera\n> > to satisfy the Android framework request.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Jacopo Mondi \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++-\n> > 1 file changed, 108 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 7e8b2818..0c58c1c5 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -27,6 +27,8 @@\n> >\n> > using namespace libcamera;\n> >\n> > +LOG_DECLARE_CATEGORY(HAL)\n> > +\n> > namespace {\n> >\n> > /*\n> > @@ -144,9 +146,112 @@ struct Camera3StreamConfig {\n> > std::vector streams;\n> > StreamConfiguration config;\n> > };\n> > -} /* namespace */\n> >\n> > -LOG_DECLARE_CATEGORY(HAL)\n> > +/*\n> > + * Reorder the configurations so that libcamera::Camera can accept them as much\n> > + * as possible. The sort rule is as follows.\n> > + * 1.) The configuration for NV12 request whose resolution is the largest.\n> > + * 2.) The configuration for JPEG request.\n> > + * 3.) Others. Larger resolutions and different formats are put earlier.\n> > + */\n> > +void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> > + const camera3_stream_t *jpegStream)\n> > +{\n> > + const Camera3StreamConfig *jpegConfig = nullptr;\n> > +\n> > + std::map> formatToConfigs;\n> > + for (const auto &streamConfig : unsortedConfigs) {\n> > + if (jpegStream && !jpegConfig) {\n> > + const auto &streams = streamConfig.streams;\n> > + if (std::find_if(streams.begin(), streams.end(),\n> > + [jpegStream](const auto &stream) {\n> > + return stream.stream == jpegStream;\n> > + }) != streams.end()) {\n> > + jpegConfig = &streamConfig;\n> > + continue;\n> > + }\n> > + }\n> > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig);\n> > + }\n> > + if (jpegStream && !jpegConfig)\n> > + LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for JPEG\";\n> > +\n> > + for (auto &[format, streamConfigs] : formatToConfigs) {\n>\n> This breaks compilation on gcc 7 :-(\n>\n> ../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’:\n> ../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable]\n> for (auto &[format, streamConfigs] : formatToConfigs) {\n>\n> gcc 7 doesn't seem to support [[maybe_unused]] for structured bindings,\n> so the only option that seem to work is\n>\n> for (auto &fmt : formatToConfigs) {\n> auto &streamConfigs = fmt.second;\n>\n> > + /* Sorted by resolution. Smaller is put first. */\n> > + std::sort(streamConfigs.begin(), streamConfigs.end(),\n> > + [](const auto *streamConfigA, const auto *streamConfigB) {\n> > + const Size &sizeA = streamConfigA->config.size;\n> > + const Size &sizeB = streamConfigB->config.size;\n> > + return sizeA < sizeB;\n> > + });\n> > + }\n> > +\n> > + std::vector sortedConfigs;\n> > + sortedConfigs.reserve(unsortedConfigs.size());\n> > +\n> > + /*\n> > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > + * and the largest resolution first.\n> > + */\n> > + const auto nv12It = formatToConfigs.find(formats::NV12);\n> > + if (nv12It != formatToConfigs.end()) {\n> > + auto &nv12Configs = nv12It->second;\n> > + const auto &nv12Largest = nv12Configs.back();\n>\n> The use of auto makes it more difficult to read the code, as one has to\n> look up the variables types. auto has its uses, as some types are just\n> too long to type out explicitly (like for nv12It for instance, which\n> would be std::map>::iterator),\n> but for nv12Largest typing out\n>\n> const Camera3StreamConfig *nv12Largest = nv12Configs.back();\n>\n> isn't too bad, and improves readability I think. This is nothing that\n> has to be addressed now, but could we keep this in mind for the future ?\n>\n> > +\n> > + /*\n> > + * If JPEG will be created from NV12 and the size is larger than\n> > + * the largest NV12 configurations, then put the NV12\n> > + * configuration for JPEG first.\n> > + */\n> > + if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) {\n> > + const Size &nv12SizeForJpeg = jpegConfig->config.size;\n> > + const Size &nv12LargestSize = nv12Largest->config.size;\n> > +\n> > + if (nv12LargestSize < nv12SizeForJpeg) {\n> > + LOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> > + sortedConfigs.push_back(std::move(*jpegConfig));\n> > + jpegConfig = nullptr;\n> > + }\n> > + }\n> > +\n> > + LOG(HAL, Debug) << \"Insert \" << nv12Largest->config.toString();\n> > + sortedConfigs.push_back(*nv12Largest);\n> > + nv12Configs.pop_back();\n> > +\n> > + if (nv12Configs.empty())\n> > + formatToConfigs.erase(nv12It);\n> > + }\n> > +\n> > + /* If the configuration for JPEG is there, then put it. */\n> > + if (jpegConfig) {\n> > + LOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> > + sortedConfigs.push_back(std::move(*jpegConfig));\n> > + jpegConfig = nullptr;\n> > + }\n> > +\n> > + /*\n> > + * Put configurations with different formats and larger resolutions\n> > + * earlier.\n> > + */\n> > + while (!formatToConfigs.empty()) {\n> > + for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) {\n> > + auto &configs = it->second;\n> > + LOG(HAL, Debug) << \"Insert \" << configs.back()->config.toString();\n> > + sortedConfigs.push_back(*configs.back());\n> > + configs.pop_back();\n> > +\n> > + if (configs.empty())\n> > + it = formatToConfigs.erase(it);\n> > + else\n> > + it++;\n> > + }\n> > + }\n> > +\n> > + ASSERT(sortedConfigs.size() == unsortedConfigs.size());\n> > +\n> > + unsortedConfigs = sortedConfigs;\n> > +}\n>\n> I've added a blank line here.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> And pushed with the compilation fix and the blank line. Thanks for\n> bearing with us and the lengthy review. I'll do my best to review your\n> future patches faster.\n>\n> > +} /* namespace */\n> >\n> > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > int flags)\n> > @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > streamConfigs[index].streams.push_back({ jpegStream, type });\n> > }\n> >\n> > + sortCamera3StreamConfigs(streamConfigs, jpegStream);\n> > for (const auto &streamConfig : streamConfigs) {\n> > config_->addConfiguration(streamConfig.config);\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 95619BD80A\n\tfor ;\n\tSat, 12 Dec 2020 01:44:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E76D0746F0;\n\tSat, 12 Dec 2020 02:44:18 +0100 (CET)","from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 723D668053\n\tfor ;\n\tSat, 12 Dec 2020 02:44:16 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id m12so16137568lfo.7\n\tfor ;\n\tFri, 11 Dec 2020 17:44:16 -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=\"Lz4MoSfH\"; 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:content-transfer-encoding;\n\tbh=STfXeENd5odH7zeRR54j7zLox1JL3EINtZ09eMQ1xn8=;\n\tb=Lz4MoSfHyRhDFVP5/1Y+jn1i3Hnq+cO/5imIejHBLO7bXx5GMwKhVXxqdgAvA0AvjH\n\tVYgvorLnTDokd1z37AlZkdF6Meo7Cp+hb9X18ZQVuFNu3gkaI+c6U07+ohCEBofzjr1F\n\t/sD84zHn82bZpwlLFvX/FayNIZN1ReJt9dYEE=","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:content-transfer-encoding;\n\tbh=STfXeENd5odH7zeRR54j7zLox1JL3EINtZ09eMQ1xn8=;\n\tb=nxuWhurtU1cR0/4Ak2Fqm2jprSELW380pnJaok1zCxdZKXxqhIw8n7pd7+j69SJBqD\n\taDklpGmxbKI5CFv932Moyvh7ID9Tpb8Or9H4z9WnEmmYxsx2SKTKjAEd0IDgTZlG/jfq\n\tXt2LsvTNv9AhQmktWV/1GxWJelXl8JzFpLMCAz4GKRslyUltLy44vCkPoWjYPUiwiGdG\n\teQcJS7XfSoAX/5aofRkfZ2IcPtV7/HOyUF32TMWuewCXnW3YGk4wZSF/1i+kq/iWPFLa\n\tm7YatswLIZJMYwD2jutghP0mAkwMbBqWcEmsvve9TvIWkbIXmWaq7yi2nZKm12bunZdG\n\tcBbQ==","X-Gm-Message-State":"AOAM531WbMbm/yAJtKS3eEr6SSAiR9NtxN87B5YBpBlHu+FT1o5M7Qnd\n\tpPGteCyeX79NUjKXZPwsGTuBszbn5BnXloj1FK19vLOAj9Og/w==","X-Google-Smtp-Source":"ABdhPJzWdZCPkQUIsA4ud7j0N5neKKnykOzgPVI/hFi02BetsUZWjY48Iq9dlZkanNTNqlp6455CuPQM0sxpmCt2kHM=","X-Received":"by 2002:a17:906:dd3:: with SMTP id\n\tp19mr12965107eji.221.1607733758210; \n\tFri, 11 Dec 2020 16:42:38 -0800 (PST)","MIME-Version":"1.0","References":"<20201211095336.40500-1-hiroh@chromium.org>\n\t<20201211095336.40500-3-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Sat, 12 Dec 2020 09:42:28 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v7 3/3] android: camera_device:\n\tReorder configurations before requesting","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\" "}}]