[{"id":3174,"web_url":"https://patchwork.libcamera.org/comment/3174/","msgid":"<20191204154012.GB7811@pendragon.ideasonboard.com>","date":"2019-12-04T15:40:12","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: properties: Generate\n\tlibcamera properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Dec 04, 2019 at 02:20:59PM +0100, Jacopo Mondi wrote:\n> Re-use the Control generation infrastructure to generate libcamera properties.\n> \n> Introduce three additional files, one that enumerates the properties ids\n> (include/libcamera/property_ids.h) and one the defines Control<> instances,\n> one for each property (src/libcamera/property_ids.cpp) and provide\n> properties definitions in src/libcamera/property_ids.yaml\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/meson.build | 9 ++++++\n> include/libcamera/property_ids.h.in | 33 ++++++++++++++++++++++\n> src/libcamera/meson.build | 6 ++++\n> src/libcamera/property_ids.cpp.in | 43 +++++++++++++++++++++++++++++\n> src/libcamera/property_ids.yaml | 34 +++++++++++++++++++++++\n> 5 files changed, 125 insertions(+)\n> create mode 100644 include/libcamera/property_ids.h.in\n> create mode 100644 src/libcamera/property_ids.cpp.in\n> create mode 100644 src/libcamera/property_ids.yaml\n> \n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 99abf0609940..0ec32ad84c96 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -31,7 +31,16 @@ control_ids_h = custom_target('control_ids_h',\n> install : true,\n> install_dir : join_paths('include', include_dir))\n> \n> +property_ids_h = custom_target('property_ids_h',\n> + input : files('../../src/libcamera/property_ids.yaml', 'property_ids.h.in'),\n\nIt would be lovely if we could avoid the ../.., but that's not a problem\nintroduced by this patch. I wonder if it would make sense to move the\nyaml files to a separate directory at some point. No need to care about\nit now, it's just a random comment.\n\n> + output : 'property_ids.h',\n> + depend_files : gen_controls,\n> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> + install : true,\n> + install_dir : join_paths('include', include_dir))\n> +\n> libcamera_api += control_ids_h\n> +libcamera_api += property_ids_h\n\nIf we want to follow the 0, 1, N rule, this could be\n\ncontrol_headers_sources = [\n 'control_ids.h',\n 'property_ids.h',\n]\n\ncontrol_headers = []\n\nforeach header : control_header_sources\n inputs = files('../../src/libcamera/' + header + '.yaml', header + '.h.in')\n\n control_headers += custom_target(header + '_h',\n input : inputs,\n output : header + '.h',\n depend_files : gen_controls,\n command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n install : true,\n install_dir : join_paths('include', include_dir))\nendforeach\n\n(untested) and something similar for the .cpp files. But maybe that's\noverkill ? Do you think we'll have a third generated file for metadata ?\n\n> \n> gen_header = files('gen-header.sh')\n> \n> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> new file mode 100644\n> index 000000000000..62799b3e8c54\n> --- /dev/null\n> +++ b/include/libcamera/property_ids.h.in\n> @@ -0,0 +1,33 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * property_ids.h : Property ID list\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#ifndef __LIBCAMERA_PROPERTY_IDS_H__\n> +#define __LIBCAMERA_PROPERTY_IDS_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace properties {\n> +\n> +enum {\n> +${ids}\n> +};\n> +\n> +${controls}\n> +\n> +extern const ControlIdMap properties;\n> +\n> +} /* namespace propertiess */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif // __LIBCAMERA_PROPERTY_IDS_H__\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index c4f965bd7413..abd7046bd95d 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -73,7 +73,13 @@ control_ids_cpp = custom_target('control_ids_cpp',\n> depend_files : gen_controls,\n> command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> \n> +property_ids_cpp = custom_target('property_ids_cpp',\n> + input : files('property_ids.yaml', 'property_ids.cpp.in'),\n> + output : 'property_ids.cpp',\n> + depend_files : gen_controls,\n> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n\nMissing blank line.\n\n> libcamera_sources += control_ids_cpp\n> +libcamera_sources += property_ids_cpp\n> libcamera_sources += control_ids_h\n\nShouldn't you add property_ids_h here ? And let's keep them\nalphabetically sorted.\n\n> \n> gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> new file mode 100644\n> index 000000000000..635a56f7d647\n> --- /dev/null\n> +++ b/src/libcamera/property_ids.cpp.in\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * property_ids.cpp : Property ID list\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#include \n> +\n> +/**\n> + * \\file property_ids.h\n> + * \\brief Camera property identifiers\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\brief Namespace for libcamera controls\n\ns/controls/properties/\n\nI wonder if we could find a single term that would encompass what we\ncall controls, properties, and later metadata.\n\n> + */\n> +namespace properties {\n> +\n> +${controls_doc}\n> +\n> +#ifndef __DOXYGEN__\n> +/*\n> + * Keep the properties definitions hidden from doxygen as it incorrectly parses\n> + * them as functions.\n> + */\n> +${controls_def}\n> +#endif\n> +\n> +/**\n> + * \\brief List of all supported libcamera properties\n> + */\n> +extern const ControlIdMap properties {\n> +${controls_map}\n> +};\n> +\n> +} /* namespace properties */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> new file mode 100644\n> index 000000000000..61be2ab5298c\n> --- /dev/null\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -0,0 +1,34 @@\n> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> +#\n> +# Copyright (C) 2019, Google Inc.\n> +#\n> +%YAML 1.2\n> +---\n> +controls:\n> + - Location:\n> + type: int32_t\n> + description: |\n> + Camera mounting location\n> + values:\n> + - name: \"CAMERA_LOCATION_FRONT\"\n> + value: 0\n> + description: |\n> + The camera is mounted on the front side of the device, facing the\n> + user\n> + - name: \"CAMERA_LOCATION_BACK\"\n> + value: 1\n> + description: |\n> + The camera is mounted on the back facing side of the device\n> + - name: \"CAMERA_LOCATION_EXTERNAL\"\n> + value: 2\n> + description: |\n> + The camera is attached to the device in a way that allows it to\n> + move freely\n\nI think we can make this a bit more yaml by encoding the name directly\nas the key:\n\n values:\n - CAMERA_LOCATION_FRONT:\n value: 0\n description: |\n The camera is mounted on the front side of the device, facing the\n user\n - CAMERA_LOCATION_BACK:\n value: 1\n description: |\n The camera is mounted on the back facing side of the device\n - CAMERA_LOCATION_EXTERNAL\n value: 2\n description: |\n The camera is attached to the device in a way that allows it to\n move freely\n\nWhat do you think ? \"values\" could also be renamed to \"items\" to match\nthe naming used in DT bindings (I know they're unrelated, but there may\nbe a value in using a similar scheme where applicable).\n\n> +\n> + - Rotation:\n> + type: int32_t\n> + description: |\n> + Camera mounting rotation expressed as counterclockwise rotation degrees\n> + towards the axis perpendicular to the sensor surface and directed away\n> + from it\n\nGiven that we will have to expose more precise information in order to\nsupport depth-related cameras later, would it make sense to already go\nfor a full rotation matrix ?\n\n> +...","headers":{"Return-Path":"","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 BE90060BFF\n\tfor ;\n\tWed, 4 Dec 2019 16:40:19 +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 48BE42E5;\n\tWed, 4 Dec 2019 16:40:19 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575474019;\n\tbh=qGSpRgTCHXn9qWcbRE77TM7PUyVJoKIle3y+lD+qUd4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OQVLP18b06tjx3g13H3fPrcpF9zRcMnr2cXvYF44m9SbTC62qpft8RXBL5OUSBEY3\n\twIZXctyBWrssH1/hOCD2hXAkvNMDylzJQzccmGiNF3Wvhm95HoIjUGnq6W3bSWolvi\n\t5UIHARny1yeIiVbWsS8MWFnNs37dhY4/ys8ohQ98=","Date":"Wed, 4 Dec 2019 17:40:12 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191204154012.GB7811@pendragon.ideasonboard.com>","References":"<20191204132106.21582-1-jacopo@jmondi.org>\n\t<20191204132106.21582-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191204132106.21582-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: properties: Generate\n\tlibcamera properties","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":"Wed, 04 Dec 2019 15:40:19 -0000"}},{"id":3186,"web_url":"https://patchwork.libcamera.org/comment/3186/","msgid":"<20191205102239.xjetvhldurvbi32o@uno.localdomain>","date":"2019-12-05T10:22:39","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: properties: Generate\n\tlibcamera properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Dec 04, 2019 at 05:40:12PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Dec 04, 2019 at 02:20:59PM +0100, Jacopo Mondi wrote:\n> > Re-use the Control generation infrastructure to generate libcamera properties.\n> >\n> > Introduce three additional files, one that enumerates the properties ids\n> > (include/libcamera/property_ids.h) and one the defines Control<> instances,\n> > one for each property (src/libcamera/property_ids.cpp) and provide\n> > properties definitions in src/libcamera/property_ids.yaml\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/meson.build | 9 ++++++\n> > include/libcamera/property_ids.h.in | 33 ++++++++++++++++++++++\n> > src/libcamera/meson.build | 6 ++++\n> > src/libcamera/property_ids.cpp.in | 43 +++++++++++++++++++++++++++++\n> > src/libcamera/property_ids.yaml | 34 +++++++++++++++++++++++\n> > 5 files changed, 125 insertions(+)\n> > create mode 100644 include/libcamera/property_ids.h.in\n> > create mode 100644 src/libcamera/property_ids.cpp.in\n> > create mode 100644 src/libcamera/property_ids.yaml\n> >\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 99abf0609940..0ec32ad84c96 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -31,7 +31,16 @@ control_ids_h = custom_target('control_ids_h',\n> > install : true,\n> > install_dir : join_paths('include', include_dir))\n> >\n> > +property_ids_h = custom_target('property_ids_h',\n> > + input : files('../../src/libcamera/property_ids.yaml', 'property_ids.h.in'),\n>\n> It would be lovely if we could avoid the ../.., but that's not a problem\n> introduced by this patch. I wonder if it would make sense to move the\n> yaml files to a separate directory at some point. No need to care about\n> it now, it's just a random comment.\n>\n> > + output : 'property_ids.h',\n> > + depend_files : gen_controls,\n> > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > + install : true,\n> > + install_dir : join_paths('include', include_dir))\n> > +\n> > libcamera_api += control_ids_h\n> > +libcamera_api += property_ids_h\n>\n> If we want to follow the 0, 1, N rule, this could be\n>\n> control_headers_sources = [\n> 'control_ids.h',\n> 'property_ids.h',\n> ]\n>\n> control_headers = []\n>\n> foreach header : control_header_sources\n> inputs = files('../../src/libcamera/' + header + '.yaml', header + '.h.in')\n>\n> control_headers += custom_target(header + '_h',\n> input : inputs,\n> output : header + '.h',\n> depend_files : gen_controls,\n> command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> install : true,\n> install_dir : join_paths('include', include_dir))\n> endforeach\n>\n> (untested) and something similar for the .cpp files. But maybe that's\n> overkill ? Do you think we'll have a third generated file for metadata ?\n\nI would say yes, as we now have controls and properties, a metadata\nfile would probably be the best option. We might end up unifying all\nof them one day if we'll have to, but having controls and properties\nseprately defined and metadata in one of the two does not make much\nsense to me.\n\nI'll give the above proposal a try.\n\n>\n> >\n> > gen_header = files('gen-header.sh')\n> >\n> > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> > new file mode 100644\n> > index 000000000000..62799b3e8c54\n> > --- /dev/null\n> > +++ b/include/libcamera/property_ids.h.in\n> > @@ -0,0 +1,33 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * property_ids.h : Property ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_PROPERTY_IDS_H__\n> > +#define __LIBCAMERA_PROPERTY_IDS_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace properties {\n> > +\n> > +enum {\n> > +${ids}\n> > +};\n> > +\n> > +${controls}\n> > +\n> > +extern const ControlIdMap properties;\n> > +\n> > +} /* namespace propertiess */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif // __LIBCAMERA_PROPERTY_IDS_H__\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index c4f965bd7413..abd7046bd95d 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -73,7 +73,13 @@ control_ids_cpp = custom_target('control_ids_cpp',\n> > depend_files : gen_controls,\n> > command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> >\n> > +property_ids_cpp = custom_target('property_ids_cpp',\n> > + input : files('property_ids.yaml', 'property_ids.cpp.in'),\n> > + output : 'property_ids.cpp',\n> > + depend_files : gen_controls,\n> > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n>\n> Missing blank line.\n>\n\nThanks\n\n> > libcamera_sources += control_ids_cpp\n> > +libcamera_sources += property_ids_cpp\n> > libcamera_sources += control_ids_h\n>\n> Shouldn't you add property_ids_h here ? And let's keep them\n> alphabetically sorted.\n>\n\nI'll fix\n\n> >\n> > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n> > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> > new file mode 100644\n> > index 000000000000..635a56f7d647\n> > --- /dev/null\n> > +++ b/src/libcamera/property_ids.cpp.in\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * property_ids.cpp : Property ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file property_ids.h\n> > + * \\brief Camera property identifiers\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\brief Namespace for libcamera controls\n>\n> s/controls/properties/\n>\n> I wonder if we could find a single term that would encompass what we\n> call controls, properties, and later metadata.\n>\n\nGood question... I tried thinking a bit of that, but I have no answer\nat the moment. Also, the three are semantically different things, so\nhaving them separate makes sense to me, even if a blanket term would\nhelp when referring to all of them. Android seems to use metadata,\nwhich to me is strictly related to information produced by the camera\nand associated with an image. V4L2 has control, which might work, but\ncontrols are the tunable part of a request as of now, so it's easy to\nconfuse... We could s/controls/switches/ and use 'controls' as a\ngeneric term, but I'm not too excited by that.. what do you think ?\n\nNot sure to be honest. Let's keep the separate for the moment...\n\n> > + */\n> > +namespace properties {\n> > +\n> > +${controls_doc}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +/*\n> > + * Keep the properties definitions hidden from doxygen as it incorrectly parses\n> > + * them as functions.\n> > + */\n> > +${controls_def}\n> > +#endif\n> > +\n> > +/**\n> > + * \\brief List of all supported libcamera properties\n> > + */\n> > +extern const ControlIdMap properties {\n> > +${controls_map}\n> > +};\n> > +\n> > +} /* namespace properties */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > new file mode 100644\n> > index 000000000000..61be2ab5298c\n> > --- /dev/null\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -0,0 +1,34 @@\n> > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > +#\n> > +# Copyright (C) 2019, Google Inc.\n> > +#\n> > +%YAML 1.2\n> > +---\n> > +controls:\n> > + - Location:\n> > + type: int32_t\n> > + description: |\n> > + Camera mounting location\n> > + values:\n> > + - name: \"CAMERA_LOCATION_FRONT\"\n> > + value: 0\n> > + description: |\n> > + The camera is mounted on the front side of the device, facing the\n> > + user\n> > + - name: \"CAMERA_LOCATION_BACK\"\n> > + value: 1\n> > + description: |\n> > + The camera is mounted on the back facing side of the device\n> > + - name: \"CAMERA_LOCATION_EXTERNAL\"\n> > + value: 2\n> > + description: |\n> > + The camera is attached to the device in a way that allows it to\n> > + move freely\n>\n> I think we can make this a bit more yaml by encoding the name directly\n> as the key:\n>\n> values:\n> - CAMERA_LOCATION_FRONT:\n> value: 0\n> description: |\n> The camera is mounted on the front side of the device, facing the\n> user\n> - CAMERA_LOCATION_BACK:\n> value: 1\n> description: |\n> The camera is mounted on the back facing side of the device\n> - CAMERA_LOCATION_EXTERNAL\n> value: 2\n> description: |\n> The camera is attached to the device in a way that allows it to\n> move freely\n\nI'll trust your judgment on the 'more yaml' part, I know very few\nthings on that. I'll try to use this.\n\n>\n> What do you think ? \"values\" could also be renamed to \"items\" to match\n> the naming used in DT bindings (I know they're unrelated, but there may\n> be a value in using a similar scheme where applicable).\n\nTo be honest, I don't see much value in this.. But at the same time I\ndon't feel strong about 'values' either... If you prefer 'items' I can\nindeed use that one..\n\n\n>\n> > +\n> > + - Rotation:\n> > + type: int32_t\n> > + description: |\n> > + Camera mounting rotation expressed as counterclockwise rotation degrees\n> > + towards the axis perpendicular to the sensor surface and directed away\n> > + from it\n>\n> Given that we will have to expose more precise information in order to\n> support depth-related cameras later, would it make sense to already go\n> for a full rotation matrix ?\n>\n\nAh, that painful rotation matrix :)\n\nAs per the discussion we had on the kernel side, I consider it an\noverkill for 2D cameras, and I would make it a property specific for\n3D cameras while still having the simpler \"rotation\" for simpler use cases.\nAs per the discussion on the kernel, having vendors provide a 9-values\nmatrix just to say \"my camera is mounted upside down\" is not so nice\nin my opinion.\n\nDo you think having to deal with 2 properties might cause issues ?\n\n> > +...\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 219B360BBC\n\tfor ;\n\tThu, 5 Dec 2019 11:20:29 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 967BC24000E;\n\tThu, 5 Dec 2019 10:20:28 +0000 (UTC)"],"Date":"Thu, 5 Dec 2019 11:22:39 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191205102239.xjetvhldurvbi32o@uno.localdomain>","References":"<20191204132106.21582-1-jacopo@jmondi.org>\n\t<20191204132106.21582-4-jacopo@jmondi.org>\n\t<20191204154012.GB7811@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"pnchunemmrcobfmk\"","Content-Disposition":"inline","In-Reply-To":"<20191204154012.GB7811@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: properties: Generate\n\tlibcamera properties","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":"Thu, 05 Dec 2019 10:20:29 -0000"}},{"id":3191,"web_url":"https://patchwork.libcamera.org/comment/3191/","msgid":"<20191205145203.GK4734@pendragon.ideasonboard.com>","date":"2019-12-05T14:52:03","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: properties: Generate\n\tlibcamera properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Dec 05, 2019 at 11:22:39AM +0100, Jacopo Mondi wrote:\n> On Wed, Dec 04, 2019 at 05:40:12PM +0200, Laurent Pinchart wrote:\n> > On Wed, Dec 04, 2019 at 02:20:59PM +0100, Jacopo Mondi wrote:\n> > > Re-use the Control generation infrastructure to generate libcamera properties.\n> > >\n> > > Introduce three additional files, one that enumerates the properties ids\n> > > (include/libcamera/property_ids.h) and one the defines Control<> instances,\n> > > one for each property (src/libcamera/property_ids.cpp) and provide\n> > > properties definitions in src/libcamera/property_ids.yaml\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/meson.build | 9 ++++++\n> > > include/libcamera/property_ids.h.in | 33 ++++++++++++++++++++++\n> > > src/libcamera/meson.build | 6 ++++\n> > > src/libcamera/property_ids.cpp.in | 43 +++++++++++++++++++++++++++++\n> > > src/libcamera/property_ids.yaml | 34 +++++++++++++++++++++++\n> > > 5 files changed, 125 insertions(+)\n> > > create mode 100644 include/libcamera/property_ids.h.in\n> > > create mode 100644 src/libcamera/property_ids.cpp.in\n> > > create mode 100644 src/libcamera/property_ids.yaml\n> > >\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index 99abf0609940..0ec32ad84c96 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -31,7 +31,16 @@ control_ids_h = custom_target('control_ids_h',\n> > > install : true,\n> > > install_dir : join_paths('include', include_dir))\n> > >\n> > > +property_ids_h = custom_target('property_ids_h',\n> > > + input : files('../../src/libcamera/property_ids.yaml', 'property_ids.h.in'),\n> >\n> > It would be lovely if we could avoid the ../.., but that's not a problem\n> > introduced by this patch. I wonder if it would make sense to move the\n> > yaml files to a separate directory at some point. No need to care about\n> > it now, it's just a random comment.\n> >\n> > > + output : 'property_ids.h',\n> > > + depend_files : gen_controls,\n> > > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > > + install : true,\n> > > + install_dir : join_paths('include', include_dir))\n> > > +\n> > > libcamera_api += control_ids_h\n> > > +libcamera_api += property_ids_h\n> >\n> > If we want to follow the 0, 1, N rule, this could be\n> >\n> > control_headers_sources = [\n> > 'control_ids.h',\n> > 'property_ids.h',\n> > ]\n> >\n> > control_headers = []\n> >\n> > foreach header : control_header_sources\n> > inputs = files('../../src/libcamera/' + header + '.yaml', header + '.h.in')\n> >\n> > control_headers += custom_target(header + '_h',\n> > input : inputs,\n> > output : header + '.h',\n> > depend_files : gen_controls,\n> > command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > install : true,\n> > install_dir : join_paths('include', include_dir))\n> > endforeach\n> >\n> > (untested) and something similar for the .cpp files. But maybe that's\n> > overkill ? Do you think we'll have a third generated file for metadata ?\n> \n> I would say yes, as we now have controls and properties, a metadata\n> file would probably be the best option. We might end up unifying all\n> of them one day if we'll have to, but having controls and properties\n> seprately defined and metadata in one of the two does not make much\n> sense to me.\n> \n> I'll give the above proposal a try.\n> \n> > >\n> > > gen_header = files('gen-header.sh')\n> > >\n> > > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> > > new file mode 100644\n> > > index 000000000000..62799b3e8c54\n> > > --- /dev/null\n> > > +++ b/include/libcamera/property_ids.h.in\n> > > @@ -0,0 +1,33 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * property_ids.h : Property ID list\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#ifndef __LIBCAMERA_PROPERTY_IDS_H__\n> > > +#define __LIBCAMERA_PROPERTY_IDS_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace properties {\n> > > +\n> > > +enum {\n> > > +${ids}\n> > > +};\n> > > +\n> > > +${controls}\n> > > +\n> > > +extern const ControlIdMap properties;\n> > > +\n> > > +} /* namespace propertiess */\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif // __LIBCAMERA_PROPERTY_IDS_H__\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index c4f965bd7413..abd7046bd95d 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -73,7 +73,13 @@ control_ids_cpp = custom_target('control_ids_cpp',\n> > > depend_files : gen_controls,\n> > > command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> > >\n> > > +property_ids_cpp = custom_target('property_ids_cpp',\n> > > + input : files('property_ids.yaml', 'property_ids.cpp.in'),\n> > > + output : 'property_ids.cpp',\n> > > + depend_files : gen_controls,\n> > > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> >\n> > Missing blank line.\n> \n> Thanks\n> \n> > > libcamera_sources += control_ids_cpp\n> > > +libcamera_sources += property_ids_cpp\n> > > libcamera_sources += control_ids_h\n> >\n> > Shouldn't you add property_ids_h here ? And let's keep them\n> > alphabetically sorted.\n> \n> I'll fix\n> \n> > > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n> > > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> > > new file mode 100644\n> > > index 000000000000..635a56f7d647\n> > > --- /dev/null\n> > > +++ b/src/libcamera/property_ids.cpp.in\n> > > @@ -0,0 +1,43 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * property_ids.cpp : Property ID list\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +/**\n> > > + * \\file property_ids.h\n> > > + * \\brief Camera property identifiers\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\brief Namespace for libcamera controls\n> >\n> > s/controls/properties/\n> >\n> > I wonder if we could find a single term that would encompass what we\n> > call controls, properties, and later metadata.\n> \n> Good question... I tried thinking a bit of that, but I have no answer\n> at the moment. Also, the three are semantically different things, so\n> having them separate makes sense to me, even if a blanket term would\n> help when referring to all of them. Android seems to use metadata,\n> which to me is strictly related to information produced by the camera\n> and associated with an image. V4L2 has control, which might work, but\n> controls are the tunable part of a request as of now, so it's easy to\n> confuse... We could s/controls/switches/ and use 'controls' as a\n> generic term, but I'm not too excited by that.. what do you think ?\n> \n> Not sure to be honest. Let's keep the separate for the moment...\n\nOK. Let's revisit it if we find a better term.\n\n> > > + */\n> > > +namespace properties {\n> > > +\n> > > +${controls_doc}\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +/*\n> > > + * Keep the properties definitions hidden from doxygen as it incorrectly parses\n> > > + * them as functions.\n> > > + */\n> > > +${controls_def}\n> > > +#endif\n> > > +\n> > > +/**\n> > > + * \\brief List of all supported libcamera properties\n> > > + */\n> > > +extern const ControlIdMap properties {\n> > > +${controls_map}\n> > > +};\n> > > +\n> > > +} /* namespace properties */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > new file mode 100644\n> > > index 000000000000..61be2ab5298c\n> > > --- /dev/null\n> > > +++ b/src/libcamera/property_ids.yaml\n> > > @@ -0,0 +1,34 @@\n> > > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > > +#\n> > > +# Copyright (C) 2019, Google Inc.\n> > > +#\n> > > +%YAML 1.2\n> > > +---\n> > > +controls:\n> > > + - Location:\n> > > + type: int32_t\n> > > + description: |\n> > > + Camera mounting location\n> > > + values:\n> > > + - name: \"CAMERA_LOCATION_FRONT\"\n> > > + value: 0\n> > > + description: |\n> > > + The camera is mounted on the front side of the device, facing the\n> > > + user\n> > > + - name: \"CAMERA_LOCATION_BACK\"\n> > > + value: 1\n> > > + description: |\n> > > + The camera is mounted on the back facing side of the device\n> > > + - name: \"CAMERA_LOCATION_EXTERNAL\"\n> > > + value: 2\n> > > + description: |\n> > > + The camera is attached to the device in a way that allows it to\n> > > + move freely\n> >\n> > I think we can make this a bit more yaml by encoding the name directly\n> > as the key:\n> >\n> > values:\n> > - CAMERA_LOCATION_FRONT:\n> > value: 0\n> > description: |\n> > The camera is mounted on the front side of the device, facing the\n> > user\n> > - CAMERA_LOCATION_BACK:\n> > value: 1\n> > description: |\n> > The camera is mounted on the back facing side of the device\n> > - CAMERA_LOCATION_EXTERNAL\n> > value: 2\n> > description: |\n> > The camera is attached to the device in a way that allows it to\n> > move freely\n> \n> I'll trust your judgment on the 'more yaml' part, I know very few\n> things on that. I'll try to use this.\n> \n> > What do you think ? \"values\" could also be renamed to \"items\" to match\n> > the naming used in DT bindings (I know they're unrelated, but there may\n> > be a value in using a similar scheme where applicable).\n> \n> To be honest, I don't see much value in this.. But at the same time I\n> don't feel strong about 'values' either... If you prefer 'items' I can\n> indeed use that one..\n> \n> > > +\n> > > + - Rotation:\n> > > + type: int32_t\n> > > + description: |\n> > > + Camera mounting rotation expressed as counterclockwise rotation degrees\n> > > + towards the axis perpendicular to the sensor surface and directed away\n> > > + from it\n> >\n> > Given that we will have to expose more precise information in order to\n> > support depth-related cameras later, would it make sense to already go\n> > for a full rotation matrix ?\n> \n> Ah, that painful rotation matrix :)\n> \n> As per the discussion we had on the kernel side, I consider it an\n> overkill for 2D cameras, and I would make it a property specific for\n> 3D cameras while still having the simpler \"rotation\" for simpler use cases.\n> As per the discussion on the kernel, having vendors provide a 9-values\n> matrix just to say \"my camera is mounted upside down\" is not so nice\n> in my opinion.\n> \n> Do you think having to deal with 2 properties might cause issues ?\n\nAs we're designing something front scratch, having to deal with a single\nproperty could be nice. We would probably need to offer some helper for\napplications to get the 'simple' orientation value from the rotation\nmatrix though. Maybe it's then overkill and we should just go for two\nproperties. Any third-party opinion ?\n\n> > > +...","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 72C7760BCA\n\tfor ;\n\tThu, 5 Dec 2019 15:52:11 +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 9DBEE2E5;\n\tThu, 5 Dec 2019 15:52:10 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575557531;\n\tbh=ktai+3icq7IvhQ7ax057f1l7OYpcOptK1lhpw0Gs91Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gD+VWhkC9VFIvtf7ZKbohbfeHDevdv0Qzviw8HmDqC77hWzkOm/PeGT4jiCR6W2DV\n\toQiPGCQCtQlsnFFSoM07+05HQwJr4JnojHc57b/VmqIu7XfnqSCKCeN3PhDBnS1VQR\n\tMwA8UefjS4O//xaETQZFEH4rdOqWMHy+GgJh2iwc=","Date":"Thu, 5 Dec 2019 16:52:03 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191205145203.GK4734@pendragon.ideasonboard.com>","References":"<20191204132106.21582-1-jacopo@jmondi.org>\n\t<20191204132106.21582-4-jacopo@jmondi.org>\n\t<20191204154012.GB7811@pendragon.ideasonboard.com>\n\t<20191205102239.xjetvhldurvbi32o@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191205102239.xjetvhldurvbi32o@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: properties: Generate\n\tlibcamera properties","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":"Thu, 05 Dec 2019 14:52:11 -0000"}}]