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

[Barnabás Pőcze]

Hi


2023. május 18., csütörtök 18:13 keltezéssel, Kieran Bingham <kieran.bingham at 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 at 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 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, 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