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

3845 diff mbox series

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@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

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@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@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>
>  
>  /**
>

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@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@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>
> >  
> >  /**