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

[Jacopo Mondi]

Hello

On Fri, Aug 23, 2024 at 02:29:09PM GMT, Harvey Yang wrote:
> From: Yudhistira Erlandinata <yerlandinata at chromium.org>
>
> Add FaceDetectMode, FaceDetectFaceRectangles, FaceDetectFaceScores,
> and FaceDetectFaceLandmark. Also add ControlTypePoint for supporting
> FaceDetectFaceLandmark.
>
> BUG=b:308713855
> TEST=emerge-geralt libcamera-mtkisp7

Please drop these

>
> 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_core.yaml | 42 +++++++++++++++++++++++++++++
>  src/libcamera/controls.cpp          |  6 +++++
>  3 files changed, 54 insertions(+)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 7c2bb2872..bf1b8609c 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_core.yaml b/src/libcamera/control_ids_core.yaml
> index 6381970b4..5ff46c71e 100644
> --- a/src/libcamera/control_ids_core.yaml
> +++ b/src/libcamera/control_ids_core.yaml

I wonder if we can already made these core controls or as long as we
don't have more users they should in control_ids_draft

> @@ -860,6 +860,48 @@ controls:
>              No further state changes or lens movements will occur until the
>              AfPauseResume control is sent.
>
> +  - FaceDetectMode:
> +      type: uint8_t
> +      description: |
> +        Reporting mode of face detection.

I presume this is a metadata only control (iow can only be reported by
the library to the application). If that's the case this should be
made explicit.

We don't do so consistently in our controls definition unfortunately,
but some controls already specify that, like, in example,
ColorTemperature:

        The ColourTemperature control can only be returned in metadata.

I would do the same here if that's the case.

However, reading your next patch, this control doesn't seem to be for
reporting only, but can also be set by applications to control how the
pipeline behaves. That's not what the description says.


> +
> +      enum:
> +        - name: FaceDetectModeOff
> +          value: 0
> +          description: |
> +            Pipeline should not report face detection result.
> +        - name: FaceDetectModeSimple
> +          value: 1
> +          description: |
> +            Pipeline should at least report faces boundary rectangles and

Why "at least" ?

> +            confidence score for each of them.

How ? Please describe here how these information are reported,
mentioning explicitly

                \sa FaceDetectFaceRectangles
                \sa FaceDetectFaceScores

I presume it is expected from the pipline handlers to report two lists
of the same sizes of rectangles and scores.

Please keep in mind this documentation should be usable for other
pipeline handler implementers to implement face detection and from
application to understand how to interpret the results.

> +
> +  - FaceDetectFaceRectangles:
> +      type: Rectangle
> +      description: |
> +        Boundary rectangle of the detected faces.

This is reported only when FaceDetectMode is set to
FaceDetectModeSimple I presume. It has to be specified. If it's a
metadata only control the above suggestion of saying that explicitly
applies.

> +
> +      size: [n]
> +
> +  - FaceDetectFaceScores:
> +      type: uint8_t
> +      description: |
> +        Confidence score of each of the detected faces by face detector.
> +        The range of score is [0, 100], but usually those with low confidence
> +        scores will not be included.

I think you should eitehr clarify what determines what "low confidence" is
or drop that part, as it's a pipeline handler specific decision.

I presume a score needs a rectangle assigned, please mention that.

> +        Currently identical to ANDROID_STATISTICS_FACE_SCORES.

This makes me think all of this should go in draft controls, where we
have collected controls whose definition comes from the Android ones.

> +
> +      size: [n]
> +
> +  - FaceDetectFaceLandmark:
> +      type: Point
> +      description: |
> +        Array of human face landmark coordinates in format:
> +        [..., left_eye_i, right_eye_i, mouth_i, left_eye_i+1, ...],
> +        with i = index of face.

This is quite vague I'm afraid.

Which are the face landmarks ? eyes and mouth ? Why not the nose ? A more
sophisticated mechanism is required to support this and have it usable
for multiple platforms I'm afraid. If this is only useful for Android and
its description comes from an Android control, move it to draf where
we can be slighly more liberal in defining things, as draf controls
are by definition not expected to be stable.

> +
> +      size: [n]
> +
>    - HdrMode:
>        type: int32_t
>        description: |
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 11d35321c..ce73ae9d7 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -61,6 +61,7 @@ static constexpr size_t ControlValueSize[] = {
>  	[ControlTypeString]		= sizeof(char),
>  	[ControlTypeRectangle]		= sizeof(Rectangle),
>  	[ControlTypeSize]		= sizeof(Size),
> +	[ControlTypePoint]		= sizeof(Point),
>  };
>
>  } /* namespace */
> @@ -255,6 +256,11 @@ std::string ControlValue::toString() const
>  			str += value->toString();
>  			break;
>  		}
> +		case ControlTypePoint: {
> +			const Point *value = reinterpret_cast<const Point *>(data);
> +			str += value->toString();
> +			break;
> +		}
>  		case ControlTypeNone:
>  		case ControlTypeString:
>  			break;
> --
> 2.46.0.295.g3b9ea8a38a-goog
>