[libcamera-devel] [PATCH v2] libcamera: PixelFormat: Replace hex with fourcc and modifiers
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 21 14:35:02 CEST 2020
Hi Kaaira,
On 04/04/2020 15:31, Kaaira Gupta wrote:
> Print fourCC characters instead of the hex values in toString() as they
> are easier to comprehend. Also, print the corresponding modifiers of the
> DRM formats so that it can be more specific.
>
> Write the tests for them as well.
>
> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> ---
>
> 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.
>
> src/libcamera/pixelformats.cpp | 197 ++++++++++++++++++++++++++++++++-
> test/meson.build | 1 +
> test/pixel-format.cpp | 49 ++++++++
> 3 files changed, 244 insertions(+), 3 deletions(-)
> create mode 100644 test/pixel-format.cpp
>
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index 1330dc5..81159ee 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -6,6 +6,17 @@
> */
>
> #include <libcamera/pixelformats.h>
> +#include <map>
> +#include <string.h>
> +
> +#define PIXELFORMAT_VENDOR(vendor) \
> + { DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }
> +
> +#define AFBC_FORMAT_MOD_BLOCK(afbcBlock) \
> + { AFBC_FORMAT_MOD_## afbcBlock, #afbcBlock }
> +
> +#define AFBC_FORMAT_MOD(modifier, mod) \
> + { modifier & (AFBC_FORMAT_MOD_## mod), #mod }
I'm sorry - as it's likely my fault, but I think we went a little too
deep here.
The only vendor code modifier we are likely to use is the
DRM_FORMAT_MOD_VENDOR_MIPI, so we probably want to greatly simplify this
conversion and only handle that vendor code.
We should still print the other vendor codes if they are given as a
hint, but theres no need to decode those directly.
>
> /**
> * \file pixelformats.h
> @@ -108,9 +119,189 @@ bool PixelFormat::operator<(const PixelFormat &other) const
> */
> std::string PixelFormat::toString() const
> {
> - char str[11];
> - snprintf(str, 11, "0x%08x", fourcc_);
> - return str;
> + if (fourcc_ == DRM_FORMAT_INVALID)
> + return "<INVALID>";
> +
> + char ss[8] = { static_cast<char>(fourcc_ & 0x7f),
> + static_cast<char>((fourcc_ >> 8) & 0x7f),
> + static_cast<char>((fourcc_ >> 16) & 0x7f),
> + static_cast<char>((fourcc_ >> 24) & 0x7f) };
> +
> + for (unsigned int i = 0; i < 4; i++) {
> + if (!isprint(ss[i]))
> + ss[i] = '.';
> + }
> +
> + if (fourcc_ & (1 << 31))
> + strcat(ss, "-BE");
> +
> + std::string modifier;
> +
> + if (modifier_ == DRM_FORMAT_MOD_INVALID) {
> + modifier = " - <INVALID> modifier";
> + return ss + modifier;
> + }
> +
> + /* Map of all the vendors with their Ids: */
> + std::map<long int, std::string> vendors = {
> + PIXELFORMAT_VENDOR(NONE),
> + PIXELFORMAT_VENDOR(INTEL),
> + PIXELFORMAT_VENDOR(AMD),
> + PIXELFORMAT_VENDOR(NVIDIA),
> + PIXELFORMAT_VENDOR(SAMSUNG),
> + PIXELFORMAT_VENDOR(QCOM),
> + PIXELFORMAT_VENDOR(VIVANTE),
> + PIXELFORMAT_VENDOR(BROADCOM),
> + PIXELFORMAT_VENDOR(ARM),
> + PIXELFORMAT_VENDOR(ALLWINNER),
> + PIXELFORMAT_VENDOR(MIPI)
> + };
> +
> + std::map<long int, std::string> afbc_format_blocks = {
> + AFBC_FORMAT_MOD_BLOCK(BLOCK_SIZE_16x16),
> + AFBC_FORMAT_MOD_BLOCK(BLOCK_SIZE_32x8),
> + AFBC_FORMAT_MOD_BLOCK(BLOCK_SIZE_64x4),
> + AFBC_FORMAT_MOD_BLOCK(BLOCK_SIZE_32x8_64x4),
> +
> + };
> +
> + std::map<long int, std::string> afbc_format_mod = {
> + AFBC_FORMAT_MOD(modifier_, YTR),
> + AFBC_FORMAT_MOD(modifier_, SPLIT),
> + AFBC_FORMAT_MOD(modifier_, SPARSE),
> + AFBC_FORMAT_MOD(modifier_, CBR),
> + AFBC_FORMAT_MOD(modifier_, TILED),
> + AFBC_FORMAT_MOD(modifier_, SC),
> + AFBC_FORMAT_MOD(modifier_, DB),
> + AFBC_FORMAT_MOD(modifier_, BCH),
> + };
> +
> + /* Get the vendor name using its Id */
> + long int vendorCode = (modifier_ >> 56) & 0xff;
> + std::string vendor = vendors[vendorCode];
> +
> + uint64_t modifierValue = modifier_ & 0xff;
> + std::string vendorSpecification;
> +
> + switch (vendorCode) {
> + case DRM_FORMAT_MOD_VENDOR_NONE: {
> + if (modifierValue == 0)
> + vendorSpecification = "Linear Layout";
> + break;
> + }
We'd keep DRM_FORMAT_MOD_VENDOR_NONE of course ;-)
> + case DRM_FORMAT_MOD_VENDOR_INTEL: {
> + if (modifierValue == 1)
> + vendorSpecification = "X-tiled";
> + else if (modifierValue == 2)
> + vendorSpecification = "Y-tiled";
> + else if (modifierValue == 3)
> + vendorSpecification = "Yf-tiled";
> + else if (modifierValue == 4)
> + vendorSpecification = "Y-tiled CCS";
> + else if (modifierValue == 5)
> + vendorSpecification = "Yf-tiled CSS";
> + else if (modifierValue == 8)
> + vendorSpecification = "Bayer packed";
> + break;
> + }
> + case DRM_FORMAT_MOD_VENDOR_AMD: {
> + // no specifications
> + break;
> + }
> + case DRM_FORMAT_MOD_VENDOR_NVIDIA: {
> + if (modifierValue == 1)
> + vendorSpecification = "Tegra-tiled";
> + else if (modifierValue == 0x10)
> + vendorSpecification = "ONE_GOB | v = 0";
> + else if (modifierValue == 0x11)
> + vendorSpecification = "TWO_GOB | v = 1";
> + else if (modifierValue == 0x12)
> + vendorSpecification = "FOUR_GOB | v = 2";
> + else if (modifierValue == 0x13)
> + vendorSpecification = "EIGHT_GOB | v = 3";
> + else if (modifierValue == 0x14)
> + vendorSpecification = "SIXTEEN_GOB | v = 4";
> + else if (modifierValue == 0x15)
> + vendorSpecification = "THIRTYTWO_GOB | v = 5";
> + break;
> + }
> + case DRM_FORMAT_MOD_VENDOR_SAMSUNG: {
> + if (modifierValue == 1)
> + vendorSpecification = "Tiled, 64 (pixels) x 32 (lines)";
> + else if (modifierValue == 2)
> + vendorSpecification = "Tiled, 16 (pixels) x 16 (lines)";
> + break;
> + }
> + case DRM_FORMAT_MOD_VENDOR_QCOM: {
> + if (modifierValue == 1)
> + vendorSpecification = "Compressed";
> + break;
> + }
> + case DRM_FORMAT_MOD_VENDOR_VIVANTE: {
> + if (modifierValue == 1)
> + vendorSpecification = "4 x 4 tiled";
> + else if (modifierValue == 2)
> + vendorSpecification = "64 x 64 super-tiled";
> + else if (modifierValue == 3)
> + vendorSpecification = "4 x 4 split-tiled";
> + else if (modifierValue == 4)
> + vendorSpecification = "64 x 64 split-super-tiled";
> + break;
> + }
> + case DRM_FORMAT_MOD_VENDOR_BROADCOM: {
> + if (modifierValue == 1)
> + vendorSpecification = "VC4-T-Tiled";
> + else if (modifierValue == 6)
> + vendorSpecification = "UIF";
> + else if (modifierValue == 2)
> + vendorSpecification = "SAND32";
> + else if (modifierValue == 3)
> + vendorSpecification = "SAND64";
> + else if (modifierValue == 4)
> + vendorSpecification = "SAND128";
> + else if (modifierValue == 5)
> + vendorSpecification = "SAND256";
> + break;
> + }
> + case DRM_FORMAT_MOD_VENDOR_ARM: {
> + uint8_t block_size = modifier_ & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;
> +
> + if (afbc_format_blocks.count(block_size)) {
> + vendorSpecification = afbc_format_blocks[block_size] + " | ";
> + };
> +
> + std::map<long int, std::string>::iterator it;
> + for (it = afbc_format_mod.begin(); it != afbc_format_mod.end(); it++) {
> + if (it->first != 0) {
> + vendorSpecification += it->second + " | ";
> + }
> + }
> + break;
> + }
> + case DRM_FORMAT_MOD_VENDOR_ALLWINNER: {
> + if (modifierValue == 1)
> + vendorSpecification = "Tiled";
> + break;
> + }
So for all the other vendor, printing just the hex value should be fine,
as a default catch all (and it would be in the default case statement below)
> + case DRM_FORMAT_MOD_VENDOR_MIPI: {
> + if (modifierValue == 1)
> + vendorSpecification = "CSI-2 packed";
> + break;
> + }
And we should keep that one, but using modifierValue == 1 is a 'magic
value', so we should get that directly from the modifier code. We might
need to use a bitfield mask.
> + default:
> + break;
> + }
> +
> + modifier = vendor + " : " + vendorSpecification;
> + std::string formatString(ss);
> + formatString = formatString + " - " + modifier;
> +
> + /* Trim last three characters of the string if last char is a space. */
> + if (isspace(formatString.back())) {
> + formatString = formatString.substr(0, formatString.size() - 3);
> + }
Try to see if you can prevent adding the characters in the first place,
so they don't need to be trimmed back.
> +
> + return formatString;
> }
>
> } /* namespace libcamera */
> diff --git a/test/meson.build b/test/meson.build
> index 8ab58ac..3f97278 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -30,6 +30,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..55cd916
> --- /dev/null
> +++ b/test/pixel-format.cpp
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Kaaira Gupta
> + *
> + * libcamera pixel format handling test
> + */
> +
> +#include <iostream>
> +#include <vector>
> +
> +#include "libcamera/pixelformats.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class PixelFormatTest : public Test
> +{
> +protected:
> + int run()
> + {
> + std::vector<std::pair<PixelFormat, const char *>> formats{
> + { PixelFormat(DRM_FORMAT_SRGGB8, DRM_FORMAT_MOD_INVALID), "RGGB - <INVALID> modifier" },
> + { PixelFormat(DRM_FORMAT_SRGGB8, DRM_FORMAT_MOD_LINEAR), "RGGB - NONE : Linear Layout" },
> + { PixelFormat(DRM_FORMAT_C8, DRM_FORMAT_MOD_ARM_AFBC(4)), "C8 - ARM : BLOCK_SIZE_32x8_64x4" },
> + { PixelFormat(DRM_FORMAT_YUV410, DRM_FORMAT_MOD_ARM_AFBC(4|1ULL << 11)), "YUV9 - ARM : BLOCK_SIZE_32x8_64x4 | BCH"},
> + { PixelFormat(DRM_FORMAT_BIG_ENDIAN, DRM_FORMAT_MOD_ALLWINNER_TILED), "....-BE - ALLWINNER : Tiled" }
> + };
> + for (const auto &format : formats) {
> + if ((format.first).toString() != format.second) {
> + cerr << "Failed to convert PixelFormat "
> + << 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
--
Kieran
More information about the libcamera-devel
mailing list