[{"id":14097,"web_url":"https://patchwork.libcamera.org/comment/14097/","msgid":"<20201207110256.jlipupfi6o7gnkvy@uno.localdomain>","date":"2020-12-07T11:02:56","subject":"Re: [libcamera-devel] [PATCH v3 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 Mon, Dec 07, 2020 at 08:42:18AM +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> src/android/camera_device.cpp | 104 +++++++++++++++++++++++++++++++++-\n> 1 file changed, 102 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b7bf3d88..dcba4e7d 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,104 @@ 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 CameraDevice can accept them as much as\n\ns/CameraDevice/libcamera::Camera/\n\n> + * 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 unsortedStreamConfigs,\n\njust 'unsortedConfigs' ?\n\n> +\tconst camera3_stream_t *jpegStream) {\n> +\tconst size_t unsortedStreamConfigsSize = unsortedStreamConfigs.size();\n> +\tstd::optional streamConfigForJpeg = std::nullopt;\n\nThese can be just 'unsortedSize' and 'jpegConfig'. Up to you.\n\nEmpty line ?\n> +\tif (jpegStream) {\n> +\t\tfor (auto it = unsortedStreamConfigs.begin();\n> +\t\t it != unsortedStreamConfigs.end(); it++) {\n> +\t\t\tconst auto &streams = it->streams;\n> +\t\t\tif (std::find(streams.begin(), streams.end(),\n> +\t\t\t\t jpegStream) != streams.end()) {\n> +\t\t\t\tstreamConfigForJpeg = *it;\n> +\t\t\t\tunsortedStreamConfigs.erase(it);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> + if (!streamConfigForJpeg)\n> + LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for Jpeg\";\n\nWhat happened to indentation ?\nAlso, can this really happen ?\n\n> +\t}\n> +\n> +\tstd::map> formatToConfigs;\n> +\tfor (const auto &streamConfig : unsortedStreamConfigs) {\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\nauto &[] or does the compiler complains ?\n\n> +\t\t/* Sorted by resolution. Smaller is put first. */\n> +\t\tstd::sort(streamConfigs.begin(), streamConfigs.end(),\n> +\t\t\t [](const auto &streamConfigA, const auto &streamConfigB) {\n> +\t\t\t\t const Size &sizeA = streamConfigA.config.size;\n> +\t\t\t\t const Size &sizeB = streamConfigB.config.size;\n> + return sizeA < sizeB;\n> +\t\t\t });\n> +\t}\n> +\n> +\tstd::vector sortedStreamConfigs;\n> + sortedStreamConfigs.reserve(unsortedStreamConfigsSize);\n\nIndentation ?\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 nv12Stream = formatToConfigs.find(formats::NV12);\n> +\tif (nv12Stream != formatToConfigs.end()) {\n> +\t\tauto &nv12StreamConfigs = nv12Stream->second;\n> +\t\tconst Size &nv12LargestSize = nv12StreamConfigs.back().config.size;\n\nEmpty line ?\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 (streamConfigForJpeg &&\n> +\t\t streamConfigForJpeg->config.pixelFormat == formats::NV12) {\n> +\t\t\tconst Size &nv12SizeForJpeg = streamConfigForJpeg->config.size;\n> +\t\t\tif (nv12LargestSize < nv12SizeForJpeg) {\n> +\t\t\t\tsortedStreamConfigs.push_back(*streamConfigForJpeg);\n> +\t\t\t\tstreamConfigForJpeg = std::nullopt;\n> +\t\t\t}\n> +\t\t}\n> +\t\tsortedStreamConfigs.push_back(nv12StreamConfigs.back());\n> +\t\tnv12StreamConfigs.pop_back();\n> +\t}\n> +\n> +\t/* If the configuration for JPEG is there, then put it. */\n> +\tif (streamConfigForJpeg) {\n> +\t\tsortedStreamConfigs.push_back(*streamConfigForJpeg);\n> +\t\tstreamConfigForJpeg = 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& streamConfigs = it->second;\n> +\t\t\tif (streamConfigs.empty()) {\n> +\t\t\t\tit = formatToConfigs.erase(it);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tsortedStreamConfigs.push_back(streamConfigs.back());\n> +\t\t\tstreamConfigs.pop_back();\n> +\t\t\tit++;\n\nAH! this it++ was not here in the previous version :)\nNow I see what you wanted.\n\nThis makes me think that a LOG(Debug) printout of the the sorted\nstream configs would help making sure we obtain what we intend.\n\n\n> +\t\t}\n> +\t}\n> +\tassert(sortedStreamConfigs.size() == unsortedStreamConfigsSize);\n> +\n> +\treturn sortedStreamConfigs;\n> +}\n> +} /* namespace */\n>\n> MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> \t\t\t\t\t int flags)\n> @@ -1333,6 +1431,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","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 03CE4BDB20\n\tfor ;\n\tMon, 7 Dec 2020 11:02:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C79D634C6;\n\tMon, 7 Dec 2020 12:02:48 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D26C60325\n\tfor ;\n\tMon, 7 Dec 2020 12:02:47 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id D65964000F;\n\tMon, 7 Dec 2020 11:02:46 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 7 Dec 2020 12:02:56 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20201207110256.jlipupfi6o7gnkvy@uno.localdomain>","References":"<20201207084218.2307410-1-hiroh@chromium.org>\n\t<20201207084218.2307410-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201207084218.2307410-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 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":14123,"web_url":"https://patchwork.libcamera.org/comment/14123/","msgid":"","date":"2020-12-08T03:41:43","subject":"Re: [libcamera-devel] [PATCH v3 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,\n\nOn Mon, Dec 7, 2020 at 8:02 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Dec 07, 2020 at 08:42:18AM +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> > src/android/camera_device.cpp | 104 +++++++++++++++++++++++++++++++++-\n> > 1 file changed, 102 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index b7bf3d88..dcba4e7d 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,104 @@ 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 CameraDevice can accept them as much as\n>\n> s/CameraDevice/libcamera::Camera/\n>\n> > + * 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 unsortedStreamConfigs,\n>\n> just 'unsortedConfigs' ?\n>\n> > + const camera3_stream_t *jpegStream) {\n> > + const size_t unsortedStreamConfigsSize = unsortedStreamConfigs.size();\n> > + std::optional streamConfigForJpeg = std::nullopt;\n>\n> These can be just 'unsortedSize' and 'jpegConfig'. Up to you.\n>\n> Empty line ?\n> > + if (jpegStream) {\n> > + for (auto it = unsortedStreamConfigs.begin();\n> > + it != unsortedStreamConfigs.end(); it++) {\n> > + const auto &streams = it->streams;\n> > + if (std::find(streams.begin(), streams.end(),\n> > + jpegStream) != streams.end()) {\n> > + streamConfigForJpeg = *it;\n> > + unsortedStreamConfigs.erase(it);\n> > + break;\n> > + }\n> > + }\n> > + if (!streamConfigForJpeg)\n> > + LOG(HAL, Fatal) << \"No Camera3StreamConfig is found for Jpeg\";\n>\n> What happened to indentation ?\n> Also, can this really happen ?\n>\n\nThis must not happen. I originally assert this.\n\n> > + }\n> > +\n> > + std::map> formatToConfigs;\n> > + for (const auto &streamConfig : unsortedStreamConfigs) {\n> > + const StreamConfiguration &config = streamConfig.config;\n> > + formatToConfigs[config.pixelFormat].push_back(streamConfig);\n> > +\n> > + }\n> > + for (auto& [format, streamConfigs] : formatToConfigs) {\n>\n> auto &[] or does the compiler complains ?\n>\n> > + /* Sorted by resolution. Smaller is put first. */\n> > + std::sort(streamConfigs.begin(), streamConfigs.end(),\n> > + [](const auto &streamConfigA, const auto &streamConfigB) {\n> > + const Size &sizeA = streamConfigA.config.size;\n> > + const Size &sizeB = streamConfigB.config.size;\n> > + return sizeA < sizeB;\n> > + });\n> > + }\n> > +\n> > + std::vector sortedStreamConfigs;\n> > + sortedStreamConfigs.reserve(unsortedStreamConfigsSize);\n>\n> Indentation ?\n>\n> > + /*\n> > + * NV12 is the most prioritized format. Put the configuration with NV12\n> > + * and the largest resolution first.\n> > + */\n> > + const auto nv12Stream = formatToConfigs.find(formats::NV12);\n> > + if (nv12Stream != formatToConfigs.end()) {\n> > + auto &nv12StreamConfigs = nv12Stream->second;\n> > + const Size &nv12LargestSize = nv12StreamConfigs.back().config.size;\n>\n> Empty line ?\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 (streamConfigForJpeg &&\n> > + streamConfigForJpeg->config.pixelFormat == formats::NV12) {\n> > + const Size &nv12SizeForJpeg = streamConfigForJpeg->config.size;\n> > + if (nv12LargestSize < nv12SizeForJpeg) {\n> > + sortedStreamConfigs.push_back(*streamConfigForJpeg);\n> > + streamConfigForJpeg = std::nullopt;\n> > + }\n> > + }\n> > + sortedStreamConfigs.push_back(nv12StreamConfigs.back());\n> > + nv12StreamConfigs.pop_back();\n> > + }\n> > +\n> > + /* If the configuration for JPEG is there, then put it. */\n> > + if (streamConfigForJpeg) {\n> > + sortedStreamConfigs.push_back(*streamConfigForJpeg);\n> > + streamConfigForJpeg = 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& streamConfigs = it->second;\n> > + if (streamConfigs.empty()) {\n> > + it = formatToConfigs.erase(it);\n> > + continue;\n> > + }\n> > + sortedStreamConfigs.push_back(streamConfigs.back());\n> > + streamConfigs.pop_back();\n> > + it++;\n>\n> AH! this it++ was not here in the previous version :)\n> Now I see what you wanted.\n>\n> This makes me think that a LOG(Debug) printout of the the sorted\n> stream configs would help making sure we obtain what we intend.\n>\n\nI somehow missed it++ in the previous patch. Sorry!\n\nRegards,\n-Hiro\n>\n> > + }\n> > + }\n> > + assert(sortedStreamConfigs.size() == unsortedStreamConfigsSize);\n> > +\n> > + return sortedStreamConfigs;\n> > +}\n> > +} /* namespace */\n> >\n> > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> > int flags)\n> > @@ -1333,6 +1431,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> > 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","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 1345EBDB20\n\tfor ;\n\tTue, 8 Dec 2020 03:41:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D24C667E6C;\n\tTue, 8 Dec 2020 04:41:57 +0100 (CET)","from mail-ej1-x634.google.com (mail-ej1-x634.google.com\n\t[IPv6:2a00:1450:4864:20::634])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E345560323\n\tfor ;\n\tTue, 8 Dec 2020 04:41:55 +0100 (CET)","by mail-ej1-x634.google.com with SMTP id x16so22608920ejj.7\n\tfor ;\n\tMon, 07 Dec 2020 19:41:55 -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=\"ColR8JUA\"; 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=uKuVGegWcuJL3nqj9EYg5nU63Ad3nXfl5YdQQIeaNiQ=;\n\tb=ColR8JUA2mUwJMZZ6yAbwSArvxmh0F2wTAa4MleKgDYiuGX8Ro2MjuT7HKn2kc4sR9\n\tDjwdlYQsc/lAbZwksZOaXkrzOGcC7KvYXhftg3aoWv4VUR+pjuTmKLP4cshLqGcjgUIr\n\tm7IiKB3Dc3YBNmrbtrDFHTyzuJsDCU00Pd2AE=","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=uKuVGegWcuJL3nqj9EYg5nU63Ad3nXfl5YdQQIeaNiQ=;\n\tb=lkUqcavI91SOb2ilTcJqZziZudv38BQY4pPPhBoCnSquYiKkrMTH9FIs8T57L3INmt\n\t8sZ9Q2nGz+a2V4ZTUiCCLQIG6zphrKgjxnLjdYWoDv/iPYwBNeJU5iyWCnAGdDV3BdoQ\n\tjNO710jWZlvkzqv2Doimw76Ihz8/WwnVgiYeIleUHuMidBuIhDF7beDKxYRzG6XrGMMQ\n\tIL6z5T6xJ0qxFVD6nOqpoOhm71u1Ff9Q6GG7mK5nvSLinc+551gQXO7s5eKg2Bo8eW62\n\tauLflX7A0Ab2JwcIx7PLTA+xaF1nMj3Jd8+wM+h1gRlU0L+Qkj3YCmCNtRogifT6Pzq/\n\tJtcA==","X-Gm-Message-State":"AOAM531AoHU1FnRkVjKWhRtugfgEKw677k4uww+Khy/9/pw33uIavIof\n\tI7iFwVozWyoF0ARsw6krej4kPDfhZaO5b8IqWJhPwZFfRRQ2uw==","X-Google-Smtp-Source":"ABdhPJxDPyaSoXuK/XcXctmIQ/sUF/+NyW+EYjR0SopZaSl05h/YPVu1N4gVCjjIE8Qq9mZrlqiuZJrbloOeAcIVsG0=","X-Received":"by 2002:a17:906:94d4:: with SMTP id\n\td20mr21448134ejy.475.1607398915600; \n\tMon, 07 Dec 2020 19:41:55 -0800 (PST)","MIME-Version":"1.0","References":"<20201207084218.2307410-1-hiroh@chromium.org>\n\t<20201207084218.2307410-3-hiroh@chromium.org>\n\t<20201207110256.jlipupfi6o7gnkvy@uno.localdomain>","In-Reply-To":"<20201207110256.jlipupfi6o7gnkvy@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 8 Dec 2020 12:41:43 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]