[PATCH v2 3/3] libcamera: android: Add face detection control support
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Sat Aug 31 15:58:56 CEST 2024
Hi Harvey
On Fri, Aug 30, 2024 at 11:03:39PM GMT, Cheng-Hao Yang wrote:
> 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.
ok, but when we'll read the commit message in three years from now,
this won't make sense as the 'previous CL' will be forgotten. You can
add this to the changelog below --- or in the cover letter if you want
to.
>
> [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
>
No no, I mean "ups, we should have included that already!"
>
> >
> > > #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.
>
Ok! please drop it then!
>
> >
> > > + 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
> > >
> >
More information about the libcamera-devel
mailing list