[libcamera-devel] [PATCH/RFC 00/11] Introduce formats:: namespace for libcamera pixel formats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri May 22 22:47:10 CEST 2020
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%)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list