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