[libcamera-devel] [PATCH v2 1/3] qcam, cam: Move DNGWriter from qcam to cam

[Jacopo Mondi]

Hi Laurent

On Wed, Oct 19, 2022 at 12:38:24PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Oct 19, 2022 at 10:22:05AM +0200, Jacopo Mondi via libcamera-devel wrote:
> > On Tue, Oct 18, 2022 at 05:09:06PM +0900, Paul Elder via libcamera-devel wrote:
> > > To prepare for adding DNG support to cam, move DNGWriter from qcam to
> > > cam so that we only have inclusions from qcam to cam and not the other
> > > way around.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > ---
> > > Changes in v2:
> > > - move libtiff dependency for cam to the next patch
> > > ---
> > >  src/{qcam => cam}/dng_writer.cpp | 0
> > >  src/{qcam => cam}/dng_writer.h   | 0
> > >  src/cam/meson.build              | 1 +
> > >  src/qcam/main_window.cpp         | 2 +-
> > >  src/qcam/meson.build             | 2 +-
> > >  5 files changed, 3 insertions(+), 2 deletions(-)
> > >  rename src/{qcam => cam}/dng_writer.cpp (100%)
> > >  rename src/{qcam => cam}/dng_writer.h (100%)
> > >
> > > diff --git a/src/qcam/dng_writer.cpp b/src/cam/dng_writer.cpp
> > > similarity index 100%
> > > rename from src/qcam/dng_writer.cpp
> > > rename to src/cam/dng_writer.cpp
> > > diff --git a/src/qcam/dng_writer.h b/src/cam/dng_writer.h
> > > similarity index 100%
> > > rename from src/qcam/dng_writer.h
> > > rename to src/cam/dng_writer.h
> > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > index 8259239f..9c766221 100644
> > > --- a/src/cam/meson.build
> > > +++ b/src/cam/meson.build
> > > @@ -26,6 +26,7 @@ cam_cpp_args = []
> > >  libdrm = dependency('libdrm', required : false)
> > >  libjpeg = dependency('libjpeg', required : false)
> > >  libsdl2 = dependency('SDL2', required : false)
> > > +libtiff = dependency('libtiff-4', required : false)
> > >
> > >  if libdrm.found()
> > >      cam_cpp_args += [ '-DHAVE_KMS' ]
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index e0e5092e..f553ccb0 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -26,10 +26,10 @@
> > >  #include <QToolButton>
> > >  #include <QtDebug>
> > >
> > > +#include "../cam/dng_writer.h"
> > >  #include "../cam/image.h"
> > >
> > >  #include "cam_select_dialog.h"
> > > -#include "dng_writer.h"
> > >  #ifndef QT_NO_OPENGL
> > >  #include "viewfinder_gl.h"
> > >  #endif
> > > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > > index 61861ea6..9f5759ff 100644
> > > --- a/src/qcam/meson.build
> > > +++ b/src/qcam/meson.build
> > > @@ -49,7 +49,7 @@ if tiff_dep.found()
> >
> > You can now remove tiff_dep and use libtiff ?
> >
> >
> > >      qt5_cpp_args += ['-DHAVE_TIFF']
> > >      qcam_deps += [tiff_dep]
> > >      qcam_sources += files([
> > > -        'dng_writer.cpp',
> > > +        '../cam/dng_writer.cpp',
> >
> > With 2/3 applied, I would rather define a symbol for the dng_sources
> > in cam/meson.build
> >
> >         dng_sources = []
> >         if libtiff.found()
> >             cam_cpp_args += ['-DHAVE_TIFF']
> >             dng_sources = files([
> >                 'dng_writer.cpp',
> >             ])
> >             cam_sources += dng_sources
> >         endif
> >
> > And re-use it here in qcam/meson.build
> >
> >         if libtiff.found()
> >             qt5_cpp_args += ['-DHAVE_TIFF']
> >             qcam_deps += [libtiff]
> >             qcam_sources += dng_sources
> >         endif
> >
> > The overall diff on top of 2/3 I have is:
> >
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index 06dbea0645b7..b4336ba25c35 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -52,11 +52,13 @@ if libsdl2.found()
> >      endif
> >  endif
> >
> > +dng_sources = []
> >  if libtiff.found()
> >      cam_cpp_args += ['-DHAVE_TIFF']
> > -    cam_sources += files([
> > +    dng_sources = files([
> >          'dng_writer.cpp',
> >      ])
> > +    cam_sources += dng_sources
> >  endif
> >
> >  cam  = executable('cam', cam_sources,
> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > index 9f5759ff0786..8a816f86c91a 100644
> > --- a/src/qcam/meson.build
> > +++ b/src/qcam/meson.build
> > @@ -44,13 +44,10 @@ qcam_deps = [
> >
> >  qt5_cpp_args = ['-DQT_NO_KEYWORDS']
> >
> > -tiff_dep = dependency('libtiff-4', required : false)
> > -if tiff_dep.found()
> > +if libtiff.found()
> >      qt5_cpp_args += ['-DHAVE_TIFF']
> > -    qcam_deps += [tiff_dep]
> > -    qcam_sources += files([
> > -        '../cam/dng_writer.cpp',
> > -    ])
> > +    qcam_deps += [libtiff]
> > +    qcam_sources += dng_sources
> >  endif
>
> This will break on systems where libevent is not found. cam will be
> skipped, and dng_sources won't be defined.
>

Ah! Ok sorry for the bad suggestion then

> If we want to avoid code duplication between cam/meson.build and
> qcam/meson.build, let's create an application static library for the
> code shared between the two applictions. It wouldn't be installed, just
> used internally to avoid compiling the same source files twice. I think
> this can be done on top.
>

Surely not for this patches..

As my comments clearly do not apply
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

> >
> >  if cxx.has_header_symbol('QOpenGLWidget', 'QOpenGLWidget',
> >
> > >      ])
> > >  endif
> > >
>
> --
> Regards,
>
> Laurent Pinchart