[{"id":28124,"web_url":"https://patchwork.libcamera.org/comment/28124/","msgid":"<00e6d96f1192d7ff417475cda232d1f26699b2e4.camel@ndufresne.ca>","date":"2023-11-22T15:08:03","subject":"Re: [libcamera-devel] [PATCH 1/2] gstreamer: Move negotiation logic\n\tto separate function","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi thanks for the patch.\n\nLe mercredi 22 novembre 2023 à 14:43 +0100, Jaslo Ziska via libcamera-devel a\nécrit :\n> Move the code which negotiates all the source pad caps into a separate\n> function called gst_libcamera_src_negotiate. When the negotiation fails\n> this function will return FALSE and TRUE otherwise.\n> \n> Use this function instead of doing the negotiation manually in\n> gst_libcamera_src_task_enter and remove the now redundant error handling\n> code accordingly.\n> \n> Signed-off-by: Jaslo Ziska <jaslo@ziska.de>\n> ---\n>  src/gstreamer/gstlibcamerasrc.cpp | 176 +++++++++++++++---------------\n>  1 file changed, 88 insertions(+), 88 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 767017db..e7a49fef 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -379,6 +379,87 @@ gst_libcamera_src_open(GstLibcameraSrc *self)\n>  \treturn true;\n>  }\n>  \n> +static gboolean\n> +gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n\nTry and document the locking in a comment above, this is very helpful for future\nwork.\n\nnit: Just a general rule we have followed, if the function is a GObject virtual\nfuntion, callback, etc. then we use gboolean, otherwise we always use the C++\nbool.\n\nWith these minor changes, the refactoring looks good.\n\nReviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>\n\n> +{\n> +\tGstLibcameraSrcState *state = self->state;\n> +\n> +\tg_autoptr(GstStructure) element_caps = gst_structure_new_empty(\"caps\");\n> +\n> +\tfor (gsize i = 0; i < state->srcpads_.size(); i++) {\n> +\t\tGstPad *srcpad = state->srcpads_[i];\n> +\t\tStreamConfiguration &stream_cfg = state->config_->at(i);\n> +\n> +\t\t/* Retrieve the supported caps. */\n> +\t\tg_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> +\t\tg_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);\n> +\t\tif (gst_caps_is_empty(caps))\n> +\t\t\treturn FALSE;\n> +\n> +\t\t/* Fixate caps and configure the stream. */\n> +\t\tcaps = gst_caps_make_writable(caps);\n> +\t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> +\t\tgst_libcamera_get_framerate_from_caps(caps, element_caps);\n> +\t}\n> +\n> +\t/* Validate the configuration. */\n> +\tif (state->config_->validate() == CameraConfiguration::Invalid)\n> +\t\treturn FALSE;\n> +\n> +\tint ret = state->cam_->configure(state->config_.get());\n> +\tif (ret) {\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t  (\"Failed to configure camera: %s\", g_strerror(-ret)),\n> +\t\t\t\t  (\"Camera::configure() failed with error code %i\", ret));\n> +\t\treturn FALSE;\n> +\t}\n> +\n> +\t/* Check frame duration bounds within controls::FrameDurationLimits */\n> +\tgst_libcamera_clamp_and_set_frameduration(state->initControls_,\n> +\t\t\t\t\t\t  state->cam_->controls(), element_caps);\n> +\n> +\t/*\n> +\t * Regardless if it has been modified, create clean caps and push the\n> +\t * caps event. Downstream will decide if the caps are acceptable.\n> +\t */\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> +\n> +\t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> +\t\tgst_libcamera_framerate_to_caps(caps, element_caps);\n> +\n> +\t\tif (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))\n> +\t\t\treturn FALSE;\n> +\n> +\t}\n> +\n> +\tif (self->allocator)\n> +\t\tg_clear_object(&self->allocator);\n> +\n> +\tself->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get());\n> +\tif (!self->allocator) {\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n> +\t\t\t\t  (\"Failed to allocate memory\"),\n> +\t\t\t\t  (\"gst_libcamera_allocator_new() failed.\"));\n> +\t\treturn FALSE;\n> +\t}\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> +\n> +\t\tGstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,\n> +\t\t\t\t\t\t\t\tstream_cfg.stream());\n> +\t\tg_signal_connect_swapped(pool, \"buffer-notify\",\n> +\t\t\t\t\t G_CALLBACK(gst_task_resume), self->task);\n> +\n> +\t\tgst_libcamera_pad_set_pool(srcpad, pool);\n> +\t}\n> +\n> +\treturn TRUE;\n> +}\n> +\n>  static void\n>  gst_libcamera_src_task_run(gpointer user_data)\n>  {\n> @@ -468,11 +549,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n>  \tGLibRecLocker lock(&self->stream_lock);\n>  \tGstLibcameraSrcState *state = self->state;\n> -\tGstFlowReturn flow_ret = GST_FLOW_OK;\n>  \tgint ret;\n>  \n> -\tg_autoptr(GstStructure) element_caps = gst_structure_new_empty(\"caps\");\n> -\n>  \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n>  \n>  \tgint stream_id_num = 0;\n> @@ -500,89 +578,22 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>  \t}\n>  \tg_assert(state->config_->size() == state->srcpads_.size());\n>  \n> -\tfor (gsize i = 0; i < state->srcpads_.size(); i++) {\n> -\t\tGstPad *srcpad = state->srcpads_[i];\n> -\t\tStreamConfiguration &stream_cfg = state->config_->at(i);\n> -\n> -\t\t/* Retrieve the supported caps. */\n> -\t\tg_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> -\t\tg_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);\n> -\t\tif (gst_caps_is_empty(caps)) {\n> -\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> -\t\t\tbreak;\n> -\t\t}\n> -\n> -\t\t/* Fixate caps and configure the stream. */\n> -\t\tcaps = gst_caps_make_writable(caps);\n> -\t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> -\t\tgst_libcamera_get_framerate_from_caps(caps, element_caps);\n> -\t}\n> -\n> -\tif (flow_ret != GST_FLOW_OK)\n> -\t\tgoto done;\n> -\n> -\t/* Validate the configuration. */\n> -\tif (state->config_->validate() == CameraConfiguration::Invalid) {\n> -\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> -\t\tgoto done;\n> -\t}\n> -\n> -\tret = state->cam_->configure(state->config_.get());\n> -\tif (ret) {\n> -\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> -\t\t\t\t  (\"Failed to configure camera: %s\", g_strerror(-ret)),\n> -\t\t\t\t  (\"Camera::configure() failed with error code %i\", ret));\n> +\tif (!gst_libcamera_src_negotiate(self)) {\n> +\t\tstate->initControls_.clear();\n> +\t\tGST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED);\n>  \t\tgst_task_stop(task);\n> -\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> -\t\tgoto done;\n> +\t\treturn;\n>  \t}\n>  \n> -\t/* Check frame duration bounds within controls::FrameDurationLimits */\n> -\tgst_libcamera_clamp_and_set_frameduration(state->initControls_,\n> -\t\t\t\t\t\t  state->cam_->controls(), element_caps);\n> -\n> -\t/*\n> -\t * Regardless if it has been modified, create clean caps and push the\n> -\t * caps event. Downstream will decide if the caps are acceptable.\n> -\t */\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> -\n> -\t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> -\t\tgst_libcamera_framerate_to_caps(caps, element_caps);\n> -\n> -\t\tif (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n> -\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> -\t\t\tbreak;\n> -\t\t}\n> +\tself->flow_combiner = gst_flow_combiner_new();\n> +\tfor (GstPad *srcpad : state->srcpads_) {\n> +\t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>  \n>  \t\t/* Send an open segment event with time format. */\n>  \t\tGstSegment segment;\n>  \t\tgst_segment_init(&segment, GST_FORMAT_TIME);\n>  \t\tgst_pad_push_event(srcpad, gst_event_new_segment(&segment));\n> -\t}\n> -\n> -\tself->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get());\n> -\tif (!self->allocator) {\n> -\t\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n> -\t\t\t\t  (\"Failed to allocate memory\"),\n> -\t\t\t\t  (\"gst_libcamera_allocator_new() failed.\"));\n> -\t\tgst_task_stop(task);\n> -\t\treturn;\n> -\t}\n> -\n> -\tself->flow_combiner = gst_flow_combiner_new();\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\tg_signal_connect_swapped(pool, \"buffer-notify\",\n> -\t\t\t\t\t G_CALLBACK(gst_task_resume), task);\n>  \n> -\t\tgst_libcamera_pad_set_pool(srcpad, pool);\n> -\t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>  \t}\n>  \n>  \tif (self->auto_focus_mode != controls::AfModeManual) {\n> @@ -605,17 +616,6 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>  \t\tgst_task_stop(task);\n>  \t\treturn;\n>  \t}\n> -\n> -done:\n> -\tstate->initControls_.clear();\n> -\tswitch (flow_ret) {\n> -\tcase GST_FLOW_NOT_NEGOTIATED:\n> -\t\tGST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> -\t\tgst_task_stop(task);\n> -\t\tbreak;\n> -\tdefault:\n> -\t\tbreak;\n> -\t}\n>  }\n>  \n>  static void","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5D111BDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 22 Nov 2023 15:08:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2D55629BC;\n\tWed, 22 Nov 2023 16:08:07 +0100 (CET)","from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com\n\t[IPv6:2607:f8b0:4864:20::72e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD90661DAD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Nov 2023 16:08:05 +0100 (CET)","by mail-qk1-x72e.google.com with SMTP id\n\taf79cd13be357-77d645c0e06so49239785a.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Nov 2023 07:08:05 -0800 (PST)","from nicolas-tpx395.localdomain ([2606:6d00:10:3ac8::7a9])\n\tby smtp.gmail.com with ESMTPSA id\n\tc20-20020a05620a165400b0077589913a8bsm4473309qko.132.2023.11.22.07.08.04\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 22 Nov 2023 07:08:04 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1700665687;\n\tbh=ICPujijmue2urGGii03IZ5I2vIpTRq/PW9zunm1EgI0=;\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:\n\tFrom;\n\tb=MSnDCJ7c9Ee0mbfE8UpbjEMBthWNfQqRUgPbMduTBUMra9l2ViOXxth065sGzEb39\n\tAWG2l5AN2XFEkxOW33wppM9laKm3tTfT2m1Rr5s3P0aRwIiLI2nvBqUnkqYDxEX3fV\n\tS3pB/OT8Uf3mQf8p+qM3lEu9AVaMdnzuaytoHMwvlOY+4KviS11yC0Wmj0coOA5AOC\n\ti7Y3dzGr5J4lyTUfFLzEiic0KQIa8nQzUrS5hSDP7dHa8cTt2PZOpCLWmL7G+iEe7p\n\tTzPm0dLeMervGBYYTCTYb0Z4PV+bauNNFdF4V5IAPjNAioF6lotBie8JXTTBRUiSUZ\n\tSHNs5Cy8MFTuw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20230601.gappssmtp.com; s=20230601; t=1700665684;\n\tx=1701270484; darn=lists.libcamera.org; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Cl+NlFeAcxV4J3VPHAl9vJGo1yJxZRrdetaq1nGQ6ew=;\n\tb=P3NFcYHNs6FxKhet4gWllmGAgrTrx+93hstfeEdMuPe4BryIvtYOnvMWA2prnXFLY/\n\taag88zT/tdteFbQjE90k0hKkQf0ffKl0nEgwKQWEh1WK24qM3Wb1XnIZ9q0ShXnX6oRV\n\tK+Rn0FNIkYO1AQj9yUJ5eTWrzbJjsqNjEARsV7tQTTtC71Vh5Fq9DvMP6DrE/hD892BW\n\tVWM19DtKK5JRRjtIL1aCMqitfgAcFqSC2EUywzaoOi3oCL7k9LaPgXV/e+PtZBvPFI0t\n\tjZswEu1K37btdNGYfRswbwtlaAFEcDfaDBYc0v6vldOc9ey/HXV5KWFo5T/EAVYDumIl\n\tgKZQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20230601.gappssmtp.com\n\theader.i=@ndufresne-ca.20230601.gappssmtp.com header.b=\"P3NFcYHN\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1700665684; x=1701270484;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=Cl+NlFeAcxV4J3VPHAl9vJGo1yJxZRrdetaq1nGQ6ew=;\n\tb=VbSjNHQjO5+7XhLanCV1yMlBoK7HmJJhmh4SpL446jOd4KMNptgtS2Rpst1FXoclgY\n\tUVNXszcjuppdCtwCJ4p28MOauxq8vB8Ao7MVrhcPmrcmI8qvANPPO+OT0kAyHc6Yd8RF\n\t7qIW9U4Br+UkgnpJbhxNxC4e9N7P9pMBj8YsnLFIxcS5K9l+vGu5caDA7CFknwVIFEKm\n\t8jp1BoMMVyVE6C8D/bcnL0UE6+HBFmW3Dh4b4avmZ28v4MjHWYfLqBv5Xb2eYKac9xVR\n\t09it6mJ/USdu13v06RJRWOEQvxSZPjm17IR8eyMynmKmKByS8tYW1b38DSZSx+/K3hHs\n\t5gkg==","X-Gm-Message-State":"AOJu0YyL7bJD0jrKmtgOzL1+niko/PKnn3Z0tIAnKA0DbDvB5/6V2Reb\n\tGsMaVlVGqnCtbr1YT4NONTd1JVsxhUTfwC7us0E=","X-Google-Smtp-Source":"AGHT+IGKN53zOv1ND9mxRCCjuZTlRlA8MzXdC0fSnG7KlLL6xkmz2fiOq+j9y1I3XmuG1bMIXDTKvQ==","X-Received":"by 2002:a05:620a:40c2:b0:77d:518c:79f2 with SMTP id\n\tg2-20020a05620a40c200b0077d518c79f2mr2575552qko.47.1700665684470; \n\tWed, 22 Nov 2023 07:08:04 -0800 (PST)","Message-ID":"<00e6d96f1192d7ff417475cda232d1f26699b2e4.camel@ndufresne.ca>","To":"Jaslo Ziska <jaslo@ziska.de>, libcamera-devel@lists.libcamera.org","Date":"Wed, 22 Nov 2023 10:08:03 -0500","In-Reply-To":"<20231122135406.14921-2-jaslo@ziska.de>","References":"<20231122135406.14921-1-jaslo@ziska.de>\n\t<20231122135406.14921-2-jaslo@ziska.de>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.48.4 (3.48.4-1.fc38) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH 1/2] gstreamer: Move negotiation logic\n\tto separate function","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>","From":"Nicolas Dufresne via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Nicolas Dufresne <nicolas@ndufresne.ca>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]