[{"id":4048,"web_url":"https://patchwork.libcamera.org/comment/4048/","msgid":"<20200317113445.GH4864@pendragon.ideasonboard.com>","date":"2020-03-17T11:34:45","subject":"Re: [libcamera-devel] [PATCH v2 0/8] libcamera: PixelFormat: Turn\n\tinto a class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Mar 17, 2020 at 04:52:31AM +0100, Niklas Söderlund wrote:\n> Hello,\n> \n> This series replaces the PixelFormat definition of a unisgned int with a\n> class implementation that can hold both a FourCC and a set of modifiers.\n> This is important to allow users of libcamera to look at modifiers.\n> \n> This series do not make use of the modifiers, a follow up series that\n> adds RAW capture to the IPU3 will make use of them to describe the IPU3\n> Bayer format memory layout.\n> \n> I have looked at Laurent's series for V4L2PixelFormat [1] and I agree we \n> should align the interface between the two. I have however not taken all \n> concepts from it (yet). The two outstanding questions for me are\n> \n> 1. operator uint32_t()\n> \n> I agree this looks real nice for V4l2PixelFormat, would it be as nice \n> for PixelFormot which can have modifiers? I have no strong feelings. \n> I feel that if we go for it V4L2PixelFormat::value() and \n> PixelFormat::fourcc() should then be dropped.\n\nWe could drop V4L2PixelFormat::value(), but then when you need to access\nthe numerical value you will need to static_cast(format),\nwhich isn't nice. For PixelFormat I wouldn't drop fourcc(), as it's the\ncounterpart to modifiers().\n\n> Two ways to do the same thing can't be good ;-)\n\nIt's use case-dependent :-) If we have to drop one, I would drop the\nconversion operator, but then you can't write\n\n\tif (format == V4L2_PIX_FMT_YUYV)\n\t\t...\n\nbut will have to write either\n\n\tif (format == V4L2PixelFormat(V4L2_PIX_FMT_YUYV))\n\t\t...\n\nor\n\n\tif (format.value() == V4L2_PIX_FMT_YUYV)\n\t\t...\n\nThe goal of the PixelFormat and V4L2PixelFormat isn't to made writing\ncode more difficult everywhere, but to force the user to check if the\nformat they're using is actually of the right type. There's value in\nforcing replacement of\n\n\tPixelFormat format = fourcc;\n\nwith\n\n\tPixelFormat format = PixelFormat(fourcc);\n\nas you have to think if fourcc is really a DRM fourcc, not a V4L2\nfourcc. In the cases above though,\n\n\tif (format.value() == V4L2_PIX_FMT_YUYV)\n\t\t...\n\ndoesn't provide any additional value in my opinion, as there's no\nsafeguard against format being a PixelFormat (OK, PixelFormat has\n.fourcc() and format has .value(), but that's easy to overlook). On the\nother hand,\n\n\tif (format == V4L2PixelFormat(V4L2_PIX_FMT_YUYV))\n\t\t...\n\nwould not compile if format was a PixelFormat.\n\nIf we decide to drop the conversion operators, and really want maximum\nsafety, we should thus use the second form of comparison, not the first\none. Note that this wouldn't work for a switch() statement anyway, as\nneither PixelFormat nor V4L2PixelFormat have constexpr constructors, so\nfor that we'll have to use .value() or .fourcc(), which defeats the\npoint a little bit.\n\n> 2. isValid()\n> \n> Is this good or bad, I can't make up my mind. Yet again the modifiers \n> plays in. How would PixelFormat::isValid() deal with \n> DRM_FORMAT_SRGGB10 without any modifiers to describe the memory \n> layout?\n\nThere's a need for a default constructor for the PixelFormat and\nV4L2PixelFormat classes. That constructor sets the fourcc to 0, creating\nan \"invalid\" pixel format. I think it's a good idea to add an isValid()\nfunction to check that, to avoid hardcoding a comparison with 0 in the\ncaller. This effectively makes the fact that an invalid pixel format has\na 0 fourcc an implementation detail, and allows us to change that in the\nfuture if needed. In also makes the code more explicit, as it's clear\nfrom .isValid() that you're checking if the pixel format is valid or\nnot. We could however rename that to isInitialized() but I don't like it\nmuch. Feel free to propose a better name if you think of one.\n\n> 1. [PATCH 0/2] Add a V4L2PixelFormat class\n> \n> Laurent Pinchart (1):\n> libcamera: PixelFormat: Make constructor explicit\n> \n> Niklas Söderlund (7):\n> libcamera: Use PixelFormat instead of unsigned int where appropriate\n> test: v4l2_videodevice: buffer_cache: Use DRM pixel format\n> libcamera: pipeline: vimc: Remove internal usage of ImageFormats\n> libcamera: pipeline: uvcvideo: Translate from V4L2 to DRM pixel\n> formats\n> libcamera: v4l2_videodevice: Remove usage of ImageFormats\n> libcamera: PixelFormat: Turn into a class\n> libcamera: PixelFormat: Mark all function arguments of type\n> PixelFormat as const reference\n> \n> include/libcamera/pixelformats.h | 21 ++++++-\n> include/libcamera/stream.h | 4 +-\n> src/cam/main.cpp | 8 +--\n> src/gstreamer/gstlibcamera-utils.cpp | 18 +++---\n> src/libcamera/include/v4l2_videodevice.h | 7 ++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +--\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 22 +++----\n> src/libcamera/pipeline/uvcvideo.cpp | 19 ++++--\n> src/libcamera/pipeline/vimc.cpp | 20 +++---\n> src/libcamera/pixelformats.cpp | 77 ++++++++++++++++++++++--\n> src/libcamera/stream.cpp | 8 +--\n> src/libcamera/v4l2_videodevice.cpp | 47 +++++++--------\n> src/qcam/format_converter.cpp | 6 +-\n> src/qcam/format_converter.h | 6 +-\n> src/qcam/viewfinder.cpp | 4 +-\n> src/qcam/viewfinder.h | 6 +-\n> src/v4l2/v4l2_camera.cpp | 2 +-\n> src/v4l2/v4l2_camera.h | 2 +-\n> src/v4l2/v4l2_camera_proxy.cpp | 34 +++++------\n> src/v4l2/v4l2_camera_proxy.h | 2 +-\n> test/stream/stream_formats.cpp | 24 ++++----\n> test/v4l2_videodevice/buffer_cache.cpp | 4 +-\n> 22 files changed, 221 insertions(+), 128 deletions(-)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89EEC62923\n\tfor ;\n\tTue, 17 Mar 2020 12:34:51 +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 F16DEF9;\n\tTue, 17 Mar 2020 12:34:50 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584444891;\n\tbh=arUK2UaeU3HG904zGLjiRoZqfAgw92q6wwQ/N0FLIfM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ix47rGS2V9LFNxr5ynojWCXcQkjJObAaNykVo0Wl71+4PkwlYWmMEmNE+ev6zkbHR\n\tI9iS452GEW7lS/KxJs8giJKnsEzDQAWNnzNfmu2/jlNaKFlJN2dI4Cf+jSpClhGGRt\n\tRnDso12pXQR672Q+cY1Q2Jb5KPJvgtGukwaXm88w=","Date":"Tue, 17 Mar 2020 13:34:45 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200317113445.GH4864@pendragon.ideasonboard.com>","References":"<20200317035239.2697679-1-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":"<20200317035239.2697679-1-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 0/8] libcamera: PixelFormat: Turn\n\tinto a 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","X-List-Received-Date":"Tue, 17 Mar 2020 11:34:51 -0000"}}]