[libcamera-devel] [PATCH v2] apps: Share common source between applications

[Kieran Bingham]

Quoting Laurent Pinchart (2022-10-24 21:11:34)
> Hi Kieran,
> 
> 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.

> 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.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,
...


[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.

--
Kieran


> 
> > Anyway, with or without that, grouping this makes sense.
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >