[{"id":3873,"web_url":"https://patchwork.libcamera.org/comment/3873/","msgid":"<20200229145458.GT18738@pendragon.ideasonboard.com>","date":"2020-02-29T14:54:58","subject":"Re: [libcamera-devel] [PATCH v2 22/27] gst: libcamerasrc: Implement\n\tinitial streaming","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 Thu, Feb 27, 2020 at 03:04:02PM -0500, Nicolas Dufresne wrote:\n> With this patch, the element is now able to push buffers to the next\n> element in the graph. The buffers are currently missing any metadata\n> like timestamp, sequence number. This will be added in the next commit.\n> \n> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> ---\n>  src/gstreamer/gstlibcamerapad.cpp |  11 +-\n>  src/gstreamer/gstlibcamerapad.h   |   2 +\n>  src/gstreamer/gstlibcamerasrc.cpp | 192 +++++++++++++++++++++++++++++-\n>  3 files changed, 202 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp\n> index 8662c92..2cf1630 100644\n> --- a/src/gstreamer/gstlibcamerapad.cpp\n> +++ b/src/gstreamer/gstlibcamerapad.cpp\n> @@ -18,6 +18,7 @@ struct _GstLibcameraPad {\n>  \tStreamRole role;\n>  \tGstLibcameraPool *pool;\n>  \tGQueue pending_buffers;\n> +\tGstClockTime latency;\n>  };\n>  \n>  enum {\n> @@ -159,7 +160,15 @@ gst_libcamera_pad_push_pending(GstPad *pad)\n>  \t}\n>  \n>  \tif (!buffer)\n> -\t\treturn GST_FLOW_CUSTOM_SUCCESS;\n> +\t\treturn GST_FLOW_OK;\n\nDoes this belong to the previous patch ?\n\n>  \n>  \treturn gst_pad_push(pad, buffer);\n>  }\n> +\n> +bool\n> +gst_libcamera_pad_has_pending(GstPad *pad)\n> +{\n> +\tauto *self = GST_LIBCAMERA_PAD(pad);\n> +\tGLibLocker lock(GST_OBJECT(self));\n> +\treturn (self->pending_buffers.length > 0);\n\nNo need for the outer parentheses.\n\n> +}\n> diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h\n> index d928570..eb24000 100644\n> --- a/src/gstreamer/gstlibcamerapad.h\n> +++ b/src/gstreamer/gstlibcamerapad.h\n> @@ -30,4 +30,6 @@ void gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer);\n>  \n>  GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);\n>  \n> +bool gst_libcamera_pad_has_pending(GstPad *pad);\n> +\n>  #endif /* __GST_LIBCAMERA_PAD_H__ */\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 83f93cc..70f2048 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -18,8 +18,10 @@\n>  #include \"gstlibcamerapool.h\"\n>  #include \"gstlibcamerasrc.h\"\n>  \n> +#include <gst/base/base.h>\n>  #include <libcamera/camera.h>\n>  #include <libcamera/camera_manager.h>\n> +#include <queue>\n>  #include <vector>\n>  \n>  using namespace libcamera;\n> @@ -27,12 +29,71 @@ using namespace libcamera;\n>  GST_DEBUG_CATEGORY_STATIC(source_debug);\n>  #define GST_CAT_DEFAULT source_debug\n>  \n> +struct RequestWrap {\n> +\tRequestWrap(Request *request);\n> +\t~RequestWrap();\n> +\n> +\tvoid attachBuffer(GstBuffer *buffer);\n> +\tGstBuffer *detachBuffer(Stream *stream);\n> +\n> +\t/* For ptr comparision only. */\n\ns/comparision/comparison/\n\n> +\tRequest *request_;\n> +\tstd::map<Stream *, GstBuffer *> buffers_;\n> +};\n> +\n> +RequestWrap::RequestWrap(Request *request)\n> +\t: request_(request)\n> +{\n> +}\n> +\n> +RequestWrap::~RequestWrap()\n> +{\n> +\tfor (std::pair<Stream *const, GstBuffer *> &item : buffers_) {\n> +\t\tif (item.second)\n> +\t\t\tgst_buffer_unref(item.second);\n> +\t}\n> +}\n> +\n> +void RequestWrap::attachBuffer(GstBuffer *buffer)\n> +{\n> +\tFrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);\n> +\tStream *stream = gst_libcamera_buffer_get_stream(buffer);\n> +\n> +\trequest_->addBuffer(stream, fb);\n> +\n> +\tauto item = buffers_.find(stream);\n> +\tif (item != buffers_.end()) {\n> +\t\tgst_buffer_unref(item->second);\n> +\t\titem->second = buffer;\n> +\t} else {\n> +\t\tbuffers_[stream] = buffer;\n> +\t}\n> +}\n> +\n> +GstBuffer *RequestWrap::detachBuffer(Stream *stream)\n> +{\n> +\tGstBuffer *buffer = nullptr;\n> +\n> +\tauto item = buffers_.find(stream);\n> +\tif (item != buffers_.end()) {\n> +\t\tbuffer = item->second;\n> +\t\titem->second = nullptr;\n\nDo you really want to set second to nullptr here, or should you\n\n\t\tbuffers_.erase(item);\n\n?\n\n> +\t}\n> +\n> +\treturn buffer;\n> +}\n> +\n>  /* Used for C++ object with destructors. */\n>  struct GstLibcameraSrcState {\n> +\tGstLibcameraSrc *src;\n> +\n>  \tstd::unique_ptr<CameraManager> cm;\n>  \tstd::shared_ptr<Camera> cam;\n>  \tstd::unique_ptr<CameraConfiguration> config;\n>  \tstd::vector<GstPad *> srcpads;\n> +\tstd::queue<std::unique_ptr<RequestWrap>> requests;\n> +\n> +\tvoid requestCompleted(Request *request);\n>  };\n>  \n>  struct _GstLibcameraSrc {\n> @@ -45,6 +106,7 @@ struct _GstLibcameraSrc {\n>  \n>  \tGstLibcameraSrcState *state;\n>  \tGstLibcameraAllocator *allocator;\n> +\tGstFlowCombiner *flow_combiner;\n>  };\n>  \n>  enum {\n> @@ -68,6 +130,41 @@ GstStaticPadTemplate request_src_template = {\n>  \t\"src_%s\", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS\n>  };\n>  \n> +void\n> +GstLibcameraSrcState::requestCompleted(Request *request)\n> +{\n> +\tGLibLocker lock(GST_OBJECT(this->src));\n\nThis is a member function, so you can drop the \"this->\" and write \"src\".\nSame for everything below. You may want to suffix the data members with\n_ to avoid confusion though.\n\nDo we need to lock the whole function ? The requestComplete slot is\ncalled from the pipeline handler thread, so we should minimize the time\nwe spent here, especially with locks held. Are there any expensive\noperation below, is there a risk of delay due to lock contention ?\n\n> +\n> +\tGST_DEBUG_OBJECT(this->src, \"buffers are ready\");\n> +\n> +\tstd::unique_ptr<RequestWrap> wrap = std::move(this->requests.front());\n> +\tthis->requests.pop();\n> +\n> +\tg_return_if_fail(wrap->request_ == request);\n> +\n> +\tif ((request->status() == Request::RequestCancelled)) {\n> +\t\tGST_DEBUG_OBJECT(this->src, \"Request was cancelled\");\n> +\t\treturn;\n> +\t}\n> +\n> +\tGstBuffer *buffer;\n> +\tfor (GstPad *srcpad : this->srcpads) {\n> +\t\tStream *stream = gst_libcamera_pad_get_stream(srcpad);\n> +\t\tbuffer = wrap->detachBuffer(stream);\n> +\t\tgst_libcamera_pad_queue_buffer(srcpad, buffer);\n> +\t}\n> +\n> +\t{\n> +\t\t/* We only want to resume the task if it's paused. */\n> +\t\tGstTask *task = this->src->task;\n> +\t\tGLibLocker lock(GST_OBJECT(task));\n> +\t\tif (GST_TASK_STATE(task) == GST_TASK_PAUSED) {\n> +\t\t\tGST_TASK_STATE(task) = GST_TASK_STARTED;\n> +\t\t\tGST_TASK_SIGNAL(task);\n> +\t\t}\n> +\t}\n> +}\n> +\n>  static bool\n>  gst_libcamera_src_open(GstLibcameraSrc *self)\n>  {\n> @@ -120,6 +217,8 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n>  \t\treturn false;\n>  \t}\n>  \n> +\tcam->requestCompleted.connect(self->state, &GstLibcameraSrcState::requestCompleted);\n> +\n>  \t/* No need to lock here, we didn't start our threads yet. */\n>  \tself->state->cm = std::move(cm);\n>  \tself->state->cam = cam;\n> @@ -131,17 +230,85 @@ static void\n>  gst_libcamera_src_task_run(gpointer user_data)\n>  {\n>  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> +\tGstLibcameraSrcState *state = self->state;\n> +\n> +\tRequest *request = new Request(state->cam.get());\n\nYou should use cam->createRequest(). Looks like we forgot to make the\nRequest constructor private.\n\n> +\tauto wrap = std::make_unique<RequestWrap>(request);\n> +\tfor (GstPad *srcpad : state->srcpads) {\n> +\t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> +\t\tGstBuffer *buffer;\n> +\t\tGstFlowReturn ret;\n> +\n> +\t\tret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),\n> +\t\t\t\t\t\t     &buffer, nullptr);\n> +\t\tif (ret != GST_FLOW_OK) {\n> +\t\t\t/* RequestWrap does not take ownership, and we won't be\n> +\t\t\t * queueing this one due to lack of buffers. */\n\n\t\t\t/*\n\t\t\t * ...\n\t\t\t */\n\nand we should fix this in libcamera, the API makes request ownership\nawkward (not that the implementation is broken, it's just the API).\n\n> +\t\t\tdelete request;\n> +\t\t\trequest = NULL;\n\ns/NULL/nullptr/\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\twrap->attachBuffer(buffer);\n> +\t}\n> +\n> +\tif (request) {\n> +\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> +\t\tstate->cam->queueRequest(request);\n> +\t\tstate->requests.push(std::move(wrap));\n> +\t}\n> +\n> +\tGstFlowReturn ret = GST_FLOW_OK;\n> +\tgst_flow_combiner_reset(self->flow_combiner);\n> +\tfor (GstPad *srcpad : state->srcpads) {\n> +\t\tret = gst_libcamera_pad_push_pending(srcpad);\n> +\t\tret = gst_flow_combiner_update_pad_flow(self->flow_combiner,\n> +\t\t\t\t\t\t\tsrcpad, ret);\n\nWe have a single pad so that's no an issue yet, but this looks ignores\nerrors from any pad but the last.\n\n> +\t}\n>  \n> -\tGST_DEBUG_OBJECT(self, \"Streaming thread is now capturing\");\n> +\t{\n> +\t\t/*\n> +\t\t * Here we need to decide if we want to pause or stop the task. This\n> +\t\t * needs to happen in lock step with the callback thread which may want\n> +\t\t * to resume the task.\n> +\t\t */\n> +\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\tif (ret != GST_FLOW_OK) {\n> +\t\t\tif (ret == GST_FLOW_EOS) {\n> +\t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> +\t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> +\t\t\t\tgst_event_set_seqnum(eos, seqnum);\n> +\t\t\t\tfor (GstPad *srcpad : state->srcpads)\n> +\t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> +\t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> +\t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> +\t\t\t}\n> +\t\t\tgst_task_stop(self->task);\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tgboolean do_pause = true;\n\nI would replace this with a bool, or use TRUE here (and FALSE below).\n\n> +\t\tfor (GstPad *srcpad : state->srcpads) {\n> +\t\t\tif (gst_libcamera_pad_has_pending(srcpad)) {\n> +\t\t\t\tdo_pause = false;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (do_pause)\n> +\t\t\tgst_task_pause(self->task);\n> +\t}\n>  }\n>  \n>  static void\n>  gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n>  {\n>  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> -\tGLibRecLocker(&self->stream_lock);\n> +\tGLibRecLocker lock(&self->stream_lock);\n\nThis this fix a compilation error in a previous patch ?\n\n>  \tGstLibcameraSrcState *state = self->state;\n>  \tGstFlowReturn flow_ret = GST_FLOW_OK;\n> +\tgint ret = 0;\n\nI don't think you need to initialize this to 0.\n\n>  \n>  \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n>  \n> @@ -230,12 +397,23 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n>  \t\treturn;\n>  \t}\n>  \n> +\tself->flow_combiner = gst_flow_combiner_new();\n\nIf an error occurs above and flow_combiner isn't set there, won't the\ng_clear_pointer() call below operate on an unitialized variable ?\n\n>  \tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n>  \t\tGstPad *srcpad = state->srcpads[i];\n>  \t\tconst StreamConfiguration &stream_cfg = state->config->at(i);\n>  \t\tGstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,\n>  \t\t\t\t\t\t\t\tstream_cfg.stream());\n>  \t\tgst_libcamera_pad_set_pool(srcpad, pool);\n> +\t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> +\t}\n> +\n> +\tret = state->cam->start();\n> +\tif (ret) {\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t  (\"Failed to start the camera: %s\", g_strerror(-ret)),\n> +\t\t\t\t  (\"Camera.start() failed with error code %i\", ret));\n> +\t\tgst_task_stop(task);\n> +\t\treturn;\n>  \t}\n>  \n>  done:\n> @@ -257,10 +435,14 @@ gst_libcamera_src_task_leave(GstTask *task, GThread *thread, gpointer user_data)\n>  \n>  \tGST_DEBUG_OBJECT(self, \"Streaming thread is about to stop\");\n>  \n> +\tstate->cam->stop();\n> +\n>  \tfor (GstPad *srcpad : state->srcpads)\n>  \t\tgst_libcamera_pad_set_pool(srcpad, NULL);\n\ns/NULL/nullptr/\n\n>  \n>  \tg_clear_object(&self->allocator);\n> +\tg_clear_pointer(&self->flow_combiner,\n> +\t\t\t(GDestroyNotify)gst_flow_combiner_free);\n>  }\n>  \n>  static void\n> @@ -340,6 +522,9 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n>  \t\t\treturn GST_STATE_CHANGE_FAILURE;\n>  \t\tret = GST_STATE_CHANGE_NO_PREROLL;\n>  \t\tbreak;\n> +\tcase GST_STATE_CHANGE_PAUSED_TO_PLAYING:\n> +\t\tgst_task_start(self->task);\n> +\t\tbreak;\n>  \tcase GST_STATE_CHANGE_PLAYING_TO_PAUSED:\n>  \t\tret = GST_STATE_CHANGE_NO_PREROLL;\n>  \t\tbreak;\n> @@ -389,6 +574,9 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n>  \n>  \tstate->srcpads.push_back(gst_pad_new_from_template(templ, \"src\"));\n>  \tgst_element_add_pad(GST_ELEMENT(self), state->srcpads[0]);\n> +\n> +\t/* C-style friend. */\n> +\tstate->src = self;\n>  \tself->state = state;\n>  }\n>","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 8F5DA62689\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 29 Feb 2020 15:55:22 +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 05EC633E;\n\tSat, 29 Feb 2020 15:55:21 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582988122;\n\tbh=y+/ujP/BehzLcQImv8Mwv98q3nq1ixYb42k0YwBs6Mc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GscHEoHWHr7bPjjlGECLkTkIVJaRK7DFJlpH/xD2ucMOBq26tqura8eBYjVcAlYDG\n\t4VCT91r0Xza3ISDHNYcb3zzYRnABqU9GIgTGGefL6eVpHr6SfwE0/2l74E+zGuLZYe\n\tfmSMwmUAFI0gdZ0CKBNxAloGjIX4nSdnEkYSga0M=","Date":"Sat, 29 Feb 2020 16:54:58 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200229145458.GT18738@pendragon.ideasonboard.com>","References":"<20200227200407.490616-1-nicolas.dufresne@collabora.com>\n\t<20200227200407.490616-23-nicolas.dufresne@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200227200407.490616-23-nicolas.dufresne@collabora.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 22/27] gst: libcamerasrc: Implement\n\tinitial streaming","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":"Sat, 29 Feb 2020 14:55:22 -0000"}},{"id":3880,"web_url":"https://patchwork.libcamera.org/comment/3880/","msgid":"<6e9635a2c8f22c6f86e392dd0751161d3f2e1890.camel@collabora.com>","date":"2020-02-29T15:19:02","subject":"Re: [libcamera-devel] [PATCH v2 22/27] gst: libcamerasrc: Implement\n\tinitial streaming","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le samedi 29 février 2020 à 16:54 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 27, 2020 at 03:04:02PM -0500, Nicolas Dufresne wrote:\n> > With this patch, the element is now able to push buffers to the next\n> > element in the graph. The buffers are currently missing any metadata\n> > like timestamp, sequence number. This will be added in the next commit.\n> > \n> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > ---\n> >  src/gstreamer/gstlibcamerapad.cpp |  11 +-\n> >  src/gstreamer/gstlibcamerapad.h   |   2 +\n> >  src/gstreamer/gstlibcamerasrc.cpp | 192 +++++++++++++++++++++++++++++-\n> >  3 files changed, 202 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp\n> > index 8662c92..2cf1630 100644\n> > --- a/src/gstreamer/gstlibcamerapad.cpp\n> > +++ b/src/gstreamer/gstlibcamerapad.cpp\n> > @@ -18,6 +18,7 @@ struct _GstLibcameraPad {\n> >  \tStreamRole role;\n> >  \tGstLibcameraPool *pool;\n> >  \tGQueue pending_buffers;\n> > +\tGstClockTime latency;\n> >  };\n> >  \n> >  enum {\n> > @@ -159,7 +160,15 @@ gst_libcamera_pad_push_pending(GstPad *pad)\n> >  \t}\n> >  \n> >  \tif (!buffer)\n> > -\t\treturn GST_FLOW_CUSTOM_SUCCESS;\n> > +\t\treturn GST_FLOW_OK;\n> \n> Does this belong to the previous patch ?\n\nI forgot why it was custom success initially, it's needed to make\nstreaming work, I should probably try and squash this into previous\ncommits indeed.\n\n> \n> >  \n> >  \treturn gst_pad_push(pad, buffer);\n> >  }\n> > +\n> > +bool\n> > +gst_libcamera_pad_has_pending(GstPad *pad)\n> > +{\n> > +\tauto *self = GST_LIBCAMERA_PAD(pad);\n> > +\tGLibLocker lock(GST_OBJECT(self));\n> > +\treturn (self->pending_buffers.length > 0);\n> \n> No need for the outer parentheses.\n> \n> > +}\n> > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h\n> > index d928570..eb24000 100644\n> > --- a/src/gstreamer/gstlibcamerapad.h\n> > +++ b/src/gstreamer/gstlibcamerapad.h\n> > @@ -30,4 +30,6 @@ void gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer);\n> >  \n> >  GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);\n> >  \n> > +bool gst_libcamera_pad_has_pending(GstPad *pad);\n> > +\n> >  #endif /* __GST_LIBCAMERA_PAD_H__ */\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 83f93cc..70f2048 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -18,8 +18,10 @@\n> >  #include \"gstlibcamerapool.h\"\n> >  #include \"gstlibcamerasrc.h\"\n> >  \n> > +#include <gst/base/base.h>\n> >  #include <libcamera/camera.h>\n> >  #include <libcamera/camera_manager.h>\n> > +#include <queue>\n> >  #include <vector>\n> >  \n> >  using namespace libcamera;\n> > @@ -27,12 +29,71 @@ using namespace libcamera;\n> >  GST_DEBUG_CATEGORY_STATIC(source_debug);\n> >  #define GST_CAT_DEFAULT source_debug\n> >  \n> > +struct RequestWrap {\n> > +\tRequestWrap(Request *request);\n> > +\t~RequestWrap();\n> > +\n> > +\tvoid attachBuffer(GstBuffer *buffer);\n> > +\tGstBuffer *detachBuffer(Stream *stream);\n> > +\n> > +\t/* For ptr comparision only. */\n> \n> s/comparision/comparison/\n> \n> > +\tRequest *request_;\n> > +\tstd::map<Stream *, GstBuffer *> buffers_;\n> > +};\n> > +\n> > +RequestWrap::RequestWrap(Request *request)\n> > +\t: request_(request)\n> > +{\n> > +}\n> > +\n> > +RequestWrap::~RequestWrap()\n> > +{\n> > +\tfor (std::pair<Stream *const, GstBuffer *> &item : buffers_) {\n> > +\t\tif (item.second)\n> > +\t\t\tgst_buffer_unref(item.second);\n> > +\t}\n> > +}\n> > +\n> > +void RequestWrap::attachBuffer(GstBuffer *buffer)\n> > +{\n> > +\tFrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);\n> > +\tStream *stream = gst_libcamera_buffer_get_stream(buffer);\n> > +\n> > +\trequest_->addBuffer(stream, fb);\n> > +\n> > +\tauto item = buffers_.find(stream);\n> > +\tif (item != buffers_.end()) {\n> > +\t\tgst_buffer_unref(item->second);\n> > +\t\titem->second = buffer;\n> > +\t} else {\n> > +\t\tbuffers_[stream] = buffer;\n> > +\t}\n> > +}\n> > +\n> > +GstBuffer *RequestWrap::detachBuffer(Stream *stream)\n> > +{\n> > +\tGstBuffer *buffer = nullptr;\n> > +\n> > +\tauto item = buffers_.find(stream);\n> > +\tif (item != buffers_.end()) {\n> > +\t\tbuffer = item->second;\n> > +\t\titem->second = nullptr;\n> \n> Do you really want to set second to nullptr here, or should you\n> \n> \t\tbuffers_.erase(item);\n> \n> ?\n\nI'm just not familiar enough with the data structure to really answer,\nwhat does erase do ? This basically map a stream and a buffer, if the\nbuffer is out for delivery, isn't it logical to map it back to no\nbuffer rather then removing the mapping? Will erasing the mapping will\ncause an free/alloc ?\n> \n> > +\t}\n> > +\n> > +\treturn buffer;\n> > +}\n> > +\n> >  /* Used for C++ object with destructors. */\n> >  struct GstLibcameraSrcState {\n> > +\tGstLibcameraSrc *src;\n> > +\n> >  \tstd::unique_ptr<CameraManager> cm;\n> >  \tstd::shared_ptr<Camera> cam;\n> >  \tstd::unique_ptr<CameraConfiguration> config;\n> >  \tstd::vector<GstPad *> srcpads;\n> > +\tstd::queue<std::unique_ptr<RequestWrap>> requests;\n> > +\n> > +\tvoid requestCompleted(Request *request);\n> >  };\n> >  \n> >  struct _GstLibcameraSrc {\n> > @@ -45,6 +106,7 @@ struct _GstLibcameraSrc {\n> >  \n> >  \tGstLibcameraSrcState *state;\n> >  \tGstLibcameraAllocator *allocator;\n> > +\tGstFlowCombiner *flow_combiner;\n> >  };\n> >  \n> >  enum {\n> > @@ -68,6 +130,41 @@ GstStaticPadTemplate request_src_template = {\n> >  \t\"src_%s\", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS\n> >  };\n> >  \n> > +void\n> > +GstLibcameraSrcState::requestCompleted(Request *request)\n> > +{\n> > +\tGLibLocker lock(GST_OBJECT(this->src));\n> \n> This is a member function, so you can drop the \"this->\" and write \"src\".\n> Same for everything below. You may want to suffix the data members with\n> _ to avoid confusion though.\n\nThe _ style is probably what I dislike the most of libcamera coding\nstyle, so I was trying to get away with making Request a struct and\nusing c-style :-), but here I thought it was not obvious if I didn't\nuse this->. I can adhere to the style, I'll need to do that in all\ninternal C++ structs.\n\n> \n> Do we need to lock the whole function ? The requestComplete slot is\n> called from the pipeline handler thread, so we should minimize the time\n> we spent here, especially with locks held. Are there any expensive\n> operation below, is there a risk of delay due to lock contention ?\n> \n> > +\n> > +\tGST_DEBUG_OBJECT(this->src, \"buffers are ready\");\n> > +\n> > +\tstd::unique_ptr<RequestWrap> wrap = std::move(this->requests.front());\n> > +\tthis->requests.pop();\n\nMust be locked, fast.\n\n> > +\n> > +\tg_return_if_fail(wrap->request_ == request);\n> > +\n> > +\tif ((request->status() == Request::RequestCancelled)) {\n> > +\t\tGST_DEBUG_OBJECT(this->src, \"Request was cancelled\");\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tGstBuffer *buffer;\n> > +\tfor (GstPad *srcpad : this->srcpads) {\n> > +\t\tStream *stream = gst_libcamera_pad_get_stream(srcpad);\n> > +\t\tbuffer = wrap->detachBuffer(stream);\n> > +\t\tgst_libcamera_pad_queue_buffer(srcpad, buffer);\n> > +\t}\n\nMust be locked, also fast.\n\n> > +\n> > +\t{\n> > +\t\t/* We only want to resume the task if it's paused. */\n> > +\t\tGstTask *task = this->src->task;\n> > +\t\tGLibLocker lock(GST_OBJECT(task));\n> > +\t\tif (GST_TASK_STATE(task) == GST_TASK_PAUSED) {\n> > +\t\t\tGST_TASK_STATE(task) = GST_TASK_STARTED;\n> > +\t\t\tGST_TASK_SIGNAL(task);\n> > +\t\t}\n> > +\t}\n\nThis also really fast. So I think it's fine. Object locks should only\nbe taken in GStreamer for short operation (in fact, if you try to do\nsomething fancy with these held, you will have a deadlock). So I don't\nthink holding over the function is a concern. The entire thing here has\nbeen designed so this callback only queues the ready buffers in various\nplaces and wake up the pusher thread if needed. The pusher thread is\nthat one dealing with stream locks, which is by design contentious for\npush back purpose.\n\n> > +}\n> > +\n> >  static bool\n> >  gst_libcamera_src_open(GstLibcameraSrc *self)\n> >  {\n> > @@ -120,6 +217,8 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n> >  \t\treturn false;\n> >  \t}\n> >  \n> > +\tcam->requestCompleted.connect(self->state, &GstLibcameraSrcState::requestCompleted);\n> > +\n> >  \t/* No need to lock here, we didn't start our threads yet. */\n> >  \tself->state->cm = std::move(cm);\n> >  \tself->state->cam = cam;\n> > @@ -131,17 +230,85 @@ static void\n> >  gst_libcamera_src_task_run(gpointer user_data)\n> >  {\n> >  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > +\tGstLibcameraSrcState *state = self->state;\n> > +\n> > +\tRequest *request = new Request(state->cam.get());\n> \n> You should use cam->createRequest(). Looks like we forgot to make the\n> Request constructor private.\n> \n> > +\tauto wrap = std::make_unique<RequestWrap>(request);\n> > +\tfor (GstPad *srcpad : state->srcpads) {\n> > +\t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> > +\t\tGstBuffer *buffer;\n> > +\t\tGstFlowReturn ret;\n> > +\n> > +\t\tret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),\n> > +\t\t\t\t\t\t     &buffer, nullptr);\n> > +\t\tif (ret != GST_FLOW_OK) {\n> > +\t\t\t/* RequestWrap does not take ownership, and we won't be\n> > +\t\t\t * queueing this one due to lack of buffers. */\n> \n> \t\t\t/*\n> \t\t\t * ...\n> \t\t\t */\n> \n> and we should fix this in libcamera, the API makes request ownership\n> awkward (not that the implementation is broken, it's just the API).\n\nOk, will add a \\todo while at it.\n\n> \n> > +\t\t\tdelete request;\n> > +\t\t\trequest = NULL;\n> \n> s/NULL/nullptr/\n\nDamn, I was sure I managed to catch them all.\n\n> \n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\twrap->attachBuffer(buffer);\n> > +\t}\n> > +\n> > +\tif (request) {\n> > +\t\tGLibLocker lock(GST_OBJECT(self));\n> > +\t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> > +\t\tstate->cam->queueRequest(request);\n> > +\t\tstate->requests.push(std::move(wrap));\n> > +\t}\n> > +\n> > +\tGstFlowReturn ret = GST_FLOW_OK;\n> > +\tgst_flow_combiner_reset(self->flow_combiner);\n> > +\tfor (GstPad *srcpad : state->srcpads) {\n> > +\t\tret = gst_libcamera_pad_push_pending(srcpad);\n> > +\t\tret = gst_flow_combiner_update_pad_flow(self->flow_combiner,\n> > +\t\t\t\t\t\t\tsrcpad, ret);\n> \n> We have a single pad so that's no an issue yet, but this looks ignores\n> errors from any pad but the last.\n\nI've guessed you mean \"this loop\". The flow combiner has a state, and\nremember the last flow that was into it. See the last argument of\ngst_flow_combiner_update_pad_flow(), but maybe you see something that I\ndon't.\n\n> \n> > +\t}\n> >  \n> > -\tGST_DEBUG_OBJECT(self, \"Streaming thread is now capturing\");\n> > +\t{\n> > +\t\t/*\n> > +\t\t * Here we need to decide if we want to pause or stop the task. This\n> > +\t\t * needs to happen in lock step with the callback thread which may want\n> > +\t\t * to resume the task.\n> > +\t\t */\n> > +\t\tGLibLocker lock(GST_OBJECT(self));\n> > +\t\tif (ret != GST_FLOW_OK) {\n> > +\t\t\tif (ret == GST_FLOW_EOS) {\n> > +\t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > +\t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > +\t\t\t\tgst_event_set_seqnum(eos, seqnum);\n> > +\t\t\t\tfor (GstPad *srcpad : state->srcpads)\n> > +\t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > +\t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > +\t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > +\t\t\t}\n> > +\t\t\tgst_task_stop(self->task);\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +\t\tgboolean do_pause = true;\n> \n> I would replace this with a bool, or use TRUE here (and FALSE below).\n\nI'd use a bool indeed. Whenever I use TRUE/FALSE is when it's an\nexternal API using gboolean, cause gboolean is not the same size as\nbool.\n\n> \n> > +\t\tfor (GstPad *srcpad : state->srcpads) {\n> > +\t\t\tif (gst_libcamera_pad_has_pending(srcpad)) {\n> > +\t\t\t\tdo_pause = false;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (do_pause)\n> > +\t\t\tgst_task_pause(self->task);\n> > +\t}\n> >  }\n> >  \n> >  static void\n> >  gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> >  {\n> >  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > -\tGLibRecLocker(&self->stream_lock);\n> > +\tGLibRecLocker lock(&self->stream_lock);\n> \n> This this fix a compilation error in a previous patch ?\n\nThis one was badly squashed, I'll fix. Was a typo I made, which can\nhave disastrous effect, but surprisingly GCC didn't even warn about\nthis (calling a constructor, but not storing the object).\n\n> \n> >  \tGstLibcameraSrcState *state = self->state;\n> >  \tGstFlowReturn flow_ret = GST_FLOW_OK;\n> > +\tgint ret = 0;\n> \n> I don't think you need to initialize this to 0.\n> \n> >  \n> >  \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> >  \n> > @@ -230,12 +397,23 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> >  \t\treturn;\n> >  \t}\n> >  \n> > +\tself->flow_combiner = gst_flow_combiner_new();\n> \n> If an error occurs above and flow_combiner isn't set there, won't the\n> g_clear_pointer() call below operate on an unitialized variable ?\n\ng_clear_pointer() is made for GObject dispose() function, which can be\ncalled multiple time if there is circular references. Short story, it's\nnul-safe. As for GObject instance, they are always initialized with 0,\nthis is part of the API.\n\n> \n> >  \tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n> >  \t\tGstPad *srcpad = state->srcpads[i];\n> >  \t\tconst StreamConfiguration &stream_cfg = state->config->at(i);\n> >  \t\tGstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,\n> >  \t\t\t\t\t\t\t\tstream_cfg.stream());\n> >  \t\tgst_libcamera_pad_set_pool(srcpad, pool);\n> > +\t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > +\t}\n> > +\n> > +\tret = state->cam->start();\n> > +\tif (ret) {\n> > +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +\t\t\t\t  (\"Failed to start the camera: %s\", g_strerror(-ret)),\n> > +\t\t\t\t  (\"Camera.start() failed with error code %i\", ret));\n> > +\t\tgst_task_stop(task);\n> > +\t\treturn;\n> >  \t}\n> >  \n> >  done:\n> > @@ -257,10 +435,14 @@ gst_libcamera_src_task_leave(GstTask *task, GThread *thread, gpointer user_data)\n> >  \n> >  \tGST_DEBUG_OBJECT(self, \"Streaming thread is about to stop\");\n> >  \n> > +\tstate->cam->stop();\n> > +\n> >  \tfor (GstPad *srcpad : state->srcpads)\n> >  \t\tgst_libcamera_pad_set_pool(srcpad, NULL);\n> \n> s/NULL/nullptr/\n> \n> >  \n> >  \tg_clear_object(&self->allocator);\n> > +\tg_clear_pointer(&self->flow_combiner,\n> > +\t\t\t(GDestroyNotify)gst_flow_combiner_free);\n> >  }\n> >  \n> >  static void\n> > @@ -340,6 +522,9 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> >  \t\t\treturn GST_STATE_CHANGE_FAILURE;\n> >  \t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> >  \t\tbreak;\n> > +\tcase GST_STATE_CHANGE_PAUSED_TO_PLAYING:\n> > +\t\tgst_task_start(self->task);\n> > +\t\tbreak;\n> >  \tcase GST_STATE_CHANGE_PLAYING_TO_PAUSED:\n> >  \t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> >  \t\tbreak;\n> > @@ -389,6 +574,9 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> >  \n> >  \tstate->srcpads.push_back(gst_pad_new_from_template(templ, \"src\"));\n> >  \tgst_element_add_pad(GST_ELEMENT(self), state->srcpads[0]);\n> > +\n> > +\t/* C-style friend. */\n> > +\tstate->src = self;\n> >  \tself->state = state;\n> >  }\n> >","headers":{"Return-Path":"<nicolas.dufresne@collabora.com>","Received":["from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 42DB962689\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 29 Feb 2020 16:19:06 +0100 (CET)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id 631F3260D43"],"Message-ID":"<6e9635a2c8f22c6f86e392dd0751161d3f2e1890.camel@collabora.com>","From":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","Reply-To":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Date":"Sat, 29 Feb 2020 10:19:02 -0500","In-Reply-To":"<20200229145458.GT18738@pendragon.ideasonboard.com>","References":"<20200227200407.490616-1-nicolas.dufresne@collabora.com>\n\t<20200227200407.490616-23-nicolas.dufresne@collabora.com>\n\t<20200229145458.GT18738@pendragon.ideasonboard.com>","Organization":"Collabora","Content-Type":"multipart/signed; micalg=\"pgp-sha1\";\n\tprotocol=\"application/pgp-signature\"; \n\tboundary=\"=-D2VBs2XlbKx2Vj0DcHZx\"","User-Agent":"Evolution 3.34.4 (3.34.4-1.fc31) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v2 22/27] gst: libcamerasrc: Implement\n\tinitial streaming","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":"Sat, 29 Feb 2020 15:19:06 -0000"}},{"id":3890,"web_url":"https://patchwork.libcamera.org/comment/3890/","msgid":"<20200302105404.GK11960@pendragon.ideasonboard.com>","date":"2020-03-02T10:54:04","subject":"Re: [libcamera-devel] [PATCH v2 22/27] gst: libcamerasrc: Implement\n\tinitial streaming","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Sat, Feb 29, 2020 at 10:19:02AM -0500, Nicolas Dufresne wrote:\n> Le samedi 29 février 2020 à 16:54 +0200, Laurent Pinchart a écrit :\n> > On Thu, Feb 27, 2020 at 03:04:02PM -0500, Nicolas Dufresne wrote:\n> > > With this patch, the element is now able to push buffers to the next\n> > > element in the graph. The buffers are currently missing any metadata\n> > > like timestamp, sequence number. This will be added in the next commit.\n> > > \n> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n> > > ---\n> > >  src/gstreamer/gstlibcamerapad.cpp |  11 +-\n> > >  src/gstreamer/gstlibcamerapad.h   |   2 +\n> > >  src/gstreamer/gstlibcamerasrc.cpp | 192 +++++++++++++++++++++++++++++-\n> > >  3 files changed, 202 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamerapad.cpp b/src/gstreamer/gstlibcamerapad.cpp\n> > > index 8662c92..2cf1630 100644\n> > > --- a/src/gstreamer/gstlibcamerapad.cpp\n> > > +++ b/src/gstreamer/gstlibcamerapad.cpp\n> > > @@ -18,6 +18,7 @@ struct _GstLibcameraPad {\n> > >  \tStreamRole role;\n> > >  \tGstLibcameraPool *pool;\n> > >  \tGQueue pending_buffers;\n> > > +\tGstClockTime latency;\n> > >  };\n> > >  \n> > >  enum {\n> > > @@ -159,7 +160,15 @@ gst_libcamera_pad_push_pending(GstPad *pad)\n> > >  \t}\n> > >  \n> > >  \tif (!buffer)\n> > > -\t\treturn GST_FLOW_CUSTOM_SUCCESS;\n> > > +\t\treturn GST_FLOW_OK;\n> > \n> > Does this belong to the previous patch ?\n> \n> I forgot why it was custom success initially, it's needed to make\n> streaming work, I should probably try and squash this into previous\n> commits indeed.\n> \n> > >  \n> > >  \treturn gst_pad_push(pad, buffer);\n> > >  }\n> > > +\n> > > +bool\n> > > +gst_libcamera_pad_has_pending(GstPad *pad)\n> > > +{\n> > > +\tauto *self = GST_LIBCAMERA_PAD(pad);\n> > > +\tGLibLocker lock(GST_OBJECT(self));\n> > > +\treturn (self->pending_buffers.length > 0);\n> > \n> > No need for the outer parentheses.\n> > \n> > > +}\n> > > diff --git a/src/gstreamer/gstlibcamerapad.h b/src/gstreamer/gstlibcamerapad.h\n> > > index d928570..eb24000 100644\n> > > --- a/src/gstreamer/gstlibcamerapad.h\n> > > +++ b/src/gstreamer/gstlibcamerapad.h\n> > > @@ -30,4 +30,6 @@ void gst_libcamera_pad_queue_buffer(GstPad *pad, GstBuffer *buffer);\n> > >  \n> > >  GstFlowReturn gst_libcamera_pad_push_pending(GstPad *pad);\n> > >  \n> > > +bool gst_libcamera_pad_has_pending(GstPad *pad);\n> > > +\n> > >  #endif /* __GST_LIBCAMERA_PAD_H__ */\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 83f93cc..70f2048 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -18,8 +18,10 @@\n> > >  #include \"gstlibcamerapool.h\"\n> > >  #include \"gstlibcamerasrc.h\"\n> > >  \n> > > +#include <gst/base/base.h>\n> > >  #include <libcamera/camera.h>\n> > >  #include <libcamera/camera_manager.h>\n> > > +#include <queue>\n> > >  #include <vector>\n> > >  \n> > >  using namespace libcamera;\n> > > @@ -27,12 +29,71 @@ using namespace libcamera;\n> > >  GST_DEBUG_CATEGORY_STATIC(source_debug);\n> > >  #define GST_CAT_DEFAULT source_debug\n> > >  \n> > > +struct RequestWrap {\n> > > +\tRequestWrap(Request *request);\n> > > +\t~RequestWrap();\n> > > +\n> > > +\tvoid attachBuffer(GstBuffer *buffer);\n> > > +\tGstBuffer *detachBuffer(Stream *stream);\n> > > +\n> > > +\t/* For ptr comparision only. */\n> > \n> > s/comparision/comparison/\n> > \n> > > +\tRequest *request_;\n> > > +\tstd::map<Stream *, GstBuffer *> buffers_;\n> > > +};\n> > > +\n> > > +RequestWrap::RequestWrap(Request *request)\n> > > +\t: request_(request)\n> > > +{\n> > > +}\n> > > +\n> > > +RequestWrap::~RequestWrap()\n> > > +{\n> > > +\tfor (std::pair<Stream *const, GstBuffer *> &item : buffers_) {\n> > > +\t\tif (item.second)\n> > > +\t\t\tgst_buffer_unref(item.second);\n> > > +\t}\n> > > +}\n> > > +\n> > > +void RequestWrap::attachBuffer(GstBuffer *buffer)\n> > > +{\n> > > +\tFrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);\n> > > +\tStream *stream = gst_libcamera_buffer_get_stream(buffer);\n> > > +\n> > > +\trequest_->addBuffer(stream, fb);\n> > > +\n> > > +\tauto item = buffers_.find(stream);\n> > > +\tif (item != buffers_.end()) {\n> > > +\t\tgst_buffer_unref(item->second);\n> > > +\t\titem->second = buffer;\n> > > +\t} else {\n> > > +\t\tbuffers_[stream] = buffer;\n> > > +\t}\n> > > +}\n> > > +\n> > > +GstBuffer *RequestWrap::detachBuffer(Stream *stream)\n> > > +{\n> > > +\tGstBuffer *buffer = nullptr;\n> > > +\n> > > +\tauto item = buffers_.find(stream);\n> > > +\tif (item != buffers_.end()) {\n> > > +\t\tbuffer = item->second;\n> > > +\t\titem->second = nullptr;\n> > \n> > Do you really want to set second to nullptr here, or should you\n> > \n> > \t\tbuffers_.erase(item);\n> > \n> > ?\n> \n> I'm just not familiar enough with the data structure to really answer,\n> what does erase do ? This basically map a stream and a buffer, if the\n> buffer is out for delivery, isn't it logical to map it back to no\n> buffer rather then removing the mapping? Will erasing the mapping will\n> cause an free/alloc ?\n\nerase() will remove the element from the map completely. It will cause\nmemory to be freed, so setting it to nullptr as you do here probably\nmakes more sense to avoid constant reallocations. Please disregard my\ncomment.\n\n> > > +\t}\n> > > +\n> > > +\treturn buffer;\n> > > +}\n> > > +\n> > >  /* Used for C++ object with destructors. */\n> > >  struct GstLibcameraSrcState {\n> > > +\tGstLibcameraSrc *src;\n> > > +\n> > >  \tstd::unique_ptr<CameraManager> cm;\n> > >  \tstd::shared_ptr<Camera> cam;\n> > >  \tstd::unique_ptr<CameraConfiguration> config;\n> > >  \tstd::vector<GstPad *> srcpads;\n> > > +\tstd::queue<std::unique_ptr<RequestWrap>> requests;\n> > > +\n> > > +\tvoid requestCompleted(Request *request);\n> > >  };\n> > >  \n> > >  struct _GstLibcameraSrc {\n> > > @@ -45,6 +106,7 @@ struct _GstLibcameraSrc {\n> > >  \n> > >  \tGstLibcameraSrcState *state;\n> > >  \tGstLibcameraAllocator *allocator;\n> > > +\tGstFlowCombiner *flow_combiner;\n> > >  };\n> > >  \n> > >  enum {\n> > > @@ -68,6 +130,41 @@ GstStaticPadTemplate request_src_template = {\n> > >  \t\"src_%s\", GST_PAD_SRC, GST_PAD_REQUEST, TEMPLATE_CAPS\n> > >  };\n> > >  \n> > > +void\n> > > +GstLibcameraSrcState::requestCompleted(Request *request)\n> > > +{\n> > > +\tGLibLocker lock(GST_OBJECT(this->src));\n> > \n> > This is a member function, so you can drop the \"this->\" and write \"src\".\n> > Same for everything below. You may want to suffix the data members with\n> > _ to avoid confusion though.\n> \n> The _ style is probably what I dislike the most of libcamera coding\n> style, so I was trying to get away with making Request a struct and\n> using c-style :-), but here I thought it was not obvious if I didn't\n> use this->. I can adhere to the style, I'll need to do that in all\n> internal C++ structs.\n\nI don't like it much either, but being able to tell members from\nnon-members at a quick glance is nice. I dislike the 'm' prefix more,\nand haven't found a better option.\n\n> > Do we need to lock the whole function ? The requestComplete slot is\n> > called from the pipeline handler thread, so we should minimize the time\n> > we spent here, especially with locks held. Are there any expensive\n> > operation below, is there a risk of delay due to lock contention ?\n> > \n> > > +\n> > > +\tGST_DEBUG_OBJECT(this->src, \"buffers are ready\");\n> > > +\n> > > +\tstd::unique_ptr<RequestWrap> wrap = std::move(this->requests.front());\n> > > +\tthis->requests.pop();\n> \n> Must be locked, fast.\n> \n> > > +\n> > > +\tg_return_if_fail(wrap->request_ == request);\n> > > +\n> > > +\tif ((request->status() == Request::RequestCancelled)) {\n> > > +\t\tGST_DEBUG_OBJECT(this->src, \"Request was cancelled\");\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tGstBuffer *buffer;\n> > > +\tfor (GstPad *srcpad : this->srcpads) {\n> > > +\t\tStream *stream = gst_libcamera_pad_get_stream(srcpad);\n> > > +\t\tbuffer = wrap->detachBuffer(stream);\n> > > +\t\tgst_libcamera_pad_queue_buffer(srcpad, buffer);\n> > > +\t}\n> \n> Must be locked, also fast.\n> \n> > > +\n> > > +\t{\n> > > +\t\t/* We only want to resume the task if it's paused. */\n> > > +\t\tGstTask *task = this->src->task;\n> > > +\t\tGLibLocker lock(GST_OBJECT(task));\n> > > +\t\tif (GST_TASK_STATE(task) == GST_TASK_PAUSED) {\n> > > +\t\t\tGST_TASK_STATE(task) = GST_TASK_STARTED;\n> > > +\t\t\tGST_TASK_SIGNAL(task);\n> > > +\t\t}\n> > > +\t}\n> \n> This also really fast. So I think it's fine. Object locks should only\n> be taken in GStreamer for short operation (in fact, if you try to do\n> something fancy with these held, you will have a deadlock). So I don't\n> think holding over the function is a concern. The entire thing here has\n> been designed so this callback only queues the ready buffers in various\n> places and wake up the pusher thread if needed. The pusher thread is\n> that one dealing with stream locks, which is by design contentious for\n> push back purpose.\n> \n> > > +}\n> > > +\n> > >  static bool\n> > >  gst_libcamera_src_open(GstLibcameraSrc *self)\n> > >  {\n> > > @@ -120,6 +217,8 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n> > >  \t\treturn false;\n> > >  \t}\n> > >  \n> > > +\tcam->requestCompleted.connect(self->state, &GstLibcameraSrcState::requestCompleted);\n> > > +\n> > >  \t/* No need to lock here, we didn't start our threads yet. */\n> > >  \tself->state->cm = std::move(cm);\n> > >  \tself->state->cam = cam;\n> > > @@ -131,17 +230,85 @@ static void\n> > >  gst_libcamera_src_task_run(gpointer user_data)\n> > >  {\n> > >  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > > +\tGstLibcameraSrcState *state = self->state;\n> > > +\n> > > +\tRequest *request = new Request(state->cam.get());\n> > \n> > You should use cam->createRequest(). Looks like we forgot to make the\n> > Request constructor private.\n> > \n> > > +\tauto wrap = std::make_unique<RequestWrap>(request);\n> > > +\tfor (GstPad *srcpad : state->srcpads) {\n> > > +\t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> > > +\t\tGstBuffer *buffer;\n> > > +\t\tGstFlowReturn ret;\n> > > +\n> > > +\t\tret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),\n> > > +\t\t\t\t\t\t     &buffer, nullptr);\n> > > +\t\tif (ret != GST_FLOW_OK) {\n> > > +\t\t\t/* RequestWrap does not take ownership, and we won't be\n> > > +\t\t\t * queueing this one due to lack of buffers. */\n> > \n> > \t\t\t/*\n> > \t\t\t * ...\n> > \t\t\t */\n> > \n> > and we should fix this in libcamera, the API makes request ownership\n> > awkward (not that the implementation is broken, it's just the API).\n> \n> Ok, will add a \\todo while at it.\n> \n> > \n> > > +\t\t\tdelete request;\n> > > +\t\t\trequest = NULL;\n> > \n> > s/NULL/nullptr/\n> \n> Damn, I was sure I managed to catch them all.\n> \n> > \n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\n> > > +\t\twrap->attachBuffer(buffer);\n> > > +\t}\n> > > +\n> > > +\tif (request) {\n> > > +\t\tGLibLocker lock(GST_OBJECT(self));\n> > > +\t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> > > +\t\tstate->cam->queueRequest(request);\n> > > +\t\tstate->requests.push(std::move(wrap));\n> > > +\t}\n> > > +\n> > > +\tGstFlowReturn ret = GST_FLOW_OK;\n> > > +\tgst_flow_combiner_reset(self->flow_combiner);\n> > > +\tfor (GstPad *srcpad : state->srcpads) {\n> > > +\t\tret = gst_libcamera_pad_push_pending(srcpad);\n> > > +\t\tret = gst_flow_combiner_update_pad_flow(self->flow_combiner,\n> > > +\t\t\t\t\t\t\tsrcpad, ret);\n> > \n> > We have a single pad so that's no an issue yet, but this looks ignores\n> > errors from any pad but the last.\n> \n> I've guessed you mean \"this loop\". The flow combiner has a state, and\n> remember the last flow that was into it. See the last argument of\n> gst_flow_combiner_update_pad_flow(), but maybe you see something that I\n> don't.\n\nSo assigning ret in the gst_flow_combiner_update_pad_flow() line is for\nthe sole purpose of getting return value of the last iteration, to be\nused below ? That makes sense.\n\n> > > +\t}\n> > >  \n> > > -\tGST_DEBUG_OBJECT(self, \"Streaming thread is now capturing\");\n> > > +\t{\n> > > +\t\t/*\n> > > +\t\t * Here we need to decide if we want to pause or stop the task. This\n> > > +\t\t * needs to happen in lock step with the callback thread which may want\n> > > +\t\t * to resume the task.\n> > > +\t\t */\n> > > +\t\tGLibLocker lock(GST_OBJECT(self));\n> > > +\t\tif (ret != GST_FLOW_OK) {\n> > > +\t\t\tif (ret == GST_FLOW_EOS) {\n> > > +\t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > +\t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > +\t\t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > +\t\t\t\tfor (GstPad *srcpad : state->srcpads)\n> > > +\t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > +\t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > +\t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > +\t\t\t}\n> > > +\t\t\tgst_task_stop(self->task);\n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > > +\t\tgboolean do_pause = true;\n> > \n> > I would replace this with a bool, or use TRUE here (and FALSE below).\n> \n> I'd use a bool indeed. Whenever I use TRUE/FALSE is when it's an\n> external API using gboolean, cause gboolean is not the same size as\n> bool.\n> \n> > > +\t\tfor (GstPad *srcpad : state->srcpads) {\n> > > +\t\t\tif (gst_libcamera_pad_has_pending(srcpad)) {\n> > > +\t\t\t\tdo_pause = false;\n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\tif (do_pause)\n> > > +\t\t\tgst_task_pause(self->task);\n> > > +\t}\n> > >  }\n> > >  \n> > >  static void\n> > >  gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> > >  {\n> > >  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> > > -\tGLibRecLocker(&self->stream_lock);\n> > > +\tGLibRecLocker lock(&self->stream_lock);\n> > \n> > This this fix a compilation error in a previous patch ?\n> \n> This one was badly squashed, I'll fix. Was a typo I made, which can\n> have disastrous effect, but surprisingly GCC didn't even warn about\n> this (calling a constructor, but not storing the object).\n> \n> > >  \tGstLibcameraSrcState *state = self->state;\n> > >  \tGstFlowReturn flow_ret = GST_FLOW_OK;\n> > > +\tgint ret = 0;\n> > \n> > I don't think you need to initialize this to 0.\n> > \n> > >  \n> > >  \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > >  \n> > > @@ -230,12 +397,23 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)\n> > >  \t\treturn;\n> > >  \t}\n> > >  \n> > > +\tself->flow_combiner = gst_flow_combiner_new();\n> > \n> > If an error occurs above and flow_combiner isn't set there, won't the\n> > g_clear_pointer() call below operate on an unitialized variable ?\n> \n> g_clear_pointer() is made for GObject dispose() function, which can be\n> called multiple time if there is circular references. Short story, it's\n> nul-safe. As for GObject instance, they are always initialized with 0,\n> this is part of the API.\n\nSo self is guaranteed to be 0-initialized, and g_clear_pointer() is\nnull-safe. That's what I needed to know :-)\n\n> > >  \tfor (gsize i = 0; i < state->srcpads.size(); i++) {\n> > >  \t\tGstPad *srcpad = state->srcpads[i];\n> > >  \t\tconst StreamConfiguration &stream_cfg = state->config->at(i);\n> > >  \t\tGstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,\n> > >  \t\t\t\t\t\t\t\tstream_cfg.stream());\n> > >  \t\tgst_libcamera_pad_set_pool(srcpad, pool);\n> > > +\t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> > > +\t}\n> > > +\n> > > +\tret = state->cam->start();\n> > > +\tif (ret) {\n> > > +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > +\t\t\t\t  (\"Failed to start the camera: %s\", g_strerror(-ret)),\n> > > +\t\t\t\t  (\"Camera.start() failed with error code %i\", ret));\n> > > +\t\tgst_task_stop(task);\n> > > +\t\treturn;\n> > >  \t}\n> > >  \n> > >  done:\n> > > @@ -257,10 +435,14 @@ gst_libcamera_src_task_leave(GstTask *task, GThread *thread, gpointer user_data)\n> > >  \n> > >  \tGST_DEBUG_OBJECT(self, \"Streaming thread is about to stop\");\n> > >  \n> > > +\tstate->cam->stop();\n> > > +\n> > >  \tfor (GstPad *srcpad : state->srcpads)\n> > >  \t\tgst_libcamera_pad_set_pool(srcpad, NULL);\n> > \n> > s/NULL/nullptr/\n> > \n> > >  \n> > >  \tg_clear_object(&self->allocator);\n> > > +\tg_clear_pointer(&self->flow_combiner,\n> > > +\t\t\t(GDestroyNotify)gst_flow_combiner_free);\n> > >  }\n> > >  \n> > >  static void\n> > > @@ -340,6 +522,9 @@ gst_libcamera_src_change_state(GstElement *element, GstStateChange transition)\n> > >  \t\t\treturn GST_STATE_CHANGE_FAILURE;\n> > >  \t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> > >  \t\tbreak;\n> > > +\tcase GST_STATE_CHANGE_PAUSED_TO_PLAYING:\n> > > +\t\tgst_task_start(self->task);\n> > > +\t\tbreak;\n> > >  \tcase GST_STATE_CHANGE_PLAYING_TO_PAUSED:\n> > >  \t\tret = GST_STATE_CHANGE_NO_PREROLL;\n> > >  \t\tbreak;\n> > > @@ -389,6 +574,9 @@ gst_libcamera_src_init(GstLibcameraSrc *self)\n> > >  \n> > >  \tstate->srcpads.push_back(gst_pad_new_from_template(templ, \"src\"));\n> > >  \tgst_element_add_pad(GST_ELEMENT(self), state->srcpads[0]);\n> > > +\n> > > +\t/* C-style friend. */\n> > > +\tstate->src = self;\n> > >  \tself->state = state;\n> > >  }\n> > >","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 224D160427\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  2 Mar 2020 11:54:31 +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 05E9B9D0;\n\tMon,  2 Mar 2020 11:54:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1583146470;\n\tbh=nEGIRoYqz8kpzQxl0h3drBC+j5cSDQ+/GVQVaRlyZEo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gyX2Rv4CgjsBBRfsX7aY32nx25pkziOqZ1ADrsbZhCoxajX+m4seoVFjoGoePr5ix\n\tnqRQFSqURhJDRoV+yNPnpDtXE12TYpDlwOdWmEV4PMiUzieyCPOtug0Ufb87WBrjEr\n\tY8cz8S8ll999SnEA5MApO5iGrQ9vttW2LNrhdZEs=","Date":"Mon, 2 Mar 2020 12:54:04 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Nicolas Dufresne <nicolas.dufresne@collabora.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200302105404.GK11960@pendragon.ideasonboard.com>","References":"<20200227200407.490616-1-nicolas.dufresne@collabora.com>\n\t<20200227200407.490616-23-nicolas.dufresne@collabora.com>\n\t<20200229145458.GT18738@pendragon.ideasonboard.com>\n\t<6e9635a2c8f22c6f86e392dd0751161d3f2e1890.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<6e9635a2c8f22c6f86e392dd0751161d3f2e1890.camel@collabora.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 22/27] gst: libcamerasrc: Implement\n\tinitial streaming","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":"Mon, 02 Mar 2020 10:54:31 -0000"}}]