[libcamera-devel,v1,09/23] gst: libcamerasrc: Implement selection and acquisition
diff mbox series

Message ID 20200129033210.278800-10-nicolas@ndufresne.ca
State Superseded
Headers show
Series
  • GStreamer Element for libcamera
Related show

Commit Message

Nicolas Dufresne Jan. 29, 2020, 3:31 a.m. UTC 
From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

This add code to select and acquire a camera. With this, it is now
possible to run pipeline like:

   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@collabora.com>
---
 src/gstreamer/gstlibcamerasrc.cpp | 124 ++++++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)

Comments

Laurent Pinchart Feb. 11, 2020, 7:43 p.m. UTC | #1 
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@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@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 ?

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

> +	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",
Nicolas Dufresne Feb. 11, 2020, 10:23 p.m. UTC | #2 
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@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@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",
Laurent Pinchart Feb. 11, 2020, 11:07 p.m. UTC | #3 
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@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@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",

Patch
diff mbox series 

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;
+	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;
+	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)));
+		return false;
+	}
+
+	{
+		GST_OBJECT_LOCKER(self);
+		if (self->camera_name)
+			camera_name = g_strdup(self->camera_name);
+	}
+
+	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 */
+	self->state->cm = cm;
+	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;
 }
 
 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",