<div dir="ltr"><div dir="ltr">Thanks Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 28, 2024 at 2:39 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello<br>
<br>
On Fri, Aug 23, 2024 at 02:29:09PM GMT, Harvey Yang wrote:<br>
> From: Yudhistira Erlandinata <<a href="mailto:yerlandinata@chromium.org" target="_blank">yerlandinata@chromium.org</a>><br>
><br>
> Add FaceDetectMode, FaceDetectFaceRectangles, FaceDetectFaceScores,<br>
> and FaceDetectFaceLandmark. Also add ControlTypePoint for supporting<br>
> FaceDetectFaceLandmark.<br>
><br>
> BUG=b:308713855<br>
> TEST=emerge-geralt libcamera-mtkisp7<br>
<br>
Please drop these<br></blockquote><div>Removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> Signed-off-by: Yudhistira Erlandinata <<a href="mailto:yerlandinata@chromium.org" target="_blank">yerlandinata@chromium.org</a>><br>
> Co-developed-by: becker hsieh <<a href="mailto:beckerh@chromium.org" target="_blank">beckerh@chromium.org</a>><br>
> Co-developed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> include/libcamera/controls.h | 6 +++++<br>
> src/libcamera/control_ids_core.yaml | 42 +++++++++++++++++++++++++++++<br>
> src/libcamera/controls.cpp | 6 +++++<br>
> 3 files changed, 54 insertions(+)<br>
><br>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h<br>
> index 7c2bb2872..bf1b8609c 100644<br>
> --- a/include/libcamera/controls.h<br>
> +++ b/include/libcamera/controls.h<br>
> @@ -34,6 +34,7 @@ enum ControlType {<br>
> ControlTypeString,<br>
> ControlTypeRectangle,<br>
> ControlTypeSize,<br>
> + ControlTypePoint,<br>
> };<br>
><br>
> namespace details {<br>
> @@ -87,6 +88,11 @@ struct control_type<Size> {<br>
> static constexpr ControlType value = ControlTypeSize;<br>
> };<br>
><br>
> +template<><br>
> +struct control_type<Point> {<br>
> + static constexpr ControlType value = ControlTypePoint;<br>
> +};<br>
> +<br>
> template<typename T, std::size_t N><br>
> struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {<br>
> };<br>
> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml<br>
> index 6381970b4..5ff46c71e 100644<br>
> --- a/src/libcamera/control_ids_core.yaml<br>
> +++ b/src/libcamera/control_ids_core.yaml<br>
<br>
I wonder if we can already made these core controls or as long as we<br>
don't have more users they should in control_ids_draft<br>
<br>
> @@ -860,6 +860,48 @@ controls:<br>
> No further state changes or lens movements will occur until the<br>
> AfPauseResume control is sent.<br>
><br>
> + - FaceDetectMode:<br>
> + type: uint8_t<br>
> + description: |<br>
> + Reporting mode of face detection.<br>
<br>
I presume this is a metadata only control (iow can only be reported by<br>
the library to the application). If that's the case this should be<br>
made explicit.<br>
<br>
We don't do so consistently in our controls definition unfortunately,<br>
but some controls already specify that, like, in example,<br>
ColorTemperature:<br>
<br>
The ColourTemperature control can only be returned in metadata.<br>
<br>
I would do the same here if that's the case.<br>
<br>
However, reading your next patch, this control doesn't seem to be for<br>
reporting only, but can also be set by applications to control how the<br>
pipeline behaves. That's not what the description says.<br></blockquote><div><br></div><div>Right, in Android adapter, HAL returns a list of available</div><div>FaceDetectMode in static metadata. Application then sets a mode it </div><div>desires per-framely in controls. HAL returns the final mode that it</div><div>supports per-framely in the result metadata.</div><div><br></div><div>Therefore, I'll keep it this way :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> +<br>
> + enum:<br>
> + - name: FaceDetectModeOff<br>
> + value: 0<br>
> + description: |<br>
> + Pipeline should not report face detection result.<br>
> + - name: FaceDetectModeSimple<br>
> + value: 1<br>
> + description: |<br>
> + Pipeline should at least report faces boundary rectangles and<br>
<br>
Why "at least" ?<br></blockquote><div><br>In Android, there's another Full mode [1] that includes face landmarks and</div><div>face IDs. I think Yudhis didn't list Full mode here because mtkisp7 doesn't</div><div>support face IDs in their face detector lib/hardware...</div><div><br></div><div>Android CTS doesn't seem to care if more metadata is returned, when not</div><div>asked for though.</div><div><br></div><div>We can also list FULL mode here, and let mtkisp7 pipeline handler returns</div><div>Off & Simple modes in the static metadata. WDYT?</div><div><br></div><div>[1]: <a href="https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#STATISTICS_FACE_DETECT_MODE_FULL" target="_blank">https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#STATISTICS_FACE_DETECT_MODE_FULL</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + confidence score for each of them.<br>
<br>
How ? Please describe here how these information are reported,<br>
mentioning explicitly<br>
<br>
\sa FaceDetectFaceRectangles<br>
\sa FaceDetectFaceScores<br>
<br>
I presume it is expected from the pipline handlers to report two lists<br>
of the same sizes of rectangles and scores.<br></blockquote><div><br></div><div>Exactly. Thanks! Updated, please check again.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Please keep in mind this documentation should be usable for other<br>
pipeline handler implementers to implement face detection and from<br>
application to understand how to interpret the results.<br></blockquote><div><br></div><div>Right, I'll keep that in mind.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + - FaceDetectFaceRectangles:<br>
> + type: Rectangle<br>
> + description: |<br>
> + Boundary rectangle of the detected faces.<br>
<br>
This is reported only when FaceDetectMode is set to<br>
FaceDetectModeSimple I presume. It has to be specified. If it's a<br>
metadata only control the above suggestion of saying that explicitly<br>
applies.<br></blockquote><div><br></div><div>Actually, as I mentioned above, Android doesn't seem to complain </div><div>about extra metadata. Therefore, it's `when` instead of `only when`.</div><div>Updated.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + size: [n]<br>
> +<br>
> + - FaceDetectFaceScores:<br>
> + type: uint8_t<br>
> + description: |<br>
> + Confidence score of each of the detected faces by face detector.<br>
> + The range of score is [0, 100], but usually those with low confidence<br>
> + scores will not be included.<br>
<br>
I think you should eitehr clarify what determines what "low confidence" is<br>
or drop that part, as it's a pipeline handler specific decision.<br></blockquote><div>Right, dropped.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I presume a score needs a rectangle assigned, please mention that.<br></blockquote><div>Do we need to mention that the size of faces in each metadata should</div><div>align in every control id involved? How about just mentioning once in </div><div>FaceDetectModeSimple?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + Currently identical to ANDROID_STATISTICS_FACE_SCORES.<br>
<br>
This makes me think all of this should go in draft controls, where we<br>
have collected controls whose definition comes from the Android ones.<br></blockquote><div><br></div><div>Right, moved to the draft ids.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + size: [n]<br>
> +<br>
> + - FaceDetectFaceLandmark:<br>
> + type: Point<br>
> + description: |<br>
> + Array of human face landmark coordinates in format:<br>
> + [..., left_eye_i, right_eye_i, mouth_i, left_eye_i+1, ...],<br>
> + with i = index of face.<br>
<br>
This is quite vague I'm afraid.<br>
<br>
Which are the face landmarks ? eyes and mouth ? Why not the nose ? A more<br>
sophisticated mechanism is required to support this and have it usable<br>
for multiple platforms I'm afraid. If this is only useful for Android and<br>
its description comes from an Android control, move it to draf where<br>
we can be slighly more liberal in defining things, as draf controls<br>
are by definition not expected to be stable.<br></blockquote><div><br></div><div>Yes, it's to be aligned with Android control [2]. Moved to the draft ids.</div><div><br></div><div>[2]: <a href="https://android.googlesource.com/platform/system/media/+/refs/heads/main/camera/docs/docs.html#32906" target="_blank">https://android.googlesource.com/platform/system/media/+/refs/heads/main/camera/docs/docs.html#32906</a> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + size: [n]<br>
> +<br>
> - HdrMode:<br>
> type: int32_t<br>
> description: |<br>
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp<br>
> index 11d35321c..ce73ae9d7 100644<br>
> --- a/src/libcamera/controls.cpp<br>
> +++ b/src/libcamera/controls.cpp<br>
> @@ -61,6 +61,7 @@ static constexpr size_t ControlValueSize[] = {<br>
> [ControlTypeString] = sizeof(char),<br>
> [ControlTypeRectangle] = sizeof(Rectangle),<br>
> [ControlTypeSize] = sizeof(Size),<br>
> + [ControlTypePoint] = sizeof(Point),<br>
> };<br>
><br>
> } /* namespace */<br>
> @@ -255,6 +256,11 @@ std::string ControlValue::toString() const<br>
> str += value->toString();<br>
> break;<br>
> }<br>
> + case ControlTypePoint: {<br>
> + const Point *value = reinterpret_cast<const Point *>(data);<br>
> + str += value->toString();<br>
> + break;<br>
> + }<br>
> case ControlTypeNone:<br>
> case ControlTypeString:<br>
> break;<br>
> --<br>
> 2.46.0.295.g3b9ea8a38a-goog<br>
><br>
</blockquote></div></div>