[libcamera-devel] [PATCH v2 0/7] Introduce formats:: namespace for libcamera pixel formats

[Laurent Pinchart]

Hi Stéphane,

On Wed, Jun 10, 2020 at 02:49:31PM +0200, scerveau wrote:
> Hello Laurent,
> 
> Yesterday I tried a different approach from previous attempt, where I 
> proposed to install the libdrm headers in libcamera include folders.
> 
> Indeed I succeeded to make libcamera compile without including 
> drm_fourcc.h in pixel_format.h, see attached patches.
> 
> This is building correctly on my machine (ubuntu 18.04) with a simple 
> `meson build`. But I might have missed use cases.
> 
> Concerning the libdrm dependency or includes, I haven't noticed with 
> Debian/Buster or ArchLinux/latest (docker images) any issue mentioned 
> with libdrm include folder. Both can be found with pkg-config on Ubuntu 
> as well.

The issue is that we wanted to depend in the Linux kernel DRM headers,
not the libdrm package. libcamera doesn't interact with DRM directly, so
depending on libdrm wasn't something we wanted to consider. The Linux
kernel DRM headers contain drm_fourcc.h, which we initially thought
would be enough, but it turned out that distributions don't all ship the
DRM headers as part of the Linux kernel headers package.

> On the other hand your approach might be better, to remove the 
> dependency to libdrm and create your own define for the format if it's 
> going to be the only dependency, the project will have to libdrm.
> 
> But in this case the patch is missing to remove those files then:
> 
> - drm.h
> - drm_fourcc.h
> - drm_mode.h

drm.h and drm_mode.h should indeed be dropped. drm_fourcc.h is still
used by gen-formats.py to generate the formats.h header, so we need to
keep it.4

> On 10/6/20 1:23, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch series is an attempt to fix a problem caused by a direct
> > dependency on drm_fourcc.h in the libcamera public API.
> > 
> > libcamera specifies pixel formats using the DRM pixel format FourCC and
> > modifiers. This requires both internal code and applications to include
> > drm_fourcc.h. For internal code, we carry a copy of the header to avoid
> > external dependencies. For third-party applications, however,
> > drm_fourcc.h needs to be accessible from system includes. It turns out
> > that the file is shipped by two different packages:
> > 
> > - libdrm, which typically installs it in /usr/include/libdrm/
> > - kernel headers or libc headers, which typically installs it in
> >    /usr/include/drm
> > 
> > We don't want to make libdrm a dependency for applications using
> > libcamera. Unfortunately, depending on drm_fourcc.h being provided by
> > the distribution kernel headers or libc headers package causes issues,
> > as not all distributions install the header in /usr/include/drm/. Ubuntu
> > and Gentoo do, but Debian and Arch don't.
> > 
> > The best option may be to remove the dependency on the macros provided
> > by drm_fourcc.h, while keeping the pixel format numerical values
> > identical. This is what this patch series attempts to do.
> > 
> > Patch 1/7 creates a new libcamera::formats:: namespace and populates it
> > with constants (in the form of constexpr) for all supported pixel
> > formats. The values are automatically generated from a list of formats,
> > stored in formats.yaml, and the numerical values for the DRM FourCCs and
> > modifiers are extracted from drm_fourcc.h.
> > 
> > Patches 2/7 to 7/8 then replace usage of the DRM_FORMAT_* macros through
> > the code.
> > 
> > Laurent Pinchart (7):
> >    libcamera: Define constants for pixel formats in the public API
> >    gst: Replace explicit DRM FourCCs with libcamera formats
> >    qcam: Replace explicit DRM FourCCs with libcamera formats
> >    v4l2: Replace explicit DRM FourCCs with libcamera formats
> >    test: Replace explicit DRM FourCCs with libcamera formats
> >    libcamera: pipeline: Replace explicit DRM FourCCs with libcamera
> >      formats
> >    libcamera: Replace explicit DRM FourCCs with libcamera formats
> > 
> >   include/libcamera/formats.h.in                |  44 ++++++
> >   include/libcamera/gen-formats.py              | 118 ++++++++++++++
> >   include/libcamera/meson.build                 |  22 +++
> >   include/libcamera/pixel_format.h              |   2 -
> >   src/android/camera_device.cpp                 |   9 +-
> >   src/gstreamer/gstlibcamera-utils.cpp          |  62 ++++----
> >   src/libcamera/formats.cpp                     | 148 +++++++++---------
> >   src/libcamera/formats.yaml                    | 124 +++++++++++++++
> >   src/libcamera/pipeline/ipu3/ipu3.cpp          |  16 +-
> >   .../pipeline/raspberrypi/raspberrypi.cpp      |  10 +-
> >   src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  19 +--
> >   src/libcamera/pipeline/vimc/vimc.cpp          |  11 +-
> >   src/libcamera/pixel_format.cpp                |   4 +-
> >   src/libcamera/v4l2_pixelformat.cpp            |  83 +++++-----
> >   src/qcam/dng_writer.cpp                       |  17 +-
> >   src/qcam/format_converter.cpp                 |  36 +++--
> >   src/qcam/viewfinder.cpp                       |  10 +-
> >   src/v4l2/v4l2_camera_proxy.cpp                |  29 ++--
> >   test/v4l2_videodevice/buffer_cache.cpp        |   3 +-
> >   19 files changed, 540 insertions(+), 227 deletions(-)
> >   create mode 100644 include/libcamera/formats.h.in
> >   create mode 100755 include/libcamera/gen-formats.py
> >   create mode 100644 src/libcamera/formats.yaml

-- 
Regards,

Laurent Pinchart