[libcamera-devel] [PATCH] libcamera: PixelFormat: Replace hex with fourcc and modifiers
Kaaira Gupta
kgupta at es.iitr.ac.in
Sat Apr 18 13:43:36 CEST 2020
On Wed, Apr 01, 2020 at 12:35:56PM +0100, Kieran Bingham wrote:
> Hi Kaaira,
>
> On 31/03/2020 22:05, 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>
> > ---
> > src/libcamera/pixelformats.cpp | 192 ++++++++++++++++++++++++++++++++-
> > test/meson.build | 1 +
> > test/pixel-format.cpp | 55 ++++++++++
> > 3 files changed, 245 insertions(+), 3 deletions(-)
> > create mode 100644 test/pixel-format.cpp
> >
> > diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> > index 87557d9..398dbb2 100644
> > --- a/src/libcamera/pixelformats.cpp
> > +++ b/src/libcamera/pixelformats.cpp
> > @@ -6,6 +6,8 @@
> > */
> >
> > #include <libcamera/pixelformats.h>
> > +#include <map>
> > +#include <string.h>
> >
> > /**
> > * \file pixelformats.h
> > @@ -108,9 +110,193 @@ 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_ == 0)
>
> As DRM_FORMAT_INVALID == 0, I'd express this as:
>
> 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;
> > +
>
> As we continually reference the 'first' modifier only, perhaps it's
> worth taking copy of it to a local var rather than referencing the
> *modifiers_.begin() each time.
>
> Or alternatively, convert modifiers_ from being a set to a single value
> before this patch.
Okay, I'll do this first :)
>
> > + if (*modifiers_.begin() == 72057594037927935) {
>
> Try to avoid magic numbers, What is 72057594037927935 ?
> If this is DRM_FORMAT_MOD_INVALID, then just use that directly.
>
> > + modifier = " <INVALID> modifier";
> > + return ss + modifier;
> > + }
> > +
> > + if (*modifiers_.begin() == 0xf) {
> > + modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_MASK";
> > + return ss + modifier;
>
>
> This 'mask' is not a value to express, but it is stating the 'mask'
> which can be used to extract the 'MOD_BLOCK_SIZE'.
I didn't understand you well, can you elaborate please?
>
> So you could express somethign like:
>
> uint8_t block_size = modifierValue & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK;
>
> But note that the AFBC modifiers appear to be ARM vendor specific, so
> they should be handled in that vendor code switch statement.
Oh, okay.
>
>
> > + } else if (*modifiers_.begin() == 1ULL) {
> > + modifier = "AFBC_FORMAT_MOD_BLOCK_SIZE_16x16";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == 2ULL) {
> > + modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == 3ULL) {
> > + modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_64x4";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == 4ULL) {
> > + modifier = " AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == (1ULL << 4)) {
> > + modifier = " AFBC_FORMAT_MOD_YTR";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == (1ULL << 5)) {
> > + modifier = " AFBC_FORMAT_MOD_SPLIT";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == (1ULL << 6)) {
> > + modifier = " AFBC_FORMAT_MOD_SPARS";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == (1ULL << 7)) {
> > + modifier = " AFBC_FORMAT_MOD_CBR";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == (1ULL << 8)) {
> > + modifier = " AFBC_FORMAT_MOD_TILED";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == (1ULL << 9)) {
> > + modifier = " AFBC_FORMAT_MOD_SC";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == (1ULL << 10)) {
> > + modifier = " AFBC_FORMAT_MOD_DB";
> > + return ss + modifier;
> > + } else if (*modifiers_.begin() == (1ULL << 11)) {
> > + modifier = " AFBC_FORMAT_MOD_BCH";
> > + return ss + modifier;
> > + }
>
>
> I suspect that we could simplify all of the conversions by using tables
> such as the vendor table below (populated with a similar macro to make
> sure the values and strings come directly from the defines given in
> drm_fourcc.h).
Again, I don't understand what exactly you want here? :3
Can you elaborate please?
>
> Would you be able to experiment with that a little to see if you can
> simplify the code?
>
>
> > +
> > + std::map<long int, std::string> vendors = {
>
> I would suggest we use the preprocessor to populate this table from the
> defines directly:
Yes
>
> #define PIXELFORMAT_VENDOR(vendor) \
> { DRM_FORMAT_MOD_VENDOR_## vendor, #vendor }
>
> std::map<long int, std::string> vendors = {
> PIXELFORMAT_VENDOR(NONE),
> PIXELFORMAT_VENDOR(INTEL),
> PIXELFORMAT_VENDOR(AMD),
> PIXELFORMAT_VENDOR(NVIDIA),
> ... and the rest ...
> }
>
>
> > + { 0, "NONE" },
> > + { 0x01, "INTEL" },
> > + { 0x02, "AMD" },
> > + { 0x03, "NVIDIA" },
> > + { 0x04, "SAMSUNG" },
> > + { 0x05, "QCOM" },
> > + { 0x06, "VIVANTE" },
> > + { 0x07, "BROADCOM" },
> > + { 0x08, "ARM" },
> > + { 0x09, "ALLWINNER" },
> > + { 0x0a, "MIPI" }
> > + };
> > +
> > + long int vendorCode = (*modifiers_.begin() >> 56) & 0xff;
> > +
> > + std::string vendor = vendors[vendorCode];
> > +
> > + int modifierValue = *modifiers_.begin() & 0xff;
>
> Some vendors can utilise more bits, I think the modifierValue should be
> a uint64_t, with the vendor bits masked out.
Yes you are right..I'll change this
>
>
> > + std::string vendorSpecification;
> > +
> > + switch (vendorCode) {
> > + case 0: {
>
> The switch statements should be more expressive, and not use magic
> numbers either:
Okay, will make the changes
>
> case DRM_FORMAT_MOD_VENDOR_NONE: {
>
>
> > + if (modifierValue == 0)
> > + vendorSpecification = "Linear Layout";
> > + break;
> > + }
> > + case 0x01: {
>
> 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 0x02: {
> > + // no specifications
> > + break;
> > + }
> > + case 0x03: {
> > + 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 0x04: {
> > + if (modifierValue == 1)
> > + vendorSpecification = "Tiled, 64 (pixels) x 32 (lines)";
> > + else if (modifierValue == 2)
> > + vendorSpecification = "Tiled, 16 (pixels) x 16 (lines)";
> > + break;
> > + }
> > + case 0x05: {
> > + if (modifierValue == 1)
> > + vendorSpecification = "Compressed";
> > + break;
> > + }
> > + case 0x06: {
> > + 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 0x07: {
> > + 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 0x08: {
> > + vendorSpecification = "DRM_FORMAT_MOD_ARM_AFBC(" + modifierValue;
> > + break;
> > + }
> > + case 0x09: {
> > + if (modifierValue == 1)
> > + vendorSpecification = "Tiled";
> > + break;
> > + }
> > + case 0x0a: {
> > + if (modifierValue == 1)
> > + vendorSpecification = "CSI-2 packed";
> > + break;
> > + }
> > + default:
> > + break;
> > + }
> > +
> > + modifier = vendor + ' ' + vendorSpecification;
> > + std::string formatString(ss);
> > +
> > + return formatString + ' ' + modifier;
> > }
> >
> > } /* 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..9676f27
> > --- /dev/null
> > +++ b/test/pixel-format.cpp
> > @@ -0,0 +1,55 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
>
> This is a new test being added, so the year is 2020, and I think you can
> own the copyright on files you create from scratch...
Okayy..I didn't know how this thing works, so I just copied it from a
file :P
>
> Copyright (C) 2020, Kaaira Gupta
>
> > + *
> > + * libcamera pixel format handling test
> > + */
> > +
> > +#include <iostream>
> > +#include <vector>
> > +
> > +#include "libcamera/pixelformats.h"
> > +#include "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 *>> formats{
> > + { PixelFormat(0, { DRM_FORMAT_MOD_INVALID }), "<INVALID>" },
> > + { PixelFormat(DRM_FORMAT_SRGGB8, { DRM_FORMAT_MOD_LINEAR }),
> > + "RGGB NONE Linear Layout" },
> > + { PixelFormat(DRM_FORMAT_C8,
> > + { DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(0) }),
> > + "C8 BROADCOM SAND32" },
> > + { PixelFormat(DRM_FORMAT_YUV410, { AFBC_FORMAT_MOD_SPLIT }),
> > + "YUV9 AFBC_FORMAT_MOD_SPLIT" },
> > + { PixelFormat(DRM_FORMAT_BIG_ENDIAN, { DRM_FORMAT_MOD_INVALID }),
> > + "....-BE <INVALID> modifier" }
> > + };
> > + 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)
> >
Thanks for the time!
>
> --
> Regards
> --
> Kieran
More information about the libcamera-devel
mailing list