[libcamera-devel] [PATCH v6] libcamera:PixelFormat: Replace hex with format names
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 23 01:40:00 CEST 2020
Hi Kaaira,
Thank you for the patch.
One small comment, the subject line should be
libcamera: pixel_format: Replace hex with format names
(space after libcamera:, and following the convention to use file names
instead of classes).
On Mon, Jun 22, 2020 at 04:33:51PM +0530, Kaaira Gupta wrote:
> Print format names defined in formats namespace instead of the hex
> values in toString() as they are easier to comprehend. For this add
> a property of 'name' in PixelFormatInfo' so as to map the formats
> with their names. Print fourcc for formats which are not used in
> libcamera.
>
> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Resending due to typo in mailing list address.
>
> Changes since v5:
> - formatting changes.
> - based on top of [PATCH v2] libcamera: add support
> for planar YUV422 and YUV420 formats
I was ready to have to fix this, and to my surprise the compiler didn't
complain. Very good initiative :-)
> Changes since v4:
> -Print libcamera defined names instead of fourcc.
>
> Changes since v3:
> -shortened the texts.
> -Removed default case as well.
> -changed commit message and tests to reflect the changes.
>
> Changes since v2:
> - Remove description for all vendors except for MIPI
> - Change commit message to reflect this change.
> - Change tests accordingly.
>
> Changes since v1:
> - Replaced magic numbers with expressive values.
> - Re-wrote ARM vendor's modifiers
> - Re-wrote the vendors' map with a macro.
> - Changed the copyrights in test file.
> - Changed the tests.
>
> include/libcamera/internal/formats.h | 1 +
> src/libcamera/formats.cpp | 42 +++++++++++++++++++++--
> src/libcamera/pixel_format.cpp | 27 +++++++++++++--
> test/meson.build | 1 +
> test/pixel-format.cpp | 51 ++++++++++++++++++++++++++++
> 5 files changed, 117 insertions(+), 5 deletions(-)
> create mode 100644 test/pixel-format.cpp
>
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index 4b172ef..f59ac8f 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -46,6 +46,7 @@ public:
> static const PixelFormatInfo &info(const PixelFormat &format);
>
> /* \todo Add support for non-contiguous memory planes */
> + const char *name;
This field isn't documented. Hasn't doxygen warned you ?
I've added
* \var PixelFormatInfo::name
* \brief The format name as a human-readable string, used as the test
* representation of the PixelFormat
I'll push the patch tomorrow with those small changes unless someone has
further comments.
> PixelFormat format;
> V4L2PixelFormat v4l2Format;
> unsigned int bitsPerPixel;
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index c0b53ce..9de1434 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -169,6 +169,7 @@ namespace {
> const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> /* RGB formats. */
> { formats::BGR888, {
> + .name = "BGR888",
> .format = formats::BGR888,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),
> .bitsPerPixel = 24,
> @@ -176,6 +177,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::RGB888, {
> + .name = "RGB888",
> .format = formats::RGB888,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGR24),
> .bitsPerPixel = 24,
> @@ -183,6 +185,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::ABGR8888, {
> + .name = "ABGR8888",
> .format = formats::ABGR8888,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGBA32),
> .bitsPerPixel = 32,
> @@ -190,6 +193,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::ARGB8888, {
> + .name = "ARGB8888",
> .format = formats::ARGB8888,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ABGR32),
> .bitsPerPixel = 32,
> @@ -197,6 +201,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::BGRA8888, {
> + .name = "BGRA8888",
> .format = formats::BGRA8888,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_ARGB32),
> .bitsPerPixel = 32,
> @@ -204,6 +209,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::RGBA8888, {
> + .name = "RGBA8888",
> .format = formats::RGBA8888,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_BGRA32),
> .bitsPerPixel = 32,
> @@ -213,6 +219,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>
> /* YUV packed formats. */
> { formats::YUYV, {
> + .name = "YUYV",
> .format = formats::YUYV,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUYV),
> .bitsPerPixel = 16,
> @@ -220,6 +227,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::YVYU, {
> + .name = "YVYU",
> .format = formats::YVYU,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YVYU),
> .bitsPerPixel = 16,
> @@ -227,6 +235,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::UYVY, {
> + .name = "UYVY",
> .format = formats::UYVY,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_UYVY),
> .bitsPerPixel = 16,
> @@ -234,6 +243,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::VYUY, {
> + .name = "VYUY",
> .format = formats::VYUY,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_VYUY),
> .bitsPerPixel = 16,
> @@ -243,6 +253,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>
> /* YUV planar formats. */
> { formats::NV16, {
> + .name = "NV16",
> .format = formats::NV16,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV16),
> .bitsPerPixel = 16,
> @@ -250,6 +261,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::NV61, {
> + .name = "NV61",
> .format = formats::NV61,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV61),
> .bitsPerPixel = 16,
> @@ -257,6 +269,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::NV12, {
> + .name = "NV12",
> .format = formats::NV12,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV12),
> .bitsPerPixel = 12,
> @@ -264,6 +277,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::NV21, {
> + .name = "NV21",
> .format = formats::NV21,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_NV21),
> .bitsPerPixel = 12,
> @@ -271,6 +285,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::YUV420, {
> + .name = "YUV420",
> .format = PixelFormat(formats::YUV420),
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV420),
> .bitsPerPixel = 12,
> @@ -278,6 +293,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::YUV422, {
> + .name = "YUV422",
> .format = PixelFormat(formats::YUV422),
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV422P),
> .bitsPerPixel = 16,
> @@ -287,6 +303,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>
> /* Greyscale formats. */
> { formats::R8, {
> + .name = "R8",
> .format = formats::R8,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_GREY),
> .bitsPerPixel = 8,
> @@ -296,6 +313,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>
> /* Bayer formats. */
> { formats::SBGGR8, {
> + .name = "SBGGR8",
> .format = formats::SBGGR8,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8),
> .bitsPerPixel = 8,
> @@ -303,6 +321,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SGBRG8, {
> + .name = "SGBRG8",
> .format = formats::SGBRG8,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8),
> .bitsPerPixel = 8,
> @@ -310,6 +329,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SGRBG8, {
> + .name = "SGRBG8",
> .format = formats::SGRBG8,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8),
> .bitsPerPixel = 8,
> @@ -317,6 +337,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SRGGB8, {
> + .name = "SRGGB8",
> .format = formats::SRGGB8,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8),
> .bitsPerPixel = 8,
> @@ -324,6 +345,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SBGGR10, {
> + .name = "SBGGR10",
> .format = formats::SBGGR10,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10),
> .bitsPerPixel = 10,
> @@ -331,6 +353,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SGBRG10, {
> + .name = "SGBRG10",
> .format = formats::SGBRG10,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10),
> .bitsPerPixel = 10,
> @@ -338,6 +361,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SGRBG10, {
> + .name = "SGRBG10",
> .format = formats::SGRBG10,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10),
> .bitsPerPixel = 10,
> @@ -345,6 +369,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SRGGB10, {
> + .name = "SRGGB10",
> .format = formats::SRGGB10,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10),
> .bitsPerPixel = 10,
> @@ -352,6 +377,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SBGGR10_CSI2P, {
> + .name = "SBGGR10_CSI2P",
> .format = formats::SBGGR10_CSI2P,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P),
> .bitsPerPixel = 10,
> @@ -359,6 +385,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = true,
> } },
> { formats::SGBRG10_CSI2P, {
> + .name = "SGBRG10_CSI2P",
> .format = formats::SGBRG10_CSI2P,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P),
> .bitsPerPixel = 10,
> @@ -366,6 +393,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = true,
> } },
> { formats::SGRBG10_CSI2P, {
> + .name = "SGRBG10_CSI2P",
> .format = formats::SGRBG10_CSI2P,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P),
> .bitsPerPixel = 10,
> @@ -373,6 +401,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = true,
> } },
> { formats::SRGGB10_CSI2P, {
> + .name = "SRGGB10_CSI2P",
> .format = formats::SRGGB10_CSI2P,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P),
> .bitsPerPixel = 10,
> @@ -380,6 +409,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = true,
> } },
> { formats::SBGGR12, {
> + .name = "SBGGR12",
> .format = formats::SBGGR12,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12),
> .bitsPerPixel = 12,
> @@ -387,6 +417,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SGBRG12, {
> + .name = "SGBRG12",
> .format = formats::SGBRG12,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12),
> .bitsPerPixel = 12,
> @@ -394,6 +425,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SGRBG12, {
> + .name = "SGRBG12",
> .format = formats::SGRBG12,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12),
> .bitsPerPixel = 12,
> @@ -401,6 +433,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SRGGB12, {
> + .name = "SRGGB12",
> .format = formats::SRGGB12,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12),
> .bitsPerPixel = 12,
> @@ -408,6 +441,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = false,
> } },
> { formats::SBGGR12_CSI2P, {
> + .name = "SBGGR12_CSI2P",
> .format = formats::SBGGR12_CSI2P,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P),
> .bitsPerPixel = 12,
> @@ -415,6 +449,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = true,
> } },
> { formats::SGBRG12_CSI2P, {
> + .name = "SGBRG12_CSI2P",
> .format = formats::SGBRG12_CSI2P,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P),
> .bitsPerPixel = 12,
> @@ -422,6 +457,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = true,
> } },
> { formats::SGRBG12_CSI2P, {
> + .name = "SGRBG12_CSI2P",
> .format = formats::SGRBG12_CSI2P,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P),
> .bitsPerPixel = 12,
> @@ -429,6 +465,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> .packed = true,
> } },
> { formats::SRGGB12_CSI2P, {
> + .name = "SRGGB12_CSI2P",
> .format = formats::SRGGB12_CSI2P,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P),
> .bitsPerPixel = 12,
> @@ -438,6 +475,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>
> /* Compressed formats. */
> { formats::MJPEG, {
> + .name = "MJPEG",
> .format = formats::MJPEG,
> .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> .bitsPerPixel = 0,
> @@ -467,8 +505,8 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
> const auto iter = pixelFormatInfo.find(format);
> if (iter == pixelFormatInfo.end()) {
> LOG(Formats, Warning)
> - << "Unsupported pixel format "
> - << format.toString();
> + << "Unsupported pixel format 0x"
> + << utils::hex(format.fourcc());
> return invalid;
> }
>
> diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
> index f191851..14addb5 100644
> --- a/src/libcamera/pixel_format.cpp
> +++ b/src/libcamera/pixel_format.cpp
> @@ -8,6 +8,8 @@
> #include <libcamera/formats.h>
> #include <libcamera/pixel_format.h>
>
> +#include "libcamera/internal/formats.h"
> +
> /**
> * \file pixel_format.h
> * \brief libcamera pixel format
> @@ -104,9 +106,28 @@ bool PixelFormat::operator<(const PixelFormat &other) const
> */
> std::string PixelFormat::toString() const
> {
> - char str[11];
> - snprintf(str, 11, "0x%08x", fourcc_);
> - return str;
> + const PixelFormatInfo &info = PixelFormatInfo::info(*this);
> +
> + if (!info.isValid()) {
> + if (*this == PixelFormat())
> + return "<INVALID>";
> +
> + char fourcc[7] = { '<',
> + static_cast<char>(fourcc_),
> + static_cast<char>(fourcc_ >> 8),
> + static_cast<char>(fourcc_ >> 16),
> + static_cast<char>(fourcc_ >> 24),
> + '>' };
> +
> + for (unsigned int i = 1; i < 5; i++) {
> + if (!isprint(fourcc[i]))
> + fourcc[i] = '.';
> + }
> +
> + return fourcc;
> + }
> +
> + return info.name;
> }
>
> } /* namespace libcamera */
> diff --git a/test/meson.build b/test/meson.build
> index a868813..7808a26 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -34,6 +34,7 @@ internal_tests = [
> ['message', 'message.cpp'],
> ['object', 'object.cpp'],
> ['object-invoke', 'object-invoke.cpp'],
> + ['pixel-format', 'pixel-format.cpp'],
> ['signal-threads', 'signal-threads.cpp'],
> ['threads', 'threads.cpp'],
> ['timer', 'timer.cpp'],
> diff --git a/test/pixel-format.cpp b/test/pixel-format.cpp
> new file mode 100644
> index 0000000..c4a08f4
> --- /dev/null
> +++ b/test/pixel-format.cpp
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Kaaira Gupta
> + * libcamera pixel format handling test
> + */
> +
> +#include <iostream>
> +#include <vector>
> +
> +#include <libcamera/formats.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "libcamera/internal/utils.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class PixelFormatTest : public Test
> +{
> +protected:
> + int run()
> + {
> + std::vector<std::pair<PixelFormat, const char *>> formatsMap{
> + { formats::R8, "R8" },
> + { formats::SRGGB10_CSI2P, "SRGGB10_CSI2P" },
> + { PixelFormat(0, 0), "<INVALID>" },
> + { PixelFormat(0x20203843), "<C8 >" }
> + };
> +
> + for (const auto &format : formatsMap) {
> + if ((format.first).toString() != format.second) {
> + cerr << "Failed to convert PixelFormat "
> + << utils::hex(format.first.fourcc()) << " to string"
> + << endl;
> + return TestFail;
> + }
> + }
> +
> + if (PixelFormat().toString() != "<INVALID>") {
> + cerr << "Failed to convert default PixelFormat to string"
> + << endl;
> + return TestFail;
> + }
> +
> + return TestPass;
> + }
> +};
> +
> +TEST_REGISTER(PixelFormatTest)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list