[{"id":14167,"web_url":"https://patchwork.libcamera.org/comment/14167/","msgid":"<3b3c9d27-f4ed-a6d8-dc5d-f85f9d2b80a8@ideasonboard.com>","date":"2020-12-09T11:10:15","subject":"Re: [libcamera-devel] [PATCH v5 3/3] android: camera_device:\n\tReorder configurations before requesting","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 09/12/2020 05:54, 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> ---\n> src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++-\n> 1 file changed, 107 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b7bf3d88..c9e0ec98 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,6 +9,7 @@\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -27,6 +28,8 @@\n> \n> using namespace libcamera;\n> \n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> namespace {\n> \n> /*\n> @@ -140,9 +143,109 @@ struct Camera3StreamConfig {\n> \tstd::vector types;\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> +std::vector sortCamera3StreamConfigs(\n> +\tstd::vector unsortedConfigs,\n> +\tconst camera3_stream_t *jpegStream)\n> +{\n> +\tconst size_t unsortedSize = unsortedConfigs.size();\n> +\tstd::optional jpegConfig = std::nullopt;\n> +\n> +\tif (jpegStream) {\n> +\t\tfor (size_t i = 0; i < unsortedSize; ++i) {\n> +\t\t\tconst auto &streams = unsortedConfigs[i].streams;\n> +\t\t\tif (std::find(streams.begin(), streams.end(),\n> +\t\t\t\t jpegStream) != streams.end()) {\n> +\t\t\t\tjpegConfig = std::move(unsortedConfigs[i]);\n> +\t\t\t\tunsortedConfigs.erase(unsortedConfigs.begin() + i);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (!jpegConfig)\n> +\t\t\tLOG(HAL, Fatal) << \"No Camera3StreamConfig is found for Jpeg\";\n> +\t}\n> +\n> +\tstd::map> formatToConfigs;\n> +\tfor (const auto &streamConfig : unsortedConfigs) {\n> +\t\tconst StreamConfiguration &config = streamConfig.config;\n> +\t\tformatToConfigs[config.pixelFormat].push_back(streamConfig);\n> +\n> +\t}\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(unsortedSize);\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> +\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> +\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 = std::nullopt;\n> +\t\t\t}\n> +\t\t}\n> +\t\tLOG(HAL, Debug) << \"Insert \" << nv12Configs.back().config.toString();\n> +\t\tsortedConfigs.push_back(std::move(nv12Configs.back()));\n> +\t\tnv12Configs.pop_back();\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 = std::nullopt;\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\tif (configs.empty()) {\n> +\t\t\t\tit = formatToConfigs.erase(it);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tLOG(HAL, Debug) << \"Insert \" << configs.back().config.toString();\n> +\t\t\tsortedConfigs.push_back(std::move(configs.back()));\n> +\t\t\tconfigs.pop_back();\n> +\t\t\tit++;\n> +\t\t}\n> +\t}\n> +\tassert(sortedConfigs.size() == unsortedSize);\n\nShould we assert(unsortedConfigs == 0) too?\n\nBut I don't think it's a blocker or required.\n\nThe sorting seems like a good idea otherwise.\n\n\n> +\n> +\treturn sortedConfigs;\n> +}\n> +} /* namespace */\n> \n> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> \t\t\t\t\t int flags)\n> @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\tstreamConfigs[index].types.push_back(type);\n> \t}\n> \n> +\tstreamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs),\n> +\t\t\t\t\t\t jpegStream);\n\nIt's hard to see here, but I assume we now sort before we infer any\nindexes, so the index remains valid.\n\nAs long as that statement is true,\n\nReviewed-by: Kieran Bingham \n\n\n\n> \tfor (const auto &streamConfig : streamConfigs) {\n> \t\tconfig_->addConfiguration(streamConfig.config);\n> \t\tfor (size_t i = 0; i < streamConfig.streams.size(); ++i) {\n> --\n> 2.29.2.576.ga3fc446d84-goog\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 9FF14BD80A\n\tfor ;\n\tWed, 9 Dec 2020 11:10:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B99767E6E;\n\tWed, 9 Dec 2020 12:10:20 +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 BCB3F635D3\n\tfor ;\n\tWed, 9 Dec 2020 12:10:18 +0100 (CET)","from [192.168.0.217]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33D95DD;\n\tWed, 9 Dec 2020 12:10:18 +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=\"bihhqjbo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607512218;\n\tbh=op0j7BP+0xGCBefPFCIMpyqBbTy4eawAhfiQfqRxS/c=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=bihhqjbo/tmn4swKZn60M3hsvcTfRH6qZioRzA8y1nIteo3YI16jwoV/jRTR60cme\n\tj8M8LwjG5RhVwUYL9DsTpqIrJv3yHXR8yRXsa7FxtYhWo9EgAbdniGLeeWn6ovAvJY\n\t2VVJy8FCeU+7+aFG2sLUN3EFpW6L1ZO+2pTh14M0=","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20201209055410.3232987-1-hiroh@chromium.org>\n\t<20201209055410.3232987-3-hiroh@chromium.org>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<3b3c9d27-f4ed-a6d8-dc5d-f85f9d2b80a8@ideasonboard.com>","Date":"Wed, 9 Dec 2020 11:10:15 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201209055410.3232987-3-hiroh@chromium.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v5 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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14168,"web_url":"https://patchwork.libcamera.org/comment/14168/","msgid":"","date":"2020-12-09T11:15:28","subject":"Re: [libcamera-devel] [PATCH v5 3/3] android: camera_device:\n\tReorder configurations before requesting","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"One more thought,\n\nOn 09/12/2020 11:10, Kieran Bingham wrote:\n> Hi Hiro,\n> \n> On 09/12/2020 05:54, 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>> ---\n>> src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++-\n>> 1 file changed, 107 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index b7bf3d88..c9e0ec98 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -9,6 +9,7 @@\n>> #include \"camera_ops.h\"\n>> #include \"post_processor.h\"\n>>\n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -27,6 +28,8 @@\n>>\n>> using namespace libcamera;\n>>\n>> +LOG_DECLARE_CATEGORY(HAL)\n>> +\n>> namespace {\n>>\n>> /*\n>> @@ -140,9 +143,109 @@ struct Camera3StreamConfig {\n>> \tstd::vector types;\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>> +std::vector sortCamera3StreamConfigs(\n>> +\tstd::vector unsortedConfigs,\n>> +\tconst camera3_stream_t *jpegStream)\n>> +{\n>> +\tconst size_t unsortedSize = unsortedConfigs.size();\n>> +\tstd::optional jpegConfig = std::nullopt;\n>> +\n>> +\tif (jpegStream) {\n>> +\t\tfor (size_t i = 0; i < unsortedSize; ++i) {\n>> +\t\t\tconst auto &streams = unsortedConfigs[i].streams;\n>> +\t\t\tif (std::find(streams.begin(), streams.end(),\n>> +\t\t\t\t jpegStream) != streams.end()) {\n>> +\t\t\t\tjpegConfig = std::move(unsortedConfigs[i]);\n>> +\t\t\t\tunsortedConfigs.erase(unsortedConfigs.begin() + i);\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t\tif (!jpegConfig)\n>> +\t\t\tLOG(HAL, Fatal) << \"No Camera3StreamConfig is found for Jpeg\";\n>> +\t}\n>> +\n>> +\tstd::map> formatToConfigs;\n>> +\tfor (const auto &streamConfig : unsortedConfigs) {\n>> +\t\tconst StreamConfiguration &config = streamConfig.config;\n>> +\t\tformatToConfigs[config.pixelFormat].push_back(streamConfig);\n>> +\n>> +\t}\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(unsortedSize);\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>> +\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>> +\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 = std::nullopt;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t\tLOG(HAL, Debug) << \"Insert \" << nv12Configs.back().config.toString();\n>> +\t\tsortedConfigs.push_back(std::move(nv12Configs.back()));\n>> +\t\tnv12Configs.pop_back();\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 = std::nullopt;\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\tif (configs.empty()) {\n>> +\t\t\t\tit = formatToConfigs.erase(it);\n>> +\t\t\t\tcontinue;\n>> +\t\t\t}\n>> +\t\t\tLOG(HAL, Debug) << \"Insert \" << configs.back().config.toString();\n>> +\t\t\tsortedConfigs.push_back(std::move(configs.back()));\n>> +\t\t\tconfigs.pop_back();\n>> +\t\t\tit++;\n>> +\t\t}\n>> +\t}\n>> +\tassert(sortedConfigs.size() == unsortedSize);\n> \n> Should we assert(unsortedConfigs == 0) too?\n\nWe use a LOG(Fatal) above, which is an assert with a print...\n\nWe could either use that, or we have our own ASSERT macro defined, which\nwe use throughout libcamera.\n\nBut that's an easy change to capitalise to the macro instead, and could\nperhaps be done when applying.\n\n--\nKieran\n\n\n\n> But I don't think it's a blocker or required.\n> \n> The sorting seems like a good idea otherwise.\n> \n> \n>> +\n>> +\treturn sortedConfigs;\n>> +}\n>> +} /* namespace */\n>>\n>> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n>> \t\t\t\t\t int flags)\n>> @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>> \t\tstreamConfigs[index].types.push_back(type);\n>> \t}\n>>\n>> +\tstreamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs),\n>> +\t\t\t\t\t\t jpegStream);\n> \n> It's hard to see here, but I assume we now sort before we infer any\n> indexes, so the index remains valid.\n> \n> As long as that statement is true,\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> \n>> \tfor (const auto &streamConfig : streamConfigs) {\n>> \t\tconfig_->addConfiguration(streamConfig.config);\n>> \t\tfor (size_t i = 0; i < streamConfig.streams.size(); ++i) {\n>> --\n>> 2.29.2.576.ga3fc446d84-goog\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5AAE4BD808\n\tfor ;\n\tWed, 9 Dec 2020 11:15:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DCF8C635F3;\n\tWed, 9 Dec 2020 12:15:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B3AB635D3\n\tfor ;\n\tWed, 9 Dec 2020 12:15:32 +0100 (CET)","from [192.168.0.217]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B249CDD;\n\tWed, 9 Dec 2020 12:15:31 +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=\"C9zLBu3R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607512531;\n\tbh=BWXaHIuHvevP4O5ia13XO1SB3mEzCTm9AsF9CRrON90=;\n\th=Reply-To:Subject:From:To:References:Date:In-Reply-To:From;\n\tb=C9zLBu3RQHVS0Kwv5/yZWhJrm29ZVJpruf/W0mmZrbk/QF3d9fR4ROut6NMa9jBOp\n\tKirLe+cwJV+RFlBIGs2068Ar9ziAwDaHqWM46iRlVmVkG0cHKqRuQUVpQ35cHNtyx0\n\t2Wmj8EXw2zjaCE1CymJHHBqUr14IXH9cTUZK4FDc=","From":"Kieran Bingham ","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20201209055410.3232987-1-hiroh@chromium.org>\n\t<20201209055410.3232987-3-hiroh@chromium.org>\n\t<3b3c9d27-f4ed-a6d8-dc5d-f85f9d2b80a8@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 9 Dec 2020 11:15:28 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<3b3c9d27-f4ed-a6d8-dc5d-f85f9d2b80a8@ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v5 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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14173,"web_url":"https://patchwork.libcamera.org/comment/14173/","msgid":"","date":"2020-12-09T14:33:17","subject":"Re: [libcamera-devel] [PATCH v5 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 Kieran,\n\nOn Wed, Dec 9, 2020 at 8:15 PM Kieran Bingham\n wrote:\n>\n> One more thought,\n>\n> On 09/12/2020 11:10, Kieran Bingham wrote:\n> > Hi Hiro,\n> >\n> > On 09/12/2020 05:54, 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> >> ---\n> >> src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++-\n> >> 1 file changed, 107 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index b7bf3d88..c9e0ec98 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -9,6 +9,7 @@\n> >> #include \"camera_ops.h\"\n> >> #include \"post_processor.h\"\n> >>\n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> @@ -27,6 +28,8 @@\n> >>\n> >> using namespace libcamera;\n> >>\n> >> +LOG_DECLARE_CATEGORY(HAL)\n> >> +\n> >> namespace {\n> >>\n> >> /*\n> >> @@ -140,9 +143,109 @@ struct Camera3StreamConfig {\n> >> std::vector types;\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> >> +std::vector sortCamera3StreamConfigs(\n> >> + std::vector unsortedConfigs,\n> >> + const camera3_stream_t *jpegStream)\n> >> +{\n> >> + const size_t unsortedSize = unsortedConfigs.size();\n> >> + std::optional jpegConfig = std::nullopt;\n> >> +\n> >> + if (jpegStream) {\n> >> + for (size_t i = 0; i < unsortedSize; ++i) {\n> >> + const auto &streams = unsortedConfigs[i].streams;\n> >> + if (std::find(streams.begin(), streams.end(),\n> >> + jpegStream) != streams.end()) {\n> >> + jpegConfig = std::move(unsortedConfigs[i]);\n> >> + unsortedConfigs.erase(unsortedConfigs.begin() + i);\n> >> + break;\n> >> + }\n> >> + }\n> >> + if (!jpegConfig)\n> >> + LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for Jpeg\";\n> >> + }\n> >> +\n> >> + std::map> formatToConfigs;\n> >> + for (const auto &streamConfig : unsortedConfigs) {\n> >> + const StreamConfiguration &config = streamConfig.config;\n> >> + formatToConfigs[config.pixelFormat].push_back(streamConfig);\n> >> +\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(unsortedSize);\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> >> + * 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> >> +\n> >> + if (nv12LargestSize < nv12SizeForJpeg) {\n> >> + LOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> >> + sortedConfigs.push_back(std::move(*jpegConfig));\n> >> + jpegConfig = std::nullopt;\n> >> + }\n> >> + }\n> >> + LOG(HAL, Debug) << \"Insert \" << nv12Configs.back().config.toString();\n> >> + sortedConfigs.push_back(std::move(nv12Configs.back()));\n> >> + nv12Configs.pop_back();\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 = std::nullopt;\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> >> + if (configs.empty()) {\n> >> + it = formatToConfigs.erase(it);\n> >> + continue;\n> >> + }\n> >> + LOG(HAL, Debug) << \"Insert \" << configs.back().config.toString();\n> >> + sortedConfigs.push_back(std::move(configs.back()));\n> >> + configs.pop_back();\n> >> + it++;\n> >> + }\n> >> + }\n> >> + assert(sortedConfigs.size() == unsortedSize);\n> >\n> > Should we assert(unsortedConfigs == 0) too?\n>\n> We use a LOG(Fatal) above, which is an assert with a print...\n>\n> We could either use that, or we have our own ASSERT macro defined, which\n> we use throughout libcamera.\n>\n> But that's an easy change to capitalise to the macro instead, and could\n> perhaps be done when applying.\n>\n\nSure thing. Please feel free to modify the parts in applying.\n\nRegards,\n-Hiro\n\n> --\n> Kieran\n>\n>\n>\n> > But I don't think it's a blocker or required.\n> >\n> > The sorting seems like a good idea otherwise.\n> >\n> >\n> >> +\n> >> + return sortedConfigs;\n> >> +}\n> >> +} /* namespace */\n> >>\n> >> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> >> int flags)\n> >> @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >> streamConfigs[index].types.push_back(type);\n> >> }\n> >>\n> >> + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs),\n> >> + jpegStream);\n> >\n> > It's hard to see here, but I assume we now sort before we infer any\n> > indexes, so the index remains valid.\n> >\n> > As long as that statement is true,\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> >\n> >> for (const auto &streamConfig : streamConfigs) {\n> >> config_->addConfiguration(streamConfig.config);\n> >> for (size_t i = 0; i < streamConfig.streams.size(); ++i) {\n> >> --\n> >> 2.29.2.576.ga3fc446d84-goog\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n> >>\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 E4336BD80A\n\tfor ;\n\tWed, 9 Dec 2020 14:33:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77B1C635D3;\n\tWed, 9 Dec 2020 15:33:30 +0100 (CET)","from mail-ej1-x642.google.com (mail-ej1-x642.google.com\n\t[IPv6:2a00:1450:4864:20::642])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7AEFD600F9\n\tfor ;\n\tWed, 9 Dec 2020 15:33:28 +0100 (CET)","by mail-ej1-x642.google.com with SMTP id m19so2390146ejj.11\n\tfor ;\n\tWed, 09 Dec 2020 06:33:28 -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=\"BJgR2pLU\"; 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=ynU+Pa3qHywm5g3TzUK4SJGLkDgqyUZsTr1qdzSFpx8=;\n\tb=BJgR2pLUtQBqouuLZQARvv5lN9lJjLTtOKc0g3zYe0wGAmNx0PTTfHSSTXZ+EZ0XyG\n\tH9OVfBU3nyJWDZGugXA9PI1hMBaMabySfWlZVPqfzi8Y5oz+zqN+8oLjKCQ8Pt78Ahlz\n\t9zLoF366Glx4bEmIn6gam2HhGGEBKaMrfv800=","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=ynU+Pa3qHywm5g3TzUK4SJGLkDgqyUZsTr1qdzSFpx8=;\n\tb=eDFF3v1FeUxobLeY4EOBYloIrcq2aGPc98I5uf2oqvFu1K7qk/eWL/FONA66HYpOXd\n\t9rvSzVgypzX1Q8OCaoSt1aF9ZKoYHir+ojxYoG+tckxFRVl9R4Rtjzud6BQKt4U2UhuB\n\tvCH+rzqVyo6F/Sxsl3WDr+f48/xMjr9EpQeR44QODiIIyhMCgC3nemfPxQqhI9hQ2qfD\n\t8d+c3mtHf5kEUFKM0oszxZSV9QqcsEKi9K3evHMW/tLaduPyKELtXY38NxWu41q//cYS\n\tF1roYenuYfNEVyrfZWeLimW4DurDDVyYX//THcuIB9S7zSen6VKD/aHPgB2KluCn6nP1\n\td8Sg==","X-Gm-Message-State":"AOAM5300tbDGLI0/i7K9SiK2mCSazC0ghzeqjknimhlM0ciN3h3yNB8b\n\tSl6DCbNICZHy1AtyCsP6WYIJFEvFtBRd1+N7F/JdWPCysFRyAg==","X-Google-Smtp-Source":"ABdhPJxeiXa59minLek3g/Bn5f7ujzUXmgQeAGBnA+0wpUiqT9nWkv/bcdFYO6I7bB9VJXosp5g2P1oa8aCIbI3YBGY=","X-Received":"by 2002:a17:906:dd3:: with SMTP id\n\tp19mr2251677eji.221.1607524408032; \n\tWed, 09 Dec 2020 06:33:28 -0800 (PST)","MIME-Version":"1.0","References":"<20201209055410.3232987-1-hiroh@chromium.org>\n\t<20201209055410.3232987-3-hiroh@chromium.org>\n\t<3b3c9d27-f4ed-a6d8-dc5d-f85f9d2b80a8@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 9 Dec 2020 23:33:17 +0900","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v5 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":14176,"web_url":"https://patchwork.libcamera.org/comment/14176/","msgid":"<0976a653-21a5-7e67-f8ec-afe75209722a@uajain.com>","date":"2020-12-09T15:35:49","subject":"Re: [libcamera-devel] [PATCH v5 3/3] android: camera_device:\n\tReorder configurations before requesting","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"HI Hiro,\n\nAfter much careful reading of the previous iterations,\n\nOn 12/9/20 11:24 AM, 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 \nReviewed-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++-\n> 1 file changed, 107 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b7bf3d88..c9e0ec98 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,6 +9,7 @@\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -27,6 +28,8 @@\n>\n> using namespace libcamera;\n>\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> namespace {\n>\n> /*\n> @@ -140,9 +143,109 @@ struct Camera3StreamConfig {\n> \tstd::vector types;\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> +std::vector sortCamera3StreamConfigs(\n> +\tstd::vector unsortedConfigs,\n> +\tconst camera3_stream_t *jpegStream)\n> +{\n> +\tconst size_t unsortedSize = unsortedConfigs.size();\n> +\tstd::optional jpegConfig = std::nullopt;\n> +\n> +\tif (jpegStream) {\n> +\t\tfor (size_t i = 0; i < unsortedSize; ++i) {\n> +\t\t\tconst auto &streams = unsortedConfigs[i].streams;\n> +\t\t\tif (std::find(streams.begin(), streams.end(),\n> +\t\t\t\t jpegStream) != streams.end()) {\n> +\t\t\t\tjpegConfig = std::move(unsortedConfigs[i]);\n> +\t\t\t\tunsortedConfigs.erase(unsortedConfigs.begin() + i);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (!jpegConfig)\n> +\t\t\tLOG(HAL, Fatal) << \"No Camera3StreamConfig is found for Jpeg\";\n> +\t}\n> +\n> +\tstd::map> formatToConfigs;\n> +\tfor (const auto &streamConfig : unsortedConfigs) {\n> +\t\tconst StreamConfiguration &config = streamConfig.config;\n> +\t\tformatToConfigs[config.pixelFormat].push_back(streamConfig);\n> +\n> +\t}\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(unsortedSize);\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> +\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> +\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 = std::nullopt;\n> +\t\t\t}\n> +\t\t}\n> +\t\tLOG(HAL, Debug) << \"Insert \" << nv12Configs.back().config.toString();\n> +\t\tsortedConfigs.push_back(std::move(nv12Configs.back()));\n> +\t\tnv12Configs.pop_back();\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 = std::nullopt;\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\tif (configs.empty()) {\n> +\t\t\t\tit = formatToConfigs.erase(it);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tLOG(HAL, Debug) << \"Insert \" << configs.back().config.toString();\n> +\t\t\tsortedConfigs.push_back(std::move(configs.back()));\n> +\t\t\tconfigs.pop_back();\n> +\t\t\tit++;\n> +\t\t}\n> +\t}\n> +\tassert(sortedConfigs.size() == unsortedSize);\n> +\n> +\treturn sortedConfigs;\n> +}\n> +} /* namespace */\n>\n> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> \t\t\t\t\t int flags)\n> @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\tstreamConfigs[index].types.push_back(type);\n> \t}\n>\n> +\tstreamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs),\n> +\t\t\t\t\t\t jpegStream);\n> \tfor (const auto &streamConfig : streamConfigs) {\n> \t\tconfig_->addConfiguration(streamConfig.config);\n> \t\tfor (size_t i = 0; i < streamConfig.streams.size(); ++i) {\n> --\n> 2.29.2.576.ga3fc446d84-goog\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 C3744BD808\n\tfor ;\n\tWed, 9 Dec 2020 15:35:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50BC6635D3;\n\tWed, 9 Dec 2020 16:35:55 +0100 (CET)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88DC3600F9\n\tfor ;\n\tWed, 9 Dec 2020 16:35:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"D7zC4R5H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1607528152; bh=ny20Vy5oRB7Xes9sNwXkDybU9H/1harnK0clA0abphc=;\n\th=Subject:To:References:From:In-Reply-To;\n\tb=D7zC4R5HcAK+TOkmXxFNKvZESXIKMCBtkcf081NN0rMunO60DOzpj2Iv0CBI4joJH\n\t9FPCAu8jCOgkCWdhHxH62nfZKoBlv9wZeSVoT3u7HL9D0SyHac7U64use0Ll/kQQzq\n\tUEqMHzoo9SUBIaM8SIQHTP+wpx51FrZ+ndwJ4bTgZAoMEybROJugOUh+PmlZJ7BuZr\n\tRCdPhV7eVPzDW+R9XLMOlpcjOWsVfHRPfZcJuwKmF6UGbm9cJ+Ece7cd09m40BKfbE\n\tAxvKer9cNc1lj/gowNl4I35+gYkEMTdu1u4pKdxsevmftI5qOeOezdYSkEpQtQxuWz\n\tXiIJ9d5ROF5JQ==","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20201209055410.3232987-1-hiroh@chromium.org>\n\t<20201209055410.3232987-3-hiroh@chromium.org>","From":"Umang Jain ","Message-ID":"<0976a653-21a5-7e67-f8ec-afe75209722a@uajain.com>","Date":"Wed, 9 Dec 2020 21:05:49 +0530","Mime-Version":"1.0","In-Reply-To":"<20201209055410.3232987-3-hiroh@chromium.org>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 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","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14211,"web_url":"https://patchwork.libcamera.org/comment/14211/","msgid":"","date":"2020-12-10T22:25:02","subject":"Re: [libcamera-devel] [PATCH v5 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 Wed, Dec 09, 2020 at 05:54:10AM +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> ---\n> src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++-\n> 1 file changed, 107 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b7bf3d88..c9e0ec98 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,6 +9,7 @@\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -27,6 +28,8 @@\n> \n> using namespace libcamera;\n> \n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> namespace {\n> \n> /*\n> @@ -140,9 +143,109 @@ struct Camera3StreamConfig {\n> \tstd::vector types;\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> +std::vector sortCamera3StreamConfigs(\n> +\tstd::vector unsortedConfigs,\n> +\tconst camera3_stream_t *jpegStream)\n\nHow about making this a private member function ?\n\n> +{\n> +\tconst size_t unsortedSize = unsortedConfigs.size();\n> +\tstd::optional jpegConfig = std::nullopt;\n\nstd::nullopt is the default value, so you can write\n\n\tstd::optional jpegConfig;\n\n> +\n> +\tif (jpegStream) {\n> +\t\tfor (size_t i = 0; i < unsortedSize; ++i) {\n> +\t\t\tconst auto &streams = unsortedConfigs[i].streams;\n> +\t\t\tif (std::find(streams.begin(), streams.end(),\n> +\t\t\t\t jpegStream) != streams.end()) {\n> +\t\t\t\tjpegConfig = std::move(unsortedConfigs[i]);\n> +\t\t\t\tunsortedConfigs.erase(unsortedConfigs.begin() + i);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (!jpegConfig)\n> +\t\t\tLOG(HAL, Fatal) << \"No Camera3StreamConfig is found for Jpeg\";\n> +\t}\n> +\n> +\tstd::map> formatToConfigs;\n\nShould the key be a PixelFormat instead of a uint32_t ?\n\n> +\tfor (const auto &streamConfig : unsortedConfigs) {\n> +\t\tconst StreamConfiguration &config = streamConfig.config;\n> +\t\tformatToConfigs[config.pixelFormat].push_back(streamConfig);\n> +\n> +\t}\n\nI wonder if this could be simplified to\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(streams.begin(), streams.end(),\n\t\t\t\t jpegStream) != streams.end()) {\n\t\t\t\tjpegConfig = &streamConfig;\n\t\t\t\tcontinue;\n\t\t\t}\n\t\t}\n\n\t\tconst StreamConfiguration &config = streamConfig.config;\n\t\tformatToConfigs[config.pixelFormat].push_back(streamConfig);\n\t}\n\n\tif (jpegStream && !jpegConfig)\n\t\tLOG(HAL, Fatal) << \"No Camera3StreamConfig is found for JPEG\";\n\nThe advantage is that unsortedStream would now be const. formatToConfigs\ncould then be a map of PixelFormat to vector of pointers to\nCamera3StreamConfig, avoiding a copy. The only copy would be done when\ncreating the entries in sortedConfigs.\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(unsortedSize);\n\nA blank line would be nice here.\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\nHere too.\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> +\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 = std::nullopt;\n> +\t\t\t}\n> +\t\t}\n\nAnd here too.\n\n> +\t\tLOG(HAL, Debug) << \"Insert \" << nv12Configs.back().config.toString();\n> +\t\tsortedConfigs.push_back(std::move(nv12Configs.back()));\n> +\t\tnv12Configs.pop_back();\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 = std::nullopt;\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\t\t\tauto &configs = it->second;\n\n> +\t\t\tif (configs.empty()) {\n> +\t\t\t\tit = formatToConfigs.erase(it);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nThis could be moved to the end, ...\n\n> +\t\t\tLOG(HAL, Debug) << \"Insert \" << configs.back().config.toString();\n> +\t\t\tsortedConfigs.push_back(std::move(configs.back()));\n> +\t\t\tconfigs.pop_back();\n> +\t\t\tit++;\n\n... it would become\n\n\t\t\tconfigs.pop_back();\n\t\t\tif (configs.empty())\n\t\t\t\tit = formatToConfigs.erase(it);\n\t\t\telse\n\t\t\t\tit++;\n\n> +\t\t}\n> +\t}\n> +\tassert(sortedConfigs.size() == unsortedSize);\n> +\n> +\treturn sortedConfigs;\n> +}\n> +} /* namespace */\n> \n> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> \t\t\t\t\t int flags)\n> @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\tstreamConfigs[index].types.push_back(type);\n> \t}\n> \n> +\tstreamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs),\n> +\t\t\t\t\t\t jpegStream);\n\nThis is a bit of a weird function signature. Could we sort the vector\nin-place ?\n\n\tsortCamera3StreamConfigs(streamConfigs, jpegStream);\n\nIt doesn't mean that the implementation must be in-place,\nsortCamera3StreamConfigs() can make an internal copy.\n\nAs I wanted to test my suggestions, I've ended up reworking the code to\nmake sure it would compile. I've pushed the result to\nhttps://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=android/hiro\nin case it could be useful.\n\n> \tfor (const auto &streamConfig : streamConfigs) {\n> \t\tconfig_->addConfiguration(streamConfig.config);\n> \t\tfor (size_t i = 0; i < streamConfig.streams.size(); ++i) {","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 6AAD3BD808\n\tfor ;\n\tThu, 10 Dec 2020 22:25:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F03A6608A9;\n\tThu, 10 Dec 2020 23:25:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE80A60887\n\tfor ;\n\tThu, 10 Dec 2020 23:25:07 +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 6D1C925E;\n\tThu, 10 Dec 2020 23:25:07 +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=\"KtXIKFMV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607639107;\n\tbh=x4tgtUgX1k3I8hliG5+wLVO0ZOg6O8PMl/MiZpFJaNg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KtXIKFMV39eJcAzlGLGxy66wbLU7JwsFDeB5p2BU46nt0U5lA3NnbuIy87yn8It0p\n\t1sUCXkrRo24DStBOkzoKXGqWv3eKTSIHOuk+72pdbJQIFRARl6i6J8/EegEkvOmnXO\n\tYK3F97bE92nrEumeH5N3xFZRFjcveuJ4qtpRpvkg=","Date":"Fri, 11 Dec 2020 00:25:02 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20201209055410.3232987-1-hiroh@chromium.org>\n\t<20201209055410.3232987-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201209055410.3232987-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v5 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":14212,"web_url":"https://patchwork.libcamera.org/comment/14212/","msgid":"","date":"2020-12-11T06:02:28","subject":"Re: [libcamera-devel] [PATCH v5 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, Kieran, Laurent and Umang\n\nThanks for reviewing!\n\nOn Fri, Dec 11, 2020 at 7:25 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Wed, Dec 09, 2020 at 05:54:10AM +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> > ---\n> > src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++-\n> > 1 file changed, 107 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index b7bf3d88..c9e0ec98 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -9,6 +9,7 @@\n> > #include \"camera_ops.h\"\n> > #include \"post_processor.h\"\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -27,6 +28,8 @@\n> >\n> > using namespace libcamera;\n> >\n> > +LOG_DECLARE_CATEGORY(HAL)\n> > +\n> > namespace {\n> >\n> > /*\n> > @@ -140,9 +143,109 @@ struct Camera3StreamConfig {\n> > std::vector types;\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> > +std::vector sortCamera3StreamConfigs(\n> > + std::vector unsortedConfigs,\n> > + const camera3_stream_t *jpegStream)\n>\n> How about making this a private member function ?\n>\n\nI would keep here so that we don't expose Camera3StreamConfig in\ncamera_device.h.\n\nRegards,\n-Hiro\n\n> > +{\n> > + const size_t unsortedSize = unsortedConfigs.size();\n> > + std::optional jpegConfig = std::nullopt;\n>\n> std::nullopt is the default value, so you can write\n>\n> std::optional jpegConfig;\n>\n> > +\n> > + if (jpegStream) {\n> > + for (size_t i = 0; i < unsortedSize; ++i) {\n> > + const auto &streams = unsortedConfigs[i].streams;\n> > + if (std::find(streams.begin(), streams.end(),\n> > + jpegStream) != streams.end()) {\n> > + jpegConfig = std::move(unsortedConfigs[i]);\n> > + unsortedConfigs.erase(unsortedConfigs.begin() + i);\n> > + break;\n> > + }\n> > + }\n> > + if (!jpegConfig)\n> > + LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for Jpeg\";\n> > + }\n> > +\n> > + std::map> formatToConfigs;\n>\n> Should the key be a PixelFormat instead of a uint32_t ?\n>\n> > + for (const auto &streamConfig : unsortedConfigs) {\n> > + const StreamConfiguration &config = streamConfig.config;\n> > + formatToConfigs[config.pixelFormat].push_back(streamConfig);\n> > +\n> > + }\n>\n> I wonder if this could be simplified to\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(streams.begin(), streams.end(),\n> jpegStream) != streams.end()) {\n> jpegConfig = &streamConfig;\n> continue;\n> }\n> }\n>\n> const StreamConfiguration &config = streamConfig.config;\n> formatToConfigs[config.pixelFormat].push_back(streamConfig);\n> }\n>\n> if (jpegStream && !jpegConfig)\n> LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for JPEG\";\n>\n> The advantage is that unsortedStream would now be const. formatToConfigs\n> could then be a map of PixelFormat to vector of pointers to\n> Camera3StreamConfig, avoiding a copy. The only copy would be done when\n> creating the entries in sortedConfigs.\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(unsortedSize);\n>\n> A blank line would be nice here.\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> Here too.\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> > +\n> > + if (nv12LargestSize < nv12SizeForJpeg) {\n> > + LOG(HAL, Debug) << \"Insert \" << jpegConfig->config.toString();\n> > + sortedConfigs.push_back(std::move(*jpegConfig));\n> > + jpegConfig = std::nullopt;\n> > + }\n> > + }\n>\n> And here too.\n>\n> > + LOG(HAL, Debug) << \"Insert \" << nv12Configs.back().config.toString();\n> > + sortedConfigs.push_back(std::move(nv12Configs.back()));\n> > + nv12Configs.pop_back();\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 = std::nullopt;\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 = it->second;\n>\n> > + if (configs.empty()) {\n> > + it = formatToConfigs.erase(it);\n> > + continue;\n> > + }\n>\n> This could be moved to the end, ...\n>\n> > + LOG(HAL, Debug) << \"Insert \" << configs.back().config.toString();\n> > + sortedConfigs.push_back(std::move(configs.back()));\n> > + configs.pop_back();\n> > + it++;\n>\n> ... it would become\n>\n> configs.pop_back();\n> if (configs.empty())\n> it = formatToConfigs.erase(it);\n> else\n> it++;\n>\n> > + }\n> > + }\n> > + assert(sortedConfigs.size() == unsortedSize);\n> > +\n> > + return sortedConfigs;\n> > +}\n> > +} /* namespace */\n> >\n> > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > int flags)\n> > @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > streamConfigs[index].types.push_back(type);\n> > }\n> >\n> > + streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs),\n> > + jpegStream);\n>\n> This is a bit of a weird function signature. Could we sort the vector\n> in-place ?\n>\n> sortCamera3StreamConfigs(streamConfigs, jpegStream);\n>\n> It doesn't mean that the implementation must be in-place,\n> sortCamera3StreamConfigs() can make an internal copy.\n>\n> As I wanted to test my suggestions, I've ended up reworking the code to\n> make sure it would compile. I've pushed the result to\n> https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=android/hiro\n> in case it could be useful.\n>\n> > for (const auto &streamConfig : streamConfigs) {\n> > config_->addConfiguration(streamConfig.config);\n> > for (size_t i = 0; i < streamConfig.streams.size(); ++i) {\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 AD0ECBD808\n\tfor ;\n\tFri, 11 Dec 2020 06:02:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26423634A0;\n\tFri, 11 Dec 2020 07:02:40 +0100 (CET)","from mail-ej1-x644.google.com (mail-ej1-x644.google.com\n\t[IPv6:2a00:1450:4864:20::644])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D89A60321\n\tfor ;\n\tFri, 11 Dec 2020 07:02:39 +0100 (CET)","by mail-ej1-x644.google.com with SMTP id x16so10755097ejj.7\n\tfor ;\n\tThu, 10 Dec 2020 22:02:39 -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=\"KP4Xi4Ot\"; 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=iLKLErhPdPpu92BFSx6B91gx2OxUo2/oq6iK+4O0A5M=;\n\tb=KP4Xi4OtA7qhD4SfLnmNgXBz0bNxsnFuxyUYA2B3nTdcIZh0Msy15SuTr5GB6HYF4T\n\thBD0s5gVxzx9z2WbXWKGvxBr9o0Nv6Zo4Zj3NHSQ/B8fufxQd5W4dYuEdoEHVddQCPjD\n\tWuyGrR/46gJot5XaAKLYbweGAResx7Veqtfcs=","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=iLKLErhPdPpu92BFSx6B91gx2OxUo2/oq6iK+4O0A5M=;\n\tb=AnnJC6Dja6JF81mgDFa5mmB3UYLYH7Cpdn8wTocGB4muM8MjS+JLJIyys3KL3arDt+\n\t47HvF73CKYFcYj5iEVmWYonjq0ta/m87xPpLniAvX80M7Ks659ACAGTuwD7Hvbm58h0x\n\tyz7dz9I2aeKm+qJ3tycNeKTMTKpn8TJAOj9GvW3PKQdqaIBMqqp2N8zMh+A/Xc12F1pR\n\t/VU2idRC+cMHEGRU4B7JQWF5p5BkVH2CHkGt0vgI5snaVvgTXNr8w2prwpFPqrA+6uT9\n\trBbLjBmCBqxO8hOxy+bHBQFd78RL+ZCGyuOxScq5SyZRZY8tkX7LOvX+4cI0qjoS8TK4\n\t9nIA==","X-Gm-Message-State":"AOAM53336WgzHuiii7f9ALnlJ/IExhlPuS6T6gJBW8BtJA3/gigGNY6B\n\tgb4muAY3LwNNLbfmycmej+pNgho7tSzLBJRgHiD85Dw3/Y4=","X-Google-Smtp-Source":"ABdhPJzZvwijZfMpO2Awm+5VzzZm04s36PhCs9q7jocdAwVx1T4iKEbX14UAOVnzyXh1dCA52dPF1pGr5dLVoDxRquk=","X-Received":"by 2002:a17:906:6010:: with SMTP id\n\to16mr9584706ejj.55.1607666558787; \n\tThu, 10 Dec 2020 22:02:38 -0800 (PST)","MIME-Version":"1.0","References":"<20201209055410.3232987-1-hiroh@chromium.org>\n\t<20201209055410.3232987-3-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 11 Dec 2020 15:02:28 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v5 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\" "}}]