[{"id":30948,"web_url":"https://patchwork.libcamera.org/comment/30948/","msgid":"","date":"2024-08-28T13:10:51","subject":"Re: [PATCH v2 3/3] libcamera: android: Add face detection control\n\tsupport","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hello\n\nOn Fri, Aug 23, 2024 at 02:29:10PM GMT, Harvey Yang wrote:\n> From: Yudhistira Erlandinata \n>\n> Allow Android HAL adapter to pass the face detection metadata control to\n> the pipeline and also send face detection metadata to the camera client\n> if the pipeline generates it.\n>\n> Squashed CL:\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5065292\n\nPlease drop, unrelated to libcamera\n\n>\n> Don't use ControlInfoMap::count() and ControlInfoMap::at() because\n> its internal assumption is wrong: The ControlIdMap and its idmap have a\n> 1:1 mapping between their entries.\n\nNot sure I got this :)\n\n>\n> BUG=b:308713855\n> TEST=emerge-geralt libcamera-mtkisp7\n\nditto, please drop, it's your internal stuff.\n\n>\n> Signed-off-by: Yudhistira Erlandinata \n> Co-developed-by: Harvey Yang \n> ---\n> src/android/camera_capabilities.cpp | 41 ++++++++++++++++--\n> src/android/camera_device.cpp | 64 +++++++++++++++++++++++++++--\n> 2 files changed, 98 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 71043e127..31d113d51 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> #include \n> +#include \n\nups, thanks\n\n> #include \n>\n> #include \n> @@ -1176,11 +1177,43 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\t\t\t maxFrameDuration_);\n>\n> \t/* Statistics static metadata. */\n> -\tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> -\tstaticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> -\t\t\t\t faceDetectMode);\n> -\n> \tint32_t maxFaceCount = 0;\n> +\tauto iter = camera_->controls().find(controls::FaceDetectMode.id());\n> +\tif (iter != camera_->controls().end()) {\n> +\t\tconst ControlInfo &faceDetectCtrlInfo = iter->second;\n> +\t\tstd::vector faceDetectModes;\n> +\t\tbool hasFaceDetection = false;\n\nMaybe an empty line to give this code some space to breath\n\n> +\t\tfor (const auto &value : faceDetectCtrlInfo.values()) {\n> +\t\t\tauto mode = value.get();\n\nDon't use auto when the type is trivial\n\n> +\t\t\tuint8_t androidMode = 0;\n\nI would add an empty line here as well\n\n> +\t\t\tswitch (mode) {\n> +\t\t\tcase controls::FaceDetectModeOff:\n> +\t\t\t\tandroidMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> +\t\t\t\tbreak;\n> +\t\t\tcase controls::FaceDetectModeSimple:\n> +\t\t\t\tandroidMode = ANDROID_STATISTICS_FACE_DETECT_MODE_SIMPLE;\n> +\t\t\t\thasFaceDetection = true;\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(HAL, Fatal) << \"Received invalid face detect mode: \"\n> +\t\t\t\t\t\t<< static_cast(mode);\n\ndo you need the case if you make mode an uint8_t ?\n\n> +\t\t\t}\n> +\t\t\tfaceDetectModes.push_back(androidMode);\n> +\t\t}\n> +\t\tif (hasFaceDetection) {\n> +\t\t\t// todo(yerlandinata): Create new libcamera controls\n> +\t\t\t// to query max possible faces detected.\n\nNo C++ comments, please\n\nAlso, we don't doxygen the hal, but the rest of the code uses \\todo\n\n> +\t\t\tmaxFaceCount = 10;\n> +\t\t\tstaticMetadata_->addEntry(\n> +\t\t\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> +\t\t\t\tfaceDetectModes.data(), faceDetectModes.size());\n> +\t\t}\n> +\t} else {\n> +\t\tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> +\t\tstaticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> +\t\t\t\t\t faceDetectMode);\n> +\t}\n> +\n> \tstaticMetadata_->addEntry(ANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> \t\t\t\t maxFaceCount);\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 493f66e7b..cd600eade 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,12 +9,12 @@\n>\n> #include \n> #include \n> -#include \n\nwhy ?\n\n> #include \n> #include \n> #include \n>\n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -22,6 +22,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \"system/graphics.h\"\n> @@ -813,6 +814,14 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> \t\tcontrols.set(controls::ScalerCrop, cropRegion);\n> \t}\n>\n> +\tif (settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry)) {\n> +\t\tconst uint8_t *data = entry.data.u8;\n> +\t\tcontrols.set(controls::FaceDetectMode, data[0]);\n> +\t\tif (!controls.get(controls::FaceDetectMode)) {\n\nI don't get this: are you checking if the face detect mode is set to off ?\nDoes this deserves a Warning ?\n\n> +\t\t\tLOG(HAL, Warning) << \"Pipeline doesn't support controls::FaceDetectMode\";\n> +\t\t}\n\nno {} for single line statements, please\n\n> +\t}\n> +\n> \tif (settings.getEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, &entry)) {\n> \t\tconst int32_t data = *entry.data.i32;\n> \t\tint32_t testPatternMode = controls::draft::TestPatternModeOff;\n> @@ -1540,8 +1549,9 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> \tvalue32 = ANDROID_SENSOR_TEST_PATTERN_MODE_OFF;\n> \tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, value32);\n>\n> -\tvalue = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> -\tresultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, value);\n> +\tsettings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry);\n> +\tresultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> +\t\t\t\t entry.data.u8, 1);\n>\n> \tvalue = ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;\n> \tresultMetadata->addEntry(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n> @@ -1580,6 +1590,54 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> \t\tresultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,\n> \t\t\t\t\t *frameDuration * 1000);\n>\n> +\tconst auto &faceDetectRectangles =\n> +\t\tmetadata.get(controls::FaceDetectFaceRectangles);\n> +\tif (faceDetectRectangles) {\n> +\t\tconst Span rectangles = *faceDetectRectangles;\n> +\t\tstd::vector flatRectangles;\n> +\t\tfor (const Rectangle &rect : rectangles) {\n> +\t\t\tflatRectangles.push_back(rect.x);\n> +\t\t\tflatRectangles.push_back(rect.y);\n> +\t\t\tflatRectangles.push_back(rect.x + rect.width);\n> +\t\t\tflatRectangles.push_back(rect.y + rect.height);\n\nWhy x + width and y + height to express width and height ?\nThe control description should probably also specify what the\nrectangles reference system is.\n\n> +\t\t}\n> +\t\tresultMetadata->addEntry(\n> +\t\t\tANDROID_STATISTICS_FACE_RECTANGLES, flatRectangles);\n> +\t}\n> +\n> +\tconst auto &faceDetectFaceScores =\n> +\t\tmetadata.get(controls::FaceDetectFaceScores);\n> +\tif (faceDetectRectangles && faceDetectFaceScores) {\n> +\t\tconst Span &scores = *faceDetectFaceScores;\n> +\t\tif (scores.size() != faceDetectRectangles->size()) {\n> +\t\t\tLOG(HAL, Error) << \"Pipeline returned wrong number of face scores; \"\n> +\t\t\t\t\t<< \"Expected: \"\n> +\t\t\t\t\t<< faceDetectRectangles->size()\n> +\t\t\t\t\t<< \", got:\" << scores.size();\n> +\t\t}\n> +\t\tresultMetadata->addEntry(ANDROID_STATISTICS_FACE_SCORES, scores);\n> +\t}\n> +\n> +\tconst auto &faceDetectFaceLandmarks =\n> +\t\tmetadata.get(controls::FaceDetectFaceLandmark);\n> +\tif (faceDetectFaceScores && faceDetectRectangles &&\n> +\t faceDetectFaceLandmarks) {\n> +\t\tconst auto &landmarks = *faceDetectFaceLandmarks;\n> +\t\tsize_t expectedLandmarks = faceDetectRectangles->size() * 3;\n\nAs said, the control definition should express all these requirements.\n\n> +\t\tstd::vector androidLandmarks;\n> +\t\tfor (const auto &landmark : landmarks) {\n> +\t\t\tandroidLandmarks.push_back(landmark.x);\n> +\t\t\tandroidLandmarks.push_back(landmark.y);\n> +\t\t}\n> +\t\tresultMetadata->addEntry(\n> +\t\t\tANDROID_STATISTICS_FACE_LANDMARKS, androidLandmarks);\n> +\t\tif (landmarks.size() != expectedLandmarks) {\n\nYou can check this before populating the vector\n\n> +\t\t\tLOG(HAL, Error) << \"Pipeline returned wrong number of face landmarks; \"\n> +\t\t\t\t\t<< \"Expected: \" << expectedLandmarks\n> +\t\t\t\t\t<< \", got: \" << landmarks.size();\n> +\t\t}\n> +\t}\n> +\n> \tconst auto &scalerCrop = metadata.get(controls::ScalerCrop);\n> \tif (scalerCrop) {\n> \t\tconst Rectangle &crop = *scalerCrop;\n> --\n> 2.46.0.295.g3b9ea8a38a-goog\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 53EC1C323E\n\tfor ;\n\tWed, 28 Aug 2024 13:10:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A459633CD;\n\tWed, 28 Aug 2024 15:10:57 +0200 (CEST)","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 5008B61903\n\tfor ;\n\tWed, 28 Aug 2024 15:10:55 +0200 (CEST)","from ideasonboard.com (mob-5-90-141-165.net.vodafone.it\n\t[5.90.141.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69EE19FF;\n\tWed, 28 Aug 2024 15:09:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LNiYJ5Vk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724850587;\n\tbh=Fu8rRedYkJf3S6A2jYcstn589x2Ni63o3LP+dEhS8xE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LNiYJ5VkiZ2Er+NSGSu4NtOs15h71Kj5FLW5SHEZyibJMokCmqRzR5Wqvtu7xm2Yx\n\tZKdLtYtkz1RgxKf9NAQF7MkUxX38ECZlYyJca3AotPftMphd87SCpu/+Mcw/j/7w0i\n\tX2cSWsNhFjwqligNuOa/ZzPlFAURBlf1EMuVxMuA=","Date":"Wed, 28 Aug 2024 15:10:51 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tYudhistira Erlandinata ","Subject":"Re: [PATCH v2 3/3] libcamera: android: Add face detection control\n\tsupport","Message-ID":"","References":"<20240823143205.2668765-1-chenghaoyang@google.com>\n\t<20240823143205.2668765-4-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240823143205.2668765-4-chenghaoyang@google.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30995,"web_url":"https://patchwork.libcamera.org/comment/30995/","msgid":"","date":"2024-08-30T21:03:39","subject":"Re: [PATCH v2 3/3] libcamera: android: Add face detection control\n\tsupport","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Jacopo,\n\nOn Wed, Aug 28, 2024 at 3:10 PM Jacopo Mondi \nwrote:\n\n> Hello\n>\n> On Fri, Aug 23, 2024 at 02:29:10PM GMT, Harvey Yang wrote:\n> > From: Yudhistira Erlandinata \n> >\n> > Allow Android HAL adapter to pass the face detection metadata control to\n> > the pipeline and also send face detection metadata to the camera client\n> > if the pipeline generates it.\n> >\n> > Squashed CL:\n> >\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5065292\n>\n> Please drop, unrelated to libcamera\n>\nDropped.\n\n\n>\n> >\n> > Don't use ControlInfoMap::count() and ControlInfoMap::at() because\n> > its internal assumption is wrong: The ControlIdMap and its idmap have a\n> > 1:1 mapping between their entries.\n>\n> Not sure I got this :)\n>\nIn the previous CL, we were using count() & at(), while the assumption [1]\nwas\nwrong for mtkisp7's CameraData [2], as it includes other controls::draft\nentires,\nlike [3].\n\nTherefore, using find() is the safest.\n\n[1]:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n753\n[2]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=881\n[3]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=769\n\n\n> >\n> > BUG=b:308713855\n> > TEST=emerge-geralt libcamera-mtkisp7\n>\n> ditto, please drop, it's your internal stuff.\n>\nDone\n\n\n>\n> >\n> > Signed-off-by: Yudhistira Erlandinata \n> > Co-developed-by: Harvey Yang \n> > ---\n> > src/android/camera_capabilities.cpp | 41 ++++++++++++++++--\n> > src/android/camera_device.cpp | 64 +++++++++++++++++++++++++++--\n> > 2 files changed, 98 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp\n> b/src/android/camera_capabilities.cpp\n> > index 71043e127..31d113d51 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n>\n> ups, thanks\n>\nAbove other includes? Done, while the linter disagrees...\nhttps://gitlab.freedesktop.org/chenghaoyang/libcamera/-/jobs/63003432\n\n\n>\n> > #include \n> >\n> > #include \n> > @@ -1176,11 +1177,43 @@ int\n> CameraCapabilities::initializeStaticMetadata()\n> > maxFrameDuration_);\n> >\n> > /* Statistics static metadata. */\n> > - uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > -\n> staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> > - faceDetectMode);\n> > -\n> > int32_t maxFaceCount = 0;\n> > + auto iter =\n> camera_->controls().find(controls::FaceDetectMode.id());\n> > + if (iter != camera_->controls().end()) {\n> > + const ControlInfo &faceDetectCtrlInfo = iter->second;\n> > + std::vector faceDetectModes;\n> > + bool hasFaceDetection = false;\n>\n> Maybe an empty line to give this code some space to breath\n>\nDone\n\n\n>\n> > + for (const auto &value : faceDetectCtrlInfo.values()) {\n> > + auto mode = value.get();\n>\n> Don't use auto when the type is trivial\n>\nDone\n\n\n>\n> > + uint8_t androidMode = 0;\n>\n> I would add an empty line here as well\n>\nDone\n\n\n>\n> > + switch (mode) {\n> > + case controls::FaceDetectModeOff:\n> > + androidMode =\n> ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > + break;\n> > + case controls::FaceDetectModeSimple:\n> > + androidMode =\n> ANDROID_STATISTICS_FACE_DETECT_MODE_SIMPLE;\n> > + hasFaceDetection = true;\n> > + break;\n> > + default:\n> > + LOG(HAL, Fatal) << \"Received invalid face\n> detect mode: \"\n> > + << static_cast(mode);\n>\n> do you need the case if you make mode an uint8_t ?\n>\nRemoved the cast.\n\n\n>\n> > + }\n> > + faceDetectModes.push_back(androidMode);\n> > + }\n> > + if (hasFaceDetection) {\n> > + // todo(yerlandinata): Create new libcamera\n> controls\n> > + // to query max possible faces detected.\n>\n> No C++ comments, please\n>\n> Also, we don't doxygen the hal, but the rest of the code uses \\todo\n>\nUpdated.\n\n\n>\n> > + maxFaceCount = 10;\n> > + staticMetadata_->addEntry(\n> > +\n> ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> > + faceDetectModes.data(),\n> faceDetectModes.size());\n> > + }\n> > + } else {\n> > + uint8_t faceDetectMode =\n> ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > +\n> staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> > + faceDetectMode);\n> > + }\n> > +\n> > staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> > maxFaceCount);\n> >\n> > diff --git a/src/android/camera_device.cpp\n> b/src/android/camera_device.cpp\n> > index 493f66e7b..cd600eade 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -9,12 +9,12 @@\n> >\n> > #include \n> > #include \n> > -#include \n>\n> why ?\n>\nAdded it back.\n\n\n>\n> > #include \n> > #include \n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -22,6 +22,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \"system/graphics.h\"\n> > @@ -813,6 +814,14 @@ int\n> CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > controls.set(controls::ScalerCrop, cropRegion);\n> > }\n> >\n> > + if (settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> &entry)) {\n> > + const uint8_t *data = entry.data.u8;\n> > + controls.set(controls::FaceDetectMode, data[0]);\n> > + if (!controls.get(controls::FaceDetectMode)) {\n>\n> I don't get this: are you checking if the face detect mode is set to off ?\n> Does this deserves a Warning ?\n>\nI think Yudhis was checking if the control is set properly, while it should\nbe removed after debugging.\n\n\n>\n> > + LOG(HAL, Warning) << \"Pipeline doesn't support\n> controls::FaceDetectMode\";\n> > + }\n>\n> no {} for single line statements, please\n>\nRemoved.\n\n\n>\n> > + }\n> > +\n> > if (settings.getEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, &entry)) {\n> > const int32_t data = *entry.data.i32;\n> > int32_t testPatternMode =\n> controls::draft::TestPatternModeOff;\n> > @@ -1540,8 +1549,9 @@ CameraDevice::getResultMetadata(const\n> Camera3RequestDescriptor &descriptor) cons\n> > value32 = ANDROID_SENSOR_TEST_PATTERN_MODE_OFF;\n> > resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> value32);\n> >\n> > - value = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > - resultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> value);\n> > + settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry);\n> > + resultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> > + entry.data.u8, 1);\n> >\n> > value = ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;\n> > resultMetadata->addEntry(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n> > @@ -1580,6 +1590,54 @@ CameraDevice::getResultMetadata(const\n> Camera3RequestDescriptor &descriptor) cons\n> > resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,\n> > *frameDuration * 1000);\n> >\n> > + const auto &faceDetectRectangles =\n> > + metadata.get(controls::FaceDetectFaceRectangles);\n> > + if (faceDetectRectangles) {\n> > + const Span rectangles =\n> *faceDetectRectangles;\n> > + std::vector flatRectangles;\n> > + for (const Rectangle &rect : rectangles) {\n> > + flatRectangles.push_back(rect.x);\n> > + flatRectangles.push_back(rect.y);\n> > + flatRectangles.push_back(rect.x + rect.width);\n> > + flatRectangles.push_back(rect.y + rect.height);\n>\n> Why x + width and y + height to express width and height ?\n> The control description should probably also specify what the\n> rectangles reference system is.\n>\nUpdated in the yaml file: it follows the format of\nANDROID_STATISTICS_FACE_RECTANGLES.\n\n\n>\n> > + }\n> > + resultMetadata->addEntry(\n> > + ANDROID_STATISTICS_FACE_RECTANGLES,\n> flatRectangles);\n> > + }\n> > +\n> > + const auto &faceDetectFaceScores =\n> > + metadata.get(controls::FaceDetectFaceScores);\n> > + if (faceDetectRectangles && faceDetectFaceScores) {\n> > + const Span &scores = *faceDetectFaceScores;\n> > + if (scores.size() != faceDetectRectangles->size()) {\n> > + LOG(HAL, Error) << \"Pipeline returned wrong number\n> of face scores; \"\n> > + << \"Expected: \"\n> > + << faceDetectRectangles->size()\n> > + << \", got:\" << scores.size();\n> > + }\n> > + resultMetadata->addEntry(ANDROID_STATISTICS_FACE_SCORES,\n> scores);\n> > + }\n> > +\n> > + const auto &faceDetectFaceLandmarks =\n> > + metadata.get(controls::FaceDetectFaceLandmark);\n> > + if (faceDetectFaceScores && faceDetectRectangles &&\n> > + faceDetectFaceLandmarks) {\n> > + const auto &landmarks = *faceDetectFaceLandmarks;\n> > + size_t expectedLandmarks = faceDetectRectangles->size() *\n> 3;\n>\n> As said, the control definition should express all these requirements.\n>\nUpdated.\n\n\n>\n> > + std::vector androidLandmarks;\n> > + for (const auto &landmark : landmarks) {\n> > + androidLandmarks.push_back(landmark.x);\n> > + androidLandmarks.push_back(landmark.y);\n> > + }\n> > + resultMetadata->addEntry(\n> > + ANDROID_STATISTICS_FACE_LANDMARKS,\n> androidLandmarks);\n> > + if (landmarks.size() != expectedLandmarks) {\n>\n> You can check this before populating the vector\n>\nDone\n\n\n>\n> > + LOG(HAL, Error) << \"Pipeline returned wrong number\n> of face landmarks; \"\n> > + << \"Expected: \" <<\n> expectedLandmarks\n> > + << \", got: \" << landmarks.size();\n> > + }\n> > + }\n> > +\n> > const auto &scalerCrop = metadata.get(controls::ScalerCrop);\n> > if (scalerCrop) {\n> > const Rectangle &crop = *scalerCrop;\n> > --\n> > 2.46.0.295.g3b9ea8a38a-goog\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 65BEAC323E\n\tfor ;\n\tFri, 30 Aug 2024 21:03:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1212763469;\n\tFri, 30 Aug 2024 23:03:53 +0200 (CEST)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB4DE61E4A\n\tfor ;\n\tFri, 30 Aug 2024 23:03:51 +0200 (CEST)","by mail-lj1-x232.google.com with SMTP id\n\t38308e7fff4ca-2f50966c448so25113001fa.2\n\tfor ;\n\tFri, 30 Aug 2024 14:03:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"ofqaqLOd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725051831; x=1725656631;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=nPydojSGNfETlVw99TPaEX9UlHiOcBxfPjp8z3UOvYQ=;\n\tb=ofqaqLOddNVGZpnOxio+zZKW02DHyL/SZ9ij0P9kCdOwk565HM8VHR+2loQKLed+Ym\n\tRuME9zKS/1HXv2V0GSpdV50T0HlSUI8rQHW5IZRDSyQy066MpPjtUwX319EdFeVDZUPO\n\tDC7UovKN81Rrv21G9mQC5Nh9uyzbo377YIw1A=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725051831; x=1725656631;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=nPydojSGNfETlVw99TPaEX9UlHiOcBxfPjp8z3UOvYQ=;\n\tb=KWBx/6DuppIqFNpcwZSNqOeZNuQ30bCtbmD7fA4WnQP0nVCbbmQtEOZGHo+y1FS7q8\n\t/44/KoPU0xcsVq0IH7Q2m9jx+HHfiNQm4tccOrP2+W1M0mB+extthZQ5mTDYMRUzUF8C\n\t6K7ZAEUxhSgtEwScxTRS61TMUfbmUCaEMp7avapTFwXc8t4Xq2qQALa9/t+E/sSgJw3C\n\t+nfVHsB7UgvGYGIahDMzNTBiebugZ/tQj8p7/X4lgeFGUpdeWh5dwpAjexYu1AFaf2CP\n\t9CWjk1Hj0KBUkMdz7HBe4tME3L6o6X8ZuZFi/iHU6lfPhtpJnr0JbjBM37v+GvHg2FdF\n\tHyPA==","X-Gm-Message-State":"AOJu0Yz6FC49vkm+SHbaeMdEKOoXgwc4u5FsUv1QqJgbURrP4fIxTsLX\n\tO4GO81VFjgm9LJ9ms49hS0dmdWO0BDh5uM4Q/WtkuzBm7sC1vSKusxU4+1EPLIptoqtGe9MMVRO\n\tnNPKiHakfA2W+f7DLMBtnf7Ti1220kDo+48ca","X-Google-Smtp-Source":"AGHT+IGBsfJ8eMH+Oc00n2mJUVqMkIZ9VGrHnp+IDvocygaoFydxJepp4/xGJWmRuD/nE1udnctHHbkMoUliYDjfhLY=","X-Received":"by 2002:a2e:b88a:0:b0:2f5:1fa7:ac7d with SMTP id\n\t38308e7fff4ca-2f6104f2d54mr50144351fa.37.1725051830521;\n\tFri, 30 Aug 2024 14:03:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20240823143205.2668765-1-chenghaoyang@google.com>\n\t<20240823143205.2668765-4-chenghaoyang@google.com>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Fri, 30 Aug 2024 23:03:39 +0200","Message-ID":"","Subject":"Re: [PATCH v2 3/3] libcamera: android: Add face detection control\n\tsupport","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tYudhistira Erlandinata ","Content-Type":"multipart/alternative; boundary=\"00000000000093ef130620eced46\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]