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

[Laurent Pinchart]

Hi Nicolas,

On Tue, Feb 11, 2020 at 05:23:28PM -0500, Nicolas Dufresne wrote:
> On mar, 2020-02-11 at 21:43 +0200, Laurent Pinchart wrote:
> > 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.

I don't mind keeping it to protect against some application
misbehaviours, the question was more for my information.

> > > +	}
> > > +
> > > +	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.

The issue with decoupling the provider and the source is that a process
is supposed to have a single CameraManager instance active at a time. I
think it would work with multiple instances (as long as we don't try to
acquire the same camera twice), but that hasn't been tested. It's a
problem we an deal with later, when implementing hotplug support.

> > > +	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",

-- 
Regards,

Laurent Pinchart