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

[Laurent Pinchart]

Hi Stéphane,

On Wed, Jun 10, 2020 at 03:08:00PM +0200, scerveau wrote:
> Hi Laurent,
> 
> I was suspecting something like that regarding the drm includes but
> could you explain why you dont want to depend on the libdrm package as a 
> few runtime dependencies are necessary already ?

Because libcamera doesn't interact with DRM devices, to there's no need
for libdrm. The only thing we need is the definitions of the the pixel
formats.

> On 10/6/20 14:57, Laurent Pinchart wrote:
> > 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