[libcamera-devel] [PATCH v1 09/23] gst: libcamerasrc: Implement selection and acquisition

[Nicolas Dufresne]

On mar, 2020-02-11 at 21:43 +0200, Laurent Pinchart wrote:
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Tue, Jan 28, 2020 at 10:31:56PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > 
> > This add code to select and acquire a camera. With this, it is now
> 
> s/add/adds/
> 
> > possible to run pipeline like:
> 
> s/pipeline/a pipeline/ or s/pipeline/pipelines/
> 
> >    gst-launch-1.0 libcamerasrc ! fakesink
> > 
> > Though no buffer will be streamed yet. In this function, we implement the
> > change_state() virtual method to trigger actions on specific state transitions.
> > Note that we also return GST_STATE_CHANGE_NO_PREROLL in
> > GST_STATE_CHANGE_READY_TO_PAUSED and GST_STATE_CHANGE_PLAYING_TO_PAUSED
> > transitions as this is required for all live sources.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 124 ++++++++++++++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 2177a8d..abb376b 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -10,13 +10,26 @@
> >  #include "gstlibcamerapad.h"
> >  #include "gstlibcamera-utils.h"
> >  
> > +#include <libcamera/camera.h>
> > +#include <libcamera/camera_manager.h>
> > +
> > +using namespace libcamera;
> > +
> >  GST_DEBUG_CATEGORY_STATIC(source_debug);
> >  #define GST_CAT_DEFAULT source_debug
> >  
> > +/* Used for C++ object with destructors */
> > +struct GstLibcameraSrcState {
> > +	std::shared_ptr<CameraManager> cm;
> 
> This one should be a std::unique_ptr as there's no need to share
> ownership.
> 
> > +	std::shared_ptr<Camera> cam;
> > +};
> > +
> >  struct _GstLibcameraSrc {
> >  	GstElement parent;
> >  	GstPad *srcpad;
> >  	gchar *camera_name;
> > +
> > +	GstLibcameraSrcState *state;
> >  };
> >  
> >  enum {
> > @@ -40,6 +53,83 @@ GstStaticPadTemplate request_src_template = {
> >  	"src_%s", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS
> >  };
> >  
> > +static bool
> > +gst_libcamera_src_open(GstLibcameraSrc *self)
> > +{
> > +	std::shared_ptr<CameraManager> cm = std::make_shared<CameraManager>();
> > +	std::shared_ptr<Camera> cam = nullptr;
> 
> No need to initialize cam to nullptr explicitly, this is automatic for
> shared_ptr.
> 
> > +	gint ret = 0;
> > +	g_autofree gchar *camera_name = nullptr;
> > +
> > +	GST_DEBUG_OBJECT(self, "Opening camera device ...");
> > +
> > +	ret = cm->start();
> > +	if (ret) {
> > +		GST_ELEMENT_ERROR(self, LIBRARY, INIT,
> > +				  ("Failed listing cameras."),
> > +				  ("libcamera::CameraMananger.start() failed: %s", g_strerror(-ret)));
> 
> Maybe ::start() instead of .start() ? Same comment for all the other
> locations below (and in other patches if applicable).
> 
> > +		return false;
> > +	}
> > +
> 
> You can move the camera_name variable definition right here.
> 
> > +	{
> > +		GST_OBJECT_LOCKER(self);
> > +		if (self->camera_name)
> > +			camera_name = g_strdup(self->camera_name);
> 
> So properties can change at any time, concurrently to the open call ?

There is GST_PARAM_MUTABLE_* flag to describe the limtations on which GstState
you are allowed to change the properties, but it's mostly informative. In this
case I've set MUTABLE_READY, but it's actually a mistake, I should not set this
flag, since it's only mutable in NULL state. With that pattern, it's safe and
you can change it anytime, and then cycle through NULL state. I try to stay safe
with nul-terminated string, specifically.

In most application (or in sane application I should say), the camera-name
property will be set, prior to state change, on the same thread we apply ready
state. So I could remove that lock.

> 
> > +	}
> > +
> > +	if (camera_name) {
> > +		cam = cm->get(self->camera_name);
> > +		if (!cam) {
> > +			GST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,
> > +					  ("Could not find a camera named '%s'.", self->camera_name),
> > +					  ("libcamera::CameraMananger.get() returned NULL"));
> > +			return false;
> > +		}
> > +	} else {
> > +		if (cm->cameras().empty()) {
> > +			GST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,
> > +					  ("Could not find any supported camera on this system."),
> > +					  ("libcamera::CameraMananger.cameras() is empty"));
> > +			return false;
> > +		}
> > +		cam = cm->cameras()[0];
> > +	}
> > +
> > +	GST_INFO_OBJECT(self, "Using camera named '%s'", cam->name().c_str());
> > +
> > +	ret = cam->acquire();
> > +	if (ret) {
> > +		GST_ELEMENT_ERROR(self, RESOURCE, BUSY,
> > +				  ("Camera name '%s' is already in use.", cam->name().c_str()),
> > +				  ("libcamera::Camera.acquire() failed: %s", g_strerror(ret)));
> > +		return false;
> > +	}
> > +
> > +	/* no need to lock here we didn't start our threads */
> 
> s/no/No/ and s/threads/threads./
> 
> > +	self->state->cm = cm;
> 
> This is not an issue for now, but when we'll support hotplug, will it be
> possible to create a single CameraManager instance that will be shared
> by GstLibcameraSrc and GstLibcameraProvider ?

