[libcamera-devel] [PATCH/RFC 05/11] libcamera: Define constants for pixel formats in the public API
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu May 28 13:35:24 CEST 2020
Hi Kieran,
On Thu, May 28, 2020 at 10:56:33AM +0100, Kieran Bingham wrote:
> On 22/05/2020 15:54, Laurent Pinchart wrote:
> > libcamera uses pixel format FourCC and modifier values from DRM. This
> > requires inclusion of drm_fourcc.h, creating a dependency on a header
> > that is packaged differently between distributions, and causing possible
> > issues with third-party applications.
> >
> > Define constants for the supported pixel formats in the public API, in
> > order to remove the dependency on drm_fourcc.h. The numerical values are
> > still identical to the DRM pixel formats, keeping the direct
> > interoperability.
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/formats.h | 94 ++++++++++++++++++++++++++++++++++
> > include/libcamera/meson.build | 1 +
> > src/libcamera/formats.cpp | 2 +-
> > src/libcamera/pixel_format.cpp | 1 +
> > 4 files changed, 97 insertions(+), 1 deletion(-)
> > create mode 100644 include/libcamera/formats.h
> >
> > diff --git a/include/libcamera/formats.h b/include/libcamera/formats.h
> > new file mode 100644
> > index 000000000000..bd164226d712
> > --- /dev/null
> > +++ b/include/libcamera/formats.h
> > @@ -0,0 +1,94 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * formats.h - Formats
> > + */
> > +#ifndef __LIBCAMERA_FORMATS_H__
> > +#define __LIBCAMERA_FORMATS_H__
> > +
> > +#include <stdint.h>
> > +
> > +#include <libcamera/pixel_format.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace formats {
> > +
> > +namespace {
> > +
> > +constexpr uint32_t __fourcc(char a, char b, char c, char d)
> > +{
> > + return (static_cast<uint32_t>(a) << 0) |
> > + (static_cast<uint32_t>(b) << 8) |
> > + (static_cast<uint32_t>(c) << 16) |
> > + (static_cast<uint32_t>(d) << 24);
> > +}
> > +
> > +constexpr uint64_t __mod(unsigned int vendor, unsigned int mod)
> > +{
> > + return (static_cast<uint64_t>(vendor) << 56) |
> > + (static_cast<uint64_t>(mod) << 0);
> > +}
> > +
> > +} /* namespace */
> > +
> > +constexpr PixelFormat R8{ __fourcc('R', '8', ' ', ' ') };
> > +
> > +constexpr PixelFormat RGB888{ __fourcc('R', 'G', '2', '4') };
> > +constexpr PixelFormat BGR888{ __fourcc('B', 'G', '2', '4') };
> > +
> > +constexpr PixelFormat XRGB8888{ __fourcc('X', 'R', '2', '4') };
> > +constexpr PixelFormat XBGR8888{ __fourcc('X', 'B', '2', '4') };
> > +constexpr PixelFormat RGBX8888{ __fourcc('R', 'X', '2', '4') };
> > +constexpr PixelFormat BGRX8888{ __fourcc('B', 'X', '2', '4') };
> > +
> > +constexpr PixelFormat ARGB8888{ __fourcc('A', 'R', '2', '4') };
> > +constexpr PixelFormat ABGR8888{ __fourcc('A', 'B', '2', '4') };
> > +constexpr PixelFormat RGBA8888{ __fourcc('R', 'A', '2', '4') };
> > +constexpr PixelFormat BGRA8888{ __fourcc('B', 'A', '2', '4') };
> > +
> > +constexpr PixelFormat YUYV{ __fourcc('Y', 'U', 'Y', 'V') };
> > +constexpr PixelFormat YVYU{ __fourcc('Y', 'V', 'Y', 'U') };
> > +constexpr PixelFormat UYVY{ __fourcc('U', 'Y', 'V', 'Y') };
> > +constexpr PixelFormat VYUY{ __fourcc('V', 'Y', 'U', 'Y') };
> > +
> > +constexpr PixelFormat NV12{ __fourcc('N', 'V', '1', '2') };
> > +constexpr PixelFormat NV21{ __fourcc('N', 'V', '2', '1') };
> > +constexpr PixelFormat NV16{ __fourcc('N', 'V', '1', '6') };
> > +constexpr PixelFormat NV61{ __fourcc('N', 'V', '6', '1') };
> > +constexpr PixelFormat NV24{ __fourcc('N', 'V', '2', '4') };
> > +constexpr PixelFormat NV42{ __fourcc('N', 'V', '4', '2') };
> > +
> > +constexpr PixelFormat MJPEG{ __fourcc('M', 'J', 'P', 'G') };
> > +
> > +constexpr PixelFormat SRGGB8{ __fourcc('R', 'G', 'G', 'B') };
> > +constexpr PixelFormat SGRBG8{ __fourcc('G', 'R', 'B', 'G') };
> > +constexpr PixelFormat SGBRG8{ __fourcc('G', 'B', 'R', 'G') };
> > +constexpr PixelFormat SBGGR8{ __fourcc('B', 'A', '8', '1') };
> > +
> > +constexpr PixelFormat SRGGB10{ __fourcc('R', 'G', '1', '0') };
> > +constexpr PixelFormat SGRBG10{ __fourcc('B', 'A', '1', '0') };
> > +constexpr PixelFormat SGBRG10{ __fourcc('G', 'B', '1', '0') };
> > +constexpr PixelFormat SBGGR10{ __fourcc('B', 'G', '1', '0') };
> > +
> > +constexpr PixelFormat SRGGB12{ __fourcc('R', 'G', '1', '2') };
> > +constexpr PixelFormat SGRBG12{ __fourcc('B', 'A', '1', '2') };
> > +constexpr PixelFormat SGBRG12{ __fourcc('G', 'B', '1', '2') };
> > +constexpr PixelFormat SBGGR12{ __fourcc('B', 'G', '1', '2') };
> > +
> > +constexpr PixelFormat SRGGB10_CSI2P{ __fourcc('R', 'G', '1', '0'), __mod(10, 1) };
> > +constexpr PixelFormat SGRBG10_CSI2P{ __fourcc('B', 'A', '1', '0'), __mod(10, 1) };
> > +constexpr PixelFormat SGBRG10_CSI2P{ __fourcc('G', 'B', '1', '0'), __mod(10, 1) };
> > +constexpr PixelFormat SBGGR10_CSI2P{ __fourcc('B', 'G', '1', '0'), __mod(10, 1) };
> > +
> > +constexpr PixelFormat SRGGB12_CSI2P{ __fourcc('R', 'G', '1', '2'), __mod(10, 1) };
> > +constexpr PixelFormat SGRBG12_CSI2P{ __fourcc('B', 'A', '1', '2'), __mod(10, 1) };
> > +constexpr PixelFormat SGBRG12_CSI2P{ __fourcc('G', 'B', '1', '2'), __mod(10, 1) };
> > +constexpr PixelFormat SBGGR12_CSI2P{ __fourcc('B', 'G', '1', '2'), __mod(10, 1) };
>
> Are these modifiers upstream yet?
Not yet, no.
> Automating the generation of this file would indeed be benefical I think
> (as discussed offline) - but overall - as much as I fear it becoming
> 'yet another pixelformat standard' I think this is a good idea.
I'm more and more convinced this header should be generated (at least
until we decide to create our own formats standard :-)). I'll give it a
try.
> At least if we generate from the DRM formats, then really it's just a
> namespace/simplification - and I like that a lot.
>
> In particular I like that libcamera applications who have no care for
> DRM at all don't have to use DRM prefixes (or include an 'arbitrary'
> system header).
>
> > +
> > +} /* namespace formats */
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_FORMATS_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 73c5b999acf4..170744d0fbbc 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -9,6 +9,7 @@ libcamera_public_headers = files([
> > 'event_dispatcher.h',
> > 'event_notifier.h',
> > 'file_descriptor.h',
> > + 'formats.h',
> > 'framebuffer_allocator.h',
> > 'geometry.h',
> > 'logging.h',
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 2ac3b412ecdb..74c239a5e710 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -12,7 +12,7 @@
> > #include "libcamera/internal/log.h"
> >
> > /**
> > - * \file formats.h
> > + * \file internal/formats.h
>
> It might be useful to explain in the commit message that you are fixing
> the src/libcamera/formats.cpp doxygen tag for the header to the internal
> location. At first I thought this was an erroneous hunk in this patch.
It's actually not really a fix (I would have split it in a separate
patch otherwise), Doxygen doesn't need full paths, but now that we have
a formats.h header in include/libcamera/ and another one in
include/libcamera/internal/, Doxygen needs more information. I'll
mention this in the commit message.
> Otherwise,
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> But maybe that's a moot tag if you're going to automate this file
> generation instead. But that could also be done on top - and this could
> just be manual to progress the fix.
Let's see if auto-generation will get implemented quickly or if the
series will be deemed ready for merge before that.
> > * \brief Types and helper methods to handle libcamera image formats
> > */
> >
> > diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
> > index d501c5f09c6b..0709d1526b82 100644
> > --- a/src/libcamera/pixel_format.cpp
> > +++ b/src/libcamera/pixel_format.cpp
> > @@ -5,6 +5,7 @@
> > * pixel_format.cpp - libcamera Pixel Format
> > */
> >
> > +#include <libcamera/formats.h>
> > #include <libcamera/pixel_format.h>
> >
> > /**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list