[{"id":22973,"web_url":"https://patchwork.libcamera.org/comment/22973/","msgid":"","date":"2022-05-17T08:32:08","subject":"Re: [libcamera-devel] [PATCH 10/14] py: implement PixelFormat class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Mon, May 16, 2022 at 05:10:18PM +0300, Tomi Valkeinen wrote:\n> Implement PixelFormat bindings properly with a PixelFormat class. Change\n> the bindings to use the new class instead of a string.\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/cam/cam.py | 4 +--\n> src/py/cam/cam_kms.py | 9 +-----\n> src/py/cam/cam_qt.py | 2 +-\n> src/py/cam/cam_qtgl.py | 17 +-----------\n> src/py/cam/gl_helpers.py | 8 ------\n> src/py/libcamera/pymain.cpp | 51 ++++++++++++++++++++--------------\n> src/py/libcamera/utils/conv.py | 2 ++\n> 7 files changed, 37 insertions(+), 56 deletions(-)\n> \n> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n> index c7da97d7..63c67126 100755\n> --- a/src/py/cam/cam.py\n> +++ b/src/py/cam/cam.py\n> @@ -80,7 +80,7 @@ def do_cmd_info(ctx):\n> \n> formats = stream_config.formats\n> for fmt in formats.pixel_formats:\n> - print('\\t * Pixelformat:', fmt, formats.range(fmt))\n> + print('\\t * Pixelformat:', fmt.name, formats.range(fmt))\n> \n> for size in formats.sizes(fmt):\n> print('\\t -', size)\n> @@ -164,7 +164,7 @@ def configure(ctx):\n> stream_config.size = (stream_opts['width'], stream_opts['height'])\n> \n> if 'pixelformat' in stream_opts:\n> - stream_config.pixel_format = stream_opts['pixelformat']\n> + stream_config.pixel_format = libcam.PixelFormat.from_name(stream_opts['pixelformat'])\n> \n> stat = camconfig.validate()\n> \n> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py\n> index ae6be277..7d11564b 100644\n> --- a/src/py/cam/cam_kms.py\n> +++ b/src/py/cam/cam_kms.py\n> @@ -5,13 +5,6 @@ import pykms\n> import selectors\n> import sys\n> \n> -FMT_MAP = {\n> - 'RGB888': pykms.PixelFormat.RGB888,\n> - 'YUYV': pykms.PixelFormat.YUYV,\n> - 'ARGB8888': pykms.PixelFormat.ARGB8888,\n> - 'XRGB8888': pykms.PixelFormat.XRGB8888,\n> -}\n> -\n> \n> class KMSRenderer:\n> def __init__(self, state):\n> @@ -118,7 +111,7 @@ class KMSRenderer:\n> \n> cfg = stream.configuration\n> fmt = cfg.pixel_format\n> - fmt = FMT_MAP[fmt]\n> + fmt = pykms.PixelFormat(fmt.fourcc)\n> \n> plane = self.resman.reserve_generic_plane(self.crtc, fmt)\n> assert(plane is not None)\n> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py\n> index dc5219eb..64f49f33 100644\n> --- a/src/py/cam/cam_qt.py\n> +++ b/src/py/cam/cam_qt.py\n> @@ -139,7 +139,7 @@ class MainWindow(QtWidgets.QWidget):\n> w, h = cfg.size\n> pitch = cfg.stride\n> \n> - if cfg.pixel_format == 'MJPEG':\n> + if cfg.pixel_format.name == 'MJPEG':\n> img = Image.open(BytesIO(mfb.planes[0]))\n> qim = ImageQt(img).copy()\n> pix = QtGui.QPixmap.fromImage(qim)\n> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py\n> index 8a95994e..261accb8 100644\n> --- a/src/py/cam/cam_qtgl.py\n> +++ b/src/py/cam/cam_qtgl.py\n> @@ -30,14 +30,6 @@ from OpenGL.GL import shaders\n> \n> from gl_helpers import *\n> \n> -# libcamera format string -> DRM fourcc\n> -FMT_MAP = {\n> - 'RGB888': 'RG24',\n> - 'XRGB8888': 'XR24',\n> - 'ARGB8888': 'AR24',\n> - 'YUYV': 'YUYV',\n> -}\n> -\n> \n> class EglState:\n> def __init__(self):\n> @@ -204,12 +196,6 @@ class MainWindow(QtWidgets.QWidget):\n> self.current[ctx['idx']] = []\n> \n> for stream in ctx['streams']:\n> - fmt = stream.configuration.pixel_format\n> - size = stream.configuration.size\n> -\n> - if fmt not in FMT_MAP:\n> - raise Exception('Unsupported pixel format: ' + str(fmt))\n> -\n> self.textures[stream] = None\n> \n> num_tiles = len(self.textures)\n> @@ -281,8 +267,7 @@ class MainWindow(QtWidgets.QWidget):\n> \n> def create_texture(self, stream, fb):\n> cfg = stream.configuration\n> - fmt = cfg.pixel_format\n> - fmt = str_to_fourcc(FMT_MAP[fmt])\n> + fmt = cfg.pixel_format.fourcc\n> w, h = cfg.size\n> \n> attribs = [\n> diff --git a/src/py/cam/gl_helpers.py b/src/py/cam/gl_helpers.py\n> index ac5e6889..53b3e9df 100644\n> --- a/src/py/cam/gl_helpers.py\n> +++ b/src/py/cam/gl_helpers.py\n> @@ -30,14 +30,6 @@ def getglEGLImageTargetTexture2DOES():\n> \n> glEGLImageTargetTexture2DOES = getglEGLImageTargetTexture2DOES()\n> \n> -# \\todo This can be dropped when we have proper PixelFormat bindings\n> -def str_to_fourcc(str):\n> - assert(len(str) == 4)\n> - fourcc = 0\n> - for i, v in enumerate([ord(c) for c in str]):\n> - fourcc |= v << (i * 8)\n> - return fourcc\n> -\n> \n> def get_gl_extensions():\n> n = GLint()\n> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp\n> index af22205e..cc2ddee5 100644\n> --- a/src/py/libcamera/pymain.cpp\n> +++ b/src/py/libcamera/pymain.cpp\n> @@ -8,7 +8,6 @@\n> /*\n> * \\todo Add geometry classes (Point, Rectangle...)\n> * \\todo Add bindings for the ControlInfo class\n> - * \\todo Add bindings for the PixelFormat class\n> */\n> \n> #include \n> @@ -173,6 +172,7 @@ PYBIND11_MODULE(_libcamera, m)\n> \tauto pyColorSpaceTransferFunction = py::enum_(pyColorSpace, \"TransferFunction\");\n> \tauto pyColorSpaceYcbcrEncoding = py::enum_(pyColorSpace, \"YcbcrEncoding\");\n> \tauto pyColorSpaceRange = py::enum_(pyColorSpace, \"Range\");\n> +\tauto pyPixelFormat = py::class_(m, \"PixelFormat\");\n> \n> \t/* Global functions */\n> \tm.def(\"log_set_level\", &logSetLevel);\n> @@ -404,14 +404,7 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t\t\tself.size.width = std::get<0>(size);\n> \t\t\t\tself.size.height = std::get<1>(size);\n> \t\t\t})\n> -\t\t.def_property(\n> -\t\t\t\"pixel_format\",\n> -\t\t\t[](StreamConfiguration &self) {\n> -\t\t\t\treturn self.pixelFormat.toString();\n> -\t\t\t},\n> -\t\t\t[](StreamConfiguration &self, std::string fmt) {\n> -\t\t\t\tself.pixelFormat = PixelFormat::fromString(fmt);\n> -\t\t\t})\n> +\t\t.def_readwrite(\"pixel_format\", &StreamConfiguration::pixelFormat)\n> \t\t.def_readwrite(\"stride\", &StreamConfiguration::stride)\n> \t\t.def_readwrite(\"frame_size\", &StreamConfiguration::frameSize)\n> \t\t.def_readwrite(\"buffer_count\", &StreamConfiguration::bufferCount)\n> @@ -420,22 +413,15 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t.def_readwrite(\"color_space\", &StreamConfiguration::colorSpace);\n> \n> \tpyStreamFormats\n> -\t\t.def_property_readonly(\"pixel_formats\", [](StreamFormats &self) {\n> -\t\t\tstd::vector fmts;\n> -\t\t\tfor (auto &fmt : self.pixelformats())\n> -\t\t\t\tfmts.push_back(fmt.toString());\n> -\t\t\treturn fmts;\n> -\t\t})\n> -\t\t.def(\"sizes\", [](StreamFormats &self, const std::string &pixelFormat) {\n> -\t\t\tauto fmt = PixelFormat::fromString(pixelFormat);\n> +\t\t.def_property_readonly(\"pixel_formats\", &StreamFormats::pixelformats)\n> +\t\t.def(\"sizes\", [](StreamFormats &self, const PixelFormat &pixelFormat) {\n> \t\t\tstd::vector> fmts;\n> -\t\t\tfor (const auto &s : self.sizes(fmt))\n> +\t\t\tfor (const auto &s : self.sizes(pixelFormat))\n> \t\t\t\tfmts.push_back(std::make_tuple(s.width, s.height));\n> \t\t\treturn fmts;\n> \t\t})\n> -\t\t.def(\"range\", [](StreamFormats &self, const std::string &pixelFormat) {\n> -\t\t\tauto fmt = PixelFormat::fromString(pixelFormat);\n> -\t\t\tconst auto &range = self.range(fmt);\n> +\t\t.def(\"range\", [](StreamFormats &self, const PixelFormat &pixelFormat) {\n> +\t\t\tconst auto &range = self.range(pixelFormat);\n> \t\t\treturn make_tuple(std::make_tuple(range.hStep, range.vStep),\n> \t\t\t\t\t std::make_tuple(range.min.width, range.min.height),\n> \t\t\t\t\t std::make_tuple(range.max.width, range.max.height));\n> @@ -648,4 +634,27 @@ PYBIND11_MODULE(_libcamera, m)\n> \tpyColorSpaceRange\n> \t\t.value(\"Full\", ColorSpace::Range::Full)\n> \t\t.value(\"Limited\", ColorSpace::Range::Limited);\n> +\n> +\tpyPixelFormat\n> +\t\t.def(py::init<>())\n> +\t\t.def(py::init())\n> +\t\t.def_property_readonly(\"fourcc\", &PixelFormat::fourcc)\n> +\t\t.def_property_readonly(\"modifier\", &PixelFormat::modifier)\n> +\t\t.def_property_readonly(\"name\", &PixelFormat::toString)\n> +\t\t.def(py::self == py::self)\n> +\t\t.def(\"__str__\", [](const PixelFormat &self) {\n> +\t\t\treturn \"\";\n> +\t\t})\n\nI'd just return self.toString() here. We could also implement a __repr__\nthat prints \"libcamera.PixelFormat('\" + self.toString() + \"')\" if we add\na constructor that takes a string as __repr__ is supposed to return a\ncanonical string representation that can be run to recreate the object.\n\n> +\t\t.def_static(\"from_name\", [](const std::string &name) {\n\nCould we name this \"from_string\" to avoid diverging from the C++ API ?\n\n> +\t\t\treturn PixelFormat::fromString(name);\n> +\t\t})\n> +\t\t.def_static(\"from_fourcc\", [](const std::string &fourcc) {\n> +\t\t\tif (fourcc.size() != 4)\n> +\t\t\t\tthrow std::invalid_argument(\"Invalid fourcc length\");\n> +\n> +\t\t\tuint32_t v = fourcc[0] | (fourcc[1] << 8) |\n> +\t\t\t\t (fourcc[2] << 16) | (fourcc[3] << 24);\n> +\n> +\t\t\treturn PixelFormat(v);\n> +\t\t});\n\nThis isn't used in your series, so I'd drop it.\n\n> }\n> diff --git a/src/py/libcamera/utils/conv.py b/src/py/libcamera/utils/conv.py\n> index 2e483003..30f6733e 100644\n> --- a/src/py/libcamera/utils/conv.py\n> +++ b/src/py/libcamera/utils/conv.py\n> @@ -76,6 +76,8 @@ def to_rgb(fmt, size, data):\n> w = size[0]\n> h = size[1]\n> \n> + fmt = fmt.name\n> +\n> if fmt == 'YUYV':\n> # YUV422\n> yuyv = data.reshape((h, w // 2 * 4))","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C08DEC0F2A\n\tfor ;\n\tTue, 17 May 2022 08:32:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1AB8F65657;\n\tTue, 17 May 2022 10:32:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6EC760420\n\tfor ;\n\tTue, 17 May 2022 10:32:14 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 50D9248F;\n\tTue, 17 May 2022 10:32:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652776337;\n\tbh=GMQnSXXv6/78kp7rXuA81zURd3cCa51pc0fMLJTVWA4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YEnrDPa9X5o/p1OQddvpv8vWr1jDZrxA41qQtir7HWgocTXb56gFvpGYWrZi1bZ2L\n\tA59EWWxEbWP/BKyXgEaNr53ElrNsYrZJMDRbms6AORsNyyBxvHOfqOOjm+JE/rcRWl\n\tO5iRdLHO5PLpY6kJr+UMuAmFtqazCtmgZGIA7mHGsIW8sMVL6XtIffYj3rHvfw9y/t\n\tu2w5r2AzMlgtV8U/8C4CsasbvZ5QJA4O/4hAhxU2bMfojHfgEd7zFR4pRB+7JIfw/e\n\twfgULWn27YWMovqgUlF0w2b8FNBAFV3b8HKe+s4BS3K+qk2fz9yHpN2TjSj3aoOw+h\n\tu59EoblJkz6nQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652776334;\n\tbh=GMQnSXXv6/78kp7rXuA81zURd3cCa51pc0fMLJTVWA4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cF5ywm5fGD574I9ixHeYtRxSSwf4LqAv9ACW/OmA0K5x8GHQzh0uvOvWCUjPK1XfW\n\tqFAxYsfJpwi6QiknYh+S0EpIQW67XJ80F79nArNlvOpbjf2Sc9x0honT6sAoNy+QdO\n\tkmdDqlhVUWY+jIDikDDhMJsXZPt7l8y+agT81lP4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cF5ywm5f\"; dkim-atps=neutral","Date":"Tue, 17 May 2022 11:32:08 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-11-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220516141022.96327-11-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 10/14] py: implement PixelFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22980,"web_url":"https://patchwork.libcamera.org/comment/22980/","msgid":"<9db68f60-6de8-2b46-c728-f97c551eed07@ideasonboard.com>","date":"2022-05-17T08:53:39","subject":"Re: [libcamera-devel] [PATCH 10/14] py: implement PixelFormat class","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 17/05/2022 11:32, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Mon, May 16, 2022 at 05:10:18PM +0300, Tomi Valkeinen wrote:\n>> Implement PixelFormat bindings properly with a PixelFormat class. Change\n>> the bindings to use the new class instead of a string.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/cam/cam.py | 4 +--\n>> src/py/cam/cam_kms.py | 9 +-----\n>> src/py/cam/cam_qt.py | 2 +-\n>> src/py/cam/cam_qtgl.py | 17 +-----------\n>> src/py/cam/gl_helpers.py | 8 ------\n>> src/py/libcamera/pymain.cpp | 51 ++++++++++++++++++++--------------\n>> src/py/libcamera/utils/conv.py | 2 ++\n>> 7 files changed, 37 insertions(+), 56 deletions(-)\n>>\n>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n>> index c7da97d7..63c67126 100755\n>> --- a/src/py/cam/cam.py\n>> +++ b/src/py/cam/cam.py\n>> @@ -80,7 +80,7 @@ def do_cmd_info(ctx):\n>> \n>> formats = stream_config.formats\n>> for fmt in formats.pixel_formats:\n>> - print('\\t * Pixelformat:', fmt, formats.range(fmt))\n>> + print('\\t * Pixelformat:', fmt.name, formats.range(fmt))\n>> \n>> for size in formats.sizes(fmt):\n>> print('\\t -', size)\n>> @@ -164,7 +164,7 @@ def configure(ctx):\n>> stream_config.size = (stream_opts['width'], stream_opts['height'])\n>> \n>> if 'pixelformat' in stream_opts:\n>> - stream_config.pixel_format = stream_opts['pixelformat']\n>> + stream_config.pixel_format = libcam.PixelFormat.from_name(stream_opts['pixelformat'])\n>> \n>> stat = camconfig.validate()\n>> \n>> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py\n>> index ae6be277..7d11564b 100644\n>> --- a/src/py/cam/cam_kms.py\n>> +++ b/src/py/cam/cam_kms.py\n>> @@ -5,13 +5,6 @@ import pykms\n>> import selectors\n>> import sys\n>> \n>> -FMT_MAP = {\n>> - 'RGB888': pykms.PixelFormat.RGB888,\n>> - 'YUYV': pykms.PixelFormat.YUYV,\n>> - 'ARGB8888': pykms.PixelFormat.ARGB8888,\n>> - 'XRGB8888': pykms.PixelFormat.XRGB8888,\n>> -}\n>> -\n>> \n>> class KMSRenderer:\n>> def __init__(self, state):\n>> @@ -118,7 +111,7 @@ class KMSRenderer:\n>> \n>> cfg = stream.configuration\n>> fmt = cfg.pixel_format\n>> - fmt = FMT_MAP[fmt]\n>> + fmt = pykms.PixelFormat(fmt.fourcc)\n>> \n>> plane = self.resman.reserve_generic_plane(self.crtc, fmt)\n>> assert(plane is not None)\n>> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py\n>> index dc5219eb..64f49f33 100644\n>> --- a/src/py/cam/cam_qt.py\n>> +++ b/src/py/cam/cam_qt.py\n>> @@ -139,7 +139,7 @@ class MainWindow(QtWidgets.QWidget):\n>> w, h = cfg.size\n>> pitch = cfg.stride\n>> \n>> - if cfg.pixel_format == 'MJPEG':\n>> + if cfg.pixel_format.name == 'MJPEG':\n>> img = Image.open(BytesIO(mfb.planes[0]))\n>> qim = ImageQt(img).copy()\n>> pix = QtGui.QPixmap.fromImage(qim)\n>> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py\n>> index 8a95994e..261accb8 100644\n>> --- a/src/py/cam/cam_qtgl.py\n>> +++ b/src/py/cam/cam_qtgl.py\n>> @@ -30,14 +30,6 @@ from OpenGL.GL import shaders\n>> \n>> from gl_helpers import *\n>> \n>> -# libcamera format string -> DRM fourcc\n>> -FMT_MAP = {\n>> - 'RGB888': 'RG24',\n>> - 'XRGB8888': 'XR24',\n>> - 'ARGB8888': 'AR24',\n>> - 'YUYV': 'YUYV',\n>> -}\n>> -\n>> \n>> class EglState:\n>> def __init__(self):\n>> @@ -204,12 +196,6 @@ class MainWindow(QtWidgets.QWidget):\n>> self.current[ctx['idx']] = []\n>> \n>> for stream in ctx['streams']:\n>> - fmt = stream.configuration.pixel_format\n>> - size = stream.configuration.size\n>> -\n>> - if fmt not in FMT_MAP:\n>> - raise Exception('Unsupported pixel format: ' + str(fmt))\n>> -\n>> self.textures[stream] = None\n>> \n>> num_tiles = len(self.textures)\n>> @@ -281,8 +267,7 @@ class MainWindow(QtWidgets.QWidget):\n>> \n>> def create_texture(self, stream, fb):\n>> cfg = stream.configuration\n>> - fmt = cfg.pixel_format\n>> - fmt = str_to_fourcc(FMT_MAP[fmt])\n>> + fmt = cfg.pixel_format.fourcc\n>> w, h = cfg.size\n>> \n>> attribs = [\n>> diff --git a/src/py/cam/gl_helpers.py b/src/py/cam/gl_helpers.py\n>> index ac5e6889..53b3e9df 100644\n>> --- a/src/py/cam/gl_helpers.py\n>> +++ b/src/py/cam/gl_helpers.py\n>> @@ -30,14 +30,6 @@ def getglEGLImageTargetTexture2DOES():\n>> \n>> glEGLImageTargetTexture2DOES = getglEGLImageTargetTexture2DOES()\n>> \n>> -# \\todo This can be dropped when we have proper PixelFormat bindings\n>> -def str_to_fourcc(str):\n>> - assert(len(str) == 4)\n>> - fourcc = 0\n>> - for i, v in enumerate([ord(c) for c in str]):\n>> - fourcc |= v << (i * 8)\n>> - return fourcc\n>> -\n>> \n>> def get_gl_extensions():\n>> n = GLint()\n>> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp\n>> index af22205e..cc2ddee5 100644\n>> --- a/src/py/libcamera/pymain.cpp\n>> +++ b/src/py/libcamera/pymain.cpp\n>> @@ -8,7 +8,6 @@\n>> /*\n>> * \\todo Add geometry classes (Point, Rectangle...)\n>> * \\todo Add bindings for the ControlInfo class\n>> - * \\todo Add bindings for the PixelFormat class\n>> */\n>> \n>> #include \n>> @@ -173,6 +172,7 @@ PYBIND11_MODULE(_libcamera, m)\n>> \tauto pyColorSpaceTransferFunction = py::enum_(pyColorSpace, \"TransferFunction\");\n>> \tauto pyColorSpaceYcbcrEncoding = py::enum_(pyColorSpace, \"YcbcrEncoding\");\n>> \tauto pyColorSpaceRange = py::enum_(pyColorSpace, \"Range\");\n>> +\tauto pyPixelFormat = py::class_(m, \"PixelFormat\");\n>> \n>> \t/* Global functions */\n>> \tm.def(\"log_set_level\", &logSetLevel);\n>> @@ -404,14 +404,7 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t\t\tself.size.width = std::get<0>(size);\n>> \t\t\t\tself.size.height = std::get<1>(size);\n>> \t\t\t})\n>> -\t\t.def_property(\n>> -\t\t\t\"pixel_format\",\n>> -\t\t\t[](StreamConfiguration &self) {\n>> -\t\t\t\treturn self.pixelFormat.toString();\n>> -\t\t\t},\n>> -\t\t\t[](StreamConfiguration &self, std::string fmt) {\n>> -\t\t\t\tself.pixelFormat = PixelFormat::fromString(fmt);\n>> -\t\t\t})\n>> +\t\t.def_readwrite(\"pixel_format\", &StreamConfiguration::pixelFormat)\n>> \t\t.def_readwrite(\"stride\", &StreamConfiguration::stride)\n>> \t\t.def_readwrite(\"frame_size\", &StreamConfiguration::frameSize)\n>> \t\t.def_readwrite(\"buffer_count\", &StreamConfiguration::bufferCount)\n>> @@ -420,22 +413,15 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t.def_readwrite(\"color_space\", &StreamConfiguration::colorSpace);\n>> \n>> \tpyStreamFormats\n>> -\t\t.def_property_readonly(\"pixel_formats\", [](StreamFormats &self) {\n>> -\t\t\tstd::vector fmts;\n>> -\t\t\tfor (auto &fmt : self.pixelformats())\n>> -\t\t\t\tfmts.push_back(fmt.toString());\n>> -\t\t\treturn fmts;\n>> -\t\t})\n>> -\t\t.def(\"sizes\", [](StreamFormats &self, const std::string &pixelFormat) {\n>> -\t\t\tauto fmt = PixelFormat::fromString(pixelFormat);\n>> +\t\t.def_property_readonly(\"pixel_formats\", &StreamFormats::pixelformats)\n>> +\t\t.def(\"sizes\", [](StreamFormats &self, const PixelFormat &pixelFormat) {\n>> \t\t\tstd::vector> fmts;\n>> -\t\t\tfor (const auto &s : self.sizes(fmt))\n>> +\t\t\tfor (const auto &s : self.sizes(pixelFormat))\n>> \t\t\t\tfmts.push_back(std::make_tuple(s.width, s.height));\n>> \t\t\treturn fmts;\n>> \t\t})\n>> -\t\t.def(\"range\", [](StreamFormats &self, const std::string &pixelFormat) {\n>> -\t\t\tauto fmt = PixelFormat::fromString(pixelFormat);\n>> -\t\t\tconst auto &range = self.range(fmt);\n>> +\t\t.def(\"range\", [](StreamFormats &self, const PixelFormat &pixelFormat) {\n>> +\t\t\tconst auto &range = self.range(pixelFormat);\n>> \t\t\treturn make_tuple(std::make_tuple(range.hStep, range.vStep),\n>> \t\t\t\t\t std::make_tuple(range.min.width, range.min.height),\n>> \t\t\t\t\t std::make_tuple(range.max.width, range.max.height));\n>> @@ -648,4 +634,27 @@ PYBIND11_MODULE(_libcamera, m)\n>> \tpyColorSpaceRange\n>> \t\t.value(\"Full\", ColorSpace::Range::Full)\n>> \t\t.value(\"Limited\", ColorSpace::Range::Limited);\n>> +\n>> +\tpyPixelFormat\n>> +\t\t.def(py::init<>())\n>> +\t\t.def(py::init())\n>> +\t\t.def_property_readonly(\"fourcc\", &PixelFormat::fourcc)\n>> +\t\t.def_property_readonly(\"modifier\", &PixelFormat::modifier)\n>> +\t\t.def_property_readonly(\"name\", &PixelFormat::toString)\n>> +\t\t.def(py::self == py::self)\n>> +\t\t.def(\"__str__\", [](const PixelFormat &self) {\n>> +\t\t\treturn \"\";\n>> +\t\t})\n> \n> I'd just return self.toString() here. We could also implement a __repr__\n> that prints \"libcamera.PixelFormat('\" + self.toString() + \"')\" if we add\n> a constructor that takes a string as __repr__ is supposed to return a\n> canonical string representation that can be run to recreate the object.\n\nYes. I missed __repr__ here. I think I implemented __str__ and __repr__ \nbetter with the geometry classes. I'll improve this.\n\n>> +\t\t.def_static(\"from_name\", [](const std::string &name) {\n> \n> Could we name this \"from_string\" to avoid diverging from the C++ API ?\n\nWe could, but... The reason I didn't use \"string\" was that fourcc is a \nstring too. I used \"name\" in the property above too.\n\nBut I guess the libcamera strategy is to never expose fourcc as a \nstring? It's always uint32_t if it's fourcc, or string if it's the \"long \nname\" of the format?\n\nI've always used fourcc as a string (e.g. in kms++) as it's easy, but \nperhaps the above is a better approach.\n\n>> +\t\t\treturn PixelFormat::fromString(name);\n>> +\t\t})\n>> +\t\t.def_static(\"from_fourcc\", [](const std::string &fourcc) {\n>> +\t\t\tif (fourcc.size() != 4)\n>> +\t\t\t\tthrow std::invalid_argument(\"Invalid fourcc length\");\n>> +\n>> +\t\t\tuint32_t v = fourcc[0] | (fourcc[1] << 8) |\n>> +\t\t\t\t (fourcc[2] << 16) | (fourcc[3] << 24);\n>> +\n>> +\t\t\treturn PixelFormat(v);\n>> +\t\t});\n> \n> This isn't used in your series, so I'd drop it.\n\nTrue.\n\n Tomi","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C6185C0F2A\n\tfor ;\n\tTue, 17 May 2022 08:53:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2BBBC6565A;\n\tTue, 17 May 2022 10:53:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DCEE60420\n\tfor ;\n\tTue, 17 May 2022 10:53:43 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8F33D48F;\n\tTue, 17 May 2022 10:53:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652777624;\n\tbh=SJtz4iUdU5g8NtR5M8coW+7ArwZcwCpX1x2u02RozlY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PqYiT6E4yNszJSX1sG9R99sUL+yrorwSMUaG/mcuQZjP8oG9HLfLEXhL3frfmHX54\n\trQ77lgIp6/IaGvY1JvRJi+12D7GFu5dHx4nnWD+mUPycTEFweHTHQy/KTu7QDiJT78\n\t2OVmI51irxNWVhPwv/SpRr1fwF9DktfKaxLxYCkl8DEqR2rz0LvjSX34tgcjRJPYFp\n\ttrZKBOhlQs1dnBjo9h8zUYdxgtPje3VNl5rlTeQhgiZu2uPLy7O+/Z+P1W76qDbAuV\n\tKs6EH2IDTIA3WRW6Y/oPthVH4i/dYSn78lkmqwP15rpcIBmWDoOm6jrqU0iozokLpo\n\t8TsSQalrAGHNg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652777622;\n\tbh=SJtz4iUdU5g8NtR5M8coW+7ArwZcwCpX1x2u02RozlY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=nQU6tH/AIjiS1561aD5+rwj55xZ8aJoSMU266Icaef4Q8hSFWWeQnKg+1KZdnUejz\n\t/sP7XUp3L93ROzgSnJuZyFHsrECVV61oQaCfuyjFeDbQxwFJfFfckbEHjGUpk2IlGi\n\tqI49GlyMQDK3xdeT/N1NT7p3R/v7cr3oMN6OCLds="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nQU6tH/A\"; dkim-atps=neutral","Message-ID":"<9db68f60-6de8-2b46-c728-f97c551eed07@ideasonboard.com>","Date":"Tue, 17 May 2022 11:53:39 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.8.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-11-tomi.valkeinen@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 10/14] py: implement PixelFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22982,"web_url":"https://patchwork.libcamera.org/comment/22982/","msgid":"","date":"2022-05-17T08:58:00","subject":"Re: [libcamera-devel] [PATCH 10/14] py: implement PixelFormat class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Tue, May 17, 2022 at 11:53:39AM +0300, Tomi Valkeinen wrote:\n> On 17/05/2022 11:32, Laurent Pinchart wrote:\n> > On Mon, May 16, 2022 at 05:10:18PM +0300, Tomi Valkeinen wrote:\n> >> Implement PixelFormat bindings properly with a PixelFormat class. Change\n> >> the bindings to use the new class instead of a string.\n> >>\n> >> Signed-off-by: Tomi Valkeinen \n> >> ---\n> >> src/py/cam/cam.py | 4 +--\n> >> src/py/cam/cam_kms.py | 9 +-----\n> >> src/py/cam/cam_qt.py | 2 +-\n> >> src/py/cam/cam_qtgl.py | 17 +-----------\n> >> src/py/cam/gl_helpers.py | 8 ------\n> >> src/py/libcamera/pymain.cpp | 51 ++++++++++++++++++++--------------\n> >> src/py/libcamera/utils/conv.py | 2 ++\n> >> 7 files changed, 37 insertions(+), 56 deletions(-)\n> >>\n> >> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n> >> index c7da97d7..63c67126 100755\n> >> --- a/src/py/cam/cam.py\n> >> +++ b/src/py/cam/cam.py\n> >> @@ -80,7 +80,7 @@ def do_cmd_info(ctx):\n> >> \n> >> formats = stream_config.formats\n> >> for fmt in formats.pixel_formats:\n> >> - print('\\t * Pixelformat:', fmt, formats.range(fmt))\n> >> + print('\\t * Pixelformat:', fmt.name, formats.range(fmt))\n> >> \n> >> for size in formats.sizes(fmt):\n> >> print('\\t -', size)\n> >> @@ -164,7 +164,7 @@ def configure(ctx):\n> >> stream_config.size = (stream_opts['width'], stream_opts['height'])\n> >> \n> >> if 'pixelformat' in stream_opts:\n> >> - stream_config.pixel_format = stream_opts['pixelformat']\n> >> + stream_config.pixel_format = libcam.PixelFormat.from_name(stream_opts['pixelformat'])\n> >> \n> >> stat = camconfig.validate()\n> >> \n> >> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py\n> >> index ae6be277..7d11564b 100644\n> >> --- a/src/py/cam/cam_kms.py\n> >> +++ b/src/py/cam/cam_kms.py\n> >> @@ -5,13 +5,6 @@ import pykms\n> >> import selectors\n> >> import sys\n> >> \n> >> -FMT_MAP = {\n> >> - 'RGB888': pykms.PixelFormat.RGB888,\n> >> - 'YUYV': pykms.PixelFormat.YUYV,\n> >> - 'ARGB8888': pykms.PixelFormat.ARGB8888,\n> >> - 'XRGB8888': pykms.PixelFormat.XRGB8888,\n> >> -}\n> >> -\n> >> \n> >> class KMSRenderer:\n> >> def __init__(self, state):\n> >> @@ -118,7 +111,7 @@ class KMSRenderer:\n> >> \n> >> cfg = stream.configuration\n> >> fmt = cfg.pixel_format\n> >> - fmt = FMT_MAP[fmt]\n> >> + fmt = pykms.PixelFormat(fmt.fourcc)\n> >> \n> >> plane = self.resman.reserve_generic_plane(self.crtc, fmt)\n> >> assert(plane is not None)\n> >> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py\n> >> index dc5219eb..64f49f33 100644\n> >> --- a/src/py/cam/cam_qt.py\n> >> +++ b/src/py/cam/cam_qt.py\n> >> @@ -139,7 +139,7 @@ class MainWindow(QtWidgets.QWidget):\n> >> w, h = cfg.size\n> >> pitch = cfg.stride\n> >> \n> >> - if cfg.pixel_format == 'MJPEG':\n> >> + if cfg.pixel_format.name == 'MJPEG':\n> >> img = Image.open(BytesIO(mfb.planes[0]))\n> >> qim = ImageQt(img).copy()\n> >> pix = QtGui.QPixmap.fromImage(qim)\n> >> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py\n> >> index 8a95994e..261accb8 100644\n> >> --- a/src/py/cam/cam_qtgl.py\n> >> +++ b/src/py/cam/cam_qtgl.py\n> >> @@ -30,14 +30,6 @@ from OpenGL.GL import shaders\n> >> \n> >> from gl_helpers import *\n> >> \n> >> -# libcamera format string -> DRM fourcc\n> >> -FMT_MAP = {\n> >> - 'RGB888': 'RG24',\n> >> - 'XRGB8888': 'XR24',\n> >> - 'ARGB8888': 'AR24',\n> >> - 'YUYV': 'YUYV',\n> >> -}\n> >> -\n> >> \n> >> class EglState:\n> >> def __init__(self):\n> >> @@ -204,12 +196,6 @@ class MainWindow(QtWidgets.QWidget):\n> >> self.current[ctx['idx']] = []\n> >> \n> >> for stream in ctx['streams']:\n> >> - fmt = stream.configuration.pixel_format\n> >> - size = stream.configuration.size\n> >> -\n> >> - if fmt not in FMT_MAP:\n> >> - raise Exception('Unsupported pixel format: ' + str(fmt))\n> >> -\n> >> self.textures[stream] = None\n> >> \n> >> num_tiles = len(self.textures)\n> >> @@ -281,8 +267,7 @@ class MainWindow(QtWidgets.QWidget):\n> >> \n> >> def create_texture(self, stream, fb):\n> >> cfg = stream.configuration\n> >> - fmt = cfg.pixel_format\n> >> - fmt = str_to_fourcc(FMT_MAP[fmt])\n> >> + fmt = cfg.pixel_format.fourcc\n> >> w, h = cfg.size\n> >> \n> >> attribs = [\n> >> diff --git a/src/py/cam/gl_helpers.py b/src/py/cam/gl_helpers.py\n> >> index ac5e6889..53b3e9df 100644\n> >> --- a/src/py/cam/gl_helpers.py\n> >> +++ b/src/py/cam/gl_helpers.py\n> >> @@ -30,14 +30,6 @@ def getglEGLImageTargetTexture2DOES():\n> >> \n> >> glEGLImageTargetTexture2DOES = getglEGLImageTargetTexture2DOES()\n> >> \n> >> -# \\todo This can be dropped when we have proper PixelFormat bindings\n> >> -def str_to_fourcc(str):\n> >> - assert(len(str) == 4)\n> >> - fourcc = 0\n> >> - for i, v in enumerate([ord(c) for c in str]):\n> >> - fourcc |= v << (i * 8)\n> >> - return fourcc\n> >> -\n> >> \n> >> def get_gl_extensions():\n> >> n = GLint()\n> >> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp\n> >> index af22205e..cc2ddee5 100644\n> >> --- a/src/py/libcamera/pymain.cpp\n> >> +++ b/src/py/libcamera/pymain.cpp\n> >> @@ -8,7 +8,6 @@\n> >> /*\n> >> * \\todo Add geometry classes (Point, Rectangle...)\n> >> * \\todo Add bindings for the ControlInfo class\n> >> - * \\todo Add bindings for the PixelFormat class\n> >> */\n> >> \n> >> #include \n> >> @@ -173,6 +172,7 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \tauto pyColorSpaceTransferFunction = py::enum_(pyColorSpace, \"TransferFunction\");\n> >> \tauto pyColorSpaceYcbcrEncoding = py::enum_(pyColorSpace, \"YcbcrEncoding\");\n> >> \tauto pyColorSpaceRange = py::enum_(pyColorSpace, \"Range\");\n> >> +\tauto pyPixelFormat = py::class_(m, \"PixelFormat\");\n> >> \n> >> \t/* Global functions */\n> >> \tm.def(\"log_set_level\", &logSetLevel);\n> >> @@ -404,14 +404,7 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t\t\t\tself.size.width = std::get<0>(size);\n> >> \t\t\t\tself.size.height = std::get<1>(size);\n> >> \t\t\t})\n> >> -\t\t.def_property(\n> >> -\t\t\t\"pixel_format\",\n> >> -\t\t\t[](StreamConfiguration &self) {\n> >> -\t\t\t\treturn self.pixelFormat.toString();\n> >> -\t\t\t},\n> >> -\t\t\t[](StreamConfiguration &self, std::string fmt) {\n> >> -\t\t\t\tself.pixelFormat = PixelFormat::fromString(fmt);\n> >> -\t\t\t})\n> >> +\t\t.def_readwrite(\"pixel_format\", &StreamConfiguration::pixelFormat)\n> >> \t\t.def_readwrite(\"stride\", &StreamConfiguration::stride)\n> >> \t\t.def_readwrite(\"frame_size\", &StreamConfiguration::frameSize)\n> >> \t\t.def_readwrite(\"buffer_count\", &StreamConfiguration::bufferCount)\n> >> @@ -420,22 +413,15 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t\t.def_readwrite(\"color_space\", &StreamConfiguration::colorSpace);\n> >> \n> >> \tpyStreamFormats\n> >> -\t\t.def_property_readonly(\"pixel_formats\", [](StreamFormats &self) {\n> >> -\t\t\tstd::vector fmts;\n> >> -\t\t\tfor (auto &fmt : self.pixelformats())\n> >> -\t\t\t\tfmts.push_back(fmt.toString());\n> >> -\t\t\treturn fmts;\n> >> -\t\t})\n> >> -\t\t.def(\"sizes\", [](StreamFormats &self, const std::string &pixelFormat) {\n> >> -\t\t\tauto fmt = PixelFormat::fromString(pixelFormat);\n> >> +\t\t.def_property_readonly(\"pixel_formats\", &StreamFormats::pixelformats)\n> >> +\t\t.def(\"sizes\", [](StreamFormats &self, const PixelFormat &pixelFormat) {\n> >> \t\t\tstd::vector> fmts;\n> >> -\t\t\tfor (const auto &s : self.sizes(fmt))\n> >> +\t\t\tfor (const auto &s : self.sizes(pixelFormat))\n> >> \t\t\t\tfmts.push_back(std::make_tuple(s.width, s.height));\n> >> \t\t\treturn fmts;\n> >> \t\t})\n> >> -\t\t.def(\"range\", [](StreamFormats &self, const std::string &pixelFormat) {\n> >> -\t\t\tauto fmt = PixelFormat::fromString(pixelFormat);\n> >> -\t\t\tconst auto &range = self.range(fmt);\n> >> +\t\t.def(\"range\", [](StreamFormats &self, const PixelFormat &pixelFormat) {\n> >> +\t\t\tconst auto &range = self.range(pixelFormat);\n> >> \t\t\treturn make_tuple(std::make_tuple(range.hStep, range.vStep),\n> >> \t\t\t\t\t std::make_tuple(range.min.width, range.min.height),\n> >> \t\t\t\t\t std::make_tuple(range.max.width, range.max.height));\n> >> @@ -648,4 +634,27 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \tpyColorSpaceRange\n> >> \t\t.value(\"Full\", ColorSpace::Range::Full)\n> >> \t\t.value(\"Limited\", ColorSpace::Range::Limited);\n> >> +\n> >> +\tpyPixelFormat\n> >> +\t\t.def(py::init<>())\n> >> +\t\t.def(py::init())\n> >> +\t\t.def_property_readonly(\"fourcc\", &PixelFormat::fourcc)\n> >> +\t\t.def_property_readonly(\"modifier\", &PixelFormat::modifier)\n> >> +\t\t.def_property_readonly(\"name\", &PixelFormat::toString)\n> >> +\t\t.def(py::self == py::self)\n> >> +\t\t.def(\"__str__\", [](const PixelFormat &self) {\n> >> +\t\t\treturn \"\";\n> >> +\t\t})\n> > \n> > I'd just return self.toString() here. We could also implement a __repr__\n> > that prints \"libcamera.PixelFormat('\" + self.toString() + \"')\" if we add\n> > a constructor that takes a string as __repr__ is supposed to return a\n> > canonical string representation that can be run to recreate the object.\n> \n> Yes. I missed __repr__ here. I think I implemented __str__ and __repr__ \n> better with the geometry classes. I'll improve this.\n> \n> >> +\t\t.def_static(\"from_name\", [](const std::string &name) {\n> > \n> > Could we name this \"from_string\" to avoid diverging from the C++ API ?\n> \n> We could, but... The reason I didn't use \"string\" was that fourcc is a \n> string too. I used \"name\" in the property above too.\n> \n> But I guess the libcamera strategy is to never expose fourcc as a \n> string? It's always uint32_t if it's fourcc, or string if it's the \"long \n> name\" of the format?\n> \n> I've always used fourcc as a string (e.g. in kms++) as it's easy, but \n> perhaps the above is a better approach.\n\nCorrect, in libcamera we don't expose the fourcc as a string. A pixel\nformat is defined as a combination of a fourcc and modifiers, so I think\nhandling fourccs as string could easily get misleading. The only string\nfor pixel formats is their canonical name.\n\n> >> +\t\t\treturn PixelFormat::fromString(name);\n> >> +\t\t})\n> >> +\t\t.def_static(\"from_fourcc\", [](const std::string &fourcc) {\n> >> +\t\t\tif (fourcc.size() != 4)\n> >> +\t\t\t\tthrow std::invalid_argument(\"Invalid fourcc length\");\n> >> +\n> >> +\t\t\tuint32_t v = fourcc[0] | (fourcc[1] << 8) |\n> >> +\t\t\t\t (fourcc[2] << 16) | (fourcc[3] << 24);\n> >> +\n> >> +\t\t\treturn PixelFormat(v);\n> >> +\t\t});\n> > \n> > This isn't used in your series, so I'd drop it.\n> \n> True.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E930FC3256\n\tfor ;\n\tTue, 17 May 2022 08:58:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 580BC6565C;\n\tTue, 17 May 2022 10:58:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FDF760420\n\tfor ;\n\tTue, 17 May 2022 10:58:08 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3B4092A5;\n\tTue, 17 May 2022 10:58:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652777889;\n\tbh=hpFdTlVrJ28IATCCXqbuQVxxLMP6nML8IW5LKvQ1X5o=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=LhV8BZKVAEESl9kfh8oX6eiBaFFtFZSaXKf/6hKrvHjCe5pdnPr4GuI/0wrJsptRo\n\tfgLdso8gCeK8H7PETIH18ddOAcOmGBufK0lNoT5x6Bk9+gjpdl08wXB0NFN77qrig2\n\tRzu+anCQ99w2DfcMSkTL3OmmFdGd0EcIDJQtStv45cX4fps86fRVSrZwwVgbmDKNVm\n\thP8WuVvbcTTDNNCcCjKODms//TvRkYpTGbzKUucACaB/54orU0Ug/q/qATnTC85Za9\n\tnwHj27fwzGjAQrKccamf2iitk1UXLvRwiqko2rUCt3ajx44pSKrbAnpYJ/q7uc7LQH\n\tY3dLUzWwq5G6A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652777887;\n\tbh=hpFdTlVrJ28IATCCXqbuQVxxLMP6nML8IW5LKvQ1X5o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lOMIpcZlSww1OoAKqK5P02110rFn0jWo2BdZlz4hAs5Rg6GFSo5u64bFmNWezZdSA\n\tgUiCu3ZLEpDyuZKoNXIUbbrbWDRJCdw2Xl39pCZf+SQ2pIGuClAOOj2qcDoquefH6r\n\t9hEHWrwDJaKvZ+SQXfmYn/d1cUmWKefEquJaO3cs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lOMIpcZl\"; dkim-atps=neutral","Date":"Tue, 17 May 2022 11:58:00 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-11-tomi.valkeinen@ideasonboard.com>\n\t\n\t<9db68f60-6de8-2b46-c728-f97c551eed07@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<9db68f60-6de8-2b46-c728-f97c551eed07@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 10/14] py: implement PixelFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]