[{"id":3666,"web_url":"https://patchwork.libcamera.org/comment/3666/","msgid":"<20200211194339.GE20823@pendragon.ideasonboard.com>","date":"2020-02-11T19:43:39","subject":"Re: [libcamera-devel] [PATCH v1 09/23] gst: libcamerasrc: Implement\n\tselection and acquisition","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nThank you for the patch.\n\nOn Tue, Jan 28, 2020 at 10:31:56PM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> \n> This add code to select and acquire a camera. With this, it is now\n\ns/add/adds/\n\n> possible to run pipeline like:\n\ns/pipeline/a pipeline/ or s/pipeline/pipelines/\n\n> \n>    gst-launch-1.0 libcamerasrc ! fakesink\n> \n> Though no buffer will be streamed yet. In this function, we implement the\n> change_state() virtual method to trigger actions on specific state transitions.\n> Note that we also return GST_STATE_CHANGE_NO_PREROLL in\n> GST_STATE_CHANGE_READY_TO_PAUSED and GST_STATE_CHANGE_PLAYING_TO_PAUSED\n> transitions as this is required for all live sources.\n> \n> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> ---\n>  src/gstreamer/gstlibcamerasrc.cpp | 124 ++++++++++++++++++++++++++++++\n>  1 file changed, 124 insertions(+)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 2177a8d..abb376b 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -10,13 +10,26 @@\n>  #include \"gstlibcamerapad.h\"\n>  #include \"gstlibcamera-utils.h\"\n>  \n> +#include <libcamera/camera.h>\n> +#include <libcamera/camera_manager.h>\n> +\n> +using namespace libcamera;\n> +\n>  GST_DEBUG_CATEGORY_STATIC(source_debug);\n>  #define GST_CAT_DEFAULT source_debug\n>  \n> +/* Used for C++ object with destructors */\n> +struct GstLibcameraSrcState {\n> +\tstd::shared_ptr<CameraManager> cm;\n\nThis one should be a std::unique_ptr as there's no need to share\nownership.\n\n> +\tstd::shared_ptr<Camera> cam;\n> +};\n> +\n>  struct _GstLibcameraSrc {\n>  \tGstElement parent;\n>  \tGstPad *srcpad;\n>  \tgchar *camera_name;\n> +\n> +\tGstLibcameraSrcState *state;\n>  };\n>  \n>  enum {\n> @@ -40,6 +53,83 @@ GstStaticPadTemplate request_src_template = {\n>  \t\"src_%s\", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS\n>  };\n>  \n> +static bool\n> +gst_libcamera_src_open(GstLibcameraSrc *self)\n> +{\n> +\tstd::shared_ptr<CameraManager> cm = std::make_shared<CameraManager>();\n> +\tstd::shared_ptr<Camera> cam = nullptr;\n\nNo need to initialize cam to nullptr explicitly, this is automatic for\nshared_ptr.\n\n> +\tgint ret = 0;\n> +\tg_autofree gchar *camera_name = nullptr;\n> +\n> +\tGST_DEBUG_OBJECT(self, \"Opening camera device ...\");\n> +\n> +\tret = cm->start();\n> +\tif (ret) {\n> +\t\tGST_ELEMENT_ERROR(self, LIBRARY, INIT,\n> +\t\t\t\t  (\"Failed listing cameras.\"),\n> +\t\t\t\t  (\"libcamera::CameraMananger.start() failed: %s\", g_strerror(-ret)));\n\nMaybe ::start() instead of .start() ? Same comment for all the other\nlocations below (and in other patches if applicable).\n\n> +\t\treturn false;\n> +\t}\n> +\n\nYou can move the camera_name variable definition right here.\n\n> +\t{\n> +\t\tGST_OBJECT_LOCKER(self);\n> +\t\tif (self->camera_name)\n> +\t\t\tcamera_name = g_strdup(self->camera_name);\n\nSo properties can change at any time, concurrently to the open call ?\n\n> +\t}\n> +\n> +\tif (camera_name) {\n> +\t\tcam = cm->get(self->camera_name);\n> +\t\tif (!cam) {\n> +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,\n> +\t\t\t\t\t  (\"Could not find a camera named '%s'.\", self->camera_name),\n> +\t\t\t\t\t  (\"libcamera::CameraMananger.get() returned NULL\"));\n> +\t\t\treturn false;\n> +\t\t}\n> +\t} else {\n> +\t\tif (cm->cameras().empty()) {\n> +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,\n> +\t\t\t\t\t  (\"Could not find any supported camera on this system.\"),\n> +\t\t\t\t\t  (\"libcamera::CameraMananger.cameras() is empty\"));\n> +\t\t\treturn false;\n> +\t\t}\n> +\t\tcam = cm->cameras()[0];\n> +\t}\n> +\n> +\tGST_INFO_OBJECT(self, \"Using camera named '%s'\", cam->name().c_str());\n> +\n> +\tret = cam->acquire();\n> +\tif (ret) {\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, BUSY,\n> +\t\t\t\t  (\"Camera name '%s' is already in use.\", cam->name().c_str()),\n> +\t\t\t\t  (\"libcamera::Camera.acquire() failed: %s\", g_strerror(ret)));\n> +\t\treturn false;\n> +\t}\n> +\n> +\t/* no need to lock here we didn't start our threads */\n\ns/no/No/ and s/threads/threads./\n\n> +\tself->state->cm = cm;\n\nThis is not an issue for now, but when we'll support hotplug, will it be\npossible to create a single CameraManager instance that will be shared\nby GstLibcameraSrc and GstLibcameraProvider ?\n\n> +\tself->state->cam = cam;\n> +\n> +\treturn true;\n> +}\n> +\n> +static void\n> +gst_libcamera_src_close(GstLibcameraSrc *self)\n> +{\n> +\tGstLibcameraSrcState *state = self->state;\n> +\tgint ret;\n> +\n> +\tret = state->cam->release();\n> +\tif (ret) {\n> +\t\tGST_ELEMENT_WARNING(self, RESOURCE, BUSY,\n> +\t\t\t\t    (\"Camera name '%s' is still in use.\", state->cam->name().c_str()),\n> +\t\t\t\t    (\"libcamera::Camera.release() failed: %s\", g_strerror(-ret)));\n> +\t}\n> +\n> +\tstate->cam.reset();\n> +\tstate->cm->stop();\n> +\tstate->cm.reset();\n> +}\n> +\n>  static void\n>  gst_libcamera_src_set_property(GObject *object, guint prop_id,\n>  \t\t\t       const GValue *value, GParamSpec *pspec)\n> @@ -73,7 +163,36 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n>  \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n>  \t\tbreak;\n>  \t}\n> +}\n>  \n> +static GstStateChangeReturn\n> +gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> +{\n> +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);\n> +\tGstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;\n> +\tGstElementClass *klass = GST_ELEMENT_CLASS(gst_libcamera_src_parent_class);\n> +\n> +\tret = klass->change_state(element, transition);\n> +\tif (ret == GST_STATE_CHANGE_FAILURE)\n> +\t\treturn ret;\n> +\n> +\tswitch (transition) {\n> +\tcase GST_STATE_CHANGE_NULL_TO_READY:\n> +\t\tif (!gst_libcamera_src_open(self))\n> +\t\t\treturn GST_STATE_CHANGE_FAILURE;\n> +\t\tbreak;\n> +\tcase GST_STATE_CHANGE_READY_TO_PAUSED:\n> +\tcase GST_STATE_CHANGE_PLAYING_TO_PAUSED:\n> +\t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> +\t\tbreak;\n> +\tcase GST_STATE_CHANGE_READY_TO_NULL:\n> +\t\tgst_libcamera_src_close(self);\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tbreak;\n> +\t}\n> +\n> +\treturn ret;\n>  }\n>  \n>  static void\n> @@ -83,6 +202,7 @@ gst_libcamera_src_finalize(GObject *object)\n>  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(object);\n>  \n>  \tg_free(self->camera_name);\n> +\tdelete self->state;\n>  \n>  \treturn klass->finalize(object);\n>  }\n> @@ -90,10 +210,12 @@ gst_libcamera_src_finalize(GObject *object)\n>  static void\n>  gst_libcamera_src_init(GstLibcameraSrc *self)\n>  {\n> +\tGstLibcameraSrcState *state = new GstLibcameraSrcState();\n>  \tGstPadTemplate *templ = gst_element_get_pad_template(GST_ELEMENT(self), \"src\");\n>  \n>  \tself->srcpad = gst_pad_new_from_template(templ, \"src\");\n>  \tgst_element_add_pad(GST_ELEMENT(self), self->srcpad);\n> +\tself->state = state;\n\nYou can simply write\n\n\tself->state = new GstLibcameraSrcState();\n\n>  }\n>  \n>  static void\n> @@ -107,6 +229,8 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n>  \tobject_class->get_property = gst_libcamera_src_get_property;\n>  \tobject_class->finalize = gst_libcamera_src_finalize;\n>  \n> +\telement_class->change_state = gst_libcamera_src_change_state;\n> +\n>  \tgst_element_class_set_metadata(element_class,\n>  \t\t\t\t       \"LibCamera Source\", \"Source/Video\",\n>  \t\t\t\t       \"Linux Camera source using libcamera\",","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCD0760F3C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 11 Feb 2020 20:43:54 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F6D02D2;\n\tTue, 11 Feb 2020 20:43:54 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581450234;\n\tbh=B1VJtEq5zMn/AnF+WIfQAtPa6Z/aZF670x4ZcZgCyrU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l2KZ7HecdVXqoChkpa8n849JUSgqxMM3wP8P5G6/Dy0OETh4Xo2NJMOMIqyLtNZHr\n\tM9M460fzW8+ngVK43wnAPKDGTqAgjDQoANYJYF3KNYYVVfUL41Awssz3YBlLAScqDG\n\tg6nOfEyoMTIA6/6i97saStVklCSsiYmistVuwXP0=","Date":"Tue, 11 Feb 2020 21:43:39 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Nicolas Dufresne <nicolas@ndufresne.ca>","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne <nicolas.dufresne@collabora.com>","Message-ID":"<20200211194339.GE20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-10-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200129033210.278800-10-nicolas@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 09/23] gst: libcamerasrc: Implement\n\tselection and acquisition","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 11 Feb 2020 19:43:55 -0000"}},{"id":3672,"web_url":"https://patchwork.libcamera.org/comment/3672/","msgid":"<1892aeb79fa9ca12ebfa7ec5e3e59b3893a707a6.camel@collabora.com>","date":"2020-02-11T22:23:28","subject":"Re: [libcamera-devel] [PATCH v1 09/23] gst: libcamerasrc: Implement\n\tselection and acquisition","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"On mar, 2020-02-11 at 21:43 +0200, Laurent Pinchart wrote:\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 28, 2020 at 10:31:56PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > \n> > This add code to select and acquire a camera. With this, it is now\n> \n> s/add/adds/\n> \n> > possible to run pipeline like:\n> \n> s/pipeline/a pipeline/ or s/pipeline/pipelines/\n> \n> >    gst-launch-1.0 libcamerasrc ! fakesink\n> > \n> > Though no buffer will be streamed yet. In this function, we implement the\n> > change_state() virtual method to trigger actions on specific state transitions.\n> > Note that we also return GST_STATE_CHANGE_NO_PREROLL in\n> > GST_STATE_CHANGE_READY_TO_PAUSED and GST_STATE_CHANGE_PLAYING_TO_PAUSED\n> > transitions as this is required for all live sources.\n> > \n> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > ---\n> >  src/gstreamer/gstlibcamerasrc.cpp | 124 ++++++++++++++++++++++++++++++\n> >  1 file changed, 124 insertions(+)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 2177a8d..abb376b 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -10,13 +10,26 @@\n> >  #include \"gstlibcamerapad.h\"\n> >  #include \"gstlibcamera-utils.h\"\n> >  \n> > +#include <libcamera/camera.h>\n> > +#include <libcamera/camera_manager.h>\n> > +\n> > +using namespace libcamera;\n> > +\n> >  GST_DEBUG_CATEGORY_STATIC(source_debug);\n> >  #define GST_CAT_DEFAULT source_debug\n> >  \n> > +/* Used for C++ object with destructors */\n> > +struct GstLibcameraSrcState {\n> > +\tstd::shared_ptr<CameraManager> cm;\n> \n> This one should be a std::unique_ptr as there's no need to share\n> ownership.\n> \n> > +\tstd::shared_ptr<Camera> cam;\n> > +};\n> > +\n> >  struct _GstLibcameraSrc {\n> >  \tGstElement parent;\n> >  \tGstPad *srcpad;\n> >  \tgchar *camera_name;\n> > +\n> > +\tGstLibcameraSrcState *state;\n> >  };\n> >  \n> >  enum {\n> > @@ -40,6 +53,83 @@ GstStaticPadTemplate request_src_template = {\n> >  \t\"src_%s\", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS\n> >  };\n> >  \n> > +static bool\n> > +gst_libcamera_src_open(GstLibcameraSrc *self)\n> > +{\n> > +\tstd::shared_ptr<CameraManager> cm = std::make_shared<CameraManager>();\n> > +\tstd::shared_ptr<Camera> cam = nullptr;\n> \n> No need to initialize cam to nullptr explicitly, this is automatic for\n> shared_ptr.\n> \n> > +\tgint ret = 0;\n> > +\tg_autofree gchar *camera_name = nullptr;\n> > +\n> > +\tGST_DEBUG_OBJECT(self, \"Opening camera device ...\");\n> > +\n> > +\tret = cm->start();\n> > +\tif (ret) {\n> > +\t\tGST_ELEMENT_ERROR(self, LIBRARY, INIT,\n> > +\t\t\t\t  (\"Failed listing cameras.\"),\n> > +\t\t\t\t  (\"libcamera::CameraMananger.start() failed: %s\", g_strerror(-ret)));\n> \n> Maybe ::start() instead of .start() ? Same comment for all the other\n> locations below (and in other patches if applicable).\n> \n> > +\t\treturn false;\n> > +\t}\n> > +\n> \n> You can move the camera_name variable definition right here.\n> \n> > +\t{\n> > +\t\tGST_OBJECT_LOCKER(self);\n> > +\t\tif (self->camera_name)\n> > +\t\t\tcamera_name = g_strdup(self->camera_name);\n> \n> So properties can change at any time, concurrently to the open call ?\n\nThere is GST_PARAM_MUTABLE_* flag to describe the limtations on which GstState\nyou are allowed to change the properties, but it's mostly informative. In this\ncase I've set MUTABLE_READY, but it's actually a mistake, I should not set this\nflag, since it's only mutable in NULL state. With that pattern, it's safe and\nyou can change it anytime, and then cycle through NULL state. I try to stay safe\nwith nul-terminated string, specifically.\n\nIn most application (or in sane application I should say), the camera-name\nproperty will be set, prior to state change, on the same thread we apply ready\nstate. So I could remove that lock.\n\n> \n> > +\t}\n> > +\n> > +\tif (camera_name) {\n> > +\t\tcam = cm->get(self->camera_name);\n> > +\t\tif (!cam) {\n> > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,\n> > +\t\t\t\t\t  (\"Could not find a camera named '%s'.\", self->camera_name),\n> > +\t\t\t\t\t  (\"libcamera::CameraMananger.get() returned NULL\"));\n> > +\t\t\treturn false;\n> > +\t\t}\n> > +\t} else {\n> > +\t\tif (cm->cameras().empty()) {\n> > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,\n> > +\t\t\t\t\t  (\"Could not find any supported camera on this system.\"),\n> > +\t\t\t\t\t  (\"libcamera::CameraMananger.cameras() is empty\"));\n> > +\t\t\treturn false;\n> > +\t\t}\n> > +\t\tcam = cm->cameras()[0];\n> > +\t}\n> > +\n> > +\tGST_INFO_OBJECT(self, \"Using camera named '%s'\", cam->name().c_str());\n> > +\n> > +\tret = cam->acquire();\n> > +\tif (ret) {\n> > +\t\tGST_ELEMENT_ERROR(self, RESOURCE, BUSY,\n> > +\t\t\t\t  (\"Camera name '%s' is already in use.\", cam->name().c_str()),\n> > +\t\t\t\t  (\"libcamera::Camera.acquire() failed: %s\", g_strerror(ret)));\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\t/* no need to lock here we didn't start our threads */\n> \n> s/no/No/ and s/threads/threads./\n> \n> > +\tself->state->cm = cm;\n> \n> This is not an issue for now, but when we'll support hotplug, will it be\n> possible to create a single CameraManager instance that will be shared\n> by GstLibcameraSrc and GstLibcameraProvider ?\n\nThese are totally independent things, not sure it's conveniant to share it. You\ncan't rely on having a src when you have a provider and vis-versa. I'm not sure,\nI don't know enough about the intention with this manager. For sure I was under\nthe impression that if the manager in the source is monitoring, it's mostly a\nwaste of resource.\n\nNormally for hotplugin we would want the v4l2src to fail when the camera is\nunplugged and send an error message. Ideally with a specific error code, I could\nextend GstResourceError enum for that purpose. But what application do, is that\nif the camera pipeline fails (regardless of the error, they just report it to\nthe user and then when it received a message from the provider (usually just\nafter) it takes action to cleanup and possibly suggest another camera to the\nuser.\n\n> \n> > +\tself->state->cam = cam;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_src_close(GstLibcameraSrc *self)\n> > +{\n> > +\tGstLibcameraSrcState *state = self->state;\n> > +\tgint ret;\n> > +\n> > +\tret = state->cam->release();\n> > +\tif (ret) {\n> > +\t\tGST_ELEMENT_WARNING(self, RESOURCE, BUSY,\n> > +\t\t\t\t    (\"Camera name '%s' is still in use.\", state->cam->name().c_str()),\n> > +\t\t\t\t    (\"libcamera::Camera.release() failed: %s\", g_strerror(-ret)));\n> > +\t}\n> > +\n> > +\tstate->cam.reset();\n> > +\tstate->cm->stop();\n> > +\tstate->cm.reset();\n> > +}\n> > +\n> >  static void\n> >  gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> >  \t\t\t       const GValue *value, GParamSpec *pspec)\n> > @@ -73,7 +163,36 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> >  \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> >  \t\tbreak;\n> >  \t}\n> > +}\n> >  \n> > +static GstStateChangeReturn\n> > +gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> > +{\n> > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);\n> > +\tGstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;\n> > +\tGstElementClass *klass = GST_ELEMENT_CLASS(gst_libcamera_src_parent_class);\n> > +\n> > +\tret = klass->change_state(element, transition);\n> > +\tif (ret == GST_STATE_CHANGE_FAILURE)\n> > +\t\treturn ret;\n> > +\n> > +\tswitch (transition) {\n> > +\tcase GST_STATE_CHANGE_NULL_TO_READY:\n> > +\t\tif (!gst_libcamera_src_open(self))\n> > +\t\t\treturn GST_STATE_CHANGE_FAILURE;\n> > +\t\tbreak;\n> > +\tcase GST_STATE_CHANGE_READY_TO_PAUSED:\n> > +\tcase GST_STATE_CHANGE_PLAYING_TO_PAUSED:\n> > +\t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> > +\t\tbreak;\n> > +\tcase GST_STATE_CHANGE_READY_TO_NULL:\n> > +\t\tgst_libcamera_src_close(self);\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\treturn ret;\n> >  }\n> >  \n> >  static void\n> > @@ -83,6 +202,7 @@ gst_libcamera_src_finalize(GObject *object)\n> >  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(object);\n> >  \n> >  \tg_free(self->camera_name);\n> > +\tdelete self->state;\n> >  \n> >  \treturn klass->finalize(object);\n> >  }\n> > @@ -90,10 +210,12 @@ gst_libcamera_src_finalize(GObject *object)\n> >  static void\n> >  gst_libcamera_src_init(GstLibcameraSrc *self)\n> >  {\n> > +\tGstLibcameraSrcState *state = new GstLibcameraSrcState();\n> >  \tGstPadTemplate *templ = gst_element_get_pad_template(GST_ELEMENT(self), \"src\");\n> >  \n> >  \tself->srcpad = gst_pad_new_from_template(templ, \"src\");\n> >  \tgst_element_add_pad(GST_ELEMENT(self), self->srcpad);\n> > +\tself->state = state;\n> \n> You can simply write\n> \n> \tself->state = new GstLibcameraSrcState();\n> \n> >  }\n> >  \n> >  static void\n> > @@ -107,6 +229,8 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> >  \tobject_class->get_property = gst_libcamera_src_get_property;\n> >  \tobject_class->finalize = gst_libcamera_src_finalize;\n> >  \n> > +\telement_class->change_state = gst_libcamera_src_change_state;\n> > +\n> >  \tgst_element_class_set_metadata(element_class,\n> >  \t\t\t\t       \"LibCamera Source\", \"Source/Video\",\n> >  \t\t\t\t       \"Linux Camera source using libcamera\",","headers":{"Return-Path":"<nicolas.dufresne@collabora.com>","Received":["from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E963660F3C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 11 Feb 2020 23:23:38 +0100 (CET)","from nicolas-tpx395.localdomain (unknown [IPv6:2610:98:8005::527])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 295D428DA96;\n\tTue, 11 Feb 2020 22:23:38 +0000 (GMT)"],"Message-ID":"<1892aeb79fa9ca12ebfa7ec5e3e59b3893a707a6.camel@collabora.com>","From":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 11 Feb 2020 17:23:28 -0500","In-Reply-To":"<20200211194339.GE20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-10-nicolas@ndufresne.ca>\n\t<20200211194339.GE20823@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.3 (3.34.3-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v1 09/23] gst: libcamerasrc: Implement\n\tselection and acquisition","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 11 Feb 2020 22:23:39 -0000"}},{"id":3674,"web_url":"https://patchwork.libcamera.org/comment/3674/","msgid":"<20200211230711.GI20823@pendragon.ideasonboard.com>","date":"2020-02-11T23:07:11","subject":"Re: [libcamera-devel] [PATCH v1 09/23] gst: libcamerasrc: Implement\n\tselection and acquisition","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Tue, Feb 11, 2020 at 05:23:28PM -0500, Nicolas Dufresne wrote:\n> On mar, 2020-02-11 at 21:43 +0200, Laurent Pinchart wrote:\n> > On Tue, Jan 28, 2020 at 10:31:56PM -0500, Nicolas Dufresne wrote:\n> > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > > \n> > > This add code to select and acquire a camera. With this, it is now\n> > \n> > s/add/adds/\n> > \n> > > possible to run pipeline like:\n> > \n> > s/pipeline/a pipeline/ or s/pipeline/pipelines/\n> > \n> > >    gst-launch-1.0 libcamerasrc ! fakesink\n> > > \n> > > Though no buffer will be streamed yet. In this function, we implement the\n> > > change_state() virtual method to trigger actions on specific state transitions.\n> > > Note that we also return GST_STATE_CHANGE_NO_PREROLL in\n> > > GST_STATE_CHANGE_READY_TO_PAUSED and GST_STATE_CHANGE_PLAYING_TO_PAUSED\n> > > transitions as this is required for all live sources.\n> > > \n> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > > ---\n> > >  src/gstreamer/gstlibcamerasrc.cpp | 124 ++++++++++++++++++++++++++++++\n> > >  1 file changed, 124 insertions(+)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 2177a8d..abb376b 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -10,13 +10,26 @@\n> > >  #include \"gstlibcamerapad.h\"\n> > >  #include \"gstlibcamera-utils.h\"\n> > >  \n> > > +#include <libcamera/camera.h>\n> > > +#include <libcamera/camera_manager.h>\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > >  GST_DEBUG_CATEGORY_STATIC(source_debug);\n> > >  #define GST_CAT_DEFAULT source_debug\n> > >  \n> > > +/* Used for C++ object with destructors */\n> > > +struct GstLibcameraSrcState {\n> > > +\tstd::shared_ptr<CameraManager> cm;\n> > \n> > This one should be a std::unique_ptr as there's no need to share\n> > ownership.\n> > \n> > > +\tstd::shared_ptr<Camera> cam;\n> > > +};\n> > > +\n> > >  struct _GstLibcameraSrc {\n> > >  \tGstElement parent;\n> > >  \tGstPad *srcpad;\n> > >  \tgchar *camera_name;\n> > > +\n> > > +\tGstLibcameraSrcState *state;\n> > >  };\n> > >  \n> > >  enum {\n> > > @@ -40,6 +53,83 @@ GstStaticPadTemplate request_src_template = {\n> > >  \t\"src_%s\", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS\n> > >  };\n> > >  \n> > > +static bool\n> > > +gst_libcamera_src_open(GstLibcameraSrc *self)\n> > > +{\n> > > +\tstd::shared_ptr<CameraManager> cm = std::make_shared<CameraManager>();\n> > > +\tstd::shared_ptr<Camera> cam = nullptr;\n> > \n> > No need to initialize cam to nullptr explicitly, this is automatic for\n> > shared_ptr.\n> > \n> > > +\tgint ret = 0;\n> > > +\tg_autofree gchar *camera_name = nullptr;\n> > > +\n> > > +\tGST_DEBUG_OBJECT(self, \"Opening camera device ...\");\n> > > +\n> > > +\tret = cm->start();\n> > > +\tif (ret) {\n> > > +\t\tGST_ELEMENT_ERROR(self, LIBRARY, INIT,\n> > > +\t\t\t\t  (\"Failed listing cameras.\"),\n> > > +\t\t\t\t  (\"libcamera::CameraMananger.start() failed: %s\", g_strerror(-ret)));\n> > \n> > Maybe ::start() instead of .start() ? Same comment for all the other\n> > locations below (and in other patches if applicable).\n> > \n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > \n> > You can move the camera_name variable definition right here.\n> > \n> > > +\t{\n> > > +\t\tGST_OBJECT_LOCKER(self);\n> > > +\t\tif (self->camera_name)\n> > > +\t\t\tcamera_name = g_strdup(self->camera_name);\n> > \n> > So properties can change at any time, concurrently to the open call ?\n> \n> There is GST_PARAM_MUTABLE_* flag to describe the limtations on which GstState\n> you are allowed to change the properties, but it's mostly informative. In this\n> case I've set MUTABLE_READY, but it's actually a mistake, I should not set this\n> flag, since it's only mutable in NULL state. With that pattern, it's safe and\n> you can change it anytime, and then cycle through NULL state. I try to stay safe\n> with nul-terminated string, specifically.\n> \n> In most application (or in sane application I should say), the camera-name\n> property will be set, prior to state change, on the same thread we apply ready\n> state. So I could remove that lock.\n\nI don't mind keeping it to protect against some application\nmisbehaviours, the question was more for my information.\n\n> > > +\t}\n> > > +\n> > > +\tif (camera_name) {\n> > > +\t\tcam = cm->get(self->camera_name);\n> > > +\t\tif (!cam) {\n> > > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,\n> > > +\t\t\t\t\t  (\"Could not find a camera named '%s'.\", self->camera_name),\n> > > +\t\t\t\t\t  (\"libcamera::CameraMananger.get() returned NULL\"));\n> > > +\t\t\treturn false;\n> > > +\t\t}\n> > > +\t} else {\n> > > +\t\tif (cm->cameras().empty()) {\n> > > +\t\t\tGST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,\n> > > +\t\t\t\t\t  (\"Could not find any supported camera on this system.\"),\n> > > +\t\t\t\t\t  (\"libcamera::CameraMananger.cameras() is empty\"));\n> > > +\t\t\treturn false;\n> > > +\t\t}\n> > > +\t\tcam = cm->cameras()[0];\n> > > +\t}\n> > > +\n> > > +\tGST_INFO_OBJECT(self, \"Using camera named '%s'\", cam->name().c_str());\n> > > +\n> > > +\tret = cam->acquire();\n> > > +\tif (ret) {\n> > > +\t\tGST_ELEMENT_ERROR(self, RESOURCE, BUSY,\n> > > +\t\t\t\t  (\"Camera name '%s' is already in use.\", cam->name().c_str()),\n> > > +\t\t\t\t  (\"libcamera::Camera.acquire() failed: %s\", g_strerror(ret)));\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\t/* no need to lock here we didn't start our threads */\n> > \n> > s/no/No/ and s/threads/threads./\n> > \n> > > +\tself->state->cm = cm;\n> > \n> > This is not an issue for now, but when we'll support hotplug, will it be\n> > possible to create a single CameraManager instance that will be shared\n> > by GstLibcameraSrc and GstLibcameraProvider ?\n> \n> These are totally independent things, not sure it's conveniant to share it. You\n> can't rely on having a src when you have a provider and vis-versa. I'm not sure,\n> I don't know enough about the intention with this manager. For sure I was under\n> the impression that if the manager in the source is monitoring, it's mostly a\n> waste of resource.\n> \n> Normally for hotplugin we would want the v4l2src to fail when the camera is\n> unplugged and send an error message. Ideally with a specific error code, I could\n> extend GstResourceError enum for that purpose. But what application do, is that\n> if the camera pipeline fails (regardless of the error, they just report it to\n> the user and then when it received a message from the provider (usually just\n> after) it takes action to cleanup and possibly suggest another camera to the\n> user.\n\nThe issue with decoupling the provider and the source is that a process\nis supposed to have a single CameraManager instance active at a time. I\nthink it would work with multiple instances (as long as we don't try to\nacquire the same camera twice), but that hasn't been tested. It's a\nproblem we an deal with later, when implementing hotplug support.\n\n> > > +\tself->state->cam = cam;\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_src_close(GstLibcameraSrc *self)\n> > > +{\n> > > +\tGstLibcameraSrcState *state = self->state;\n> > > +\tgint ret;\n> > > +\n> > > +\tret = state->cam->release();\n> > > +\tif (ret) {\n> > > +\t\tGST_ELEMENT_WARNING(self, RESOURCE, BUSY,\n> > > +\t\t\t\t    (\"Camera name '%s' is still in use.\", state->cam->name().c_str()),\n> > > +\t\t\t\t    (\"libcamera::Camera.release() failed: %s\", g_strerror(-ret)));\n> > > +\t}\n> > > +\n> > > +\tstate->cam.reset();\n> > > +\tstate->cm->stop();\n> > > +\tstate->cm.reset();\n> > > +}\n> > > +\n> > >  static void\n> > >  gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> > >  \t\t\t       const GValue *value, GParamSpec *pspec)\n> > > @@ -73,7 +163,36 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> > >  \t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > >  \t\tbreak;\n> > >  \t}\n> > > +}\n> > >  \n> > > +static GstStateChangeReturn\n> > > +gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> > > +{\n> > > +\tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(element);\n> > > +\tGstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;\n> > > +\tGstElementClass *klass = GST_ELEMENT_CLASS(gst_libcamera_src_parent_class);\n> > > +\n> > > +\tret = klass->change_state(element, transition);\n> > > +\tif (ret == GST_STATE_CHANGE_FAILURE)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tswitch (transition) {\n> > > +\tcase GST_STATE_CHANGE_NULL_TO_READY:\n> > > +\t\tif (!gst_libcamera_src_open(self))\n> > > +\t\t\treturn GST_STATE_CHANGE_FAILURE;\n> > > +\t\tbreak;\n> > > +\tcase GST_STATE_CHANGE_READY_TO_PAUSED:\n> > > +\tcase GST_STATE_CHANGE_PLAYING_TO_PAUSED:\n> > > +\t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> > > +\t\tbreak;\n> > > +\tcase GST_STATE_CHANGE_READY_TO_NULL:\n> > > +\t\tgst_libcamera_src_close(self);\n> > > +\t\tbreak;\n> > > +\tdefault:\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\n> > > +\treturn ret;\n> > >  }\n> > >  \n> > >  static void\n> > > @@ -83,6 +202,7 @@ gst_libcamera_src_finalize(GObject *object)\n> > >  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(object);\n> > >  \n> > >  \tg_free(self->camera_name);\n> > > +\tdelete self->state;\n> > >  \n> > >  \treturn klass->finalize(object);\n> > >  }\n> > > @@ -90,10 +210,12 @@ gst_libcamera_src_finalize(GObject *object)\n> > >  static void\n> > >  gst_libcamera_src_init(GstLibcameraSrc *self)\n> > >  {\n> > > +\tGstLibcameraSrcState *state = new GstLibcameraSrcState();\n> > >  \tGstPadTemplate *templ = gst_element_get_pad_template(GST_ELEMENT(self), \"src\");\n> > >  \n> > >  \tself->srcpad = gst_pad_new_from_template(templ, \"src\");\n> > >  \tgst_element_add_pad(GST_ELEMENT(self), self->srcpad);\n> > > +\tself->state = state;\n> > \n> > You can simply write\n> > \n> > \tself->state = new GstLibcameraSrcState();\n> > \n> > >  }\n> > >  \n> > >  static void\n> > > @@ -107,6 +229,8 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > >  \tobject_class->get_property = gst_libcamera_src_get_property;\n> > >  \tobject_class->finalize = gst_libcamera_src_finalize;\n> > >  \n> > > +\telement_class->change_state = gst_libcamera_src_change_state;\n> > > +\n> > >  \tgst_element_class_set_metadata(element_class,\n> > >  \t\t\t\t       \"LibCamera Source\", \"Source/Video\",\n> > >  \t\t\t\t       \"Linux Camera source using libcamera\",","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B082060990\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 12 Feb 2020 00:07:26 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15FFB9DA;\n\tWed, 12 Feb 2020 00:07:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581462446;\n\tbh=Q20b6E4cSoaLrnJJLxmROnwWis1wbN2jQW0HB6vxD7c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dagcuE6TiuFBduCirY3JNstokj6U0KGaqPVkRVvNQsCT00GZ56LZrFaLYjXP4T238\n\t85u3tCumtuGr4ZmpiFTBAB8Nv7W+/2qrkaXe/lHhhvoOYWd1D5aZEtUDmSDTRyttbh\n\tGOt/yl8RIoyMPvrK8SUKLqFW7lh9j4oSB84tOAMA=","Date":"Wed, 12 Feb 2020 01:07:11 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200211230711.GI20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-10-nicolas@ndufresne.ca>\n\t<20200211194339.GE20823@pendragon.ideasonboard.com>\n\t<1892aeb79fa9ca12ebfa7ec5e3e59b3893a707a6.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<1892aeb79fa9ca12ebfa7ec5e3e59b3893a707a6.camel@collabora.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 09/23] gst: libcamerasrc: Implement\n\tselection and acquisition","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 11 Feb 2020 23:07:26 -0000"}}]