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

Message ID 20221018080908.2841339-2-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • cam: Add DNG support
Related show

Commit Message

Paul Elder Oct. 18, 2022, 8:09 a.m. UTC 
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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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%)

Comments

Jacopo Mondi Oct. 19, 2022, 8:22 a.m. UTC | #1 
Hi Paul

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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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

 if cxx.has_header_symbol('QOpenGLWidget', 'QOpenGLWidget',

>      ])
>  endif
>
> --
> 2.30.2
>
Laurent Pinchart Oct. 19, 2022, 9:38 a.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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.

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.

> 
>  if cxx.has_header_symbol('QOpenGLWidget', 'QOpenGLWidget',
> 
> >      ])
> >  endif
> >
Jacopo Mondi Oct. 19, 2022, 10:17 a.m. UTC | #3 
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@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@jmondi.org>

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

Patch
diff mbox series 

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()
     qt5_cpp_args += ['-DHAVE_TIFF']
     qcam_deps += [tiff_dep]
     qcam_sources += files([
-        'dng_writer.cpp',
+        '../cam/dng_writer.cpp',
     ])
 endif