[{"id":27075,"web_url":"https://patchwork.libcamera.org/comment/27075/","msgid":"<5994b9df-32e2-f9d3-b6d8-8bb55aca8708@ideasonboard.com>","date":"2023-05-09T05:24:27","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] libcamera: Remove\n\t`StreamRoles` alias","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn 5/3/23 6:17 PM, Barnabás Pőcze via libcamera-devel wrote:\n> Now that `Camera::generateConfiguration()` takes a `libcamera::Span`\n> of `StreamRole`, remove the `StreamRoles` type, which was an alias\n> to `std::vector`.\n>\n> The removal has two reasons:\n> - it is no longer strictly necessary,\n> - its presence may suggest that that is the preferred (or correct)\n> way to build/pass a list of `StreamRole`.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/stream.h | 2 --\n> src/apps/cam/camera_session.cpp | 2 +-\n> src/apps/common/stream_options.cpp | 4 ++--\n> src/apps/common/stream_options.h | 2 +-\n> src/apps/qcam/main_window.cpp | 2 +-\n> src/gstreamer/gstlibcameraprovider.cpp | 5 ++---\n> src/gstreamer/gstlibcamerasrc.cpp | 2 +-\n> src/libcamera/stream.cpp | 5 -----\n> 8 files changed, 8 insertions(+), 16 deletions(-)\n>\n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 29235ddf..4e94187d 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -69,8 +69,6 @@ enum class StreamRole {\n> \tViewfinder,\n> };\n>\n> -using StreamRoles = std::vector;\n> -\n> std::ostream &operator<<(std::ostream &out, StreamRole role);\n>\n> class Stream\n> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> index 8fcec630..ecca2a97 100644\n> --- a/src/apps/cam/camera_session.cpp\n> +++ b/src/apps/cam/camera_session.cpp\n> @@ -55,7 +55,7 @@ CameraSession::CameraSession(CameraManager *cm,\n> \t\treturn;\n> \t}\n>\n> -\tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> +\tauto roles = StreamKeyValueParser::roles(options_[OptStream]);\n>\n> \tstd::unique_ptr config =\n> \t\tcamera_->generateConfiguration(roles);\n> diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp\n> index 19dfe051..663b979a 100644\n> --- a/src/apps/common/stream_options.cpp\n> +++ b/src/apps/common/stream_options.cpp\n> @@ -40,7 +40,7 @@ KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)\n> \treturn options;\n> }\n>\n> -StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n> +std::vector StreamKeyValueParser::roles(const OptionValue &values)\n> {\n> \t/* If no configuration values to examine default to viewfinder. */\n> \tif (values.empty())\n> @@ -48,7 +48,7 @@ StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n>\n> \tconst std::vector &streamParameters = values.toArray();\n>\n> -\tStreamRoles roles;\n> +\tstd::vector roles;\n> \tfor (auto const &value : streamParameters) {\n> \t\t/* If a role is invalid default it to viewfinder. */\n> \t\troles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));\n> diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h\n> index fe298c84..a5f3bde0 100644\n> --- a/src/apps/common/stream_options.h\n> +++ b/src/apps/common/stream_options.h\n> @@ -20,7 +20,7 @@ public:\n>\n> \tKeyValueParser::Options parse(const char *arguments) override;\n>\n> -\tstatic libcamera::StreamRoles roles(const OptionValue &values);\n> +\tstatic std::vector roles(const OptionValue &values);\n> \tstatic int updateConfiguration(libcamera::CameraConfiguration *config,\n> \t\t\t\t const OptionValue &values);\n>\n> diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp\n> index 680668df..0b123d40 100644\n> --- a/src/apps/qcam/main_window.cpp\n> +++ b/src/apps/qcam/main_window.cpp\n> @@ -362,7 +362,7 @@ void MainWindow::toggleCapture(bool start)\n> */\n> int MainWindow::startCapture()\n> {\n> -\tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> +\tauto roles = StreamKeyValueParser::roles(options_[OptStream]);\n> \tint ret;\n>\n> \t/* Verify roles are supported. */\n> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> index 6eb0a0eb..332837e4 100644\n> --- a/src/gstreamer/gstlibcameraprovider.cpp\n> +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> @@ -126,13 +126,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> static GstDevice *\n> gst_libcamera_device_new(const std::shared_ptr &camera)\n> {\n> +\tstatic const StreamRole roles[] = { StreamRole::VideoRecording };\n\nAny particular reasons for static const here?\n> \tg_autoptr(GstCaps) caps = gst_caps_new_empty();\n> \tconst gchar *name = camera->id().c_str();\n> -\tStreamRoles roles;\n>\n> -\troles.push_back(StreamRole::VideoRecording);\n\nI would like to keep roles as a std::vector<> to be honest. Primary \nreason being, based on \"conditional camera capabilities\" that we might \nknow of before-hand, more roles can be requested from camera (instead of \nonly VideoRecording) at the same time and adding a role to std::vector<> \nseems quite easy in that case. What do you think?\n> \tstd::unique_ptr config = camera->generateConfiguration(roles);\n> -\tif (!config || config->size() != roles.size()) {\n> +\tif (!config || config->size() != std::size(roles)) {\n> \t\tGST_ERROR(\"Failed to generate a default configuration for %s\", name);\n> \t\treturn nullptr;\n> \t}\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index a10cbd4f..46a5400e 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -468,7 +468,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n>\n> \tgint stream_id_num = 0;\n> -\tStreamRoles roles;\n> +\tstd::vector roles;\n> \tfor (GstPad *srcpad : state->srcpads_) {\n> \t\t/* Create stream-id and push stream-start. */\n> \t\tg_autofree gchar *stream_id_intermediate = g_strdup_printf(\"%i%i\", state->group_id_, stream_id_num++);\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 67f30815..272222b7 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -436,11 +436,6 @@ std::ostream &operator<<(std::ostream &out, StreamRole role)\n> \treturn out;\n> }\n>\n> -/**\n> - * \\typedef StreamRoles\n> - * \\brief A vector of StreamRole\n> - */\n> -\n> /**\n> * \\class Stream\n> * \\brief Video stream for a camera\n> --\n> 2.40.1\n>\n>","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 4FE27BD16B\n\tfor ;\n\tTue, 9 May 2023 05:24:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD004627DC;\n\tTue, 9 May 2023 07:24:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C43F60493\n\tfor ;\n\tTue, 9 May 2023 07:24:32 +0200 (CEST)","from [192.168.1.105] (unknown [103.251.226.10])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9244B7CE;\n\tTue, 9 May 2023 07:24:25 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683609874;\n\tbh=+g8HE9irSr7/+PTYYfQPm/UOhbDcNdl7nkT294g42m4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=cACdXxYHUG/T2dao27yeVhUANHj2vyG4IZWRYLxuOcSWdDJdMBBeK9frk8Ho/vD0u\n\t3vZcuhONfDtqFpIt/kOGFIOjSN8Y5EHzV7YHqVXkgMngP4cUhRvadIx04Jehe0xg9K\n\t6NhW7t1gEo6Y+zyBzaIJCV5JE31ee8pRjJntfMVGYOVwQCA2ngZwsDpLzLV+4lbwF5\n\tgrmL7Nh14Q985KkxVbVLM3vsTwZao8Mp3MYtOsEtgvTl19WqUUBvYw5qjRef6ooU0c\n\tDanCoa9uuGkq9gnxh8BBI7GDb5dhwRo3lSYDLCLHsp6P0/tKqJvTtFGiVji1HY75gb\n\tD3vYxQ+DkOzng==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1683609866;\n\tbh=+g8HE9irSr7/+PTYYfQPm/UOhbDcNdl7nkT294g42m4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Drt8SggsWkRhPc8ladTJQ1RwVPALVIMJAeT6PQp+U2XXiL/NegSJXrfxwLoA6aEQJ\n\tLIwZmyCYGrStufctpkwLPxf8bwhc4FZKl+bwdZkkEI4CqO3saPx11LsLYnSsMvNKId\n\tqIHgeDqAcEFtf9eJecPT80vwUl1kYobWi1IKeigw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Drt8Sggs\"; dkim-atps=neutral","Message-ID":"<5994b9df-32e2-f9d3-b6d8-8bb55aca8708@ideasonboard.com>","Date":"Tue, 9 May 2023 10:54:27 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20230503124725.200550-1-pobrn@protonmail.com>\n\t<20230503124725.200550-2-pobrn@protonmail.com>","In-Reply-To":"<20230503124725.200550-2-pobrn@protonmail.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] libcamera: Remove\n\t`StreamRoles` alias","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27077,"web_url":"https://patchwork.libcamera.org/comment/27077/","msgid":"<20230509152328.GD10510@pendragon.ideasonboard.com>","date":"2023-05-09T15:23:28","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] libcamera: Remove\n\t`StreamRoles` alias","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, May 09, 2023 at 10:54:27AM +0530, Umang Jain via libcamera-devel wrote:\n> On 5/3/23 6:17 PM, Barnabás Pőcze via libcamera-devel wrote:\n> > Now that `Camera::generateConfiguration()` takes a `libcamera::Span`\n> > of `StreamRole`, remove the `StreamRoles` type, which was an alias\n> > to `std::vector`.\n> >\n> > The removal has two reasons:\n> > - it is no longer strictly necessary,\n> > - its presence may suggest that that is the preferred (or correct)\n> > way to build/pass a list of `StreamRole`.\n> >\n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > include/libcamera/stream.h | 2 --\n> > src/apps/cam/camera_session.cpp | 2 +-\n> > src/apps/common/stream_options.cpp | 4 ++--\n> > src/apps/common/stream_options.h | 2 +-\n> > src/apps/qcam/main_window.cpp | 2 +-\n> > src/gstreamer/gstlibcameraprovider.cpp | 5 ++---\n> > src/gstreamer/gstlibcamerasrc.cpp | 2 +-\n> > src/libcamera/stream.cpp | 5 -----\n> > 8 files changed, 8 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index 29235ddf..4e94187d 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -69,8 +69,6 @@ enum class StreamRole {\n> > \tViewfinder,\n> > };\n> >\n> > -using StreamRoles = std::vector;\n> > -\n> > std::ostream &operator<<(std::ostream &out, StreamRole role);\n> >\n> > class Stream\n> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > index 8fcec630..ecca2a97 100644\n> > --- a/src/apps/cam/camera_session.cpp\n> > +++ b/src/apps/cam/camera_session.cpp\n> > @@ -55,7 +55,7 @@ CameraSession::CameraSession(CameraManager *cm,\n> > \t\treturn;\n> > \t}\n> >\n> > -\tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> > +\tauto roles = StreamKeyValueParser::roles(options_[OptStream]);\n\nWe try to minimize usage of the \"auto\" keyword to increase readability,\ncould you please use the explicit type here and below in\nsrc/apps/qcam/main_window.cpp ?\n\n> >\n> > \tstd::unique_ptr config =\n> > \t\tcamera_->generateConfiguration(roles);\n> > diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp\n> > index 19dfe051..663b979a 100644\n> > --- a/src/apps/common/stream_options.cpp\n> > +++ b/src/apps/common/stream_options.cpp\n> > @@ -40,7 +40,7 @@ KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)\n> > \treturn options;\n> > }\n> >\n> > -StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n> > +std::vector StreamKeyValueParser::roles(const OptionValue &values)\n> > {\n> > \t/* If no configuration values to examine default to viewfinder. */\n> > \tif (values.empty())\n> > @@ -48,7 +48,7 @@ StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n> >\n> > \tconst std::vector &streamParameters = values.toArray();\n> >\n> > -\tStreamRoles roles;\n> > +\tstd::vector roles;\n> > \tfor (auto const &value : streamParameters) {\n> > \t\t/* If a role is invalid default it to viewfinder. */\n> > \t\troles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));\n> > diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h\n> > index fe298c84..a5f3bde0 100644\n> > --- a/src/apps/common/stream_options.h\n> > +++ b/src/apps/common/stream_options.h\n> > @@ -20,7 +20,7 @@ public:\n> >\n> > \tKeyValueParser::Options parse(const char *arguments) override;\n> >\n> > -\tstatic libcamera::StreamRoles roles(const OptionValue &values);\n> > +\tstatic std::vector roles(const OptionValue &values);\n> > \tstatic int updateConfiguration(libcamera::CameraConfiguration *config,\n> > \t\t\t\t const OptionValue &values);\n> >\n> > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp\n> > index 680668df..0b123d40 100644\n> > --- a/src/apps/qcam/main_window.cpp\n> > +++ b/src/apps/qcam/main_window.cpp\n> > @@ -362,7 +362,7 @@ void MainWindow::toggleCapture(bool start)\n> > */\n> > int MainWindow::startCapture()\n> > {\n> > -\tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> > +\tauto roles = StreamKeyValueParser::roles(options_[OptStream]);\n> > \tint ret;\n> >\n> > \t/* Verify roles are supported. */\n> > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > index 6eb0a0eb..332837e4 100644\n> > --- a/src/gstreamer/gstlibcameraprovider.cpp\n> > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > @@ -126,13 +126,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > static GstDevice *\n> > gst_libcamera_device_new(const std::shared_ptr &camera)\n> > {\n> > +\tstatic const StreamRole roles[] = { StreamRole::VideoRecording };\n> \n> Any particular reasons for static const here?\n\nIf it's never modified, it's more efficient.\n\nAnd if we want to go the C++ way, this should be a std::array.\n\n> > \tg_autoptr(GstCaps) caps = gst_caps_new_empty();\n> > \tconst gchar *name = camera->id().c_str();\n> > -\tStreamRoles roles;\n> >\n> > -\troles.push_back(StreamRole::VideoRecording);\n> \n> I would like to keep roles as a std::vector<> to be honest. Primary \n> reason being, based on \"conditional camera capabilities\" that we might \n> know of before-hand, more roles can be requested from camera (instead of \n> only VideoRecording) at the same time and adding a role to std::vector<> \n> seems quite easy in that case. What do you think?\n\nUsing a static array is a very simple change, and going back to a vector\nif/when needed later would also be simple, so I don't mind either way.\n\n> > \tstd::unique_ptr config = camera->generateConfiguration(roles);\n> > -\tif (!config || config->size() != roles.size()) {\n> > +\tif (!config || config->size() != std::size(roles)) {\n\nWith std::array this change wouldn't be needed.\n\n> > \t\tGST_ERROR(\"Failed to generate a default configuration for %s\", name);\n> > \t\treturn nullptr;\n> > \t}\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index a10cbd4f..46a5400e 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -468,7 +468,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> >\n> > \tgint stream_id_num = 0;\n> > -\tStreamRoles roles;\n> > +\tstd::vector roles;\n> > \tfor (GstPad *srcpad : state->srcpads_) {\n> > \t\t/* Create stream-id and push stream-start. */\n> > \t\tg_autofree gchar *stream_id_intermediate = g_strdup_printf(\"%i%i\", state->group_id_, stream_id_num++);\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index 67f30815..272222b7 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -436,11 +436,6 @@ std::ostream &operator<<(std::ostream &out, StreamRole role)\n> > \treturn out;\n> > }\n> >\n> > -/**\n> > - * \\typedef StreamRoles\n> > - * \\brief A vector of StreamRole\n> > - */\n> > -\n> > /**\n> > * \\class Stream\n> > * \\brief Video stream for a camera","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 C3FAFBE08B\n\tfor ;\n\tTue, 9 May 2023 15:23:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A587627DE;\n\tTue, 9 May 2023 17:23:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E70486053C\n\tfor ;\n\tTue, 9 May 2023 17:23:16 +0200 (CEST)","from pendragon.ideasonboard.com (softbank126090219015.bbtec.net\n\t[126.90.219.15])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 78B556C8;\n\tTue, 9 May 2023 17:23:09 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683645799;\n\tbh=j6HhXfyDujMIHMrfgjN8nAXjsYFHS5Gqdj1alU4x2u8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ChKMZPAeKk+7vIrlLevxSXOr/HyMmTyx6gYXmdYTY26VNSttd4AgbQORLBxIBj0Av\n\tJuEhMh5eO68LId5c8mXLJUwcPX5z7WmYdM8T7bWMuJ11tTGNQX4jnanh0knOW2gWZy\n\t4nGTtkDCNFyEH+dm2CRVqHbr3GNfnXRYDm0dqbvlg3lGvs6JXiOULfdVa0FFt4tJKe\n\to0viJ7HWMRzt7l9dnMd4dtc1OHnKvXFj9upWCA1zzGuRyR7tDHu3F0So1entrjImjY\n\tX3uXPDztZFYbpjk2X5H4Xb/tbqWhiQnEG9vKbl3C4sHheD5A0mnhD+RxsvHMfNhTqm\n\tt9YAyfcNDCHIA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1683645790;\n\tbh=j6HhXfyDujMIHMrfgjN8nAXjsYFHS5Gqdj1alU4x2u8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oVen0ymW1zW7TGJZui2Ufsc5kdfnO4hp/ST2KNV2ZlgThaofKjmpGkX66JKng6mYA\n\tMCv/p28lfwbmfcUBX+RwZTA26djOAD5njc6oS0CxFYETUHMJcSN1QGImsExp1ynBsi\n\tnK882dpO0PoyXDnsq5PNHKikTjUbhZgKACUmsNnc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oVen0ymW\"; dkim-atps=neutral","Date":"Tue, 9 May 2023 18:23:28 +0300","To":"Umang Jain ","Message-ID":"<20230509152328.GD10510@pendragon.ideasonboard.com>","References":"<20230503124725.200550-1-pobrn@protonmail.com>\n\t<20230503124725.200550-2-pobrn@protonmail.com>\n\t<5994b9df-32e2-f9d3-b6d8-8bb55aca8708@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5994b9df-32e2-f9d3-b6d8-8bb55aca8708@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] libcamera: Remove\n\t`StreamRoles` alias","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27079,"web_url":"https://patchwork.libcamera.org/comment/27079/","msgid":"","date":"2023-05-09T22:15:30","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] libcamera: Remove\n\t`StreamRoles` alias","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2023. május 9., kedd 17:23 keltezéssel, Laurent Pinchart írta:\n\n> Hello,\n> \n> On Tue, May 09, 2023 at 10:54:27AM +0530, Umang Jain via libcamera-devel wrote:\n> > On 5/3/23 6:17 PM, Barnabás Pőcze via libcamera-devel wrote:\n> > > Now that `Camera::generateConfiguration()` takes a `libcamera::Span`\n> > > of `StreamRole`, remove the `StreamRoles` type, which was an alias\n> > > to `std::vector`.\n> > >\n> > > The removal has two reasons:\n> > > - it is no longer strictly necessary,\n> > > - its presence may suggest that that is the preferred (or correct)\n> > > way to build/pass a list of `StreamRole`.\n> > >\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > include/libcamera/stream.h | 2 --\n> > > src/apps/cam/camera_session.cpp | 2 +-\n> > > src/apps/common/stream_options.cpp | 4 ++--\n> > > src/apps/common/stream_options.h | 2 +-\n> > > src/apps/qcam/main_window.cpp | 2 +-\n> > > src/gstreamer/gstlibcameraprovider.cpp | 5 ++---\n> > > src/gstreamer/gstlibcamerasrc.cpp | 2 +-\n> > > src/libcamera/stream.cpp | 5 -----\n> > > 8 files changed, 8 insertions(+), 16 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > > index 29235ddf..4e94187d 100644\n> > > --- a/include/libcamera/stream.h\n> > > +++ b/include/libcamera/stream.h\n> > > @@ -69,8 +69,6 @@ enum class StreamRole {\n> > > \tViewfinder,\n> > > };\n> > >\n> > > -using StreamRoles = std::vector;\n> > > -\n> > > std::ostream &operator<<(std::ostream &out, StreamRole role);\n> > >\n> > > class Stream\n> > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > index 8fcec630..ecca2a97 100644\n> > > --- a/src/apps/cam/camera_session.cpp\n> > > +++ b/src/apps/cam/camera_session.cpp\n> > > @@ -55,7 +55,7 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > \t\treturn;\n> > > \t}\n> > >\n> > > -\tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> > > +\tauto roles = StreamKeyValueParser::roles(options_[OptStream]);\n> \n> We try to minimize usage of the \"auto\" keyword to increase readability,\n> could you please use the explicit type here and below in\n> src/apps/qcam/main_window.cpp ?\n\nACK\n\n\n> \n> > >\n> > > \tstd::unique_ptr config =\n> > > \t\tcamera_->generateConfiguration(roles);\n> > > diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp\n> > > index 19dfe051..663b979a 100644\n> > > --- a/src/apps/common/stream_options.cpp\n> > > +++ b/src/apps/common/stream_options.cpp\n> > > @@ -40,7 +40,7 @@ KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)\n> > > \treturn options;\n> > > }\n> > >\n> > > -StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n> > > +std::vector StreamKeyValueParser::roles(const OptionValue &values)\n> > > {\n> > > \t/* If no configuration values to examine default to viewfinder. */\n> > > \tif (values.empty())\n> > > @@ -48,7 +48,7 @@ StreamRoles StreamKeyValueParser::roles(const OptionValue &values)\n> > >\n> > > \tconst std::vector &streamParameters = values.toArray();\n> > >\n> > > -\tStreamRoles roles;\n> > > +\tstd::vector roles;\n> > > \tfor (auto const &value : streamParameters) {\n> > > \t\t/* If a role is invalid default it to viewfinder. */\n> > > \t\troles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));\n> > > diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h\n> > > index fe298c84..a5f3bde0 100644\n> > > --- a/src/apps/common/stream_options.h\n> > > +++ b/src/apps/common/stream_options.h\n> > > @@ -20,7 +20,7 @@ public:\n> > >\n> > > \tKeyValueParser::Options parse(const char *arguments) override;\n> > >\n> > > -\tstatic libcamera::StreamRoles roles(const OptionValue &values);\n> > > +\tstatic std::vector roles(const OptionValue &values);\n> > > \tstatic int updateConfiguration(libcamera::CameraConfiguration *config,\n> > > \t\t\t\t const OptionValue &values);\n> > >\n> > > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp\n> > > index 680668df..0b123d40 100644\n> > > --- a/src/apps/qcam/main_window.cpp\n> > > +++ b/src/apps/qcam/main_window.cpp\n> > > @@ -362,7 +362,7 @@ void MainWindow::toggleCapture(bool start)\n> > > */\n> > > int MainWindow::startCapture()\n> > > {\n> > > -\tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> > > +\tauto roles = StreamKeyValueParser::roles(options_[OptStream]);\n> > > \tint ret;\n> > >\n> > > \t/* Verify roles are supported. */\n> > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > > index 6eb0a0eb..332837e4 100644\n> > > --- a/src/gstreamer/gstlibcameraprovider.cpp\n> > > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > > @@ -126,13 +126,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > > static GstDevice *\n> > > gst_libcamera_device_new(const std::shared_ptr &camera)\n> > > {\n> > > +\tstatic const StreamRole roles[] = { StreamRole::VideoRecording };\n> >\n> > Any particular reasons for static const here?\n> \n> If it's never modified, it's more efficient.\n\nYes, indeed, that was the motivation for the above change.\n\n\n> \n> And if we want to go the C++ way, this should be a std::array.\n\nACK\n\n\n> \n> > > \tg_autoptr(GstCaps) caps = gst_caps_new_empty();\n> > > \tconst gchar *name = camera->id().c_str();\n> > > -\tStreamRoles roles;\n> > >\n> > > -\troles.push_back(StreamRole::VideoRecording);\n> >\n> > I would like to keep roles as a std::vector<> to be honest. Primary\n> > reason being, based on \"conditional camera capabilities\" that we might\n> > know of before-hand, more roles can be requested from camera (instead of\n> > only VideoRecording) at the same time and adding a role to std::vector<>\n> > seems quite easy in that case. What do you think?\n> \n> Using a static array is a very simple change, and going back to a vector\n> if/when needed later would also be simple, so I don't mind either way.\n> \n> > > \tstd::unique_ptr config = camera->generateConfiguration(roles);\n> > > -\tif (!config || config->size() != roles.size()) {\n> > > +\tif (!config || config->size() != std::size(roles)) {\n> \n> With std::array this change wouldn't be needed.\n\nIndeed!\n\n\n> \n> > > \t\tGST_ERROR(\"Failed to generate a default configuration for %s\", name);\n> > > \t\treturn nullptr;\n> > > \t}\n> [...]\n\n\nRegards,\nBarnabás Pőcze","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 89235BE08B\n\tfor ;\n\tTue, 9 May 2023 22:15:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FA0A633B4;\n\tWed, 10 May 2023 00:15:50 +0200 (CEST)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F7426053A\n\tfor ;\n\tWed, 10 May 2023 00:15:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683670550;\n\tbh=UF3ntEdA7XCHenvCo4joQrwvVi5dJxCy4MpiXwXPdEY=;\n\th=Date:To: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=OWX7DobtFB3pgaaVc8voYXZqmAhabIEz1Q2UbwNiUoBaOj6Vvd14VN7v4XnhS9GIP\n\txhMfZjMgFPgzMZ2ZwQkhMOTuHJO8eBMmxZWlLaiDxIR9HoOgxtIs512eyJHgobEAJM\n\tVQMqshHsiwqRkjKrdgUFwpFg4egI9vwoZomxA3eIlcybrj+c9niWcWlbqsK4C87Mqy\n\tRaRyGN5jhMrY4XQPBykFl7GaoD4ZCfjoXp1DeNDVp2q9T2UfGsKAVpPh4Bki51l15E\n\tim6dEJzm/NYhsSMZgZZJ4JAjzprDtPWIO/yTlNH/Yulg1WvJuz3JVTahQCLva2nvC6\n\tZNcY1Z8deSPmA==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1683670547; x=1683929747;\n\tbh=EyzuYQMm3DCGiAhzaNJDBTHzJN0X9UBtFIZWJZs/7W8=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=ZC1Se9KICpMN2EJ6DIjcEh2qkd3VLJ7U08J1l6SM61cmtqckkvI9vWevyebK7y2MQ\n\tPt3OY4OG/K1uHn0Wc5ROMB34UgJvhZjCmbgzDEkrHFymZg3jnyLZAbCeWzpx6GTDzX\n\tIwxVDRmU4cA6UkiGA+7Ykgbn5KupricYA6LTl5V7TDSoAFGwS+oDql9abRE6HidVti\n\tQHlA2hgFNlHT8HNyj72zoYsJeb6zoUjGvZ4jv9S/rDLRQhzyKFxnEgmAy2asr2u7GK\n\tPwXpGvprTqt4wV3ozyAI/dKstcNHgxIB7YWg8jO6KaFVeoQiq77k0ZC/O2h9kLLKkj\n\tqPeyoxvLJyAIA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=protonmail.com\n\theader.i=@protonmail.com\n\theader.b=\"ZC1Se9KI\"; dkim-atps=neutral","Date":"Tue, 09 May 2023 22:15:30 +0000","To":"Laurent Pinchart ","Message-ID":"","In-Reply-To":"<20230509152328.GD10510@pendragon.ideasonboard.com>","References":"<20230503124725.200550-1-pobrn@protonmail.com>\n\t<20230503124725.200550-2-pobrn@protonmail.com>\n\t<5994b9df-32e2-f9d3-b6d8-8bb55aca8708@ideasonboard.com>\n\t<20230509152328.GD10510@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] libcamera: Remove\n\t`StreamRoles` alias","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":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze_via_libcamera-devel?=\n\t","Reply-To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]