<div dir="ltr"><div dir="ltr">Thanks Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 28, 2024 at 3:10 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello<br>
<br>
On Fri, Aug 23, 2024 at 02:29:10PM GMT, Harvey Yang wrote:<br>
> From: Yudhistira Erlandinata <<a href="mailto:yerlandinata@chromium.org" target="_blank">yerlandinata@chromium.org</a>><br>
><br>
> Allow Android HAL adapter to pass the face detection metadata control to<br>
> the pipeline and also send face detection metadata to the camera client<br>
> if the pipeline generates it.<br>
><br>
> Squashed CL:<br>
> <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5065292" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5065292</a><br>
<br>
Please drop, unrelated to libcamera<br></blockquote><div>Dropped.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> Don't use ControlInfoMap::count() and ControlInfoMap::at() because<br>
> its internal assumption is wrong: The ControlIdMap and its idmap have a<br>
> 1:1 mapping between their entries.<br>
<br>
Not sure I got this :)<br></blockquote><div>In the previous CL, we were using count() & at(), while the assumption [1] was </div><div>wrong for mtkisp7's CameraData [2], as it includes other controls::draft entires,</div><div>like [3].</div><div><br></div><div>Therefore, using find() is the safest.</div><div><br></div><div>[1]: <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n753">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n753</a></div><div>[2]: <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=881">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=881</a></div><div>[3]: <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=769">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=769</a></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> BUG=b:308713855<br>
> TEST=emerge-geralt libcamera-mtkisp7<br>
<br>
ditto, please drop, it's your internal stuff.<br></blockquote><div>Done</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> Signed-off-by: Yudhistira Erlandinata <<a href="mailto:yerlandinata@chromium.org" target="_blank">yerlandinata@chromium.org</a>><br>
> Co-developed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> src/android/camera_capabilities.cpp | 41 ++++++++++++++++--<br>
> src/android/camera_device.cpp | 64 +++++++++++++++++++++++++++--<br>
> 2 files changed, 98 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp<br>
> index 71043e127..31d113d51 100644<br>
> --- a/src/android/camera_capabilities.cpp<br>
> +++ b/src/android/camera_capabilities.cpp<br>
> @@ -11,6 +11,7 @@<br>
> #include <array><br>
> #include <cmath><br>
> #include <map><br>
> +#include <stdint.h><br>
<br>
ups, thanks<br></blockquote><div>Above other includes? Done, while the linter disagrees...</div><div><a href="https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/jobs/63003432">https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/jobs/63003432</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> #include <type_traits><br>
><br>
> #include <hardware/camera3.h><br>
> @@ -1176,11 +1177,43 @@ int CameraCapabilities::initializeStaticMetadata()<br>
> maxFrameDuration_);<br>
><br>
> /* Statistics static metadata. */<br>
> - uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;<br>
> - staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,<br>
> - faceDetectMode);<br>
> -<br>
> int32_t maxFaceCount = 0;<br>
> + auto iter = camera_->controls().find(controls::FaceDetectMode.id());<br>
> + if (iter != camera_->controls().end()) {<br>
> + const ControlInfo &faceDetectCtrlInfo = iter->second;<br>
> + std::vector<uint8_t> faceDetectModes;<br>
> + bool hasFaceDetection = false;<br>
<br>
Maybe an empty line to give this code some space to breath<br></blockquote><div>Done</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + for (const auto &value : faceDetectCtrlInfo.values()) {<br>
> + auto mode = value.get<uint8_t>();<br>
<br>
Don't use auto when the type is trivial<br></blockquote><div>Done</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + uint8_t androidMode = 0;<br>
<br>
I would add an empty line here as well<br></blockquote><div>Done</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + switch (mode) {<br>
> + case controls::FaceDetectModeOff:<br>
> + androidMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;<br>
> + break;<br>
> + case controls::FaceDetectModeSimple:<br>
> + androidMode = ANDROID_STATISTICS_FACE_DETECT_MODE_SIMPLE;<br>
> + hasFaceDetection = true;<br>
> + break;<br>
> + default:<br>
> + LOG(HAL, Fatal) << "Received invalid face detect mode: "<br>
> + << static_cast<int>(mode);<br>
<br>
do you need the case if you make mode an uint8_t ?<br></blockquote><div>Removed the cast.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + }<br>
> + faceDetectModes.push_back(androidMode);<br>
> + }<br>
> + if (hasFaceDetection) {<br>
> + // todo(yerlandinata): Create new libcamera controls<br>
> + // to query max possible faces detected.<br>
<br>
No C++ comments, please<br>
<br>
Also, we don't doxygen the hal, but the rest of the code uses \todo<br></blockquote><div>Updated.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + maxFaceCount = 10;<br>
> + staticMetadata_->addEntry(<br>
> + ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,<br>
> + faceDetectModes.data(), faceDetectModes.size());<br>
> + }<br>
> + } else {<br>
> + uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;<br>
> + staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,<br>
> + faceDetectMode);<br>
> + }<br>
> +<br>
> staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_MAX_FACE_COUNT,<br>
> maxFaceCount);<br>
><br>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> index 493f66e7b..cd600eade 100644<br>
> --- a/src/android/camera_device.cpp<br>
> +++ b/src/android/camera_device.cpp<br>
> @@ -9,12 +9,12 @@<br>
><br>
> #include <algorithm><br>
> #include <fstream><br>
> -#include <set><br>
<br>
why ?<br></blockquote><div>Added it back.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> #include <sys/mman.h><br>
> #include <unistd.h><br>
> #include <vector><br>
><br>
> #include <libcamera/base/log.h><br>
> +#include <libcamera/base/span.h><br>
> #include <libcamera/base/unique_fd.h><br>
> #include <libcamera/base/utils.h><br>
><br>
> @@ -22,6 +22,7 @@<br>
> #include <libcamera/controls.h><br>
> #include <libcamera/fence.h><br>
> #include <libcamera/formats.h><br>
> +#include <libcamera/geometry.h><br>
> #include <libcamera/property_ids.h><br>
><br>
> #include "system/graphics.h"<br>
> @@ -813,6 +814,14 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)<br>
> controls.set(controls::ScalerCrop, cropRegion);<br>
> }<br>
><br>
> + if (settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry)) {<br>
> + const uint8_t *data = entry.data.u8;<br>
> + controls.set(controls::FaceDetectMode, data[0]);<br>
> + if (!controls.get(controls::FaceDetectMode)) {<br>
<br>
I don't get this: are you checking if the face detect mode is set to off ?<br>
Does this deserves a Warning ?<br></blockquote><div>I think Yudhis was checking if the control is set properly, while it should</div><div>be removed after debugging.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + LOG(HAL, Warning) << "Pipeline doesn't support controls::FaceDetectMode";<br>
> + }<br>
<br>
no {} for single line statements, please<br></blockquote><div>Removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + }<br>
> +<br>
> if (settings.getEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, &entry)) {<br>
> const int32_t data = *entry.data.i32;<br>
> int32_t testPatternMode = controls::draft::TestPatternModeOff;<br>
> @@ -1540,8 +1549,9 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons<br>
> value32 = ANDROID_SENSOR_TEST_PATTERN_MODE_OFF;<br>
> resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, value32);<br>
><br>
> - value = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;<br>
> - resultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, value);<br>
> + settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry);<br>
> + resultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,<br>
> + entry.data.u8, 1);<br>
><br>
> value = ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;<br>
> resultMetadata->addEntry(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,<br>
> @@ -1580,6 +1590,54 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons<br>
> resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,<br>
> *frameDuration * 1000);<br>
><br>
> + const auto &faceDetectRectangles =<br>
> + metadata.get(controls::FaceDetectFaceRectangles);<br>
> + if (faceDetectRectangles) {<br>
> + const Span<const Rectangle> rectangles = *faceDetectRectangles;<br>
> + std::vector<int32_t> flatRectangles;<br>
> + for (const Rectangle &rect : rectangles) {<br>
> + flatRectangles.push_back(rect.x);<br>
> + flatRectangles.push_back(rect.y);<br>
> + flatRectangles.push_back(rect.x + rect.width);<br>
> + flatRectangles.push_back(rect.y + rect.height);<br>
<br>
Why x + width and y + height to express width and height ?<br>
The control description should probably also specify what the<br>
rectangles reference system is.<br></blockquote><div>Updated in the yaml file: it follows the format of </div><div>ANDROID_STATISTICS_FACE_RECTANGLES.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + }<br>
> + resultMetadata->addEntry(<br>
> + ANDROID_STATISTICS_FACE_RECTANGLES, flatRectangles);<br>
> + }<br>
> +<br>
> + const auto &faceDetectFaceScores =<br>
> + metadata.get(controls::FaceDetectFaceScores);<br>
> + if (faceDetectRectangles && faceDetectFaceScores) {<br>
> + const Span<const uint8_t> &scores = *faceDetectFaceScores;<br>
> + if (scores.size() != faceDetectRectangles->size()) {<br>
> + LOG(HAL, Error) << "Pipeline returned wrong number of face scores; "<br>
> + << "Expected: "<br>
> + << faceDetectRectangles->size()<br>
> + << ", got:" << scores.size();<br>
> + }<br>
> + resultMetadata->addEntry(ANDROID_STATISTICS_FACE_SCORES, scores);<br>
> + }<br>
> +<br>
> + const auto &faceDetectFaceLandmarks =<br>
> + metadata.get(controls::FaceDetectFaceLandmark);<br>
> + if (faceDetectFaceScores && faceDetectRectangles &&<br>
> + faceDetectFaceLandmarks) {<br>
> + const auto &landmarks = *faceDetectFaceLandmarks;<br>
> + size_t expectedLandmarks = faceDetectRectangles->size() * 3;<br>
<br>
As said, the control definition should express all these requirements.<br></blockquote><div>Updated.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + std::vector<int32_t> androidLandmarks;<br>
> + for (const auto &landmark : landmarks) {<br>
> + androidLandmarks.push_back(landmark.x);<br>
> + androidLandmarks.push_back(landmark.y);<br>
> + }<br>
> + resultMetadata->addEntry(<br>
> + ANDROID_STATISTICS_FACE_LANDMARKS, androidLandmarks);<br>
> + if (landmarks.size() != expectedLandmarks) {<br>
<br>
You can check this before populating the vector<br></blockquote><div>Done</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + LOG(HAL, Error) << "Pipeline returned wrong number of face landmarks; "<br>
> + << "Expected: " << expectedLandmarks<br>
> + << ", got: " << landmarks.size();<br>
> + }<br>
> + }<br>
> +<br>
> const auto &scalerCrop = metadata.get(controls::ScalerCrop);<br>
> if (scalerCrop) {<br>
> const Rectangle &crop = *scalerCrop;<br>
> --<br>
> 2.46.0.295.g3b9ea8a38a-goog<br>
><br>
</blockquote></div></div>