[libcamera-devel] [PATCH v2] apps: Share common source between applications
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 25 01:28:03 CEST 2022
Hi Kieran,
On Mon, Oct 24, 2022 at 10:16:00PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-10-24 21:11:34)
> > On Mon, Oct 24, 2022 at 08:56:56PM +0100, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart via libcamera-devel (2022-10-20 23:10:26)
> > > > Multiple source files in the src/apps/cam/ directory are used by cam,
> > > > qcam and lc-compliance. They are compiled separately for each
> > > > application. Move them to a new src/apps/common/ directory and compile
> > > > them in a static library to decrease the number of compilation
> > > > operations.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > > Changes since v1:
> > > >
> > > > - Move common code to src/apps/common/
> > > > ---
> > > > src/apps/cam/camera_session.cpp | 5 ++--
> > > > src/apps/cam/camera_session.h | 2 +-
> > > > src/apps/cam/drm.cpp | 2 +-
> > > > src/apps/cam/file_sink.cpp | 5 ++--
> > > > src/apps/cam/main.cpp | 7 +++---
> > > > src/apps/cam/meson.build | 14 ++---------
> > > > src/apps/cam/sdl_sink.cpp | 5 ++--
> > > > src/apps/cam/sdl_texture.h | 2 +-
> > > > src/apps/{cam => common}/dng_writer.cpp | 0
> > > > src/apps/{cam => common}/dng_writer.h | 0
> > > > src/apps/{cam => common}/event_loop.cpp | 0
> > > > src/apps/{cam => common}/event_loop.h | 0
> > > > src/apps/{cam => common}/image.cpp | 0
> > > > src/apps/{cam => common}/image.h | 0
> > > > src/apps/common/meson.build | 26 +++++++++++++++++++++
> > > > src/apps/{cam => common}/options.cpp | 0
> > > > src/apps/{cam => common}/options.h | 0
> > > > src/apps/{cam => common}/stream_options.cpp | 0
> > > > src/apps/{cam => common}/stream_options.h | 0
> > > > src/apps/lc-compliance/main.cpp | 3 ++-
> > > > src/apps/lc-compliance/meson.build | 3 +--
> > > > src/apps/lc-compliance/simple_capture.h | 2 +-
> > > > src/apps/meson.build | 2 ++
> > > > src/apps/qcam/format_converter.cpp | 2 +-
> > > > src/apps/qcam/main.cpp | 5 ++--
> > > > src/apps/qcam/main_window.cpp | 4 ++--
> > > > src/apps/qcam/main_window.h | 2 +-
> > > > src/apps/qcam/meson.build | 13 ++---------
> > > > src/apps/qcam/viewfinder_gl.cpp | 2 +-
> > > > src/apps/qcam/viewfinder_qt.cpp | 2 +-
> > > > 30 files changed, 61 insertions(+), 47 deletions(-)
> > > > rename src/apps/{cam => common}/dng_writer.cpp (100%)
> > > > rename src/apps/{cam => common}/dng_writer.h (100%)
> > > > rename src/apps/{cam => common}/event_loop.cpp (100%)
> > > > rename src/apps/{cam => common}/event_loop.h (100%)
> > > > rename src/apps/{cam => common}/image.cpp (100%)
> > > > rename src/apps/{cam => common}/image.h (100%)
> > > > create mode 100644 src/apps/common/meson.build
> > > > rename src/apps/{cam => common}/options.cpp (100%)
> > > > rename src/apps/{cam => common}/options.h (100%)
> > > > rename src/apps/{cam => common}/stream_options.cpp (100%)
> > > > rename src/apps/{cam => common}/stream_options.h (100%)
> > > >
> > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > > > index 6b409c983b86..8fcec6304d66 100644
> > > > --- a/src/apps/cam/camera_session.cpp
> > > > +++ b/src/apps/cam/camera_session.cpp
> > > > @@ -13,9 +13,11 @@
> > > > #include <libcamera/control_ids.h>
> > > > #include <libcamera/property_ids.h>
> > > >
> > > > +#include "../common/event_loop.h"
> > > > +#include "../common/stream_options.h"
> > > > +
> > >
> > > I still dislike the includes from a relative parent path.
> > >
> > > Is it possible to fix with the dependency or static library apps_lib
> > > object? Or just something you didn't want to do?
> >
> > I thought about it. It would involve -I, #include <...>, and the risk of
>
> it was -iquote I was actually thinking about, but it seems like meson
> doesn't really support it.
>
> We could put it directly in the args, but I'm not sure that's practical
> either.
It's not very nice indeed.
> > conflict with system includes. I think it's still a good idea, but I
> > didn't want to explore that rabbit hole right now. If someone wants to
> > send a patch on top, I'll be happy to review it :-)
>
> Maybe if meson ever actually implemented something like suggested in
> [0], but I'll not hold my breath.
I have a feeling of déjà vu. I'm pretty sure I've discussed -iquote on
the meson IRC channel a while ago. I'm not entirely sure why, but I
believe it was due to a conflict with system includes, and that may be
what caused the internal rework of the libcamera headers directories
hierarchy.
> (I.e. if this were possible, but it won't work...)
>
> # https://mesonbuild.com/Reference-manual_functions.html#arguments27
> apps_inc = include_directories(['common'], is_system: false)
> or
> # https://github.com/mesonbuild/meson/issues/9219#issuecomment-1289622872
> apps_inc = include_directories(['common'], is_quote: true)
>
> ...
>
> apps_lib = static_library('apps', apps_sources,
> cpp_args : apps_cpp_args,
> + includes : apps_inc,
> dependencies : [libcamera_public])
>
>
> lc_compliance = executable('lc-compliance', lc_compliance_sources,
> cpp_args : [ '-fexceptions' ],
> dependencies : [
> + apps_
> libatomic,
> libcamera_public,
> ...
Does it propagate the include directory to lc_compliance ? I thought
that would only use it to compile the static library. You can use
declare_dependency() to bundle a library and an include directory (and
other things) as a dependency. It won't solve the -iquote issue though.
> [0] https://github.com/mesonbuild/meson/issues/9219
>
> Maybe proposing 'is_quote' as an option to include_directories might be
> interesting, but that's not going to change this patch anytime soon.
Indeed :-)
> > > Anyway, with or without that, grouping this makes sense.
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list