| Message ID | 20230509231542.1123025-1-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:15:43) > 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, 9 insertions(+), 15 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>; > - I was curious how we might handle this moving forwards if we need to remove things. (I think removing this is the right thing to do, so this is just 'how' we remove it) I wonder if we should have an include/libcamera/deprecated.h to place things like: using StreamRoles [[deprecated("Use a span, array or vector directly")]] = std::vector<StreamRole>; Perhaps even with a reference to something that makes it clearer what to do to migrate with the issue. StreamRoles has been in all the examples so far, so I think all apps that use libcamera are probably already using this. I know we explicitly don't have ABI/API stability - but if we break things perhaps it would be helpful to have a system that lets us inform the user what they need to do next to fix it again. This probably gets quite relevant to how we handle things with: https://patchwork.libcamera.org/project/libcamera/list/?series=3877 so any thoughts on that series are welcome! > 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..066e397b 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]); > + std::vector<StreamRole> 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..0f16c038 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]); > + std::vector<StreamRole> 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..494f778b 100644 > --- a/src/gstreamer/gstlibcameraprovider.cpp > +++ b/src/gstreamer/gstlibcameraprovider.cpp > @@ -6,6 +6,8 @@ > * gstlibcameraprovider.c - GStreamer Device Provider > */ > > +#include <array> > + > #include "gstlibcameraprovider.h" > > #include <libcamera/camera.h> > @@ -126,11 +128,10 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass) > static GstDevice * > gst_libcamera_device_new(const std::shared_ptr<Camera> &camera) > { > + static const std::array 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()) { > GST_ERROR("Failed to generate a default configuration for %s", name); > 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 > >
Hi 2023. május 16., kedd 1:09 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:15:43) > > 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, 9 insertions(+), 15 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>; > > - > > I was curious how we might handle this moving forwards if we need to > remove things. (I think removing this is the right thing to do, so this > is just 'how' we remove it) > > I wonder if we should have an include/libcamera/deprecated.h to place > things like: > > using StreamRoles [[deprecated("Use a span, array or vector directly")]] > = std::vector<StreamRole>; > > Perhaps even with a reference to something that makes it clearer what to > do to migrate with the issue. > > StreamRoles has been in all the examples so far, so I think all apps > that use libcamera are probably already using this. > > I know we explicitly don't have ABI/API stability - but if we break > things perhaps it would be helpful to have a system that lets us inform > the user what they need to do next to fix it again. > > This probably gets quite relevant to how we handle things with: > https://patchwork.libcamera.org/project/libcamera/list/?series=3877 so > any thoughts on that series are welcome! Maybe it could be kept in the same header file? Otherwise users will still encounter a compiler error and have to investigate what happened, no? > [...] Regards, Barnabás Pőcze
Quoting Barnabás Pőcze (2023-05-18 16:29:10) > Hi > > > 2023. május 16., kedd 1:09 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > > > Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:15:43) > > > 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, 9 insertions(+), 15 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>; > > > - > > > > I was curious how we might handle this moving forwards if we need to > > remove things. (I think removing this is the right thing to do, so this > > is just 'how' we remove it) > > > > I wonder if we should have an include/libcamera/deprecated.h to place > > things like: > > > > using StreamRoles [[deprecated("Use a span, array or vector directly")]] > > = std::vector<StreamRole>; > > > > Perhaps even with a reference to something that makes it clearer what to > > do to migrate with the issue. > > > > StreamRoles has been in all the examples so far, so I think all apps > > that use libcamera are probably already using this. > > > > I know we explicitly don't have ABI/API stability - but if we break > > things perhaps it would be helpful to have a system that lets us inform > > the user what they need to do next to fix it again. > > > > This probably gets quite relevant to how we handle things with: > > https://patchwork.libcamera.org/project/libcamera/list/?series=3877 so > > any thoughts on that series are welcome! > > Maybe it could be kept in the same header file? Otherwise users will still > encounter a compiler error and have to investigate what happened, no? If we go down the 'deprecated' route then I would have a new deprecated.h which is included by the main libcamera.h header so the definitions would be available to applications without them changing - except they would now get the deprecated warning (or error). But by moving it - it would be easier to locate to remove old deprecations at each release point. Developer makes breaking change - Warning/update moved to deprecated.h to inform applciations Release n+1 made with that in deprecated.h .... more commits .... / time .... change removed from deprecated.h Release n+2 made. No longer carried in deprecated.h -- Kieran > > > > [...] > > > Regards, > Barnabás Pőcze
Hi 2023. május 18., csütörtök 18:13 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Quoting Barnabás Pőcze (2023-05-18 16:29:10) > > Hi > > > > > > 2023. május 16., kedd 1:09 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > > > > > Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:15:43) > > > > 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, 9 insertions(+), 15 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>; > > > > - > > > > > > I was curious how we might handle this moving forwards if we need to > > > remove things. (I think removing this is the right thing to do, so this > > > is just 'how' we remove it) > > > > > > I wonder if we should have an include/libcamera/deprecated.h to place > > > things like: > > > > > > using StreamRoles [[deprecated("Use a span, array or vector directly")]] > > > = std::vector<StreamRole>; > > > > > > Perhaps even with a reference to something that makes it clearer what to > > > do to migrate with the issue. > > > > > > StreamRoles has been in all the examples so far, so I think all apps > > > that use libcamera are probably already using this. > > > > > > I know we explicitly don't have ABI/API stability - but if we break > > > things perhaps it would be helpful to have a system that lets us inform > > > the user what they need to do next to fix it again. > > > > > > This probably gets quite relevant to how we handle things with: > > > https://patchwork.libcamera.org/project/libcamera/list/?series=3877 so > > > any thoughts on that series are welcome! > > > > Maybe it could be kept in the same header file? Otherwise users will still > > encounter a compiler error and have to investigate what happened, no? > > If we go down the 'deprecated' route then I would have a new > deprecated.h which is included by the main libcamera.h header so the > definitions would be available to applications without them changing - > except they would now get the deprecated warning (or error). In that case wouldn't you have to force applications to (only?) include libcamera/libcamera.h like gtk, glib, libdbus, etc. do it? > > But by moving it - it would be easier to locate to remove old > deprecations at each release point. Fair point, although in my mind `git grep` is pretty close to handling that. > > Developer makes breaking change > - Warning/update moved to deprecated.h to inform applciations > > Release n+1 made with that in deprecated.h > > .... more commits .... / time .... > > change removed from deprecated.h > > Release n+2 made. No longer carried in deprecated.h > > > -- > Kieran > Regards, Barnabás Pőcze
Quoting Barnabás Pőcze (2023-05-25 23:09:37) > Hi > > > 2023. május 18., csütörtök 18:13 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > > > Quoting Barnabás Pőcze (2023-05-18 16:29:10) > > > Hi > > > > > > > > > 2023. május 16., kedd 1:09 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > > > > > > > Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:15:43) > > > > > 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, 9 insertions(+), 15 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>; > > > > > - > > > > > > > > I was curious how we might handle this moving forwards if we need to > > > > remove things. (I think removing this is the right thing to do, so this > > > > is just 'how' we remove it) > > > > > > > > I wonder if we should have an include/libcamera/deprecated.h to place > > > > things like: > > > > > > > > using StreamRoles [[deprecated("Use a span, array or vector directly")]] > > > > = std::vector<StreamRole>; > > > > > > > > Perhaps even with a reference to something that makes it clearer what to > > > > do to migrate with the issue. > > > > > > > > StreamRoles has been in all the examples so far, so I think all apps > > > > that use libcamera are probably already using this. > > > > > > > > I know we explicitly don't have ABI/API stability - but if we break > > > > things perhaps it would be helpful to have a system that lets us inform > > > > the user what they need to do next to fix it again. > > > > > > > > This probably gets quite relevant to how we handle things with: > > > > https://patchwork.libcamera.org/project/libcamera/list/?series=3877 so > > > > any thoughts on that series are welcome! > > > > > > Maybe it could be kept in the same header file? Otherwise users will still > > > encounter a compiler error and have to investigate what happened, no? > > > > If we go down the 'deprecated' route then I would have a new > > deprecated.h which is included by the main libcamera.h header so the > > definitions would be available to applications without them changing - > > except they would now get the deprecated warning (or error). > > In that case wouldn't you have to force applications to (only?) include > libcamera/libcamera.h like gtk, glib, libdbus, etc. do it? I thought libcamera/libcamera.h would be the preferred choice for applications. But if they choose to include files directly, then indeed they would miss out on any 'deprecated.h' and get compile breakage on an ABI/API change. But that's no different there than without it (the deprecation helpers) though! It just means they won't have the extra level of support .. So no, I don't think we would have to 'force' applications to only use it. They 'can' use it ... or they might not. > > But by moving it - it would be easier to locate to remove old > > deprecations at each release point. > > Fair point, although in my mind `git grep` is pretty close to handling that. Maybe indeed. Just means looking in multiple places to clean up before a release rather than taking the previous section from a single file. I don't think any of this would be 'high churn' anyway so I don't think it's a big deal either way. > > Developer makes breaking change > > - Warning/update moved to deprecated.h to inform applciations > > > > Release n+1 made with that in deprecated.h > > > > .... more commits .... / time .... > > > > change removed from deprecated.h > > > > Release n+2 made. No longer carried in deprecated.h > > > > > > -- > > Kieran > > > > > Regards, > Barnabás Pőcze
Hi Barnabás, Thank you for the patch. On Tue, May 09, 2023 at 11:15:43PM +0000, 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> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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, 9 insertions(+), 15 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..066e397b 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]); > + std::vector<StreamRole> 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..0f16c038 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]); > + std::vector<StreamRole> 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..494f778b 100644 > --- a/src/gstreamer/gstlibcameraprovider.cpp > +++ b/src/gstreamer/gstlibcameraprovider.cpp > @@ -6,6 +6,8 @@ > * gstlibcameraprovider.c - GStreamer Device Provider > */ > > +#include <array> > + > #include "gstlibcameraprovider.h" > > #include <libcamera/camera.h> > @@ -126,11 +128,10 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass) > static GstDevice * > gst_libcamera_device_new(const std::shared_ptr<Camera> &camera) > { > + static const std::array 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()) { > GST_ERROR("Failed to generate a default configuration for %s", name); > 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
Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:15:43) > 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`. > It seems my idea for a deprecated.h wasn't deemed necessary and I don't want to block this patch, which is fine on it's own anyway. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > 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, 9 insertions(+), 15 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..066e397b 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]); > + std::vector<StreamRole> 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..0f16c038 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]); > + std::vector<StreamRole> 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..494f778b 100644 > --- a/src/gstreamer/gstlibcameraprovider.cpp > +++ b/src/gstreamer/gstlibcameraprovider.cpp > @@ -6,6 +6,8 @@ > * gstlibcameraprovider.c - GStreamer Device Provider > */ > > +#include <array> > + > #include "gstlibcameraprovider.h" > > #include <libcamera/camera.h> > @@ -126,11 +128,10 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass) > static GstDevice * > gst_libcamera_device_new(const std::shared_ptr<Camera> &camera) > { > + static const std::array 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()) { > GST_ERROR("Failed to generate a default configuration for %s", name); > 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 > >
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..066e397b 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]); + std::vector<StreamRole> 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..0f16c038 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]); + std::vector<StreamRole> 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..494f778b 100644 --- a/src/gstreamer/gstlibcameraprovider.cpp +++ b/src/gstreamer/gstlibcameraprovider.cpp @@ -6,6 +6,8 @@ * gstlibcameraprovider.c - GStreamer Device Provider */ +#include <array> + #include "gstlibcameraprovider.h" #include <libcamera/camera.h> @@ -126,11 +128,10 @@ gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass) static GstDevice * gst_libcamera_device_new(const std::shared_ptr<Camera> &camera) { + static const std::array 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()) { GST_ERROR("Failed to generate a default configuration for %s", name); 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
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, 9 insertions(+), 15 deletions(-)