[{"id":4887,"web_url":"https://patchwork.libcamera.org/comment/4887/","msgid":"<20200522181120.wwn6o2ktoujjoxjd@uno.localdomain>","date":"2020-05-22T18:11:20","subject":"Re: [libcamera-devel] [PATCH/RFC 00/11] Introduce formats::\n\tnamespace for libcamera pixel formats","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, May 22, 2020 at 05:54:47PM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> This patch series is an attempt to fix a problem caused by a direct\n> dependency on drm_fourcc.h in the libcamera public API.\n>\n> libcamera specifies pixel formats using the DRM pixel format FourCC and\n> modifiers. This requires both internal code and applications to include\n> drm_fourcc.h. For internal code, we carry a copy of the header to avoid\n> external dependencies. For third-party applications, however,\n> drm_fourcc.h needs to be accessible from system includes. It turns out\n> that the file is shipped by two different packages:\n>\n> - libdrm, which typically installs it in /usr/include/libdrm/\n> - kernel headers or libc headers, which typically installs it in\n> /usr/include/drm\n>\n> We don't want to make libdrm a dependency for applications using\n> libcamera. Unfortunately, depending on drm_fourcc.h being provided by\n> the distribution kernel headers or libc headers package causes issues,\n> as not all distributions install the header in /usr/include/drm/. Ubuntu\n> and Gentoo do, but Debian and Arch don't.\n>\n> The best option may be to remove the dependency on the macros provided\n> by drm_fourcc.h, while keeping the pixel format numerical values\n> identical. This is what this patch series attempts to do.\n\nI agree this is a better way forward than depending on the system\nheader.\n\nI skimmed through the series and it looks good but I have a few\nthoughts\n1) format names: now that we have our own formats, how should they be\nnamed? This version uses the DRM defined format names, and it makes\nsense indeed to make it easier for people using a DRM sink to have a\nlibcamera format. Howeverm, afair, gstreamer in example uses fourcc\nnames from fourcc.org, which I have no idea if it's authoritative or\nnot. Any opinion on that ?\n\n2) I suspect the v4l2->libcamera->drm dance on pixel format\ndefinitions we have on-going for RAW formats will repeat itself once\nwe add more platforms. This does not change the current situation\nwhere\n new platform with a new V4L2 fourcc code\n -> we define a format in libcamera associated with a temporary\n DRM fourcc code\n -> We upstream the DRM fourcc code\n -> upstream happens\n <- We have to change the format definition in libcamera,\n which is not an issue now, but once things are more stable\n it could be\n\nI wonder if instead of exposing PixelFormats (which are created using\na DRM fourcc which might not be stable) we should not expose an\nenumeration of fourcc-independent codes, and have a run-time\ntranslation to the drm fourcc ones.. If I look at\n\nconst std::map pixelFormatInfo{\n\t/* RGB formats. */\n\t{ formats::BGR888, {\n\t\t.format = formats::BGR888,\n\t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),\n\t\t.bitsPerPixel = 24,\n\t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n\t\t.packed = false,\n\t} },\n\nThe map keys and the first field of the PixelFormatInfo are actually\nthe same thing..\n\nI know this means defining our own format codes (which might very well\nbe a sequential enumeration), requires run-time translations, and kind\nof defeat using DRM fourcc (or any kind of existing fourcc codes set).\nBut what happens here basically does that already, applications gets\nprovided a set of identifiers, it does just only partially matters\nthat they're consutrcted using DRM fourccs behing the curatins, as I\nexpect applications using DRM to have to look at the header and then\nrealize that they can PixelFormat(DRM_FORMAT_xyz) instead of having to\nlook at what libcamera::formats::xys actually is...\n\nIs run-time translation frowned upon in you opinion ?\n\n> Patches 01/11 to 04/11 are preparatory cleanups, please see individual\n> patches for details. I believe they make sense regardless of whether the\n> rest is accepted or rejected.\n\nI think these could be sent already as PATCH\n\nThanks\n j\n>\n> Patch 05/11 creates a new libcamera::formats:: namespace and populates\n> it with constants (in the form of constexpr) for all supported pixel\n> formats. The values have been written manually, which isn't ideal. We\n> could partly automate this process by generating the header from a list\n> of formats (likely in a YAML file), and fetching the numerical values\n> from drm_fourcc.h to make sure they will always match. This could allow\n> documenting the pixel formats in the YAML file and generating Doxygen\n> documentation, like we do for controls.\n>\n> Patches 06/11 to 11/11 then replace usage of the DRM_FORMAT_* macros\n> through the code, with one exception in the IPU3 pipeline handler to\n> demonstrate how DRM_FORMAT_* values can still be used for internal\n> formats. I however believe this will be dropped too, as applications may\n> want to capture RAW data from the IPU3, so the format should likely be\n> defined in the public API.\n>\n> Laurent Pinchart (11):\n> libcamera: Rename pixelformats.{cpp,h} to pixel_format.{cpp,h}\n> libcamera: Replace C++ comments with C comments\n> libcamera: Rename header guards for internal headers\n> libcamera: pixel_format: Make PixelFormat usable as a constexpr\n> libcamera: Define constants for pixel formats in the public API\n> gst: Replace explicit DRM FourCCs with libcamera formats\n> qcam: Replace explicit DRM FourCCs with libcamera formats\n> v4l2: Replace explicit DRM FourCCs with libcamera formats\n> test: Replace explicit DRM FourCCs with libcamera formats\n> libcamera: pipeline: Replace explicit DRM FourCCs with libcamera\n> formats\n> libcamera: Replace explicit DRM FourCCs with libcamera formats\n>\n> include/libcamera/control_ids.h.in | 4 +-\n> include/libcamera/formats.h | 94 +++++++++++\n> .../libcamera/internal/byte_stream_buffer.h | 6 +-\n> include/libcamera/internal/camera_controls.h | 6 +-\n> include/libcamera/internal/camera_sensor.h | 6 +-\n> .../libcamera/internal/control_serializer.h | 6 +-\n> .../libcamera/internal/control_validator.h | 6 +-\n> .../libcamera/internal/device_enumerator.h | 6 +-\n> .../internal/device_enumerator_sysfs.h | 6 +-\n> .../internal/device_enumerator_udev.h | 6 +-\n> .../internal/event_dispatcher_poll.h | 6 +-\n> include/libcamera/internal/file.h | 6 +-\n> include/libcamera/internal/formats.h | 8 +-\n> .../libcamera/internal/ipa_context_wrapper.h | 6 +-\n> include/libcamera/internal/ipa_manager.h | 6 +-\n> include/libcamera/internal/ipa_module.h | 6 +-\n> include/libcamera/internal/ipa_proxy.h | 6 +-\n> include/libcamera/internal/ipc_unixsocket.h | 6 +-\n> include/libcamera/internal/log.h | 6 +-\n> include/libcamera/internal/media_device.h | 6 +-\n> include/libcamera/internal/media_object.h | 6 +-\n> include/libcamera/internal/message.h | 6 +-\n> include/libcamera/internal/pipeline_handler.h | 6 +-\n> include/libcamera/internal/process.h | 6 +-\n> include/libcamera/internal/pub_key.h | 6 +-\n> include/libcamera/internal/semaphore.h | 6 +-\n> include/libcamera/internal/thread.h | 6 +-\n> include/libcamera/internal/utils.h | 6 +-\n> include/libcamera/internal/v4l2_controls.h | 6 +-\n> include/libcamera/internal/v4l2_device.h | 6 +-\n> include/libcamera/internal/v4l2_pixelformat.h | 8 +-\n> include/libcamera/internal/v4l2_subdevice.h | 6 +-\n> include/libcamera/internal/v4l2_videodevice.h | 8 +-\n> include/libcamera/meson.build | 3 +-\n> include/libcamera/pixel_format.h | 48 ++++++\n> include/libcamera/pixelformats.h | 43 -----\n> include/libcamera/property_ids.h.in | 6 +-\n> include/libcamera/stream.h | 2 +-\n> src/gstreamer/gstlibcamera-utils.cpp | 62 ++++----\n> src/libcamera/formats.cpp | 148 +++++++++---------\n> src/libcamera/meson.build | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 10 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +--\n> src/libcamera/pipeline/simple/converter.h | 2 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 11 +-\n> .../{pixelformats.cpp => pixel_format.cpp} | 19 +--\n> src/libcamera/v4l2_pixelformat.cpp | 85 +++++-----\n> src/qcam/dng_writer.cpp | 17 +-\n> src/qcam/format_converter.cpp | 36 +++--\n> src/qcam/format_converter.h | 2 +-\n> src/qcam/viewfinder.cpp | 10 +-\n> src/qcam/viewfinder.h | 2 +-\n> src/v4l2/v4l2_camera_proxy.cpp | 27 ++--\n> test/v4l2_videodevice/buffer_cache.cpp | 3 +-\n> 55 files changed, 477 insertions(+), 378 deletions(-)\n> create mode 100644 include/libcamera/formats.h\n> create mode 100644 include/libcamera/pixel_format.h\n> delete mode 100644 include/libcamera/pixelformats.h\n> rename src/libcamera/{pixelformats.cpp => pixel_format.cpp} (89%)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9ADE603D7\n\tfor ;\n\tFri, 22 May 2020 20:08:04 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 62FF71C0005;\n\tFri, 22 May 2020 18:08:04 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 22 May 2020 20:11:20 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200522181120.wwn6o2ktoujjoxjd@uno.localdomain>","References":"<20200522145459.16836-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200522145459.16836-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH/RFC 00/11] Introduce formats::\n\tnamespace for libcamera pixel formats","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":"Fri, 22 May 2020 18:08:05 -0000"}},{"id":4889,"web_url":"https://patchwork.libcamera.org/comment/4889/","msgid":"<20200522204710.GH5824@pendragon.ideasonboard.com>","date":"2020-05-22T20:47:10","subject":"Re: [libcamera-devel] [PATCH/RFC 00/11] Introduce formats::\n\tnamespace for libcamera pixel formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, May 22, 2020 at 08:11:20PM +0200, Jacopo Mondi wrote:\n> On Fri, May 22, 2020 at 05:54:47PM +0300, Laurent Pinchart wrote:\n> > Hello,\n> >\n> > This patch series is an attempt to fix a problem caused by a direct\n> > dependency on drm_fourcc.h in the libcamera public API.\n> >\n> > libcamera specifies pixel formats using the DRM pixel format FourCC and\n> > modifiers. This requires both internal code and applications to include\n> > drm_fourcc.h. For internal code, we carry a copy of the header to avoid\n> > external dependencies. For third-party applications, however,\n> > drm_fourcc.h needs to be accessible from system includes. It turns out\n> > that the file is shipped by two different packages:\n> >\n> > - libdrm, which typically installs it in /usr/include/libdrm/\n> > - kernel headers or libc headers, which typically installs it in\n> > /usr/include/drm\n> >\n> > We don't want to make libdrm a dependency for applications using\n> > libcamera. Unfortunately, depending on drm_fourcc.h being provided by\n> > the distribution kernel headers or libc headers package causes issues,\n> > as not all distributions install the header in /usr/include/drm/. Ubuntu\n> > and Gentoo do, but Debian and Arch don't.\n> >\n> > The best option may be to remove the dependency on the macros provided\n> > by drm_fourcc.h, while keeping the pixel format numerical values\n> > identical. This is what this patch series attempts to do.\n> \n> I agree this is a better way forward than depending on the system\n> header.\n> \n> I skimmed through the series and it looks good but I have a few\n> thoughts\n> 1) format names: now that we have our own formats, how should they be\n> named? This version uses the DRM defined format names, and it makes\n> sense indeed to make it easier for people using a DRM sink to have a\n> libcamera format. Howeverm, afair, gstreamer in example uses fourcc\n> names from fourcc.org, which I have no idea if it's authoritative or\n> not. Any opinion on that ?\n\nI don't think there's any authoritative list of pixel format names\n(otherwise we would have an authoritative list of 4CC values too, and we\nwouldn't need to deal with this mess :-)). As we use the same numerical\nvalues as DRM, I think it makes sense to use similar names. I've written\n\"similar\" and not \"identical\" on purpose, as DRM has names for 4CCs and\nfor modifiers, but not for pixel formats, so we'll need to create our\nown names for modified formats (SGRBG10_CSI2P is an example of such a\nname introduced in this series).\n\n> 2) I suspect the v4l2->libcamera->drm dance on pixel format\n> definitions we have on-going for RAW formats will repeat itself once\n> we add more platforms. This does not change the current situation\n> where\n> new platform with a new V4L2 fourcc code\n> -> we define a format in libcamera associated with a temporary\n> DRM fourcc code\n> -> We upstream the DRM fourcc code\n> -> upstream happens\n> <- We have to change the format definition in libcamera,\n> which is not an issue now, but once things are more stable\n> it could be\n> \n> I wonder if instead of exposing PixelFormats (which are created using\n> a DRM fourcc which might not be stable) we should not expose an\n> enumeration of fourcc-independent codes, and have a run-time\n> translation to the drm fourcc ones.. If I look at\n> \n> const std::map pixelFormatInfo{\n> \t/* RGB formats. */\n> \t{ formats::BGR888, {\n> \t\t.format = formats::BGR888,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),\n> \t\t.bitsPerPixel = 24,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> \t\t.packed = false,\n> \t} },\n> \n> The map keys and the first field of the PixelFormatInfo are actually\n> the same thing..\n> \n> I know this means defining our own format codes (which might very well\n> be a sequential enumeration), requires run-time translations, and kind\n> of defeat using DRM fourcc (or any kind of existing fourcc codes set).\n> But what happens here basically does that already, applications gets\n> provided a set of identifiers, it does just only partially matters\n> that they're consutrcted using DRM fourccs behing the curatins, as I\n> expect applications using DRM to have to look at the header and then\n> realize that they can PixelFormat(DRM_FORMAT_xyz) instead of having to\n> look at what libcamera::formats::xys actually is...\n> \n> Is run-time translation frowned upon in you opinion ?\n\nI'd personally frown upon yet another set of pixel formats with yet\nanother translation. We have too many of them already. I'm open to\nalternatives, but only if the alternative make things simpler, not more\ncomplicated :-)\n\nAs for you concern above, once we start caring about stability, we'll\nneed to have kernel drivers merged before merging the corresponding code\nin libcamera, regardless of whether we use DRM pixel formats or our own\npixel formats anyway.\n\nI'd be willing to consider another set of pixel format values if they\nwere used by a relevant kernel API and were pushed as the new standard.\n\n> > Patches 01/11 to 04/11 are preparatory cleanups, please see individual\n> > patches for details. I believe they make sense regardless of whether the\n> > rest is accepted or rejected.\n> \n> I think these could be sent already as PATCH\n\nFeel free to review them :-) I've submitted them as part of this series\nto avoid creating dependencies between two patch series, but I'd be\nhappy to merge them already while we discuss the rest.\n\n> > Patch 05/11 creates a new libcamera::formats:: namespace and populates\n> > it with constants (in the form of constexpr) for all supported pixel\n> > formats. The values have been written manually, which isn't ideal. We\n> > could partly automate this process by generating the header from a list\n> > of formats (likely in a YAML file), and fetching the numerical values\n> > from drm_fourcc.h to make sure they will always match. This could allow\n> > documenting the pixel formats in the YAML file and generating Doxygen\n> > documentation, like we do for controls.\n> >\n> > Patches 06/11 to 11/11 then replace usage of the DRM_FORMAT_* macros\n> > through the code, with one exception in the IPU3 pipeline handler to\n> > demonstrate how DRM_FORMAT_* values can still be used for internal\n> > formats. I however believe this will be dropped too, as applications may\n> > want to capture RAW data from the IPU3, so the format should likely be\n> > defined in the public API.\n> >\n> > Laurent Pinchart (11):\n> > libcamera: Rename pixelformats.{cpp,h} to pixel_format.{cpp,h}\n> > libcamera: Replace C++ comments with C comments\n> > libcamera: Rename header guards for internal headers\n> > libcamera: pixel_format: Make PixelFormat usable as a constexpr\n> > libcamera: Define constants for pixel formats in the public API\n> > gst: Replace explicit DRM FourCCs with libcamera formats\n> > qcam: Replace explicit DRM FourCCs with libcamera formats\n> > v4l2: Replace explicit DRM FourCCs with libcamera formats\n> > test: Replace explicit DRM FourCCs with libcamera formats\n> > libcamera: pipeline: Replace explicit DRM FourCCs with libcamera\n> > formats\n> > libcamera: Replace explicit DRM FourCCs with libcamera formats\n> >\n> > include/libcamera/control_ids.h.in | 4 +-\n> > include/libcamera/formats.h | 94 +++++++++++\n> > .../libcamera/internal/byte_stream_buffer.h | 6 +-\n> > include/libcamera/internal/camera_controls.h | 6 +-\n> > include/libcamera/internal/camera_sensor.h | 6 +-\n> > .../libcamera/internal/control_serializer.h | 6 +-\n> > .../libcamera/internal/control_validator.h | 6 +-\n> > .../libcamera/internal/device_enumerator.h | 6 +-\n> > .../internal/device_enumerator_sysfs.h | 6 +-\n> > .../internal/device_enumerator_udev.h | 6 +-\n> > .../internal/event_dispatcher_poll.h | 6 +-\n> > include/libcamera/internal/file.h | 6 +-\n> > include/libcamera/internal/formats.h | 8 +-\n> > .../libcamera/internal/ipa_context_wrapper.h | 6 +-\n> > include/libcamera/internal/ipa_manager.h | 6 +-\n> > include/libcamera/internal/ipa_module.h | 6 +-\n> > include/libcamera/internal/ipa_proxy.h | 6 +-\n> > include/libcamera/internal/ipc_unixsocket.h | 6 +-\n> > include/libcamera/internal/log.h | 6 +-\n> > include/libcamera/internal/media_device.h | 6 +-\n> > include/libcamera/internal/media_object.h | 6 +-\n> > include/libcamera/internal/message.h | 6 +-\n> > include/libcamera/internal/pipeline_handler.h | 6 +-\n> > include/libcamera/internal/process.h | 6 +-\n> > include/libcamera/internal/pub_key.h | 6 +-\n> > include/libcamera/internal/semaphore.h | 6 +-\n> > include/libcamera/internal/thread.h | 6 +-\n> > include/libcamera/internal/utils.h | 6 +-\n> > include/libcamera/internal/v4l2_controls.h | 6 +-\n> > include/libcamera/internal/v4l2_device.h | 6 +-\n> > include/libcamera/internal/v4l2_pixelformat.h | 8 +-\n> > include/libcamera/internal/v4l2_subdevice.h | 6 +-\n> > include/libcamera/internal/v4l2_videodevice.h | 8 +-\n> > include/libcamera/meson.build | 3 +-\n> > include/libcamera/pixel_format.h | 48 ++++++\n> > include/libcamera/pixelformats.h | 43 -----\n> > include/libcamera/property_ids.h.in | 6 +-\n> > include/libcamera/stream.h | 2 +-\n> > src/gstreamer/gstlibcamera-utils.cpp | 62 ++++----\n> > src/libcamera/formats.cpp | 148 +++++++++---------\n> > src/libcamera/meson.build | 2 +-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +-\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +--\n> > src/libcamera/pipeline/simple/converter.h | 2 +-\n> > src/libcamera/pipeline/vimc/vimc.cpp | 11 +-\n> > .../{pixelformats.cpp => pixel_format.cpp} | 19 +--\n> > src/libcamera/v4l2_pixelformat.cpp | 85 +++++-----\n> > src/qcam/dng_writer.cpp | 17 +-\n> > src/qcam/format_converter.cpp | 36 +++--\n> > src/qcam/format_converter.h | 2 +-\n> > src/qcam/viewfinder.cpp | 10 +-\n> > src/qcam/viewfinder.h | 2 +-\n> > src/v4l2/v4l2_camera_proxy.cpp | 27 ++--\n> > test/v4l2_videodevice/buffer_cache.cpp | 3 +-\n> > 55 files changed, 477 insertions(+), 378 deletions(-)\n> > create mode 100644 include/libcamera/formats.h\n> > create mode 100644 include/libcamera/pixel_format.h\n> > delete mode 100644 include/libcamera/pixelformats.h\n> > rename src/libcamera/{pixelformats.cpp => pixel_format.cpp} (89%)","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 BAF06603D7\n\tfor ;\n\tFri, 22 May 2020 22:47:22 +0200 (CEST)","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 37F3BB55;\n\tFri, 22 May 2020 22:47:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GfqiOPax\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1590180442;\n\tbh=zJr10PxB2u5LvrP2jTDGXjpZU5nHm9wilsMOY04Er3U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GfqiOPaxSj24GSoawAGJAVFc+Dm89sTSayfCpFPasUna0N0+zniNWKATAV1knNEDM\n\tKOezy7VOovY4qfrvcr227EddPVYBdyh4DG0CEqMpVPjHhgmr4+km2jcX9FMRNPMY1H\n\tnbnrTn3K0wJ9Y8Id4xRcZznhXiRIeY1OL2kTGANA=","Date":"Fri, 22 May 2020 23:47:10 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200522204710.GH5824@pendragon.ideasonboard.com>","References":"<20200522145459.16836-1-laurent.pinchart@ideasonboard.com>\n\t<20200522181120.wwn6o2ktoujjoxjd@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200522181120.wwn6o2ktoujjoxjd@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH/RFC 00/11] Introduce formats::\n\tnamespace for libcamera pixel formats","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":"Fri, 22 May 2020 20:47:23 -0000"}},{"id":4892,"web_url":"https://patchwork.libcamera.org/comment/4892/","msgid":"<2f6cb7d11441187513d64a16bada4138879d3e57.camel@ndufresne.ca>","date":"2020-05-22T22:04:05","subject":"Re: [libcamera-devel] [PATCH/RFC 00/11] Introduce formats::\n\tnamespace for libcamera pixel formats","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le vendredi 22 mai 2020 à 20:11 +0200, Jacopo Mondi a écrit :\n> Hi Laurent,\n> \n> On Fri, May 22, 2020 at 05:54:47PM +0300, Laurent Pinchart wrote:\n> > Hello,\n> > \n> > This patch series is an attempt to fix a problem caused by a direct\n> > dependency on drm_fourcc.h in the libcamera public API.\n> > \n> > libcamera specifies pixel formats using the DRM pixel format FourCC and\n> > modifiers. This requires both internal code and applications to include\n> > drm_fourcc.h. For internal code, we carry a copy of the header to avoid\n> > external dependencies. For third-party applications, however,\n> > drm_fourcc.h needs to be accessible from system includes. It turns out\n> > that the file is shipped by two different packages:\n> > \n> > - libdrm, which typically installs it in /usr/include/libdrm/\n> > - kernel headers or libc headers, which typically installs it in\n> > /usr/include/drm\n> > \n> > We don't want to make libdrm a dependency for applications using\n> > libcamera. Unfortunately, depending on drm_fourcc.h being provided by\n> > the distribution kernel headers or libc headers package causes issues,\n> > as not all distributions install the header in /usr/include/drm/. Ubuntu\n> > and Gentoo do, but Debian and Arch don't.\n> > \n> > The best option may be to remove the dependency on the macros provided\n> > by drm_fourcc.h, while keeping the pixel format numerical values\n> > identical. This is what this patch series attempts to do.\n> \n> I agree this is a better way forward than depending on the system\n> header.\n> \n> I skimmed through the series and it looks good but I have a few\n> thoughts\n> 1) format names: now that we have our own formats, how should they be\n> named? This version uses the DRM defined format names, and it makes\n> sense indeed to make it easier for people using a DRM sink to have a\n> libcamera format. Howeverm, afair, gstreamer in example uses fourcc\n> names from fourcc.org, which I have no idea if it's authoritative or\n> not. Any opinion on that ?\n\nfourcc.org is no authority no. GStreamer uses a mix of Microsoft\ndefined fourcc, Apple define fourcc and fourcc.org. This has to do with\nfourcc found in ISOMP4, Matroska and other containers (even though you\ncannot assume consistencies, and there is even few duplicates coming\nfrom errors in major software). The rest is very custom, we never\nmanaged to actually found a working rules.\n\nNewer formats like I422_10BE, NV12_64Z32, NV12_10LE32 capture more\ninformation but pretty much all fails in one way to capture everything\na really be consistent.\n\nI think FFMPEG now has some documented rules, but I'm not certain.\n\n> \n> 2) I suspect the v4l2->libcamera->drm dance on pixel format\n> definitions we have on-going for RAW formats will repeat itself once\n> we add more platforms. This does not change the current situation\n> where\n> new platform with a new V4L2 fourcc code\n> -> we define a format in libcamera associated with a temporary\n> DRM fourcc code\n> -> We upstream the DRM fourcc code\n> -> upstream happens\n> <- We have to change the format definition in libcamera,\n> which is not an issue now, but once things are more stable\n> it could be\n> \n> I wonder if instead of exposing PixelFormats (which are created using\n> a DRM fourcc which might not be stable) we should not expose an\n> enumeration of fourcc-independent codes, and have a run-time\n> translation to the drm fourcc ones.. If I look at\n> \n> const std::map pixelFormatInfo{\n> \t/* RGB formats. */\n> \t{ formats::BGR888, {\n> \t\t.format = formats::BGR888,\n> \t\t.v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_RGB24),\n> \t\t.bitsPerPixel = 24,\n> \t\t.colourEncoding = PixelFormatInfo::ColourEncodingRGB,\n> \t\t.packed = false,\n> \t} },\n> \n> The map keys and the first field of the PixelFormatInfo are actually\n> the same thing..\n> \n> I know this means defining our own format codes (which might very well\n> be a sequential enumeration), requires run-time translations, and kind\n> of defeat using DRM fourcc (or any kind of existing fourcc codes set).\n> But what happens here basically does that already, applications gets\n> provided a set of identifiers, it does just only partially matters\n> that they're consutrcted using DRM fourccs behing the curatins, as I\n> expect applications using DRM to have to look at the header and then\n> realize that they can PixelFormat(DRM_FORMAT_xyz) instead of having to\n> look at what libcamera::formats::xys actually is...\n> \n> Is run-time translation frowned upon in you opinion ?\n> \n> > Patches 01/11 to 04/11 are preparatory cleanups, please see individual\n> > patches for details. I believe they make sense regardless of whether the\n> > rest is accepted or rejected.\n> \n> I think these could be sent already as PATCH\n> \n> Thanks\n> j\n> > Patch 05/11 creates a new libcamera::formats:: namespace and populates\n> > it with constants (in the form of constexpr) for all supported pixel\n> > formats. The values have been written manually, which isn't ideal. We\n> > could partly automate this process by generating the header from a list\n> > of formats (likely in a YAML file), and fetching the numerical values\n> > from drm_fourcc.h to make sure they will always match. This could allow\n> > documenting the pixel formats in the YAML file and generating Doxygen\n> > documentation, like we do for controls.\n> > \n> > Patches 06/11 to 11/11 then replace usage of the DRM_FORMAT_* macros\n> > through the code, with one exception in the IPU3 pipeline handler to\n> > demonstrate how DRM_FORMAT_* values can still be used for internal\n> > formats. I however believe this will be dropped too, as applications may\n> > want to capture RAW data from the IPU3, so the format should likely be\n> > defined in the public API.\n> > \n> > Laurent Pinchart (11):\n> > libcamera: Rename pixelformats.{cpp,h} to pixel_format.{cpp,h}\n> > libcamera: Replace C++ comments with C comments\n> > libcamera: Rename header guards for internal headers\n> > libcamera: pixel_format: Make PixelFormat usable as a constexpr\n> > libcamera: Define constants for pixel formats in the public API\n> > gst: Replace explicit DRM FourCCs with libcamera formats\n> > qcam: Replace explicit DRM FourCCs with libcamera formats\n> > v4l2: Replace explicit DRM FourCCs with libcamera formats\n> > test: Replace explicit DRM FourCCs with libcamera formats\n> > libcamera: pipeline: Replace explicit DRM FourCCs with libcamera\n> > formats\n> > libcamera: Replace explicit DRM FourCCs with libcamera formats\n> > \n> > include/libcamera/control_ids.h.in | 4 +-\n> > include/libcamera/formats.h | 94 +++++++++++\n> > .../libcamera/internal/byte_stream_buffer.h | 6 +-\n> > include/libcamera/internal/camera_controls.h | 6 +-\n> > include/libcamera/internal/camera_sensor.h | 6 +-\n> > .../libcamera/internal/control_serializer.h | 6 +-\n> > .../libcamera/internal/control_validator.h | 6 +-\n> > .../libcamera/internal/device_enumerator.h | 6 +-\n> > .../internal/device_enumerator_sysfs.h | 6 +-\n> > .../internal/device_enumerator_udev.h | 6 +-\n> > .../internal/event_dispatcher_poll.h | 6 +-\n> > include/libcamera/internal/file.h | 6 +-\n> > include/libcamera/internal/formats.h | 8 +-\n> > .../libcamera/internal/ipa_context_wrapper.h | 6 +-\n> > include/libcamera/internal/ipa_manager.h | 6 +-\n> > include/libcamera/internal/ipa_module.h | 6 +-\n> > include/libcamera/internal/ipa_proxy.h | 6 +-\n> > include/libcamera/internal/ipc_unixsocket.h | 6 +-\n> > include/libcamera/internal/log.h | 6 +-\n> > include/libcamera/internal/media_device.h | 6 +-\n> > include/libcamera/internal/media_object.h | 6 +-\n> > include/libcamera/internal/message.h | 6 +-\n> > include/libcamera/internal/pipeline_handler.h | 6 +-\n> > include/libcamera/internal/process.h | 6 +-\n> > include/libcamera/internal/pub_key.h | 6 +-\n> > include/libcamera/internal/semaphore.h | 6 +-\n> > include/libcamera/internal/thread.h | 6 +-\n> > include/libcamera/internal/utils.h | 6 +-\n> > include/libcamera/internal/v4l2_controls.h | 6 +-\n> > include/libcamera/internal/v4l2_device.h | 6 +-\n> > include/libcamera/internal/v4l2_pixelformat.h | 8 +-\n> > include/libcamera/internal/v4l2_subdevice.h | 6 +-\n> > include/libcamera/internal/v4l2_videodevice.h | 8 +-\n> > include/libcamera/meson.build | 3 +-\n> > include/libcamera/pixel_format.h | 48 ++++++\n> > include/libcamera/pixelformats.h | 43 -----\n> > include/libcamera/property_ids.h.in | 6 +-\n> > include/libcamera/stream.h | 2 +-\n> > src/gstreamer/gstlibcamera-utils.cpp | 62 ++++----\n> > src/libcamera/formats.cpp | 148 +++++++++---------\n> > src/libcamera/meson.build | 2 +-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +-\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 10 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 +--\n> > src/libcamera/pipeline/simple/converter.h | 2 +-\n> > src/libcamera/pipeline/vimc/vimc.cpp | 11 +-\n> > .../{pixelformats.cpp => pixel_format.cpp} | 19 +--\n> > src/libcamera/v4l2_pixelformat.cpp | 85 +++++-----\n> > src/qcam/dng_writer.cpp | 17 +-\n> > src/qcam/format_converter.cpp | 36 +++--\n> > src/qcam/format_converter.h | 2 +-\n> > src/qcam/viewfinder.cpp | 10 +-\n> > src/qcam/viewfinder.h | 2 +-\n> > src/v4l2/v4l2_camera_proxy.cpp | 27 ++--\n> > test/v4l2_videodevice/buffer_cache.cpp | 3 +-\n> > 55 files changed, 477 insertions(+), 378 deletions(-)\n> > create mode 100644 include/libcamera/formats.h\n> > create mode 100644 include/libcamera/pixel_format.h\n> > delete mode 100644 include/libcamera/pixelformats.h\n> > rename src/libcamera/{pixelformats.cpp => pixel_format.cpp} (89%)\n> > \n> > --\n> > Regards,\n> > \n> > Laurent Pinchart\n> > \n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-qt1-x835.google.com (mail-qt1-x835.google.com\n\t[IPv6:2607:f8b0:4864:20::835])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1992D60DF9\n\tfor ;\n\tSat, 23 May 2020 00:04:09 +0200 (CEST)","by mail-qt1-x835.google.com with SMTP id m64so9561475qtd.4\n\tfor ;\n\tFri, 22 May 2020 15:04:08 -0700 (PDT)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\ti184sm563637qke.82.2020.05.22.15.04.06\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 22 May 2020 15:04:06 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20150623.gappssmtp.com\n\theader.i=@ndufresne-ca.20150623.gappssmtp.com header.b=\"Vj6+y8ff\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=uld/3n7AjsVIqAGVKxA/1sPEcTCKqwaaSJkWE6rYV9w=;\n\tb=Vj6+y8ffpdNbYbHd/cqU47mvtO4z3TZUdyRmQQeU5Q+sRHcosiRahboqX7+G5uGAOU\n\tjrVKV6EMSUIB6qGZLnxAZtLEV7IUh2fk0SE72uaG8So/9n685G9+xWof/rTw2L6geEkS\n\tzzG5XQQ2ZqVHFxqMiGei2J5Aga6p/QfJ6IJly1okhUgedUnxGGoz3VctyQE5ZlM441Cu\n\ttJcqqQNjZy2WKf3dv/f4nqsfKNLPFxTkukcQ6YUMLR+GQvmX/2vdzt7FM4URjtJfd6Xw\n\tdYIAShGoTi3NSig2LKlzZ7Kor803Lg9SC39HXLTYf1anncgptn5+BoES5XsC+Ksca0cD\n\ta7Hg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=uld/3n7AjsVIqAGVKxA/1sPEcTCKqwaaSJkWE6rYV9w=;\n\tb=MKMza4IGaU3yaP8nPWdRZIBbLo/0EKlpYSFgK/6DOzwXviK/WS14pCL5DsASuxbp4J\n\tlBFxBQcpeHolcvZkw7FM3FRZba6aSCxkn6Z4YihM0s7shIB78ILfAU8TrvdZBZ9Uz/mp\n\tLBAWRbtbZMITfeJLeDjWLuKq1UmQ1xXvMgapnVevpSTcJs7LBaN28co/1c3+OQk2Kqj3\n\tuQDNNBbh14ybN6YLYRyAScWz9Rz3j3AYyFvo/QtIWNiPRsFQx+AgxQjBsPoRiIo/F5zE\n\t3Sj22txnudMJdvXKcBILPpm8zzu2bi6mFFG/VFx8s/rAiE8th5IordkgYFim/QH3m/ZO\n\t8A8w==","X-Gm-Message-State":"AOAM531DdOjkKZwN/F6eIIiixgLl3y+6yN2vgrqwOaeJSWfRmjVRQ0Kh\n\tQCEzD7Txh0G6CLKygB/6ETRVWA==","X-Google-Smtp-Source":"ABdhPJzRNL0N8Qo+1A7PYD7MLnvTN8pIyUHRiAJARSbMDOeO4KDGUwNahEe2GuhIeHZliLvv//+25w==","X-Received":"by 2002:ac8:6cf:: with SMTP id\n\tj15mr18112069qth.143.1590185047645; \n\tFri, 22 May 2020 15:04:07 -0700 (PDT)","Message-ID":"<2f6cb7d11441187513d64a16bada4138879d3e57.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Jacopo Mondi , Laurent Pinchart\n\t","Cc":"libcamera-devel@lists.libcamera.org","Date":"Fri, 22 May 2020 18:04:05 -0400","In-Reply-To":"<20200522181120.wwn6o2ktoujjoxjd@uno.localdomain>","References":"<20200522145459.16836-1-laurent.pinchart@ideasonboard.com>\n\t<20200522181120.wwn6o2ktoujjoxjd@uno.localdomain>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.36.2 (3.36.2-1.fc32) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH/RFC 00/11] Introduce formats::\n\tnamespace for libcamera pixel formats","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":"Fri, 22 May 2020 22:04:09 -0000"}}]