[libcamera-devel,RFC,v2,2/2] libcamera: Remove `StreamRoles` alias
diff mbox series

Message ID 20230503124725.200550-2-pobrn@protonmail.com
State Superseded
Headers show
Series
  • [libcamera-devel,RFC,v2,1/2] libcamera: camera: Take span of StreamRole instead of vector
Related show

Commit Message

Barnabás Pőcze May 3, 2023, 12:47 p.m. UTC 
Now that `Camera::generateConfiguration()` takes a `libcamera::Span`
of `StreamRole`, remove the `StreamRoles` type, which was an alias
to `std::vector<libcamera::StreamRole>`.

The removal has two reasons:
 - it is no longer strictly necessary,
 - its presence may suggest that that is the preferred (or correct)
   way to build/pass a list of `StreamRole`.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 include/libcamera/stream.h             | 2 --
 src/apps/cam/camera_session.cpp        | 2 +-
 src/apps/common/stream_options.cpp     | 4 ++--
 src/apps/common/stream_options.h       | 2 +-
 src/apps/qcam/main_window.cpp          | 2 +-
 src/gstreamer/gstlibcameraprovider.cpp | 5 ++---
 src/gstreamer/gstlibcamerasrc.cpp      | 2 +-
 src/libcamera/stream.cpp               | 5 -----
 8 files changed, 8 insertions(+), 16 deletions(-)

--
2.40.1

Comments

Umang Jain May 9, 2023, 5:24 a.m. UTC | #1 
Hi Barnabás,

