[libcamera-devel] [RFC PATCH v2 2/2] libcamera: Remove `StreamRoles` alias
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 9 17:23:28 CEST 2023
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 ?
> >
> > 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list