These are totally independent things, not sure it's conveniant to share it. You
can't rely on having a src when you have a provider and vis-versa. I'm not sure,
I don't know enough about the intention with this manager. For sure I was under
the impression that if the manager in the source is monitoring, it's mostly a
waste of resource.

Normally for hotplugin we would want the v4l2src to fail when the camera is
unplugged and send an error message. Ideally with a specific error code, I could
extend GstResourceError enum for that purpose. But what application do, is that
if the camera pipeline fails (regardless of the error, they just report it to
the user and then when it received a message from the provider (usually just
after) it takes action to cleanup and possibly suggest another camera to the
user.

> 
> > +	self->state->cam = cam;
> > +
> > +	return true;
> > +}
> > +
> > +static void
> > +gst_libcamera_src_close(GstLibcameraSrc *self)
> > +{
> > +	GstLibcameraSrcState *state = self->state;
> > +	gint ret;
> > +
> > +	ret = state->cam->release();
> > +	if (ret) {
> > +		GST_ELEMENT_WARNING(self, RESOURCE, BUSY,
> > +				    ("Camera name '%s' is still in use.", state->cam->name().c_str()),
> > +				    ("libcamera::Camera.release() failed: %s", g_strerror(-ret)));
> > +	}
> > +
> > +	state->cam.reset();
> > +	state->cm->stop();
> > +	state->cm.reset();
> > +}
> > +
> >  static void
> >  gst_libcamera_src_set_property(GObject *object, guint prop_id,
> >  			       const GValue *value, GParamSpec *pspec)
> > @@ -73,7 +163,36 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,
> >  		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> >  		break;
> >  	}
> > +}
> >  
> > +static GstStateChangeReturn
> > +gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)
> > +{
> > +	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);
> > +	GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;
> > +	GstElementClass *klass = GST_ELEMENT_CLASS(gst_libcamera_src_parent_class);
> > +
> > +	ret = klass->change_state(element, transition);
> > +	if (ret == GST_STATE_CHANGE_FAILURE)
> > +		return ret;
> > +
> > +	switch (transition) {
> > +	case GST_STATE_CHANGE_NULL_TO_READY:
> > +		if (!gst_libcamera_src_open(self))
> > +			return GST_STATE_CHANGE_FAILURE;
> > +		break;
> > +	case GST_STATE_CHANGE_READY_TO_PAUSED:
> > +	case GST_STATE_CHANGE_PLAYING_TO_PAUSED:
> > +		ret = GST_STATE_CHANGE_NO_PREROLL;
> > +		break;
> > +	case GST_STATE_CHANGE_READY_TO_NULL:
> > +		gst_libcamera_src_close(self);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return ret;
> >  }
> >  
> >  static void
> > @@ -83,6 +202,7 @@ gst_libcamera_src_finalize(GObject *object)
> >  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(object);
> >  
> >  	g_free(self->camera_name);
> > +	delete self->state;
> >  
> >  	return klass->finalize(object);
> >  }
> > @@ -90,10 +210,12 @@ gst_libcamera_src_finalize(GObject *object)
> >  static void
> >  gst_libcamera_src_init(GstLibcameraSrc *self)
> >  {
> > +	GstLibcameraSrcState *state = new GstLibcameraSrcState();
> >  	GstPadTemplate *templ = gst_element_get_pad_template(GST_ELEMENT(self), "src");
> >  
> >  	self->srcpad = gst_pad_new_from_template(templ, "src");
> >  	gst_element_add_pad(GST_ELEMENT(self), self->srcpad);
> > +	self->state = state;
> 
> You can simply write
> 
> 	self->state = new GstLibcameraSrcState();
> 
> >  }
> >  
> >  static void
> > @@ -107,6 +229,8 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
> >  	object_class->get_property = gst_libcamera_src_get_property;
> >  	object_class->finalize = gst_libcamera_src_finalize;
> >  
> > +	element_class->change_state = gst_libcamera_src_change_state;
> > +
> >  	gst_element_class_set_metadata(element_class,
> >  				       "LibCamera Source", "Source/Video",
> >  				       "Linux Camera source using libcamera",