[{"id":3856,"web_url":"https://patchwork.libcamera.org/comment/3856/","msgid":"<20200229105718.GC18738@pendragon.ideasonboard.com>","date":"2020-02-29T10:57:18","subject":"Re: [libcamera-devel] [RFC 4/6] libcamera: PixelFormat: Add\n\toperations to operate on names","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Feb 28, 2020 at 04:29:11AM +0100, Niklas Söderlund wrote:\n> Add a name to each pixel format and extend PixelFormat to be constructed\n> from a name and to retrieve the name for printing. Make use of the new\n> functionality to demonstrate it.\n> \n> - Update the cam utility to read and print the pixel format name instead\n>   of the fourcc integer number.\n> \n> - Update log messages to print the name instead of the fourcc integer\n>   number.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/pixelformats.h    |  3 ++\n>  src/cam/main.cpp                    |  9 ++---\n>  src/libcamera/pipeline/uvcvideo.cpp |  4 +-\n>  src/libcamera/pixelformats.cpp      | 58 ++++++++++++++++++++---------\n>  src/libcamera/stream.cpp            |  2 +-\n>  5 files changed, 51 insertions(+), 25 deletions(-)\n> \n> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> index f0951e983192d5e8..8cea3a90ef2cc7ae 100644\n> --- a/include/libcamera/pixelformats.h\n> +++ b/include/libcamera/pixelformats.h\n> @@ -24,6 +24,7 @@ public:\n>  \tPixelFormat();\n>  \tPixelFormat(const PixelFormat &other);\n>  \texplicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers);\n> +\texplicit PixelFormat(const std::string &name);\n\nShouldn't this take modifiers too ?\n\n>  \tPixelFormat &operator=(const PixelFormat &other);\n>  \n> @@ -31,6 +32,7 @@ public:\n>  \tbool operator!=(const PixelFormat &other) const;\n>  \tbool operator<(const PixelFormat &other) const;\n>  \n> +\tconst std::string &name() const;\n>  \tuint32_t v4l2() const;\n>  \tuint32_t fourcc() const;\n>  \tconst std::set<uint32_t> &modifiers() const;\n> @@ -47,6 +49,7 @@ protected:\n>  private:\n>  \tconst PixelFormatEntry *fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const;\n>  \tconst PixelFormatEntry *fromV4L2(uint32_t v4l2_fourcc) const;\n> +\tconst PixelFormatEntry *fromName(const std::string &name) const;\n>  \n>  \tconst PixelFormatEntry *format_;\n>  };\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index c8ef79daea37d8b6..0a08e362294fc9ee 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -159,7 +159,7 @@ int CamApp::parseOptions(int argc, char *argv[])\n>  \t\t\t\t ArgumentRequired);\n>  \tstreamKeyValue.addOption(\"height\", OptionInteger, \"Height in pixels\",\n>  \t\t\t\t ArgumentRequired);\n> -\tstreamKeyValue.addOption(\"pixelformat\", OptionInteger, \"Pixel format\",\n> +\tstreamKeyValue.addOption(\"pixelformat\", OptionString, \"Pixel format\",\n>  \t\t\t\t ArgumentRequired);\n>  \n>  \tOptionsParser parser;\n> @@ -247,9 +247,9 @@ int CamApp::prepareConfig()\n>  \t\t\tif (opt.isSet(\"height\"))\n>  \t\t\t\tcfg.size.height = opt[\"height\"];\n>  \n> -\t\t\t/* TODO: Translate 4CC string to ID. */\n>  \t\t\tif (opt.isSet(\"pixelformat\"))\n> -\t\t\t\tcfg.pixelFormat = PixelFormat(opt[\"pixelformat\"], {});\n> +\t\t\t\tcfg.pixelFormat =\n> +\t\t\t\t\tPixelFormat(opt[\"pixelformat\"].toString());\n>  \t\t}\n>  \t}\n>  \n> @@ -282,8 +282,7 @@ int CamApp::infoConfiguration()\n>  \n>  \t\tconst StreamFormats &formats = cfg.formats();\n>  \t\tfor (PixelFormat pixelformat : formats.pixelformats()) {\n> -\t\t\tstd::cout << \" * Pixelformat: 0x\" << std::hex\n> -\t\t\t\t  << std::setw(8) << pixelformat.fourcc() << \" \"\n> +\t\t\tstd::cout << \" * Pixelformat: \" << pixelformat.name() << \" \"\n>  \t\t\t\t  << formats.range(pixelformat).toString()\n>  \t\t\t\t  << std::endl;\n\nWould it be useful to print both the name and the hex fourcc value ?\nWhen debugging we often see the numerical value, being able to compare\nit with printed messages could be interesting. We could add a toString()\nto PixelFormat to combine the two:\n\nstd::string PixeFormat::toString()\n{\n\tstd::stringstream ss;\n\tss << name() << \" (\" << utils::hex(fourcc(), 8) << \")\";\n\treturn ss.str();\n}\n\nThis could be extended to also take modifiers into account.\n\nWe don't necessarily need to print both everywhere, but in the cam tool\nit may be useful.\n\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 5f3e52f691aaeae4..75fbfe1eb9145424 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -115,8 +115,8 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n>  \tif (iter == pixelFormats.end()) {\n>  \t\tcfg.pixelFormat = pixelFormats.front();\n>  \t\tLOG(UVC, Debug)\n> -\t\t\t<< \"Adjusting pixel format from \" << pixelFormat.fourcc()\n> -\t\t\t<< \" to \" << cfg.pixelFormat.fourcc();\n> +\t\t\t<< \"Adjusting pixel format from \" << pixelFormat.name()\n> +\t\t\t<< \" to \" << cfg.pixelFormat.name();\n>  \t\tstatus = Adjusted;\n>  \t}\n>  \n> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp\n> index b2aacbc39b9ca16a..70f41d86a23baceb 100644\n> --- a/src/libcamera/pixelformats.cpp\n> +++ b/src/libcamera/pixelformats.cpp\n> @@ -35,6 +35,7 @@ LOG_DEFINE_CATEGORY(PixelFormats)\n>   */\n>  \n>  struct PixelFormatEntry {\n> +\tstd::string name;\n\nWould const char * be a bit more efficient ?\n\n>  \tuint32_t v4l2;\n>  \tuint32_t drm;\n>  \tstd::set<uint32_t> modifiers;\n> @@ -42,27 +43,27 @@ struct PixelFormatEntry {\n>  \n>  static const std::vector<PixelFormatEntry> pixelFormats = {\n>  \t/* Invalid format, important to be first in list. */\n> -\t{ .v4l2 = 0,\t\t\t.drm = DRM_FORMAT_INVALID,\t.modifiers = {} },\n> +\t{ .name = \"INVALID\",\t.v4l2 = 0,\t\t\t.drm = DRM_FORMAT_INVALID,\t.modifiers = {} },\n>  \t/* RGB formats. */\n> -\t{ .v4l2 = V4L2_PIX_FMT_RGB24,\t.drm = DRM_FORMAT_BGR888,\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_BGR24,\t.drm = DRM_FORMAT_RGB888,\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_ARGB32,\t.drm = DRM_FORMAT_BGRA8888,\t.modifiers = {} },\n> +\t{ .name = \"RGR888\",\t.v4l2 = V4L2_PIX_FMT_RGB24,\t.drm = DRM_FORMAT_BGR888,\t.modifiers = {} },\n> +\t{ .name = \"RGB888\",\t.v4l2 = V4L2_PIX_FMT_BGR24,\t.drm = DRM_FORMAT_RGB888,\t.modifiers = {} },\n> +\t{ .name = \"BGRA8888\",\t.v4l2 = V4L2_PIX_FMT_ARGB32,\t.drm = DRM_FORMAT_BGRA8888,\t.modifiers = {} },\n>  \t/* YUV packed formats. */\n> -\t{ .v4l2 = V4L2_PIX_FMT_YUYV,\t.drm = DRM_FORMAT_YUYV,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_YVYU,\t.drm = DRM_FORMAT_YVYU,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_UYVY,\t.drm = DRM_FORMAT_UYVY,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_VYUY,\t.drm = DRM_FORMAT_VYUY,\t\t.modifiers = {} },\n> +\t{ .name = \"YUYV\",\t.v4l2 = V4L2_PIX_FMT_YUYV,\t.drm = DRM_FORMAT_YUYV,\t\t.modifiers = {} },\n> +\t{ .name = \"YVYU\",\t.v4l2 = V4L2_PIX_FMT_YVYU,\t.drm = DRM_FORMAT_YVYU,\t\t.modifiers = {} },\n> +\t{ .name = \"UYVY\",\t.v4l2 = V4L2_PIX_FMT_UYVY,\t.drm = DRM_FORMAT_UYVY,\t\t.modifiers = {} },\n> +\t{ .name = \"VYUY\",\t.v4l2 = V4L2_PIX_FMT_VYUY,\t.drm = DRM_FORMAT_VYUY,\t\t.modifiers = {} },\n>  \t/* YUY planar formats. */\n> -\t{ .v4l2 = V4L2_PIX_FMT_NV16,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_NV16M,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_NV61,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_NV61M,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_NV12,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_NV12M,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_NV21,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> -\t{ .v4l2 = V4L2_PIX_FMT_NV21M,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> +\t{ .name = \"NV16\",\t.v4l2 = V4L2_PIX_FMT_NV16,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> +\t{ .name = \"NV16M\",\t.v4l2 = V4L2_PIX_FMT_NV16M,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> +\t{ .name = \"NV61\",\t.v4l2 = V4L2_PIX_FMT_NV61,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> +\t{ .name = \"NV61M\",\t.v4l2 = V4L2_PIX_FMT_NV61M,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> +\t{ .name = \"NV12\",\t.v4l2 = V4L2_PIX_FMT_NV12,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> +\t{ .name = \"NV12M\",\t.v4l2 = V4L2_PIX_FMT_NV12M,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> +\t{ .name = \"NV21\",\t.v4l2 = V4L2_PIX_FMT_NV21,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> +\t{ .name = \"NV21M\",\t.v4l2 = V4L2_PIX_FMT_NV21M,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n>  \t/* Compressed formats. */\n> -\t{ .v4l2 = V4L2_PIX_FMT_MJPEG,\t.drm = DRM_FORMAT_MJPEG,\t.modifiers = {} },\n> +\t{ .name = \"MJPEG\",\t.v4l2 = V4L2_PIX_FMT_MJPEG,\t.drm = DRM_FORMAT_MJPEG,\t.modifiers = {} },\n>  };\n>  \n>  PixelFormat::PixelFormat()\n> @@ -85,6 +86,11 @@ PixelFormat::PixelFormat(uint32_t v4l2_fourcc)\n>  {\n>  }\n>  \n> +PixelFormat::PixelFormat(const std::string &name)\n> +\t: format_(fromName(name))\n> +{\n> +}\n> +\n>  PixelFormat &PixelFormat::operator=(const PixelFormat &other)\n>  {\n>  \tformat_ = other.format_;\n> @@ -107,6 +113,11 @@ bool PixelFormat::operator<(const PixelFormat &other) const\n>  \treturn format_ > other.format_;\n>  }\n>  \n> +const std::string &PixelFormat::name() const\n> +{\n> +\treturn format_->name;\n> +}\n> +\n>  uint32_t PixelFormat::v4l2() const\n>  {\n>  \treturn format_->v4l2;\n> @@ -149,4 +160,17 @@ const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const\n>  \treturn &pixelFormats[0];\n>  }\n>  \n> +const PixelFormatEntry *PixelFormat::fromName(const std::string &name) const\n> +{\n> +\tfor (const PixelFormatEntry &entry : pixelFormats)\n\n\tfor (const PixelFormatEntry &entry : pixelFormats) {\n\n> +\t\tif (entry.name == name)\n> +\t\t\treturn &entry;\n\n\t}\n\n> +\n> +\tLOG(PixelFormats, Error)\n> +\t\t<< \"Unsupported name for pixel format \"\n> +\t\t<< name;\n> +\n> +\treturn &pixelFormats[0];\n\nI think nullptr would be better, and I'm not sure I would log a message\nhere. I'll review 3/6, we can discuss the issue there.\n\n> +}\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index dbce550ca8d0b7b1..e82ffd8f3b211925 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -348,7 +348,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n>  std::string StreamConfiguration::toString() const\n>  {\n>  \tstd::stringstream ss;\n> -\tss << size.toString() << \"-\" << utils::hex(pixelFormat.fourcc());\n> +\tss << size.toString() << \"-\" << pixelFormat.name();\n>  \treturn ss.str();\n>  }\n>","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 947EE60429\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 29 Feb 2020 11:57:41 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15E02734;\n\tSat, 29 Feb 2020 11:57:41 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582973861;\n\tbh=biPLURm1i8VCtv8x0f/ytYch6OVDVxxRJNWFRkaWdkQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A0hljGUFT81NXGCYG4c0chKB0MEQbEjKU6Y6eqjp/URlc9uPIEvOqPHbRsc4UX4ZM\n\tIB96eaJq/E9S0x1xrkmp2kAy2t+jz1o+QDMKmi6Me75RPEFJP/uvDLZnjv3Ziizxmb\n\tsUYFjsptM231ZN0EGMWoptc3NL2lISxnyisKA7BY=","Date":"Sat, 29 Feb 2020 12:57:18 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200229105718.GC18738@pendragon.ideasonboard.com>","References":"<20200228032913.497826-1-niklas.soderlund@ragnatech.se>\n\t<20200228032913.497826-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200228032913.497826-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 4/6] libcamera: PixelFormat: Add\n\toperations to operate on names","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 29 Feb 2020 10:57:41 -0000"}}]