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

[Kieran Bingham]

Quoting Cheng-Hao Yang (2024-09-23 15:50:44)
> Hi Jacopo and Kieran,
> 
> On Mon, Sep 23, 2024 at 8:59 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> wrote:
> 
> > 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.
> >
> 
> Removed, thanks!
> 
> 
> > > >
> > > > > +
> > > > > +        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.
> >
> 
> Ah understood.
> 
> 
> > >
> > > 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.
> >
> 
> Let me add `control_ids_android.yaml` in the next version.

I'm sorry - I think my idea/proposal here was a bit too optimistic.

We're trying to make sure we do not have android specific controls, so
they should be generalised wherever possible. And creating an android
namespace here would lead us the wrong direction...

So I think I'm afraid - the version you had before was correct, keeping
these in draft - to at least convey that we should be moving these to a
generic interface as soon as possible.

Of course even better would be making sure we describe and document Face
detection as a feature without needing to refer to the Android
definitions and ensuring that the android layer handles this
accordingly.

--
Kieran


> 
> 
> >
> > >
> > > --
> > > Kieran
> >
> 
> 
> -- 
> BR,
> Harvey Yang