[libcamera-devel,v3] gstreamer: Maintain a control list to track libcamerasrc control properties
diff mbox series

Message ID 20230628213452.15531-1-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel,v3] gstreamer: Maintain a control list to track libcamerasrc control properties
Related show

Commit Message

Umang Jain June 28, 2023, 9:34 p.m. UTC 
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@ideasonboard.com>
---
Changes in v3: 
- Rebase over "[PATCH] gstreamer: Drop libcamera_private dependency"

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(-)

Comments

Kieran Bingham June 28, 2023, 10:59 p.m. UTC | #1 
Quoting Umang Jain via libcamera-devel (2023-06-28 22:34:52)
> 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@ideasonboard.com>
> ---
> Changes in v3: 
> - Rebase over "[PATCH] gstreamer: Drop libcamera_private dependency"
> 
> 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 f764a87a..b5c6e9ab 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -142,7 +142,7 @@ struct _GstLibcameraSrc {
>         GstTask *task;
>  
>         gchar *camera_name;
> -       controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
> +       ControlList *controls;
>  
>         GstLibcameraSrcState *state;
>         GstLibcameraAllocator *allocator;
> @@ -458,7 +458,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");
> @@ -575,18 +577,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 think we can remove this comment now or simplify it.

No point mentioning anything to do with libcamera-private.

Other than that, this seems like a good step forwards for getting more
controls in.

--
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,
> @@ -670,7 +684,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);
> @@ -689,9 +704,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;
> @@ -757,6 +774,7 @@ gst_libcamera_src_finalize(GObject *object)
>         g_clear_object(&self->task);
>         g_mutex_clear(&self->state->lock_);
>         g_free(self->camera_name);
> +       delete self->controls;
>         delete self->state;
>  
>         return klass->finalize(object);
> @@ -782,6 +800,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
>
Umang Jain June 29, 2023, 7:01 a.m. UTC | #2 
On 6/29/23 12:59 AM, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2023-06-28 22:34:52)
>> 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@ideasonboard.com>
>> ---
>> Changes in v3:
>> - Rebase over "[PATCH] gstreamer: Drop libcamera_private dependency"
>>
>> 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 f764a87a..b5c6e9ab 100644
>> --- a/src/gstreamer/gstlibcamerasrc.cpp
>> +++ b/src/gstreamer/gstlibcamerasrc.cpp
>> @@ -142,7 +142,7 @@ struct _GstLibcameraSrc {
>>          GstTask *task;
>>   
>>          gchar *camera_name;
>> -       controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
>> +       ControlList *controls;
>>   
>>          GstLibcameraSrcState *state;
>>          GstLibcameraAllocator *allocator;
>> @@ -458,7 +458,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");
>> @@ -575,18 +577,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 think we can remove this comment now or simplify it.

v3 was specifically sent to remove this comment, but apparently I send 
the old .patch file :-/
>
> No point mentioning anything to do with libcamera-private.
>
> Other than that, this seems like a good step forwards for getting more
> controls in.
>
> --
> 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,
>> @@ -670,7 +684,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);
>> @@ -689,9 +704,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;
>> @@ -757,6 +774,7 @@ gst_libcamera_src_finalize(GObject *object)
>>          g_clear_object(&self->task);
>>          g_mutex_clear(&self->state->lock_);
>>          g_free(self->camera_name);
>> +       delete self->controls;
>>          delete self->state;
>>   
>>          return klass->finalize(object);
>> @@ -782,6 +800,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
>>

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index f764a87a..b5c6e9ab 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -142,7 +142,7 @@  struct _GstLibcameraSrc {
 	GstTask *task;
 
 	gchar *camera_name;
-	controls::AfModeEnum auto_focus_mode = controls::AfModeManual;
+	ControlList *controls;
 
 	GstLibcameraSrcState *state;
 	GstLibcameraAllocator *allocator;
@@ -458,7 +458,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");
@@ -575,18 +577,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.
+	 */
+	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,
@@ -670,7 +684,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);
@@ -689,9 +704,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;
@@ -757,6 +774,7 @@  gst_libcamera_src_finalize(GObject *object)
 	g_clear_object(&self->task);
 	g_mutex_clear(&self->state->lock_);
 	g_free(self->camera_name);
+	delete self->controls;
 	delete self->state;
 
 	return klass->finalize(object);
@@ -782,6 +800,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",
 		},