[libcamera-devel] [PATCH] gstreamer: Maintain a control list to track libcamerasrc control properties
Umang Jain
umang.jain at ideasonboard.com
Mon Jun 26 19:36:09 CEST 2023
Hi again,
On 6/26/23 9:45 PM, Umang Jain via libcamera-devel wrote:
> HI Kieran,
>
> On 6/26/23 9:24 PM, Kieran Bingham wrote:
>> Hi Umang,
>>
>> Quoting Umang Jain via libcamera-devel (2023-06-22 12:17:17)
>>> 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, abort early and report
>>> what is
>> I would change 'abort' to 'fail negotiation' as I thought from reading
>> this there was going to be a call to abort().
>
> ack
>>
>>> 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>
>>> ---
>>> Cedric, Can you please help test this?
>>>
>>> (I'm travelling so have tested this on my RPi and inferring from
>>> libcamera/gstreamer logs, it seems to have not broken anything)
>>>
>>> An additional test from your side would be appreciated.
>>> ---
>>> src/gstreamer/gstlibcamerasrc.cpp | 45
>>> ++++++++++++++++++++++---------
>>> src/gstreamer/gstlibcamerasrc.h | 6 ++---
>>> 2 files changed, 36 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp
>>> b/src/gstreamer/gstlibcamerasrc.cpp
>>> index 47d8ff43..a2266310 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,31 @@ 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.
>>> + */
>>> + 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()));
>> Isn't this printing the name twice? Maybe this could be a single output
>> line.
>
> Ah probably, I 'll try to take a look.
So the second part, is really the DEBUG log not the main log.
https://gstreamer.freedesktop.org/documentation/gstreamer/gstelement.html?gi-language=c#GST_ELEMENT_ERROR
So this is fine as is.
>>
>>> + supported_ctrl = false;
>>> + break;
>> I would probably not break here - as if there are multiple controls that
>> fail to validate - it might make sense to report all of them at once.
>
> Good idea!
>>
>>
>>> }
>>> }
>>> + 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 +689,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 +709,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 +778,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;
>> I would have said can we use a unique_ptr, but it doesn't look like that
>> makes sense without reworking the rest of the structure...
>
> I think you can't do unique_ptr of ControlList right now.
>>
>>> delete self->state;
>>> return klass->finalize(object);
>>> @@ -783,6 +802,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,
>> Nice to see the cast is no longer needed. Is that a result of a change
>> here, or was it already possible. ?
>
> Not sure, I didn't test this isolated change with the earlier AF mode
> patch, but AFAIR i started from here developing this entire patch.
>>
>>> "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",
>>> },
>>> --
>>> 2.39.1
>>>
>
More information about the libcamera-devel
mailing list