[libcamera-devel] [PATCH/RFC 05/11] libcamera: Define constants for pixel formats in the public API

Thu May 28 11:56:33 CEST 2020

Hi Laurent,

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?

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.

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.

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.

>   * \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
--
Kieran