[libcamera-devel] [RFC PATCH v2 2/2] libcamera: Remove `StreamRoles` alias

[Barnabás Pőcze]

Hi


2023. május 9., kedd 17:23 keltezéssel, Laurent Pinchart <laurent.pinchart at 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 at 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