| Message ID | 20230622111717.32236-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
I tested this on the PiCamV3 and the auto-focus option still seems to work indeed. Cedric On Thu, Jun 22, 2023 at 1:17 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > 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 > 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@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())); > + supported_ctrl = false; > + break; > } > } > > + 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; > 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, > "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 > >
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(). > 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@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. > + 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. > } > } > > + 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... > 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. ? > "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 >
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@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. > >> + 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 >>
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@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 >>> >
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())); + supported_ctrl = false; + break; } } + 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; 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, "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", },
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 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@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(-)