[PATCH v5 2/3] libcamera: Add face detection controls

[Jacopo Mondi]

Hi Kieran

On Mon, Sep 23, 2024 at 11:35:15AM GMT, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-09-23 11:13:36)
> > Hi Harvey,
> >
> > On Mon, Sep 23, 2024 at 09:30:45AM GMT, Harvey Yang wrote:
> > > From: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > >
> > > Add FaceDetectMode, FaceDetectFaceRectangles, FaceDetectFaceScores,
> > > and FaceDetectFaceLandmark. Also add ControlTypePoint for supporting
> > > FaceDetectFaceLandmark.
> > >
> > > Signed-off-by: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > Co-developed-by: becker hsieh <beckerh at chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > ---
> > >  include/libcamera/controls.h         |  6 ++
> > >  src/libcamera/control_ids_draft.yaml | 85 ++++++++++++++++++++++++++++
> > >  src/libcamera/controls.cpp           |  6 ++
> > >  3 files changed, 97 insertions(+)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 7c2bb287..bf1b8609 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -34,6 +34,7 @@ enum ControlType {
> > >       ControlTypeString,
> > >       ControlTypeRectangle,
> > >       ControlTypeSize,
> > > +     ControlTypePoint,
> > >  };
> > >
> > >  namespace details {
> > > @@ -87,6 +88,11 @@ struct control_type<Size> {
> > >       static constexpr ControlType value = ControlTypeSize;
> > >  };
> > >
> > > +template<>
> > > +struct control_type<Point> {
> > > +     static constexpr ControlType value = ControlTypePoint;
> > > +};
> > > +
> > >  template<typename T, std::size_t N>
> > >  struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> > >  };
> > > diff --git a/src/libcamera/control_ids_draft.yaml b/src/libcamera/control_ids_draft.yaml
> > > index 9bef5bf1..c0cf5320 100644
> > > --- a/src/libcamera/control_ids_draft.yaml
> > > +++ b/src/libcamera/control_ids_draft.yaml
> > > @@ -227,4 +227,89 @@ controls:
> > >              value. All of the custom test patterns will be static (that is the
> > >              raw image must not vary from frame to frame).
> > >
> > > +  - FaceDetectMode:
> > > +      type: uint8_t
> > > +      description: |
> > > +        Reporting mode of face detection.
> > > +
> > > +        The number of faces in each list must be the same.
> >
> > I would drop this. See below how I would rather expand the
> > requirements of sizes in the single commands, now that they have been
> > described in full.
> >
> > > +
> > > +        Currently identical to ANDROID_STATISTICS_FACE_DETECT_MODE.
> > > +
> > > +        \sa FaceDetectFaceRectangles
> > > +        \sa FaceDetectFaceScores
> > > +        \sa FaceDetectFaceLandmarks
> > > +        \sa FaceDetectFaceIds
> > > +
> > > +      enum:
> > > +        - name: FaceDetectModeOff
> > > +          value: 0
> > > +          description: |
> > > +            Pipeline should not report face detection result.
> > > +        - name: FaceDetectModeSimple
> > > +          value: 1
> > > +          description: |
> > > +            Pipeline should at least report FaceDetectFaceRectangles and
> > > +            FaceDetectFaceScores for each detected faces.
> > > +            FaceDetectFaceLandmarks and FaceDetectFaceIds is optional.
> > > +
> > > +        - name: FaceDetectModeFull
> > > +          value: 2
> > > +          description: |
> > > +            Pipeline should report all face controls, including
> > > +            FaceDetectFaceRectangles, FaceDetectFaceScores,
> > > +            FaceDetectFaceLandmarks, and FaceDeteceFaceIds.
> > > +
> > > +  - FaceDetectFaceRectangles:
> > > +      type: Rectangle
> > > +      description: |
> > > +        Boundary rectangles of the detected faces.
> > > +        The number of values should be the number of faces.
> >
> > If this was not a draft I would have asked what is the reference
> > system for the rectangles. The PixelArray sizes, the output stream
> > sizes ? As it is draft, we can refer to ANDROID_STATISTICS_FACE_RECTANGLES.
>
> I thought we were trying to avoid adding controls to ::draft::

I think that asking to make out of these proper controls would require
more use cases and users than just mtkisp7

>
> Maybe we should just rename ::draft:: to ::android:: now that we have
> namespaced controls ? (or add ::android:: ?)

I wouldn't be opposed in principle, but I still would like to convey
the fact these are not stable or here to stay forever.

>
> --
> Kieran