[libcamera-devel] [PATCH v2] gstreamer: Maintain a control list to track libcamerasrc control properties
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jun 29 01:04:51 CEST 2023
Quoting Nicolas Dufresne via libcamera-devel (2023-06-28 15:21:27)
> Le lundi 26 juin 2023 à 22:52 +0200, Umang Jain a écrit :
> > Hi Nicolas,
> >
> > On 6/26/23 12:50 AM, Nicolas Dufresne wrote:
> > > Le lundi 26 juin 2023 à 19:47 +0200, Umang Jain a écrit :
> > > > As more and more libcamera controls get plumbed, more member variables
> > > > get introduced in struct GstLibcameraSrc. Instead of doing that, now
> > > > maintain a single ControlList which is more appropriate to keep track
> > > > of controls that get sets on libcamerasrc.
> > > >
> > > > This also brings easy validation of controls set on libcamerasrc. If
> > > > the controls are not supported by camera, fail negotiation and report
> > > > what is not supported.
> > > >
> > > > The current patch migrates previously introduced control,
> > > > auto-focus-mode, to be set directly in control list.
> > > >
> > > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > > ---
> > > > Changes in v2:
> > > > - Edit commit message
> > > > - Drop a break; so that all unsupported user-provided controls are
> > > > reported at once.
> > > > ---
> > > > src/gstreamer/gstlibcamerasrc.cpp | 44 ++++++++++++++++++++++---------
> > > > src/gstreamer/gstlibcamerasrc.h | 6 ++---
> > > > 2 files changed, 35 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > > index 47d8ff43..11f15068 100644
> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > > @@ -146,7 +146,7 @@ struct _GstLibcameraSrc {
> > > > GstTask *task;
> > > >
> > > > gchar *camera_name;
> > > > - controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
> > > > + ControlList *controls;
> > > >
> > > > GstLibcameraSrcState *state;
> > > > GstLibcameraAllocator *allocator;
> > > > @@ -462,7 +462,9 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> > > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> > > > GLibRecLocker lock(&self->stream_lock);
> > > > GstLibcameraSrcState *state = self->state;
> > > > + const ControlInfoMap &info_map = state->cam_->controls();
> > > > GstFlowReturn flow_ret = GST_FLOW_OK;
> > > > + gboolean supported_ctrl = true;
> > > > gint ret;
> > > >
> > > > g_autoptr(GstStructure) element_caps = gst_structure_new_empty("caps");
> > > > @@ -579,18 +581,30 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> > > > gst_flow_combiner_add_pad(self->flow_combiner, srcpad);
> > > > }
> > > >
> > > > - if (self->auto_focus_mode != controls::AfModeManual) {
> > > > - const ControlInfoMap &infoMap = state->cam_->controls();
> > > > - if (infoMap.find(&controls::AfMode) != infoMap.end()) {
> > > > - state->initControls_.set(controls::AfMode, self->auto_focus_mode);
> > > > - } else {
> > > > + /*
> > > > + * Check the controls set on libcamerasrc is supported by the camera.
> > > > + * This can be easily done by CameraControlValidator but it is an internal
> > > > + * libcamera class. Hence avoid adding a internal header even though
> > > > + * gstreamer links with libcamera-private.
> > > I don't remember why GStreamer links to that, and why we should accept this as a
> > > fact. The longer term plan, when libcamera is stable, was to migrate this
> > > element into GStreamer project, but such thing will prevent it.
> >
> > It's one of the mutex lock used in struct GstLibcameraSrcState because
> > <libcamera/base/mutex.h> is part of private [1]
> >
> > I can replace it with GMutex and GLibLocker right away but we will
> > probably lose clang's thread annotation there itself.
> >
> > [1]
> > https://git.libcamera.org/libcamera/libcamera.git/commit/?id=777b0e0a655cce258a2b11e98546c3fc5a5be031
>
> Let's move away from it and live by our choice to keep that private. We can
> annotate GLibLocker and make it a requirement to only use that, or annotated
> wrappers, but to be fair, I had no idea we had some kind of annotation and I've
> been building with GCC anyway, so it probably have never been used.
The annotations are checked at compile time I believe and we include
clang in our integration compiler matrix, so the macros have been 'used'
but it's fine to drop them here I believe.
--
Kieran
>
> > >
> > > > + */
> > > > + for (auto iter = self->controls->begin(); iter != self->controls->end(); iter++) {
> > > > + if (info_map.find(iter->first) == info_map.end()) {
> > > > + const ControlIdMap *id_map = self->controls->idMap();
> > > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > > > - ("Failed to enable auto focus"),
> > > > - ("AfMode not supported by this camera, "
> > > > - "please retry with 'auto-focus-mode=AfModeManual'"));
> > > > + ("Failed to set %s", id_map->at(iter->first)->name().c_str()),
> > > > + ("%s not supported by this camera, ", id_map->at(iter->first)->name().c_str()));
> > > > + supported_ctrl = false;
> > > > }
> > > > }
> > > >
> > > > + if (!supported_ctrl) {
> > > > + gst_task_stop(task);
> > > > + flow_ret = GST_FLOW_NOT_NEGOTIATED;
> > > > + goto done;
> > > > + }
> > > > +
> > > > + state->initControls_.merge(*self->controls);
> > > > +
> > > > ret = state->cam_->start(&state->initControls_);
> > > > if (ret) {
> > > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > > > @@ -674,7 +688,8 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,
> > > > self->camera_name = g_value_dup_string(value);
> > > > break;
> > > > case PROP_AUTO_FOCUS_MODE:
> > > > - self->auto_focus_mode = static_cast<controls::AfModeEnum>(g_value_get_enum(value));
> > > > + self->controls->set(controls::AfMode,
> > > > + static_cast<controls::AfModeEnum>(g_value_get_enum(value)));
> > > > break;
> > > > default:
> > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > > > @@ -693,9 +708,11 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,
> > > > case PROP_CAMERA_NAME:
> > > > g_value_set_string(value, self->camera_name);
> > > > break;
> > > > - case PROP_AUTO_FOCUS_MODE:
> > > > - g_value_set_enum(value, static_cast<gint>(self->auto_focus_mode));
> > > > + case PROP_AUTO_FOCUS_MODE: {
> > > > + auto auto_focus_mode = self->controls->get(controls::AfMode).value_or(controls::AfModeManual);
> > > > + g_value_set_enum(value, auto_focus_mode);
> > > > break;
> > > > + }
> > > > default:
> > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > > > break;
> > > > @@ -760,6 +777,7 @@ gst_libcamera_src_finalize(GObject *object)
> > > > g_rec_mutex_clear(&self->stream_lock);
> > > > g_clear_object(&self->task);
> > > > g_free(self->camera_name);
> > > > + delete self->controls;
> > > > delete self->state;
> > > >
> > > > return klass->finalize(object);
> > > > @@ -783,6 +801,8 @@ gst_libcamera_src_init(GstLibcameraSrc *self)
> > > > /* C-style friend. */
> > > > state->src_ = self;
> > > > self->state = state;
> > > > +
> > > > + self->controls = new ControlList(controls::controls, nullptr);
> > > > }
> > > >
> > > > static GstPad *
> > > > diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h
> > > > index 0a88ba02..2e6d74cb 100644
> > > > --- a/src/gstreamer/gstlibcamerasrc.h
> > > > +++ b/src/gstreamer/gstlibcamerasrc.h
> > > > @@ -26,17 +26,17 @@ gst_libcamera_auto_focus_get_type()
> > > > static GType type = 0;
> > > > static const GEnumValue values[] = {
> > > > {
> > > > - static_cast<gint>(libcamera::controls::AfModeManual),
> > > > + libcamera::controls::AfModeManual,
> > > > "AfModeManual",
> > > > "manual-focus",
> > > > },
> > > > {
> > > > - static_cast<gint>(libcamera::controls::AfModeAuto),
> > > > + libcamera::controls::AfModeAuto,
> > > > "AfModeAuto",
> > > > "automatic-auto-focus",
> > > > },
> > > > {
> > > > - static_cast<gint>(libcamera::controls::AfModeContinuous),
> > > > + libcamera::controls::AfModeContinuous,
> > > > "AfModeContinuous",
> > > > "continuous-auto-focus",
> > > > },
> >
>
More information about the libcamera-devel
mailing list