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

[Kieran Bingham]

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 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?

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