[{"id":14214,"web_url":"https://patchwork.libcamera.org/comment/14214/","msgid":"<20201211091933.2oavivr6uiy4xg7a@uno.localdomain>","date":"2020-12-11T09:19:33","subject":"Re: [libcamera-devel] [PATCH v6 3/3] android: camera_device:\n\tReorder configurations before requesting","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Fri, Dec 11, 2020 at 06:03:15AM +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 5e245298..a2c005af 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\nconst auto &stream\n\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> +\n> + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig);\n\nWeird indentation\n\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> +\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\tconst Size &sizeA = streamConfigA->config.size;\n> +\t\t\t\tconst Size &sizeB = streamConfigB->config.size;\n> +\t\t\t\treturn 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 Size &nv12LargestSize = nv12Configs.back()->config.size;\n\nI would here declare a variable for\n auto nv12Largest = nv12Configs.back();\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) {\nand move\n\t\tconst Size &nv12LargestSize = nv12Largest->config.size;\nhere\n\n> +\t\t\tconst Size &nv12SizeForJpeg = jpegConfig->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\nSo that\n\n> +\n> +\t\tLOG(HAL, Debug) << \"Insert \" << nv12Configs.back()->config.toString();\n\t\tLOG(HAL, Debug) << \"Insert \" << nv12Largest->config.toString();\n\n> +\t\tsortedConfigs.push_back(*nv12Configs.back());\n\t\tsortedConfigs.push_back(*nv12Largest);\n\nThat's minor stuff though, feel free to ignore.\n\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\nauto &configs;\n\nCould you please make sure to run checkstyle.py before submitting ?\nThe simplest way to make sure is to copy utils/hooks/post-commit in\n.git/hooks\n\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\nWhat about moving this after the 'configs' variable declaration ? To\nmake sure we don't try to access back() on an empty vector ? (which\nshould not happen, but...)\n\nThe resulting code would be also nicer to read\n\n\t\t\tauto &configs = it->second;\n\t\t\tif (configs.empty()) {\n\t\t\t\tit = formatToConfigs.erase(it);\n continue;\n }\n\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 it++;\nAlso a minor.\n\nWith checkstyle.py warnings addressed (there's also one warning that\nseems a false positive to me) and the here proposed suggestions\noptionally considered:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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> +} /* 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>\n> --\n> 2.29.2.576.ga3fc446d84-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 A0F96BD80A\n\tfor ;\n\tFri, 11 Dec 2020 09:19:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17A36635C6;\n\tFri, 11 Dec 2020 10:19:28 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86C63600FF\n\tfor ;\n\tFri, 11 Dec 2020 10:19:26 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id E3065FF802;\n\tFri, 11 Dec 2020 09:19:23 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 11 Dec 2020 10:19:33 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20201211091933.2oavivr6uiy4xg7a@uno.localdomain>","References":"<20201211060315.2637333-1-hiroh@chromium.org>\n\t<20201211060315.2637333-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201211060315.2637333-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v6 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14215,"web_url":"https://patchwork.libcamera.org/comment/14215/","msgid":"","date":"2020-12-11T09:23:52","subject":"Re: [libcamera-devel] [PATCH v6 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 Jacopo,\n\nOn Fri, Dec 11, 2020 at 10:19:33AM +0100, Jacopo Mondi wrote:\n> On Fri, Dec 11, 2020 at 06:03:15AM +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 5e245298..a2c005af 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> \n> const auto &stream\n> \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> > +\n> > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig);\n> \n> Weird indentation\n> \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> > +\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\tconst Size &sizeA = streamConfigA->config.size;\n> > +\t\t\t\tconst Size &sizeB = streamConfigB->config.size;\n> > +\t\t\t\treturn 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 Size &nv12LargestSize = nv12Configs.back()->config.size;\n> \n> I would here declare a variable for\n> auto nv12Largest = nv12Configs.back();\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> and move\n> \t\tconst Size &nv12LargestSize = nv12Largest->config.size;\n> here\n> \n> > +\t\t\tconst Size &nv12SizeForJpeg = jpegConfig->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> So that\n> \n> > +\n> > +\t\tLOG(HAL, Debug) << \"Insert \" << nv12Configs.back()->config.toString();\n> \t\tLOG(HAL, Debug) << \"Insert \" << nv12Largest->config.toString();\n> \n> > +\t\tsortedConfigs.push_back(*nv12Configs.back());\n> \t\tsortedConfigs.push_back(*nv12Largest);\n> \n> That's minor stuff though, feel free to ignore.\n> \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> \n> auto &configs;\n> \n> Could you please make sure to run checkstyle.py before submitting ?\n> The simplest way to make sure is to copy utils/hooks/post-commit in\n> .git/hooks\n> \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> \n> What about moving this after the 'configs' variable declaration ? To\n> make sure we don't try to access back() on an empty vector ? (which\n> should not happen, but...)\n> \n> The resulting code would be also nicer to read\n> \n> \t\t\tauto &configs = it->second;\n> \t\t\tif (configs.empty()) {\n> \t\t\t\tit = formatToConfigs.erase(it);\n> continue;\n> }\n> \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> it++;\n> Also a minor.\n\nThat's what Hiro had on v5, and I proposed the change that led to v6 :-)\nThe v5 version will cause another iteration of the while loop just to\nremove the empty items.\n\n> With checkstyle.py warnings addressed (there's also one warning that\n> seems a false positive to me) and the here proposed suggestions\n> optionally considered:\n> Reviewed-by: Jacopo Mondi \n> \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> > +} /* 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 7A39CBD808\n\tfor ;\n\tFri, 11 Dec 2020 09:23:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0ED5D6346D;\n\tFri, 11 Dec 2020 10:23:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43E35600FF\n\tfor ;\n\tFri, 11 Dec 2020 10:23:58 +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 B2ECA99;\n\tFri, 11 Dec 2020 10:23:57 +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=\"WR/ADKos\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607678637;\n\tbh=Y8YYWbLKhCSfVhlz0EnK5kf1FoVLXN8lHFlavU1UMco=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WR/ADKosI2hJP2/VHUNhlUcpIidME1h8T3j3xec2qk2c/+74pvYZA9rfAt0x2pjv8\n\tWZycJ3GPNMvK6ueQ0vlSsIXREk8ehrBemB38TN4Gg2RXxqEShu3y8tpHhMeLTAi04O\n\tB16nHEZ9XxDPptJ8Y2fjrE+wzPl9pWWUy9rxm0WU=","Date":"Fri, 11 Dec 2020 11:23:52 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201211060315.2637333-1-hiroh@chromium.org>\n\t<20201211060315.2637333-3-hiroh@chromium.org>\n\t<20201211091933.2oavivr6uiy4xg7a@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201211091933.2oavivr6uiy4xg7a@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14218,"web_url":"https://patchwork.libcamera.org/comment/14218/","msgid":"","date":"2020-12-11T09:54:21","subject":"Re: [libcamera-devel] [PATCH v6 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 Jacopo and Laurent,\n\nSorry for a lot of iterations.\nI fixed some style errors and submit the series again.\n-Hiro\n\nOn Fri, Dec 11, 2020 at 6:23 PM Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> On Fri, Dec 11, 2020 at 10:19:33AM +0100, Jacopo Mondi wrote:\n> > On Fri, Dec 11, 2020 at 06:03:15AM +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 5e245298..a2c005af 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> >\n> > const auto &stream\n> >\n> > > + return stream.stream == jpegStream;\n> > > + }) != streams.end()) {\n> > > + jpegConfig = &streamConfig;\n> > > + continue;\n> > > + }\n> > > + }\n> > > +\n> > > + formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig);\n> >\n> > Weird indentation\n> >\n> > > + }\n> > > + if (jpegStream && !jpegConfig)\n> > > + LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for JPEG\";\n> > > +\n> > > + for (auto &[format, streamConfigs] : formatToConfigs) {\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 Size &nv12LargestSize = nv12Configs.back()->config.size;\n> >\n> > I would here declare a variable for\n> > auto nv12Largest = nv12Configs.back();\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> > and move\n> > const Size &nv12LargestSize = nv12Largest->config.size;\n> > here\n> >\n> > > + const Size &nv12SizeForJpeg = jpegConfig->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> > So that\n> >\n> > > +\n> > > + LOG(HAL, Debug) << \"Insert \" << nv12Configs.back()->config.toString();\n> > LOG(HAL, Debug) << \"Insert \" << nv12Largest->config.toString();\n> >\n> > > + sortedConfigs.push_back(*nv12Configs.back());\n> > sortedConfigs.push_back(*nv12Largest);\n> >\n> > That's minor stuff though, feel free to ignore.\n> >\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> >\n> > auto &configs;\n> >\n> > Could you please make sure to run checkstyle.py before submitting ?\n> > The simplest way to make sure is to copy utils/hooks/post-commit in\n> > .git/hooks\n> >\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> >\n> > What about moving this after the 'configs' variable declaration ? To\n> > make sure we don't try to access back() on an empty vector ? (which\n> > should not happen, but...)\n> >\n> > The resulting code would be also nicer to read\n> >\n> > auto &configs = it->second;\n> > if (configs.empty()) {\n> > it = formatToConfigs.erase(it);\n> > continue;\n> > }\n> >\n> > LOG(HAL, Debug) << \"Insert \" << configs.back()->config.toString();\n> > sortedConfigs.push_back(*configs.back());\n> > configs.pop_back();\n> > it++;\n> > Also a minor.\n>\n> That's what Hiro had on v5, and I proposed the change that led to v6 :-)\n> The v5 version will cause another iteration of the while loop just to\n> remove the empty items.\n>\n> > With checkstyle.py warnings addressed (there's also one warning that\n> > seems a false positive to me) and the here proposed suggestions\n> > optionally considered:\n> > Reviewed-by: Jacopo Mondi \n> >\n> > > + else\n> > > + it++;\n> > > + }\n> > > + }\n> > > +\n> > > + ASSERT(sortedConfigs.size() == unsortedConfigs.size());\n> > > +\n> > > + unsortedConfigs = sortedConfigs;\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 252D6BD80A\n\tfor ;\n\tFri, 11 Dec 2020 09:54:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E85C2635C6;\n\tFri, 11 Dec 2020 10:54:33 +0100 (CET)","from mail-ej1-x641.google.com (mail-ej1-x641.google.com\n\t[IPv6:2a00:1450:4864:20::641])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 955DF600FF\n\tfor ;\n\tFri, 11 Dec 2020 10:54:32 +0100 (CET)","by mail-ej1-x641.google.com with SMTP id qw4so11453003ejb.12\n\tfor ;\n\tFri, 11 Dec 2020 01:54:32 -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=\"OSsACjYJ\"; 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=k/AlkAsMHm1zbMjKMHUZ5+ZZHyTaiufCFA/MT6/xfXc=;\n\tb=OSsACjYJh2J61pz6W0DU2Y5bVdMxzz+RW+ztHy+PY66k6Aql+6SAdiF2KIsayq816N\n\tCmuPXUfNXzitgNswgd+q9Zq+Vm3Wj+H3Yw2qbxZmLh6j9hFfb5ZNFG8KYvkRbxg5lMdG\n\tx/s4kec8IVLsjdH04y43THhGp7zRi6f+6wEgE=","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=k/AlkAsMHm1zbMjKMHUZ5+ZZHyTaiufCFA/MT6/xfXc=;\n\tb=gGmgIh91bzoE8+rRph9GHk0p0p3RDI0EyTW2NK0S4WmZ2rX5avZnoEot43MhWxAFjh\n\tRvqAt1sFbJG5eIyYkFYHwM2m0/WgYL+BUjh9OePO4e6oPbp+dDQ1oYlAf1ySYA47xOH0\n\tDvwARJ7tgYebxtTf3gcl0GPwZ/I+fY1tnH+TneOINEnsS4avCSfmJaXM82oRgvISxd8v\n\tMYTX36H1OI2meDLblTcxtWTVBGiOby09zgG7eE6tQklZJugFH2phI8AH4WUlFg559PR0\n\tSB5pCZA1qSy0//rne3Y7gVk3LNJmVzYtMxVJAs1M07IBbz5vwuHP9tE2FwGoAezO1jNA\n\trrVg==","X-Gm-Message-State":"AOAM533iJWDC4SAa/323/2VnWu0qtdunKDD6e808n+QLa1kfhxFJNcZT\n\t1FG1vRU0WMQHpcju76lvlSrdN69zauK4w7O1/4lOqDxXM8MIXQ==","X-Google-Smtp-Source":"ABdhPJzbMtdvojgX28HQ4O1UMgwy6kY+LcHM8kBO7n0iny09Bq8HVuC60A2zbM7BFLSXnhEw1qcW4+GsUqcHvLe94aE=","X-Received":"by 2002:a17:906:dd3:: with SMTP id\n\tp19mr9922901eji.221.1607680472120; \n\tFri, 11 Dec 2020 01:54:32 -0800 (PST)","MIME-Version":"1.0","References":"<20201211060315.2637333-1-hiroh@chromium.org>\n\t<20201211060315.2637333-3-hiroh@chromium.org>\n\t<20201211091933.2oavivr6uiy4xg7a@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 11 Dec 2020 18:54:21 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v6 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]