[PATCH v2 3/3] libcamera: android: Add face detection control support
Cheng-Hao Yang
chenghaoyang at chromium.org
Fri Aug 30 23:03:39 CEST 2024
Thanks Jacopo,
On Wed, Aug 28, 2024 at 3:10 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> 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
>
Dropped.
>
> >
> > 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 :)
>
In the previous CL, we were using count() & at(), while the assumption [1]
was
wrong for mtkisp7's CameraData [2], as it includes other controls::draft
entires,
like [3].
Therefore, using find() is the safest.
[1]:
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n753
[2]:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=881
[3]:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=769
> >
> > BUG=b:308713855
> > TEST=emerge-geralt libcamera-mtkisp7
>
> ditto, please drop, it's your internal stuff.
>
Done
>
> >
> > 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
>
Above other includes? Done, while the linter disagrees...
https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/jobs/63003432
>
> > #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
>
Done
>
> > + for (const auto &value : faceDetectCtrlInfo.values()) {
> > + auto mode = value.get<uint8_t>();
>
> Don't use auto when the type is trivial
>
Done
>
> > + uint8_t androidMode = 0;
>
> I would add an empty line here as well
>
Done
>
> > + 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 ?
>
Removed the cast.
>
> > + }
> > + 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
>
Updated.
>
> > + 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 ?
>
Added it back.
>
> > #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 ?
>
I think Yudhis was checking if the control is set properly, while it should
be removed after debugging.
>
> > + LOG(HAL, Warning) << "Pipeline doesn't support
> controls::FaceDetectMode";
> > + }
>
> no {} for single line statements, please
>
Removed.
>
> > + }
> > +
> > 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.
>
Updated in the yaml file: it follows the format of
ANDROID_STATISTICS_FACE_RECTANGLES.
>
> > + }
> > + 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.
>
Updated.
>
> > + 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
>
Done
>
> > + 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240830/5f879f9e/attachment.htm>
More information about the libcamera-devel
mailing list