| Message ID | 20200522145459.16836-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Fri, May 22, 2020 at 05:54:47PM +0300, 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. I agree this is a better way forward than depending on the system header. I skimmed through the series and it looks good but I have a few thoughts 1) format names: now that we have our own formats, how should they be named? This version uses the DRM defined format names, and it makes sense indeed to make it easier for people using a DRM sink to have a libcamera format. Howeverm, afair, gstreamer in example uses fourcc names from fourcc.org, which I have no idea if it's authoritative or not. Any opinion on that ? 2) I suspect the v4l2->libcamera->drm dance on pixel format definitions we have on-going for RAW formats will repeat itself once we add more platforms. This does not change the current situation where new platform with a new V4L2 fourcc code -> we define a format in libcamera associated with a temporary DRM fourcc code -> We upstream the DRM fourcc code -> upstream happens <- We have to change the format definition in libcamera, which is not an issue now, but once things are more stable it could be I wonder if instead of exposing PixelFormats (which are created using a DRM fourcc which might not be stable) we should not expose an enumeration of fourcc-independent codes, and have a run-time translation to the drm fourcc ones.. If I look at const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ /* RGB formats. */ { formats::BGR888, { .format = formats::BGR888, .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24), .bitsPerPixel = 24, .colourEncoding = PixelFormatInfo::ColourEncodingRGB, .packed = false, } }, The map keys and the first field of the PixelFormatInfo are actually the same thing.. I know this means defining our own format codes (which might very well be a sequential enumeration), requires run-time translations, and kind of defeat using DRM fourcc (or any kind of existing fourcc codes set). But what happens here basically does that already, applications gets provided a set of identifiers, it does just only partially matters that they're consutrcted using DRM fourccs behing the curatins, as I expect applications using DRM to have to look at the header and then realize that they can PixelFormat(DRM_FORMAT_xyz) instead of having to look at what libcamera::formats::xys actually is... Is run-time translation frowned upon in you opinion ? > Patches 01/11 to 04/11 are preparatory cleanups, please see individual > patches for details. I believe they make sense regardless of whether the > rest is accepted or rejected. I think these could be sent already as PATCH Thanks j > > Patch 05/11 creates a new libcamera::formats:: namespace and populates > it with constants (in the form of constexpr) for all supported pixel > formats. The values have been written manually, which isn't ideal. We > could partly automate this process by generating the header from a list > of formats (likely in a YAML file), and fetching the numerical values > from drm_fourcc.h to make sure they will always match. This could allow > documenting the pixel formats in the YAML file and generating Doxygen > documentation, like we do for controls. > > Patches 06/11 to 11/11 then replace usage of the DRM_FORMAT_* macros > through the code, with one exception in the IPU3 pipeline handler to > demonstrate how DRM_FORMAT_* values can still be used for internal > formats. I however believe this will be dropped too, as applications may > want to capture RAW data from the IPU3, so the format should likely be > defined in the public API. > > Laurent Pinchart (11): > libcamera: Rename pixelformats.{cpp,h} to pixel_format.{cpp,h} > libcamera: Replace C++ comments with C comments > libcamera: Rename header guards for internal headers > libcamera: pixel_format: Make PixelFormat usable as a constexpr > 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/control_ids.h.in | 4 +- > include/libcamera/formats.h | 94 +++++++++++ > .../libcamera/internal/byte_stream_buffer.h | 6 +- > include/libcamera/internal/camera_controls.h | 6 +- > include/libcamera/internal/camera_sensor.h | 6 +- > .../libcamera/internal/control_serializer.h | 6 +- > .../libcamera/internal/control_validator.h | 6 +- > .../libcamera/internal/device_enumerator.h | 6 +- > .../internal/device_enumerator_sysfs.h | 6 +- > .../internal/device_enumerator_udev.h | 6 +- > .../internal/event_dispatcher_poll.h | 6 +- > include/libcamera/internal/file.h | 6 +- > include/libcamera/internal/formats.h | 8 +- > .../libcamera/internal/ipa_context_wrapper.h | 6 +- > include/libcamera/internal/ipa_manager.h | 6 +- > include/libcamera/internal/ipa_module.h | 6 +- > include/libcamera/internal/ipa_proxy.h | 6 +- > include/libcamera/internal/ipc_unixsocket.h | 6 +- > include/libcamera/internal/log.h | 6 +- > include/libcamera/internal/media_device.h | 6 +- > include/libcamera/internal/media_object.h | 6 +- > include/libcamera/internal/message.h | 6 +- > include/libcamera/internal/pipeline_handler.h | 6 +- > include/libcamera/internal/process.h | 6 +- > include/libcamera/internal/pub_key.h | 6 +- > include/libcamera/internal/semaphore.h | 6 +- > include/libcamera/internal/thread.h | 6 +- > include/libcamera/internal/utils.h | 6 +- > include/libcamera/internal/v4l2_controls.h | 6 +- > include/libcamera/internal/v4l2_device.h | 6 +- > include/libcamera/internal/v4l2_pixelformat.h | 8 +- > include/libcamera/internal/v4l2_subdevice.h | 6 +- > include/libcamera/internal/v4l2_videodevice.h | 8 +- > include/libcamera/meson.build | 3 +- > include/libcamera/pixel_format.h | 48 ++++++ > include/libcamera/pixelformats.h | 43 ----- > include/libcamera/property_ids.h.in | 6 +- > include/libcamera/stream.h | 2 +- > src/gstreamer/gstlibcamera-utils.cpp | 62 ++++---- > src/libcamera/formats.cpp | 148 +++++++++--------- > src/libcamera/meson.build | 2 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +-- > src/libcamera/pipeline/simple/converter.h | 2 +- > src/libcamera/pipeline/vimc/vimc.cpp | 11 +- > .../{pixelformats.cpp => pixel_format.cpp} | 19 +-- > src/libcamera/v4l2_pixelformat.cpp | 85 +++++----- > src/qcam/dng_writer.cpp | 17 +- > src/qcam/format_converter.cpp | 36 +++-- > src/qcam/format_converter.h | 2 +- > src/qcam/viewfinder.cpp | 10 +- > src/qcam/viewfinder.h | 2 +- > src/v4l2/v4l2_camera_proxy.cpp | 27 ++-- > test/v4l2_videodevice/buffer_cache.cpp | 3 +- > 55 files changed, 477 insertions(+), 378 deletions(-) > create mode 100644 include/libcamera/formats.h > create mode 100644 include/libcamera/pixel_format.h > delete mode 100644 include/libcamera/pixelformats.h > rename src/libcamera/{pixelformats.cpp => pixel_format.cpp} (89%) > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Fri, May 22, 2020 at 08:11:20PM +0200, Jacopo Mondi wrote: > On Fri, May 22, 2020 at 05:54:47PM +0300, 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. > > I agree this is a better way forward than depending on the system > header. > > I skimmed through the series and it looks good but I have a few > thoughts > 1) format names: now that we have our own formats, how should they be > named? This version uses the DRM defined format names, and it makes > sense indeed to make it easier for people using a DRM sink to have a > libcamera format. Howeverm, afair, gstreamer in example uses fourcc > names from fourcc.org, which I have no idea if it's authoritative or > not. Any opinion on that ? I don't think there's any authoritative list of pixel format names (otherwise we would have an authoritative list of 4CC values too, and we wouldn't need to deal with this mess :-)). As we use the same numerical values as DRM, I think it makes sense to use similar names. I've written "similar" and not "identical" on purpose, as DRM has names for 4CCs and for modifiers, but not for pixel formats, so we'll need to create our own names for modified formats (SGRBG10_CSI2P is an example of such a name introduced in this series). > 2) I suspect the v4l2->libcamera->drm dance on pixel format > definitions we have on-going for RAW formats will repeat itself once > we add more platforms. This does not change the current situation > where > new platform with a new V4L2 fourcc code > -> we define a format in libcamera associated with a temporary > DRM fourcc code > -> We upstream the DRM fourcc code > -> upstream happens > <- We have to change the format definition in libcamera, > which is not an issue now, but once things are more stable > it could be > > I wonder if instead of exposing PixelFormats (which are created using > a DRM fourcc which might not be stable) we should not expose an > enumeration of fourcc-independent codes, and have a run-time > translation to the drm fourcc ones.. If I look at > > const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > /* RGB formats. */ > { formats::BGR888, { > .format = formats::BGR888, > .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24), > .bitsPerPixel = 24, > .colourEncoding = PixelFormatInfo::ColourEncodingRGB, > .packed = false, > } }, > > The map keys and the first field of the PixelFormatInfo are actually > the same thing.. > > I know this means defining our own format codes (which might very well > be a sequential enumeration), requires run-time translations, and kind > of defeat using DRM fourcc (or any kind of existing fourcc codes set). > But what happens here basically does that already, applications gets > provided a set of identifiers, it does just only partially matters > that they're consutrcted using DRM fourccs behing the curatins, as I > expect applications using DRM to have to look at the header and then > realize that they can PixelFormat(DRM_FORMAT_xyz) instead of having to > look at what libcamera::formats::xys actually is... > > Is run-time translation frowned upon in you opinion ? I'd personally frown upon yet another set of pixel formats with yet another translation. We have too many of them already. I'm open to alternatives, but only if the alternative make things simpler, not more complicated :-) As for you concern above, once we start caring about stability, we'll need to have kernel drivers merged before merging the corresponding code in libcamera, regardless of whether we use DRM pixel formats or our own pixel formats anyway. I'd be willing to consider another set of pixel format values if they were used by a relevant kernel API and were pushed as the new standard. > > Patches 01/11 to 04/11 are preparatory cleanups, please see individual > > patches for details. I believe they make sense regardless of whether the > > rest is accepted or rejected. > > I think these could be sent already as PATCH Feel free to review them :-) I've submitted them as part of this series to avoid creating dependencies between two patch series, but I'd be happy to merge them already while we discuss the rest. > > Patch 05/11 creates a new libcamera::formats:: namespace and populates > > it with constants (in the form of constexpr) for all supported pixel > > formats. The values have been written manually, which isn't ideal. We > > could partly automate this process by generating the header from a list > > of formats (likely in a YAML file), and fetching the numerical values > > from drm_fourcc.h to make sure they will always match. This could allow > > documenting the pixel formats in the YAML file and generating Doxygen > > documentation, like we do for controls. > > > > Patches 06/11 to 11/11 then replace usage of the DRM_FORMAT_* macros > > through the code, with one exception in the IPU3 pipeline handler to > > demonstrate how DRM_FORMAT_* values can still be used for internal > > formats. I however believe this will be dropped too, as applications may > > want to capture RAW data from the IPU3, so the format should likely be > > defined in the public API. > > > > Laurent Pinchart (11): > > libcamera: Rename pixelformats.{cpp,h} to pixel_format.{cpp,h} > > libcamera: Replace C++ comments with C comments > > libcamera: Rename header guards for internal headers > > libcamera: pixel_format: Make PixelFormat usable as a constexpr > > 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/control_ids.h.in | 4 +- > > include/libcamera/formats.h | 94 +++++++++++ > > .../libcamera/internal/byte_stream_buffer.h | 6 +- > > include/libcamera/internal/camera_controls.h | 6 +- > > include/libcamera/internal/camera_sensor.h | 6 +- > > .../libcamera/internal/control_serializer.h | 6 +- > > .../libcamera/internal/control_validator.h | 6 +- > > .../libcamera/internal/device_enumerator.h | 6 +- > > .../internal/device_enumerator_sysfs.h | 6 +- > > .../internal/device_enumerator_udev.h | 6 +- > > .../internal/event_dispatcher_poll.h | 6 +- > > include/libcamera/internal/file.h | 6 +- > > include/libcamera/internal/formats.h | 8 +- > > .../libcamera/internal/ipa_context_wrapper.h | 6 +- > > include/libcamera/internal/ipa_manager.h | 6 +- > > include/libcamera/internal/ipa_module.h | 6 +- > > include/libcamera/internal/ipa_proxy.h | 6 +- > > include/libcamera/internal/ipc_unixsocket.h | 6 +- > > include/libcamera/internal/log.h | 6 +- > > include/libcamera/internal/media_device.h | 6 +- > > include/libcamera/internal/media_object.h | 6 +- > > include/libcamera/internal/message.h | 6 +- > > include/libcamera/internal/pipeline_handler.h | 6 +- > > include/libcamera/internal/process.h | 6 +- > > include/libcamera/internal/pub_key.h | 6 +- > > include/libcamera/internal/semaphore.h | 6 +- > > include/libcamera/internal/thread.h | 6 +- > > include/libcamera/internal/utils.h | 6 +- > > include/libcamera/internal/v4l2_controls.h | 6 +- > > include/libcamera/internal/v4l2_device.h | 6 +- > > include/libcamera/internal/v4l2_pixelformat.h | 8 +- > > include/libcamera/internal/v4l2_subdevice.h | 6 +- > > include/libcamera/internal/v4l2_videodevice.h | 8 +- > > include/libcamera/meson.build | 3 +- > > include/libcamera/pixel_format.h | 48 ++++++ > > include/libcamera/pixelformats.h | 43 ----- > > include/libcamera/property_ids.h.in | 6 +- > > include/libcamera/stream.h | 2 +- > > src/gstreamer/gstlibcamera-utils.cpp | 62 ++++---- > > src/libcamera/formats.cpp | 148 +++++++++--------- > > src/libcamera/meson.build | 2 +- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +-- > > src/libcamera/pipeline/simple/converter.h | 2 +- > > src/libcamera/pipeline/vimc/vimc.cpp | 11 +- > > .../{pixelformats.cpp => pixel_format.cpp} | 19 +-- > > src/libcamera/v4l2_pixelformat.cpp | 85 +++++----- > > src/qcam/dng_writer.cpp | 17 +- > > src/qcam/format_converter.cpp | 36 +++-- > > src/qcam/format_converter.h | 2 +- > > src/qcam/viewfinder.cpp | 10 +- > > src/qcam/viewfinder.h | 2 +- > > src/v4l2/v4l2_camera_proxy.cpp | 27 ++-- > > test/v4l2_videodevice/buffer_cache.cpp | 3 +- > > 55 files changed, 477 insertions(+), 378 deletions(-) > > create mode 100644 include/libcamera/formats.h > > create mode 100644 include/libcamera/pixel_format.h > > delete mode 100644 include/libcamera/pixelformats.h > > rename src/libcamera/{pixelformats.cpp => pixel_format.cpp} (89%)
Le vendredi 22 mai 2020 à 20:11 +0200, Jacopo Mondi a écrit : > Hi Laurent, > > On Fri, May 22, 2020 at 05:54:47PM +0300, 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. > > I agree this is a better way forward than depending on the system > header. > > I skimmed through the series and it looks good but I have a few > thoughts > 1) format names: now that we have our own formats, how should they be > named? This version uses the DRM defined format names, and it makes > sense indeed to make it easier for people using a DRM sink to have a > libcamera format. Howeverm, afair, gstreamer in example uses fourcc > names from fourcc.org, which I have no idea if it's authoritative or > not. Any opinion on that ? fourcc.org is no authority no. GStreamer uses a mix of Microsoft defined fourcc, Apple define fourcc and fourcc.org. This has to do with fourcc found in ISOMP4, Matroska and other containers (even though you cannot assume consistencies, and there is even few duplicates coming from errors in major software). The rest is very custom, we never managed to actually found a working rules. Newer formats like I422_10BE, NV12_64Z32, NV12_10LE32 capture more information but pretty much all fails in one way to capture everything a really be consistent. I think FFMPEG now has some documented rules, but I'm not certain. > > 2) I suspect the v4l2->libcamera->drm dance on pixel format > definitions we have on-going for RAW formats will repeat itself once > we add more platforms. This does not change the current situation > where > new platform with a new V4L2 fourcc code > -> we define a format in libcamera associated with a temporary > DRM fourcc code > -> We upstream the DRM fourcc code > -> upstream happens > <- We have to change the format definition in libcamera, > which is not an issue now, but once things are more stable > it could be > > I wonder if instead of exposing PixelFormats (which are created using > a DRM fourcc which might not be stable) we should not expose an > enumeration of fourcc-independent codes, and have a run-time > translation to the drm fourcc ones.. If I look at > > const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > /* RGB formats. */ > { formats::BGR888, { > .format = formats::BGR888, > .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24), > .bitsPerPixel = 24, > .colourEncoding = PixelFormatInfo::ColourEncodingRGB, > .packed = false, > } }, > > The map keys and the first field of the PixelFormatInfo are actually > the same thing.. > > I know this means defining our own format codes (which might very well > be a sequential enumeration), requires run-time translations, and kind > of defeat using DRM fourcc (or any kind of existing fourcc codes set). > But what happens here basically does that already, applications gets > provided a set of identifiers, it does just only partially matters > that they're consutrcted using DRM fourccs behing the curatins, as I > expect applications using DRM to have to look at the header and then > realize that they can PixelFormat(DRM_FORMAT_xyz) instead of having to > look at what libcamera::formats::xys actually is... > > Is run-time translation frowned upon in you opinion ? > > > Patches 01/11 to 04/11 are preparatory cleanups, please see individual > > patches for details. I believe they make sense regardless of whether the > > rest is accepted or rejected. > > I think these could be sent already as PATCH > > Thanks > j > > Patch 05/11 creates a new libcamera::formats:: namespace and populates > > it with constants (in the form of constexpr) for all supported pixel > > formats. The values have been written manually, which isn't ideal. We > > could partly automate this process by generating the header from a list > > of formats (likely in a YAML file), and fetching the numerical values > > from drm_fourcc.h to make sure they will always match. This could allow > > documenting the pixel formats in the YAML file and generating Doxygen > > documentation, like we do for controls. > > > > Patches 06/11 to 11/11 then replace usage of the DRM_FORMAT_* macros > > through the code, with one exception in the IPU3 pipeline handler to > > demonstrate how DRM_FORMAT_* values can still be used for internal > > formats. I however believe this will be dropped too, as applications may > > want to capture RAW data from the IPU3, so the format should likely be > > defined in the public API. > > > > Laurent Pinchart (11): > > libcamera: Rename pixelformats.{cpp,h} to pixel_format.{cpp,h} > > libcamera: Replace C++ comments with C comments > > libcamera: Rename header guards for internal headers > > libcamera: pixel_format: Make PixelFormat usable as a constexpr > > 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/control_ids.h.in | 4 +- > > include/libcamera/formats.h | 94 +++++++++++ > > .../libcamera/internal/byte_stream_buffer.h | 6 +- > > include/libcamera/internal/camera_controls.h | 6 +- > > include/libcamera/internal/camera_sensor.h | 6 +- > > .../libcamera/internal/control_serializer.h | 6 +- > > .../libcamera/internal/control_validator.h | 6 +- > > .../libcamera/internal/device_enumerator.h | 6 +- > > .../internal/device_enumerator_sysfs.h | 6 +- > > .../internal/device_enumerator_udev.h | 6 +- > > .../internal/event_dispatcher_poll.h | 6 +- > > include/libcamera/internal/file.h | 6 +- > > include/libcamera/internal/formats.h | 8 +- > > .../libcamera/internal/ipa_context_wrapper.h | 6 +- > > include/libcamera/internal/ipa_manager.h | 6 +- > > include/libcamera/internal/ipa_module.h | 6 +- > > include/libcamera/internal/ipa_proxy.h | 6 +- > > include/libcamera/internal/ipc_unixsocket.h | 6 +- > > include/libcamera/internal/log.h | 6 +- > > include/libcamera/internal/media_device.h | 6 +- > > include/libcamera/internal/media_object.h | 6 +- > > include/libcamera/internal/message.h | 6 +- > > include/libcamera/internal/pipeline_handler.h | 6 +- > > include/libcamera/internal/process.h | 6 +- > > include/libcamera/internal/pub_key.h | 6 +- > > include/libcamera/internal/semaphore.h | 6 +- > > include/libcamera/internal/thread.h | 6 +- > > include/libcamera/internal/utils.h | 6 +- > > include/libcamera/internal/v4l2_controls.h | 6 +- > > include/libcamera/internal/v4l2_device.h | 6 +- > > include/libcamera/internal/v4l2_pixelformat.h | 8 +- > > include/libcamera/internal/v4l2_subdevice.h | 6 +- > > include/libcamera/internal/v4l2_videodevice.h | 8 +- > > include/libcamera/meson.build | 3 +- > > include/libcamera/pixel_format.h | 48 ++++++ > > include/libcamera/pixelformats.h | 43 ----- > > include/libcamera/property_ids.h.in | 6 +- > > include/libcamera/stream.h | 2 +- > > src/gstreamer/gstlibcamera-utils.cpp | 62 ++++---- > > src/libcamera/formats.cpp | 148 +++++++++--------- > > src/libcamera/meson.build | 2 +- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +-- > > src/libcamera/pipeline/simple/converter.h | 2 +- > > src/libcamera/pipeline/vimc/vimc.cpp | 11 +- > > .../{pixelformats.cpp => pixel_format.cpp} | 19 +-- > > src/libcamera/v4l2_pixelformat.cpp | 85 +++++----- > > src/qcam/dng_writer.cpp | 17 +- > > src/qcam/format_converter.cpp | 36 +++-- > > src/qcam/format_converter.h | 2 +- > > src/qcam/viewfinder.cpp | 10 +- > > src/qcam/viewfinder.h | 2 +- > > src/v4l2/v4l2_camera_proxy.cpp | 27 ++-- > > test/v4l2_videodevice/buffer_cache.cpp | 3 +- > > 55 files changed, 477 insertions(+), 378 deletions(-) > > create mode 100644 include/libcamera/formats.h > > create mode 100644 include/libcamera/pixel_format.h > > delete mode 100644 include/libcamera/pixelformats.h > > rename src/libcamera/{pixelformats.cpp => pixel_format.cpp} (89%) > > > > -- > > Regards, > > > > Laurent Pinchart > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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. Patches 01/11 to 04/11 are preparatory cleanups, please see individual patches for details. I believe they make sense regardless of whether the rest is accepted or rejected. Patch 05/11 creates a new libcamera::formats:: namespace and populates it with constants (in the form of constexpr) for all supported pixel formats. The values have been written manually, which isn't ideal. We could partly automate this process by generating the header from a list of formats (likely in a YAML file), and fetching the numerical values from drm_fourcc.h to make sure they will always match. This could allow documenting the pixel formats in the YAML file and generating Doxygen documentation, like we do for controls. Patches 06/11 to 11/11 then replace usage of the DRM_FORMAT_* macros through the code, with one exception in the IPU3 pipeline handler to demonstrate how DRM_FORMAT_* values can still be used for internal formats. I however believe this will be dropped too, as applications may want to capture RAW data from the IPU3, so the format should likely be defined in the public API. Laurent Pinchart (11): libcamera: Rename pixelformats.{cpp,h} to pixel_format.{cpp,h} libcamera: Replace C++ comments with C comments libcamera: Rename header guards for internal headers libcamera: pixel_format: Make PixelFormat usable as a constexpr 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/control_ids.h.in | 4 +- include/libcamera/formats.h | 94 +++++++++++ .../libcamera/internal/byte_stream_buffer.h | 6 +- include/libcamera/internal/camera_controls.h | 6 +- include/libcamera/internal/camera_sensor.h | 6 +- .../libcamera/internal/control_serializer.h | 6 +- .../libcamera/internal/control_validator.h | 6 +- .../libcamera/internal/device_enumerator.h | 6 +- .../internal/device_enumerator_sysfs.h | 6 +- .../internal/device_enumerator_udev.h | 6 +- .../internal/event_dispatcher_poll.h | 6 +- include/libcamera/internal/file.h | 6 +- include/libcamera/internal/formats.h | 8 +- .../libcamera/internal/ipa_context_wrapper.h | 6 +- include/libcamera/internal/ipa_manager.h | 6 +- include/libcamera/internal/ipa_module.h | 6 +- include/libcamera/internal/ipa_proxy.h | 6 +- include/libcamera/internal/ipc_unixsocket.h | 6 +- include/libcamera/internal/log.h | 6 +- include/libcamera/internal/media_device.h | 6 +- include/libcamera/internal/media_object.h | 6 +- include/libcamera/internal/message.h | 6 +- include/libcamera/internal/pipeline_handler.h | 6 +- include/libcamera/internal/process.h | 6 +- include/libcamera/internal/pub_key.h | 6 +- include/libcamera/internal/semaphore.h | 6 +- include/libcamera/internal/thread.h | 6 +- include/libcamera/internal/utils.h | 6 +- include/libcamera/internal/v4l2_controls.h | 6 +- include/libcamera/internal/v4l2_device.h | 6 +- include/libcamera/internal/v4l2_pixelformat.h | 8 +- include/libcamera/internal/v4l2_subdevice.h | 6 +- include/libcamera/internal/v4l2_videodevice.h | 8 +- include/libcamera/meson.build | 3 +- include/libcamera/pixel_format.h | 48 ++++++ include/libcamera/pixelformats.h | 43 ----- include/libcamera/property_ids.h.in | 6 +- include/libcamera/stream.h | 2 +- src/gstreamer/gstlibcamera-utils.cpp | 62 ++++---- src/libcamera/formats.cpp | 148 +++++++++--------- src/libcamera/meson.build | 2 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +- .../pipeline/raspberrypi/raspberrypi.cpp | 10 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +-- src/libcamera/pipeline/simple/converter.h | 2 +- src/libcamera/pipeline/vimc/vimc.cpp | 11 +- .../{pixelformats.cpp => pixel_format.cpp} | 19 +-- src/libcamera/v4l2_pixelformat.cpp | 85 +++++----- src/qcam/dng_writer.cpp | 17 +- src/qcam/format_converter.cpp | 36 +++-- src/qcam/format_converter.h | 2 +- src/qcam/viewfinder.cpp | 10 +- src/qcam/viewfinder.h | 2 +- src/v4l2/v4l2_camera_proxy.cpp | 27 ++-- test/v4l2_videodevice/buffer_cache.cpp | 3 +- 55 files changed, 477 insertions(+), 378 deletions(-) create mode 100644 include/libcamera/formats.h create mode 100644 include/libcamera/pixel_format.h delete mode 100644 include/libcamera/pixelformats.h rename src/libcamera/{pixelformats.cpp => pixel_format.cpp} (89%)