Message ID | 20200609232323.29628-1-laurent.pinchart@ideasonboard.com |
---|---|
Headers | show |
Series |
|
Related | show |
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. 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 Stéphane. 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 >
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
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 ? Regards. Stéphane On 10/6/20 14:57, Laurent Pinchart wrote: > 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 >
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