On 5/3/23 6:17 PM, Barnabás Pőcze via libcamera-devel wrote:
> Now that `Camera::generateConfiguration()` takes a `libcamera::Span`
> of `StreamRole`, remove the `StreamRoles` type, which was an alias
> to `std::vector<libcamera::StreamRole>`.
>
> The removal has two reasons:
>   - it is no longer strictly necessary,
>   - its presence may suggest that that is the preferred (or correct)
>     way to build/pass a list of `StreamRole`.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>   include/libcamera/stream.h             | 2 --
>   src/apps/cam/camera_session.cpp        | 2 +-
>   src/apps/common/stream_options.cpp     | 4 ++--
>   src/apps/common/stream_options.h       | 2 +-
>   src/apps/qcam/main_window.cpp          | 2 +-
>   src/gstreamer/gstlibcameraprovider.cpp | 5 ++---
>   src/gstreamer/gstlibcamerasrc.cpp      | 2 +-
>   src/libcamera/stream.cpp               | 5 -----
>   8 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 29235ddf..4e94187d 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -69,8 +69,6 @@ enum class StreamRole {
>   	Viewfinder,
>   };
>
> -using StreamRoles = std::vector<StreamRole>;
> -
>   std::ostream &operator<<(std::ostream &out, StreamRole role);
>
>   class Stream
> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> index 8fcec630..ecca2a97 100644
> --- a/src/apps/cam/camera_session.cpp
> +++ b/src/apps/cam/camera_session.cpp
> @@ -55,7 +55,7 @@ CameraSession::CameraSession(CameraManager *cm,
>   		return;
>   	}
>
> -	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> +	auto roles = StreamKeyValueParser::roles(options_[OptStream]);
>
>   	std::unique_ptr<CameraConfiguration> config =
>   		camera_->generateConfiguration(roles);
> diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp
> index 19dfe051..663b979a 100644
> --- a/src/apps/common/stream_options.cpp
> +++ b/src/apps/common/stream_options.cpp
> @@ -40,7 +40,7 @@ KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
>   	return options;
>   }
>
> -StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> +std::vector<StreamRole> StreamKeyValueParser::roles(const OptionValue &values)
>   {
>   	/* If no configuration values to examine default to viewfinder. */
>   	if (values.empty())
> @@ -48,7 +48,7 @@ StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
>
>   	const std::vector<OptionValue> &streamParameters = values.toArray();
>
> -	StreamRoles roles;
> +	std::vector<StreamRole> roles;
>   	for (auto const &value : streamParameters) {
>   		/* If a role is invalid default it to viewfinder. */
>   		roles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));
> diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h
> index fe298c84..a5f3bde0 100644
> --- a/src/apps/common/stream_options.h
> +++ b/src/apps/common/stream_options.h
> @@ -20,7 +20,7 @@ public:
>
>   	KeyValueParser::Options parse(const char *arguments) override;
>
> -	static libcamera::StreamRoles roles(const OptionValue &values);
> +	static std::vector<libcamera::StreamRole> roles(const OptionValue &values);
>   	static int updateConfiguration(libcamera::CameraConfiguration *config,
>   				       const OptionValue &values);
>
> diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> index 680668df..0b123d40 100644
> --- a/src/apps/qcam/main_window.cpp
> +++ b/src/apps/qcam/main_window.cpp
> @@ -362,7 +362,7 @@ void MainWindow::toggleCapture(bool start)
>    */
>   int MainWindow::startCapture()
>   {
> -	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> +	auto roles = StreamKeyValueParser::roles(options_[OptStream]);
>   	int ret;
>
>   	/* Verify roles are supported. */
> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> index 6eb0a0eb..332837e4 100644
> --- a/src/gstreamer/gstlibcameraprovider.cpp
> +++ b/src/gstreamer/gstlibcameraprovider.cpp
> @@ -126,13 +126,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)
>   static GstDevice *
>   gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
>   {
> +	static const StreamRole roles[] = { StreamRole::VideoRecording };

Any particular reasons for static const here?
>   	g_autoptr(GstCaps) caps = gst_caps_new_empty();
>   	const gchar *name = camera->id().c_str();
> -	StreamRoles roles;
>
> -	roles.push_back(StreamRole::VideoRecording);

I would like to keep roles as a std::vector<> to be honest. Primary 
reason being, based on "conditional camera capabilities" that we might 
know of before-hand, more roles can be requested from camera (instead of 
only VideoRecording) at the same time and adding a role to std::vector<> 
seems quite easy in that case. What do you think?
>   	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> -	if (!config || config->size() != roles.size()) {
> +	if (!config || config->size() != std::size(roles)) {
>   		GST_ERROR("Failed to generate a default configuration for %s", name);
>   		return nullptr;
>   	}
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index a10cbd4f..46a5400e 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -468,7 +468,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>   	GST_DEBUG_OBJECT(self, "Streaming thread has started");
>
>   	gint stream_id_num = 0;
> -	StreamRoles roles;
> +	std::vector<StreamRole> roles;
>   	for (GstPad *srcpad : state->srcpads_) {
>   		/* Create stream-id and push stream-start. */
>   		g_autofree gchar *stream_id_intermediate = g_strdup_printf("%i%i", state->group_id_, stream_id_num++);
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index 67f30815..272222b7 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -436,11 +436,6 @@ std::ostream &operator<<(std::ostream &out, StreamRole role)
>   	return out;
>   }
>
> -/**
> - * \typedef StreamRoles
> - * \brief A vector of StreamRole
> - */
> -
>   /**
>    * \class Stream
>    * \brief Video stream for a camera
> --
> 2.40.1
>
>
Laurent Pinchart May 9, 2023, 3:23 p.m. UTC | #2 
Hello,

On Tue, May 09, 2023 at 10:54:27AM +0530, Umang Jain via libcamera-devel wrote:
> On 5/3/23 6:17 PM, Barnabás Pőcze via libcamera-devel wrote:
> > Now that `Camera::generateConfiguration()` takes a `libcamera::Span`
> > of `StreamRole`, remove the `StreamRoles` type, which was an alias
> > to `std::vector<libcamera::StreamRole>`.
> >
> > The removal has two reasons:
> >   - it is no longer strictly necessary,
> >   - its presence may suggest that that is the preferred (or correct)
> >     way to build/pass a list of `StreamRole`.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >   include/libcamera/stream.h             | 2 --
> >   src/apps/cam/camera_session.cpp        | 2 +-
> >   src/apps/common/stream_options.cpp     | 4 ++--
> >   src/apps/common/stream_options.h       | 2 +-
> >   src/apps/qcam/main_window.cpp          | 2 +-
> >   src/gstreamer/gstlibcameraprovider.cpp | 5 ++---
> >   src/gstreamer/gstlibcamerasrc.cpp      | 2 +-
> >   src/libcamera/stream.cpp               | 5 -----
> >   8 files changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 29235ddf..4e94187d 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -69,8 +69,6 @@ enum class StreamRole {
> >   	Viewfinder,
> >   };
> >
> > -using StreamRoles = std::vector<StreamRole>;
> > -
> >   std::ostream &operator<<(std::ostream &out, StreamRole role);
> >
> >   class Stream
> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > index 8fcec630..ecca2a97 100644
> > --- a/src/apps/cam/camera_session.cpp
> > +++ b/src/apps/cam/camera_session.cpp
> > @@ -55,7 +55,7 @@ CameraSession::CameraSession(CameraManager *cm,
> >   		return;
> >   	}
> >
> > -	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > +	auto roles = StreamKeyValueParser::roles(options_[OptStream]);

We try to minimize usage of the "auto" keyword to increase readability,
could you please use the explicit type here and below in
src/apps/qcam/main_window.cpp ?

> >
> >   	std::unique_ptr<CameraConfiguration> config =
> >   		camera_->generateConfiguration(roles);
> > diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp
> > index 19dfe051..663b979a 100644
> > --- a/src/apps/common/stream_options.cpp
> > +++ b/src/apps/common/stream_options.cpp
> > @@ -40,7 +40,7 @@ KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
> >   	return options;
> >   }
> >
> > -StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> > +std::vector<StreamRole> StreamKeyValueParser::roles(const OptionValue &values)
> >   {
> >   	/* If no configuration values to examine default to viewfinder. */
> >   	if (values.empty())
> > @@ -48,7 +48,7 @@ StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> >
> >   	const std::vector<OptionValue> &streamParameters = values.toArray();
> >
> > -	StreamRoles roles;
> > +	std::vector<StreamRole> roles;
> >   	for (auto const &value : streamParameters) {
> >   		/* If a role is invalid default it to viewfinder. */
> >   		roles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));
> > diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h
> > index fe298c84..a5f3bde0 100644
> > --- a/src/apps/common/stream_options.h
> > +++ b/src/apps/common/stream_options.h
> > @@ -20,7 +20,7 @@ public:
> >
> >   	KeyValueParser::Options parse(const char *arguments) override;
> >
> > -	static libcamera::StreamRoles roles(const OptionValue &values);
> > +	static std::vector<libcamera::StreamRole> roles(const OptionValue &values);
> >   	static int updateConfiguration(libcamera::CameraConfiguration *config,
> >   				       const OptionValue &values);
> >
> > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> > index 680668df..0b123d40 100644
> > --- a/src/apps/qcam/main_window.cpp
> > +++ b/src/apps/qcam/main_window.cpp
> > @@ -362,7 +362,7 @@ void MainWindow::toggleCapture(bool start)
> >    */
> >   int MainWindow::startCapture()
> >   {
> > -	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > +	auto roles = StreamKeyValueParser::roles(options_[OptStream]);
> >   	int ret;
> >
> >   	/* Verify roles are supported. */
> > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> > index 6eb0a0eb..332837e4 100644
> > --- a/src/gstreamer/gstlibcameraprovider.cpp
> > +++ b/src/gstreamer/gstlibcameraprovider.cpp
> > @@ -126,13 +126,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)
> >   static GstDevice *
> >   gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
> >   {
> > +	static const StreamRole roles[] = { StreamRole::VideoRecording };
> 
> Any particular reasons for static const here?

If it's never modified, it's more efficient.

And if we want to go the C++ way, this should be a std::array.

> >   	g_autoptr(GstCaps) caps = gst_caps_new_empty();
> >   	const gchar *name = camera->id().c_str();
> > -	StreamRoles roles;
> >
> > -	roles.push_back(StreamRole::VideoRecording);
> 
> I would like to keep roles as a std::vector<> to be honest. Primary 
> reason being, based on "conditional camera capabilities" that we might 
> know of before-hand, more roles can be requested from camera (instead of 
> only VideoRecording) at the same time and adding a role to std::vector<> 
> seems quite easy in that case. What do you think?

Using a static array is a very simple change, and going back to a vector
if/when needed later would also be simple, so I don't mind either way.

> >   	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> > -	if (!config || config->size() != roles.size()) {
> > +	if (!config || config->size() != std::size(roles)) {

With std::array this change wouldn't be needed.

> >   		GST_ERROR("Failed to generate a default configuration for %s", name);
> >   		return nullptr;
> >   	}
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index a10cbd4f..46a5400e 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -468,7 +468,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> >   	GST_DEBUG_OBJECT(self, "Streaming thread has started");
> >
> >   	gint stream_id_num = 0;
> > -	StreamRoles roles;
> > +	std::vector<StreamRole> roles;
> >   	for (GstPad *srcpad : state->srcpads_) {
> >   		/* Create stream-id and push stream-start. */
> >   		g_autofree gchar *stream_id_intermediate = g_strdup_printf("%i%i", state->group_id_, stream_id_num++);
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index 67f30815..272222b7 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -436,11 +436,6 @@ std::ostream &operator<<(std::ostream &out, StreamRole role)
> >   	return out;
> >   }
> >
> > -/**
> > - * \typedef StreamRoles
> > - * \brief A vector of StreamRole
> > - */
> > -
> >   /**
> >    * \class Stream
> >    * \brief Video stream for a camera
Barnabás Pőcze May 9, 2023, 10:15 p.m. UTC | #3 
Hi


2023. május 9., kedd 17:23 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> Hello,
> 
> On Tue, May 09, 2023 at 10:54:27AM +0530, Umang Jain via libcamera-devel wrote:
> > On 5/3/23 6:17 PM, Barnabás Pőcze via libcamera-devel wrote:
> > > Now that `Camera::generateConfiguration()` takes a `libcamera::Span`
> > > of `StreamRole`, remove the `StreamRoles` type, which was an alias
> > > to `std::vector<libcamera::StreamRole>`.
> > >
> > > The removal has two reasons:
> > >   - it is no longer strictly necessary,
> > >   - its presence may suggest that that is the preferred (or correct)
> > >     way to build/pass a list of `StreamRole`.
> > >
> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > ---
> > >   include/libcamera/stream.h             | 2 --
> > >   src/apps/cam/camera_session.cpp        | 2 +-
> > >   src/apps/common/stream_options.cpp     | 4 ++--
> > >   src/apps/common/stream_options.h       | 2 +-
> > >   src/apps/qcam/main_window.cpp          | 2 +-
> > >   src/gstreamer/gstlibcameraprovider.cpp | 5 ++---
> > >   src/gstreamer/gstlibcamerasrc.cpp      | 2 +-
> > >   src/libcamera/stream.cpp               | 5 -----
> > >   8 files changed, 8 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > > index 29235ddf..4e94187d 100644
> > > --- a/include/libcamera/stream.h
> > > +++ b/include/libcamera/stream.h
> > > @@ -69,8 +69,6 @@ enum class StreamRole {
> > >   	Viewfinder,
> > >   };
> > >
> > > -using StreamRoles = std::vector<StreamRole>;
> > > -
> > >   std::ostream &operator<<(std::ostream &out, StreamRole role);
> > >
> > >   class Stream
> > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > > index 8fcec630..ecca2a97 100644
> > > --- a/src/apps/cam/camera_session.cpp
> > > +++ b/src/apps/cam/camera_session.cpp
> > > @@ -55,7 +55,7 @@ CameraSession::CameraSession(CameraManager *cm,
> > >   		return;
> > >   	}
> > >
> > > -	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > > +	auto roles = StreamKeyValueParser::roles(options_[OptStream]);
> 
> We try to minimize usage of the "auto" keyword to increase readability,
> could you please use the explicit type here and below in
> src/apps/qcam/main_window.cpp ?

ACK


> 
> > >
> > >   	std::unique_ptr<CameraConfiguration> config =
> > >   		camera_->generateConfiguration(roles);
> > > diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp
> > > index 19dfe051..663b979a 100644
> > > --- a/src/apps/common/stream_options.cpp
> > > +++ b/src/apps/common/stream_options.cpp
> > > @@ -40,7 +40,7 @@ KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
> > >   	return options;
> > >   }
> > >
> > > -StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> > > +std::vector<StreamRole> StreamKeyValueParser::roles(const OptionValue &values)
> > >   {
> > >   	/* If no configuration values to examine default to viewfinder. */
> > >   	if (values.empty())
> > > @@ -48,7 +48,7 @@ StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
> > >
> > >   	const std::vector<OptionValue> &streamParameters = values.toArray();
> > >
> > > -	StreamRoles roles;
> > > +	std::vector<StreamRole> roles;
> > >   	for (auto const &value : streamParameters) {
> > >   		/* If a role is invalid default it to viewfinder. */
> > >   		roles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));
> > > diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h
> > > index fe298c84..a5f3bde0 100644
> > > --- a/src/apps/common/stream_options.h
> > > +++ b/src/apps/common/stream_options.h
> > > @@ -20,7 +20,7 @@ public:
> > >
> > >   	KeyValueParser::Options parse(const char *arguments) override;
> > >
> > > -	static libcamera::StreamRoles roles(const OptionValue &values);
> > > +	static std::vector<libcamera::StreamRole> roles(const OptionValue &values);
> > >   	static int updateConfiguration(libcamera::CameraConfiguration *config,
> > >   				       const OptionValue &values);
> > >
> > > diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
> > > index 680668df..0b123d40 100644
> > > --- a/src/apps/qcam/main_window.cpp
> > > +++ b/src/apps/qcam/main_window.cpp
> > > @@ -362,7 +362,7 @@ void MainWindow::toggleCapture(bool start)
> > >    */
> > >   int MainWindow::startCapture()
> > >   {
> > > -	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > > +	auto roles = StreamKeyValueParser::roles(options_[OptStream]);
> > >   	int ret;
> > >
> > >   	/* Verify roles are supported. */
> > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> > > index 6eb0a0eb..332837e4 100644
> > > --- a/src/gstreamer/gstlibcameraprovider.cpp
> > > +++ b/src/gstreamer/gstlibcameraprovider.cpp
> > > @@ -126,13 +126,12 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)
> > >   static GstDevice *
> > >   gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
> > >   {
> > > +	static const StreamRole roles[] = { StreamRole::VideoRecording };
> >
> > Any particular reasons for static const here?
> 
> If it's never modified, it's more efficient.

Yes, indeed, that was the motivation for the above change.


> 
> And if we want to go the C++ way, this should be a std::array.

ACK


> 
> > >   	g_autoptr(GstCaps) caps = gst_caps_new_empty();
> > >   	const gchar *name = camera->id().c_str();
> > > -	StreamRoles roles;
> > >
> > > -	roles.push_back(StreamRole::VideoRecording);
> >
> > I would like to keep roles as a std::vector<> to be honest. Primary
> > reason being, based on "conditional camera capabilities" that we might
> > know of before-hand, more roles can be requested from camera (instead of
> > only VideoRecording) at the same time and adding a role to std::vector<>
> > seems quite easy in that case. What do you think?
> 
> Using a static array is a very simple change, and going back to a vector
> if/when needed later would also be simple, so I don't mind either way.
> 
> > >   	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> > > -	if (!config || config->size() != roles.size()) {
> > > +	if (!config || config->size() != std::size(roles)) {
> 
> With std::array this change wouldn't be needed.

Indeed!


> 
> > >   		GST_ERROR("Failed to generate a default configuration for %s", name);
> > >   		return nullptr;
> > >   	}
> [...]


Regards,
Barnabás Pőcze

Patch
diff mbox series 

diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
index 29235ddf..4e94187d 100644
--- a/include/libcamera/stream.h
+++ b/include/libcamera/stream.h
@@ -69,8 +69,6 @@  enum class StreamRole {
 	Viewfinder,
 };

-using StreamRoles = std::vector<StreamRole>;
-
 std::ostream &operator<<(std::ostream &out, StreamRole role);

 class Stream
diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
index 8fcec630..ecca2a97 100644
--- a/src/apps/cam/camera_session.cpp
+++ b/src/apps/cam/camera_session.cpp
@@ -55,7 +55,7 @@  CameraSession::CameraSession(CameraManager *cm,
 		return;
 	}

-	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
+	auto roles = StreamKeyValueParser::roles(options_[OptStream]);

 	std::unique_ptr<CameraConfiguration> config =
 		camera_->generateConfiguration(roles);
diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp
index 19dfe051..663b979a 100644
--- a/src/apps/common/stream_options.cpp
+++ b/src/apps/common/stream_options.cpp
@@ -40,7 +40,7 @@  KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments)
 	return options;
 }

-StreamRoles StreamKeyValueParser::roles(const OptionValue &values)
+std::vector<StreamRole> StreamKeyValueParser::roles(const OptionValue &values)
 {
 	/* If no configuration values to examine default to viewfinder. */
 	if (values.empty())
@@ -48,7 +48,7 @@  StreamRoles StreamKeyValueParser::roles(const OptionValue &values)

 	const std::vector<OptionValue> &streamParameters = values.toArray();

-	StreamRoles roles;
+	std::vector<StreamRole> roles;
 	for (auto const &value : streamParameters) {
 		/* If a role is invalid default it to viewfinder. */
 		roles.push_back(parseRole(value.toKeyValues()).value_or(StreamRole::Viewfinder));
diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h
index fe298c84..a5f3bde0 100644
--- a/src/apps/common/stream_options.h
+++ b/src/apps/common/stream_options.h
@@ -20,7 +20,7 @@  public:

 	KeyValueParser::Options parse(const char *arguments) override;

-	static libcamera::StreamRoles roles(const OptionValue &values);
+	static std::vector<libcamera::StreamRole> roles(const OptionValue &values);
 	static int updateConfiguration(libcamera::CameraConfiguration *config,
 				       const OptionValue &values);

diff --git a/src/apps/qcam/main_window.cpp b/src/apps/qcam/main_window.cpp
index 680668df..0b123d40 100644
--- a/src/apps/qcam/main_window.cpp
+++ b/src/apps/qcam/main_window.cpp
@@ -362,7 +362,7 @@  void MainWindow::toggleCapture(bool start)
  */
 int MainWindow::startCapture()
 {
-	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
+	auto roles = StreamKeyValueParser::roles(options_[OptStream]);
 	int ret;

 	/* Verify roles are supported. */
diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
index 6eb0a0eb..332837e4 100644
--- a/src/gstreamer/gstlibcameraprovider.cpp
+++ b/src/gstreamer/gstlibcameraprovider.cpp
@@ -126,13 +126,12 @@  gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)
 static GstDevice *
 gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
 {
+	static const StreamRole roles[] = { StreamRole::VideoRecording };
 	g_autoptr(GstCaps) caps = gst_caps_new_empty();
 	const gchar *name = camera->id().c_str();
-	StreamRoles roles;

-	roles.push_back(StreamRole::VideoRecording);
 	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
-	if (!config || config->size() != roles.size()) {
+	if (!config || config->size() != std::size(roles)) {
 		GST_ERROR("Failed to generate a default configuration for %s", name);
 		return nullptr;
 	}
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index a10cbd4f..46a5400e 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -468,7 +468,7 @@  gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
 	GST_DEBUG_OBJECT(self, "Streaming thread has started");

 	gint stream_id_num = 0;
-	StreamRoles roles;
+	std::vector<StreamRole> roles;
 	for (GstPad *srcpad : state->srcpads_) {
 		/* Create stream-id and push stream-start. */
 		g_autofree gchar *stream_id_intermediate = g_strdup_printf("%i%i", state->group_id_, stream_id_num++);
diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
index 67f30815..272222b7 100644
--- a/src/libcamera/stream.cpp
+++ b/src/libcamera/stream.cpp
@@ -436,11 +436,6 @@  std::ostream &operator<<(std::ostream &out, StreamRole role)
 	return out;
 }

-/**
- * \typedef StreamRoles
- * \brief A vector of StreamRole
- */
-
 /**
  * \class Stream
  * \brief Video stream for a camera