[{"id":23648,"web_url":"https://patchwork.libcamera.org/comment/23648/","msgid":"","date":"2022-06-28T13:29:07","subject":"Re: [libcamera-devel] [PATCH 12/13] gstreamer: Split completed\n\trequest processing to a separate function","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi Laurent,\n\nLe vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit :\n> Simplify the task run function futher by moving the processing of\n> completed requests to a separate function. No functional change\n> intended, only increased readability.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 125 +++++++++++++++++-------------\n> 1 file changed, 73 insertions(+), 52 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index fb39d6093a3f..3feb87254916 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -125,6 +125,7 @@ struct GstLibcameraSrcState {\n> \n> \tint queueRequest();\n> \tvoid requestCompleted(Request *request);\n> +\tint processRequest();\n> };\n> \n> struct _GstLibcameraSrc {\n> @@ -241,6 +242,63 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> \tgst_task_resume(src_->task);\n> }\n> \n> +int GstLibcameraSrcState::processRequest()\n\nPlease comment that this must be called with steam lock held (which _run() holds\nautomatically).\n\n> +{\n> +\tstd::unique_ptr wrap;\n> +\n> +\t{\n> +\t\tMutexLocker locker(lock_);\n> +\n> +\t\tif (!completedRequests_.empty()) {\n> +\t\t\twrap = std::move(completedRequests_.front());\n> +\t\t\tcompletedRequests_.pop();\n> +\t\t}\n> +\t}\n> +\n> +\tif (!wrap)\n> +\t\treturn -ENODATA;\n> +\n> +\tGstFlowReturn ret = GST_FLOW_OK;\n> +\tgst_flow_combiner_reset(src_->flow_combiner);\n> +\n> +\tfor (GstPad *srcpad : srcpads_) {\n> +\t\tStream *stream = gst_libcamera_pad_get_stream(srcpad);\n> +\t\tGstBuffer *buffer = wrap->detachBuffer(stream);\n> +\n> +\t\tFrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);\n> +\n> +\t\tif (wrap->pts_) {\n> +\t\t\tGST_BUFFER_PTS(buffer) = wrap->pts_;\n> +\t\t\tgst_libcamera_pad_set_latency(srcpad, wrap->latency_);\n> +\t\t} else {\n> +\t\t\tGST_BUFFER_PTS(buffer) = 0;\n> +\t\t}\n> +\n> +\t\tGST_BUFFER_OFFSET(buffer) = fb->metadata().sequence;\n> +\t\tGST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence;\n> +\n> +\t\tret = gst_pad_push(srcpad, buffer);\n> +\t\tret = gst_flow_combiner_update_pad_flow(src_->flow_combiner,\n> +\t\t\t\t\t\t\tsrcpad, ret);\n> +\t}\n> +\n> +\tif (ret != GST_FLOW_OK) {\n> +\t\tif (ret == GST_FLOW_EOS) {\n> +\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> +\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> +\t\t\tgst_event_set_seqnum(eos, seqnum);\n> +\t\t\tfor (GstPad *srcpad : srcpads_)\n> +\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> +\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> +\t\t\tGST_ELEMENT_FLOW_ERROR(src_, ret);\n> +\t\t}\n> +\n> +\t\treturn -EPIPE;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> static bool\n> gst_libcamera_src_open(GstLibcameraSrc *self)\n> {\n> @@ -308,8 +366,13 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> \tGstLibcameraSrcState *state = self->state;\n> \n> -\tint err = state->queueRequest();\n> -\tif (err == -ENOMEM) {\n> +\t/*\n> +\t * Create and queue one request. If no buffers are available the\n> +\t * function returns -ENOBUFS, which we ignore here as that's not a\n> +\t * fatal error.\n> +\t */\n> +\tint ret = state->queueRequest();\n> +\tif (ret == -ENOMEM) {\n> \t\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n> \t\t\t\t (\"Failed to allocate request for camera '%s'.\",\n> \t\t\t\t state->cam_->id().c_str()),\n> @@ -318,58 +381,16 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \t\treturn;\n> \t}\n> \n> -\tstd::unique_ptr wrap;\n> -\n> -\t{\n> -\t\tMutexLocker locker(state->lock_);\n> -\n> -\t\tif (!state->completedRequests_.empty()) {\n> -\t\t\twrap = std::move(state->completedRequests_.front());\n> -\t\t\tstate->completedRequests_.pop();\n> -\t\t}\n> -\t}\n> -\n> -\tif (!wrap) {\n> -\t\tgst_task_pause(self->task);\n> -\t\treturn;\n> -\t}\n> -\n> -\tGstFlowReturn ret = GST_FLOW_OK;\n> -\tgst_flow_combiner_reset(self->flow_combiner);\n> -\n> -\tfor (GstPad *srcpad : state->srcpads_) {\n> -\t\tStream *stream = gst_libcamera_pad_get_stream(srcpad);\n> -\t\tGstBuffer *buffer = wrap->detachBuffer(stream);\n> -\n> -\t\tFrameBuffer *fb = gst_libcamera_buffer_get_frame_buffer(buffer);\n> -\n> -\t\tif (wrap->pts_) {\n> -\t\t\tGST_BUFFER_PTS(buffer) = wrap->pts_;\n> -\t\t\tgst_libcamera_pad_set_latency(srcpad, wrap->latency_);\n> -\t\t} else {\n> -\t\t\tGST_BUFFER_PTS(buffer) = 0;\n> -\t\t}\n> -\n> -\t\tGST_BUFFER_OFFSET(buffer) = fb->metadata().sequence;\n> -\t\tGST_BUFFER_OFFSET_END(buffer) = fb->metadata().sequence;\n> -\n> -\t\tret = gst_pad_push(srcpad, buffer);\n> -\t\tret = gst_flow_combiner_update_pad_flow(self->flow_combiner,\n> -\t\t\t\t\t\t\tsrcpad, ret);\n> -\t}\n> -\n> -\tif (ret != GST_FLOW_OK) {\n> -\t\tif (ret == GST_FLOW_EOS) {\n> -\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> -\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> -\t\t\tgst_event_set_seqnum(eos, seqnum);\n> -\t\t\tfor (GstPad *srcpad : state->srcpads_)\n> -\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> -\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> -\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> -\t\t}\n> +\t/* Process one completed request, if available. */\n> +\tret = state->processRequest();\n> +\tswitch (ret) {\n> +\tcase -EPIPE:\n> \t\tgst_task_stop(self->task);\n> \t\treturn;\n> +\n> +\tcase -ENODATA:\n> +\t\tgst_task_pause(self->task);\n> +\t\treturn;\n> \t}\n\nLooks good to me.\n\nReviewed-by: Nicolas Dufresne \n\n> \n> \t/*","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 68A57BE173\n\tfor ;\n\tTue, 28 Jun 2022 13:29:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7ECC65635;\n\tTue, 28 Jun 2022 15:29:17 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D49C46059D\n\tfor ;\n\tTue, 28 Jun 2022 15:29:16 +0200 (CEST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net\n\t[192.222.136.102])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits))\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 13D8366015C5;\n\tTue, 28 Jun 2022 14:29:15 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656422957;\n\tbh=yCIKEIDuBz2c9NtzB4dIAgPui2pmz2ql4kjf4QLD4Sk=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=JXxZCILEb3uZ4tVhC5d0nbayHah3YR4qXoPhwsiRDZ9Tx4YL8T1AySV5nBv0wJrn9\n\tZ5c74ICZVmMw66v6hJTfC/i5aMvbqlfUqdjJtFbO6aVfCL+6U2eoK8J37pJf2IcBRU\n\tZnAaQbfuDmWVhQhtfKO1LOImHLe2sYu2BV+suBTNPPHIpzfH3qfOSnsP2+y0St3/fl\n\txWrgBlWaN3KIhE9GrBcozG6rWoh35xl/XOrPu9RcyzS/qZRc6DsfE95HLYrplILLrN\n\t94dy2snw28oH8jM+GMDMtSiJoRbMK1pwTldTaRKyVbDK1z9sdYTLiCCbDYuOyC58Wn\n\ttA3uRTRnTfurg==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1656422956;\n\tbh=yCIKEIDuBz2c9NtzB4dIAgPui2pmz2ql4kjf4QLD4Sk=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=Dz/oHPYE6k9R3RyFB0dKKCutI0JPuxnwniY8HxcCW12l6z2ixT8FMagNRlWAgIdnc\n\taqIa9mcQViORcYfRhGm+q7Q9OLPoYFemmOFz9FsFTJUwYZKvGoZP+Jux58wIYZ7tKn\n\tT8Zcxk73vYYzRF7uO5t0wfEKESQEpB+KaEAvCO1yIMYJ4tQB6ZusTQeRL+6qNj2jmT\n\tsCqExlfLaZ4LDp4ldYYe7sTIBC2L9NcbAgwzo+RQpxAAS9d4sXj47cmlSVzL7lQP45\n\txcZIUXyIXIwyEWERFix+XwUa0OsJNxbdrHQWy91AGSHfwiViBSW9pKLDL6qqQHtpjA\n\tTcrk5XtZlo4pQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"Dz/oHPYE\"; dkim-atps=neutral","Message-ID":"","To":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 28 Jun 2022 09:29:07 -0400","In-Reply-To":"<20220623232210.18742-13-laurent.pinchart@ideasonboard.com>","References":"<20220623232210.18742-1-laurent.pinchart@ideasonboard.com>\n\t<20220623232210.18742-13-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.2 (3.44.2-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH 12/13] gstreamer: Split completed\n\trequest processing to a separate function","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"Vedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]