[{"id":3180,"web_url":"https://patchwork.libcamera.org/comment/3180/","msgid":"<20191204163235.GH7811@pendragon.ideasonboard.com>","date":"2019-12-04T16:32:35","subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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:21:05PM +0100, Jacopo Mondi wrote:\n> Construct two example static metadata to be reported to the Android\n> framework using the properties reported by the Camera.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++--\n> 1 file changed, 27 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 065e0292e186..674867d313ac 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -7,6 +7,9 @@\n> \n> #include \"camera_device.h\"\n> \n> +#include \n> +#include \n> +\n> #include \"log.h\"\n> #include \"utils.h\"\n> \n> @@ -97,6 +100,8 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> \tif (staticMetadata_)\n> \t\treturn staticMetadata_->get();\n> \n> +\tconst ControlList &properties = camera_->properties();\n> +\n> \t/*\n> \t * The here reported metadata are enough to implement a basic capture\n> \t * example application, but a real camera implementation will require\n> @@ -261,9 +266,15 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> \t\t\t\t &exposureTimeRange, 2);\n> \n> +\t/*\n> +\t * Android reports orientation as clockwise correction, while Linux\n> +\t * uses counter-clockwise.\n\nWhat Linux does isn't relevant I think, what matters here is what\nlibcamera does.\n\nAndroid defines the rotation as\n\n\"Clockwise angle through which the output image needs to be rotated to\nbe upright on the device screen in its native orientation.\"\n\nI have to say I have a hard time parsing the description of the\nlibcamera rotation property:\n\n\"Camera mounting rotation expressed as counterclockwise rotation degrees\ntowards the axis perpendicular to the sensor surface and directed away\nfrom it.\"\n\nI can't figure out what this means :-S Is it just me ?\n\n> +\t */\n> \tint32_t orientation = 0;\n> -\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,\n> -\t\t\t\t &orientation, 1);\n> +\tif (properties.contains(properties::Rotation))\n> +\t\torientation = (360 - properties.get(properties::Rotation))\n\nThis could be made slightly more efficient if you used .find() to avoid\nthe double lookup. Same below.\n\n> +\t\t\t % 360;\n> +\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1);\n> \n> \tstd::vector testPatterModes = {\n> \t\tANDROID_SENSOR_TEST_PATTERN_MODE_OFF,\n> @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\t\t\t lensApertures.size());\n> \n> \tuint8_t lensFacing = ANDROID_LENS_FACING_FRONT;\n> +\tif (properties.contains(properties::Location)) {\n> +\t\tint32_t location = properties.get(properties::Location);\n> +\t\tswitch (location) {\n> +\t\tcase CAMERA_LOCATION_FRONT:\n> +\t\t\tlensFacing = ANDROID_LENS_FACING_FRONT;\n> +\t\t\tbreak;\n> +\t\tcase CAMERA_LOCATION_BACK:\n> +\t\t\tlensFacing = ANDROID_LENS_FACING_BACK;\n> +\t\t\tbreak;\n> +\t\tcase CAMERA_LOCATION_EXTERNAL:\n> +\t\t\tlensFacing = ANDROID_LENS_FACING_EXTERNAL;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> \tstaticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1);\n> \n> \tstd::vector lensFocalLenghts = {","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 BC43060BFF\n\tfor ;\n\tWed, 4 Dec 2019 17:32:43 +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 C596D2E5;\n\tWed, 4 Dec 2019 17:32:42 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575477163;\n\tbh=hO9i7rLf+jkcuzhFKfPZBCOFE09jNc0/BJQ45NY5akc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OEeZ0NsRf7IXvgZjtEeY4R8PlI9Cyj+jT+o6girjWmbLXE8IqKwnnUQW/m600LmrI\n\toVW6WAMAragxTdNAM8RjhL+14quF+cei5HPkBCrc9ztsY6Fq6C0CpTSdsKX/l5ICWw\n\tlwoStoHcHk9LiIxlqD6PLm7VYaFN5cUxlL95oDOM=","Date":"Wed, 4 Dec 2019 18:32:35 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191204163235.GH7811@pendragon.ideasonboard.com>","References":"<20191204132106.21582-1-jacopo@jmondi.org>\n\t<20191204132106.21582-10-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191204132106.21582-10-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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 16:32:43 -0000"}},{"id":3183,"web_url":"https://patchwork.libcamera.org/comment/3183/","msgid":"<20191205094550.nkr6jx6ojfsenku3@uno.localdomain>","date":"2019-12-05T09:45:50","subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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 06:32:35PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Dec 04, 2019 at 02:21:05PM +0100, Jacopo Mondi wrote:\n> > Construct two example static metadata to be reported to the Android\n> > framework using the properties reported by the Camera.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++--\n> > 1 file changed, 27 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 065e0292e186..674867d313ac 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -7,6 +7,9 @@\n> >\n> > #include \"camera_device.h\"\n> >\n> > +#include \n> > +#include \n> > +\n> > #include \"log.h\"\n> > #include \"utils.h\"\n> >\n> > @@ -97,6 +100,8 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> > \tif (staticMetadata_)\n> > \t\treturn staticMetadata_->get();\n> >\n> > +\tconst ControlList &properties = camera_->properties();\n> > +\n> > \t/*\n> > \t * The here reported metadata are enough to implement a basic capture\n> > \t * example application, but a real camera implementation will require\n> > @@ -261,9 +266,15 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> > \tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > \t\t\t\t &exposureTimeRange, 2);\n> >\n> > +\t/*\n> > +\t * Android reports orientation as clockwise correction, while Linux\n> > +\t * uses counter-clockwise.\n>\n> What Linux does isn't relevant I think, what matters here is what\n> libcamera does.\n\nLibcamera does what Linux does, might that be because the same person\ndid both ends ? Do we want to introduce a different handling of\nrotation in libcamera ?\n\n>\n> Android defines the rotation as\n>\n> \"Clockwise angle through which the output image needs to be rotated to\n> be upright on the device screen in its native orientation.\"\n>\n> I have to say I have a hard time parsing the description of the\n> libcamera rotation property:\n>\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\nThat's the mounting rotation description, almost copied from the\ndt-bindings property description. Might it be better to make the\nlibcamera rotation about the correction and not the mounting rotation\n? In that case the below [(360 - rotation) % 360] would be performed\nby the CameraSensor class and not here..\n\n>\n> I can't figure out what this means :-S Is it just me ?\n\nDidn't we have this currently in review in linux-media as a\ndt-property ? :)\n\n>\n> > +\t */\n> > \tint32_t orientation = 0;\n> > -\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,\n> > -\t\t\t\t &orientation, 1);\n> > +\tif (properties.contains(properties::Rotation))\n> > +\t\torientation = (360 - properties.get(properties::Rotation))\n>\n> This could be made slightly more efficient if you used .find() to avoid\n> the double lookup. Same below.\n\nIndeed, I'll fix\n\nThanks\n j\n\n>\n> > +\t\t\t % 360;\n> > +\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1);\n> >\n> > \tstd::vector testPatterModes = {\n> > \t\tANDROID_SENSOR_TEST_PATTERN_MODE_OFF,\n> > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> > \t\t\t\t lensApertures.size());\n> >\n> > \tuint8_t lensFacing = ANDROID_LENS_FACING_FRONT;\n> > +\tif (properties.contains(properties::Location)) {\n> > +\t\tint32_t location = properties.get(properties::Location);\n> > +\t\tswitch (location) {\n> > +\t\tcase CAMERA_LOCATION_FRONT:\n> > +\t\t\tlensFacing = ANDROID_LENS_FACING_FRONT;\n> > +\t\t\tbreak;\n> > +\t\tcase CAMERA_LOCATION_BACK:\n> > +\t\t\tlensFacing = ANDROID_LENS_FACING_BACK;\n> > +\t\t\tbreak;\n> > +\t\tcase CAMERA_LOCATION_EXTERNAL:\n> > +\t\t\tlensFacing = ANDROID_LENS_FACING_EXTERNAL;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > \tstaticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1);\n> >\n> > \tstd::vector lensFocalLenghts = {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E77CF60BBC\n\tfor ;\n\tThu, 5 Dec 2019 10:43:39 +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 relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 64D8B20009;\n\tThu, 5 Dec 2019 09:43:39 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 5 Dec 2019 10:45:50 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191205094550.nkr6jx6ojfsenku3@uno.localdomain>","References":"<20191204132106.21582-1-jacopo@jmondi.org>\n\t<20191204132106.21582-10-jacopo@jmondi.org>\n\t<20191204163235.GH7811@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"fvana2dnslhrf7pw\"","Content-Disposition":"inline","In-Reply-To":"<20191204163235.GH7811@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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 09:43:40 -0000"}},{"id":3185,"web_url":"https://patchwork.libcamera.org/comment/3185/","msgid":"<20191205100028.GD4734@pendragon.ideasonboard.com>","date":"2019-12-05T10:00:28","subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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 10:45:50AM +0100, Jacopo Mondi wrote:\n> On Wed, Dec 04, 2019 at 06:32:35PM +0200, Laurent Pinchart wrote:\n> > On Wed, Dec 04, 2019 at 02:21:05PM +0100, Jacopo Mondi wrote:\n> > > Construct two example static metadata to be reported to the Android\n> > > framework using the properties reported by the Camera.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++--\n> > > 1 file changed, 27 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 065e0292e186..674867d313ac 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -7,6 +7,9 @@\n> > >\n> > > #include \"camera_device.h\"\n> > >\n> > > +#include \n> > > +#include \n> > > +\n> > > #include \"log.h\"\n> > > #include \"utils.h\"\n> > >\n> > > @@ -97,6 +100,8 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > \tif (staticMetadata_)\n> > > \t\treturn staticMetadata_->get();\n> > >\n> > > +\tconst ControlList &properties = camera_->properties();\n> > > +\n> > > \t/*\n> > > \t * The here reported metadata are enough to implement a basic capture\n> > > \t * example application, but a real camera implementation will require\n> > > @@ -261,9 +266,15 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > \tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > > \t\t\t\t &exposureTimeRange, 2);\n> > >\n> > > +\t/*\n> > > +\t * Android reports orientation as clockwise correction, while Linux\n> > > +\t * uses counter-clockwise.\n> >\n> > What Linux does isn't relevant I think, what matters here is what\n> > libcamera does.\n> \n> Libcamera does what Linux does, might that be because the same person\n> did both ends ? Do we want to introduce a different handling of\n> rotation in libcamera ?\n\nI'm not saying we should, I'm just saying that the HAL implementation,\nwhich translates from libcamera to Android, shouldn't mention Linux as\nsuch, especially given that Linux means V4L2 in this context and we may\nin the future support other APIs too.\n\nThe fact that the libcamera rotation control uses the same semantics as\nV4L2 isn't good or bad in this context, but is relevant to the lower\nlayer, not this layer.\n\n> > Android defines the rotation as\n> >\n> > \"Clockwise angle through which the output image needs to be rotated to\n> > be upright on the device screen in its native orientation.\"\n> >\n> > I have to say I have a hard time parsing the description of the\n> > libcamera rotation property:\n> >\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> That's the mounting rotation description, almost copied from the\n> dt-bindings property description. Might it be better to make the\n> libcamera rotation about the correction and not the mounting rotation\n> ? In that case the below [(360 - rotation) % 360] would be performed\n> by the CameraSensor class and not here..\n\nNot necessarily, we don't need to align the libcamera and Android\ncontrols perfectly, but at the very least I need to understand what the\nlibcamera definition of the rotation control means :-)\n\n> > I can't figure out what this means :-S Is it just me ?\n> \n> Didn't we have this currently in review in linux-media as a\n> dt-property ? :)\n\nI'll review the series and bring this up there.\n\n> > > +\t */\n> > > \tint32_t orientation = 0;\n> > > -\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,\n> > > -\t\t\t\t &orientation, 1);\n> > > +\tif (properties.contains(properties::Rotation))\n> > > +\t\torientation = (360 - properties.get(properties::Rotation))\n> >\n> > This could be made slightly more efficient if you used .find() to avoid\n> > the double lookup. Same below.\n> \n> Indeed, I'll fix\n> \n> > > +\t\t\t % 360;\n> > > +\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1);\n> > >\n> > > \tstd::vector testPatterModes = {\n> > > \t\tANDROID_SENSOR_TEST_PATTERN_MODE_OFF,\n> > > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > \t\t\t\t lensApertures.size());\n> > >\n> > > \tuint8_t lensFacing = ANDROID_LENS_FACING_FRONT;\n> > > +\tif (properties.contains(properties::Location)) {\n> > > +\t\tint32_t location = properties.get(properties::Location);\n> > > +\t\tswitch (location) {\n> > > +\t\tcase CAMERA_LOCATION_FRONT:\n> > > +\t\t\tlensFacing = ANDROID_LENS_FACING_FRONT;\n> > > +\t\t\tbreak;\n> > > +\t\tcase CAMERA_LOCATION_BACK:\n> > > +\t\t\tlensFacing = ANDROID_LENS_FACING_BACK;\n> > > +\t\t\tbreak;\n> > > +\t\tcase CAMERA_LOCATION_EXTERNAL:\n> > > +\t\t\tlensFacing = ANDROID_LENS_FACING_EXTERNAL;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > \tstaticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1);\n> > >\n> > > \tstd::vector lensFocalLenghts = {","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 83D4060BBC\n\tfor ;\n\tThu, 5 Dec 2019 11:00:36 +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 D6D6F2E5;\n\tThu, 5 Dec 2019 11:00:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575540036;\n\tbh=Vjp2tFR3m7UbWpPvXzSiKY+DfomK68gKt8eHBXyE/sU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nUNpdhFjd0t8e+wr84waeVjEon1FKsEnVZaVsFmdIjZ94MTbm0FUidE3rOOFBluWr\n\tj/qUKZwuGmw0Re76PNe2/W0aL2IgdMZTbxt7kwi5eEkLPwnSRkgYUgpikMelCTgCSw\n\t2jLK8iZOCq9yY/jv7s7aETaZOTM2bRAMvNmumSUs=","Date":"Thu, 5 Dec 2019 12:00:28 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191205100028.GD4734@pendragon.ideasonboard.com>","References":"<20191204132106.21582-1-jacopo@jmondi.org>\n\t<20191204132106.21582-10-jacopo@jmondi.org>\n\t<20191204163235.GH7811@pendragon.ideasonboard.com>\n\t<20191205094550.nkr6jx6ojfsenku3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191205094550.nkr6jx6ojfsenku3@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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:00:36 -0000"}},{"id":3188,"web_url":"https://patchwork.libcamera.org/comment/3188/","msgid":"<20191205144017.bwx65h4mhsixpeto@uno.localdomain>","date":"2019-12-05T14:40:17","subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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 06:32:35PM +0200, Laurent Pinchart wrote:\n\n[snip]\n\n> > +\t */\n> > \tint32_t orientation = 0;\n> > -\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,\n> > -\t\t\t\t &orientation, 1);\n> > +\tif (properties.contains(properties::Rotation))\n> > +\t\torientation = (360 - properties.get(properties::Rotation))\n>\n> This could be made slightly more efficient if you used .find() to avoid\n> the double lookup. Same below.\n>\n\nI actually overlooked this one. We don't have a ControlList::find()\noperation, or at least we have one (const and non const versions) but\nit's rightfully private.\n\nThe find() we have do not return an iterator, as one would expect from\na find() on an std::map, but they return the ControlValue associated\nwith the numerical id of a ControlId, allowing the insertion of a new\nitem in the non-const version (that's why they're rightfully private).\n\nThe only current pattern to 1) check if a control is on the control\nlist and 2) get its value is the contains()+get() sequence implemented\nin thi patch, which, as you noticed, performs a double lookup.\n\nI see a few ways to avoid this:\n\n1)\n - rename the current find() operations\n - provide public find() operations which return an iterator\n - Explicitly call get on iterator::second (which is a\n ControlValue)\n\n auto it &ctrl = list.find(Control<>);\n if (ctrl != list.end())\n int32_t value = ctrl.second.get();\n\n2)\n - make ControlList::get<>() return an iterator\n - again explictly call get() on the ControlValue returned as\n iterator::second\n\n const auto &it = list.get(Control<>);\n if (it != list.end)\n int32_t value = it.second.get();\n\nI'm not particularly thrilled by none of the two to be honest\n\n\n> > +\t\t\t % 360;\n> > +\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1);\n> >\n> > \tstd::vector testPatterModes = {\n> > \t\tANDROID_SENSOR_TEST_PATTERN_MODE_OFF,\n> > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> > \t\t\t\t lensApertures.size());\n> >\n> > \tuint8_t lensFacing = ANDROID_LENS_FACING_FRONT;\n> > +\tif (properties.contains(properties::Location)) {\n> > +\t\tint32_t location = properties.get(properties::Location);\n> > +\t\tswitch (location) {\n> > +\t\tcase CAMERA_LOCATION_FRONT:\n> > +\t\t\tlensFacing = ANDROID_LENS_FACING_FRONT;\n> > +\t\t\tbreak;\n> > +\t\tcase CAMERA_LOCATION_BACK:\n> > +\t\t\tlensFacing = ANDROID_LENS_FACING_BACK;\n> > +\t\t\tbreak;\n> > +\t\tcase CAMERA_LOCATION_EXTERNAL:\n> > +\t\t\tlensFacing = ANDROID_LENS_FACING_EXTERNAL;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > \tstaticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1);\n> >\n> > \tstd::vector lensFocalLenghts = {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1DC9560BCA\n\tfor ;\n\tThu, 5 Dec 2019 15:38:07 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 7B446240002;\n\tThu, 5 Dec 2019 14:38:06 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 5 Dec 2019 15:40:17 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191205144017.bwx65h4mhsixpeto@uno.localdomain>","References":"<20191204132106.21582-1-jacopo@jmondi.org>\n\t<20191204132106.21582-10-jacopo@jmondi.org>\n\t<20191204163235.GH7811@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"mobgjcuf6hlwyyno\"","Content-Disposition":"inline","In-Reply-To":"<20191204163235.GH7811@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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:38:07 -0000"}},{"id":3190,"web_url":"https://patchwork.libcamera.org/comment/3190/","msgid":"<20191205144825.GJ4734@pendragon.ideasonboard.com>","date":"2019-12-05T14:48:25","subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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 03:40:17PM +0100, Jacopo Mondi wrote:\n> On Wed, Dec 04, 2019 at 06:32:35PM +0200, Laurent Pinchart wrote:\n> \n> [snip]\n> \n> > > +\t */\n> > > \tint32_t orientation = 0;\n> > > -\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,\n> > > -\t\t\t\t &orientation, 1);\n> > > +\tif (properties.contains(properties::Rotation))\n> > > +\t\torientation = (360 - properties.get(properties::Rotation))\n> >\n> > This could be made slightly more efficient if you used .find() to avoid\n> > the double lookup. Same below.\n> >\n> \n> I actually overlooked this one. We don't have a ControlList::find()\n> operation, or at least we have one (const and non const versions) but\n> it's rightfully private.\n> \n> The find() we have do not return an iterator, as one would expect from\n> a find() on an std::map, but they return the ControlValue associated\n> with the numerical id of a ControlId, allowing the insertion of a new\n> item in the non-const version (that's why they're rightfully private).\n> \n> The only current pattern to 1) check if a control is on the control\n> list and 2) get its value is the contains()+get() sequence implemented\n> in thi patch, which, as you noticed, performs a double lookup.\n> \n> I see a few ways to avoid this:\n> \n> 1)\n> - rename the current find() operations\n> - provide public find() operations which return an iterator\n> - Explicitly call get on iterator::second (which is a\n> ControlValue)\n> \n> auto it &ctrl = list.find(Control<>);\n> if (ctrl != list.end())\n> int32_t value = ctrl.second.get();\n> \n> 2)\n> - make ControlList::get<>() return an iterator\n> - again explictly call get() on the ControlValue returned as\n> iterator::second\n> \n> const auto &it = list.get(Control<>);\n> if (it != list.end)\n> int32_t value = it.second.get();\n> \n> I'm not particularly thrilled by none of the two to be honest\n\nI think we should fix this, but let's leave it as-is for now.\n\n> > > +\t\t\t % 360;\n> > > +\tstaticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1);\n> > >\n> > > \tstd::vector testPatterModes = {\n> > > \t\tANDROID_SENSOR_TEST_PATTERN_MODE_OFF,\n> > > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > \t\t\t\t lensApertures.size());\n> > >\n> > > \tuint8_t lensFacing = ANDROID_LENS_FACING_FRONT;\n> > > +\tif (properties.contains(properties::Location)) {\n> > > +\t\tint32_t location = properties.get(properties::Location);\n> > > +\t\tswitch (location) {\n> > > +\t\tcase CAMERA_LOCATION_FRONT:\n> > > +\t\t\tlensFacing = ANDROID_LENS_FACING_FRONT;\n> > > +\t\t\tbreak;\n> > > +\t\tcase CAMERA_LOCATION_BACK:\n> > > +\t\t\tlensFacing = ANDROID_LENS_FACING_BACK;\n> > > +\t\t\tbreak;\n> > > +\t\tcase CAMERA_LOCATION_EXTERNAL:\n> > > +\t\t\tlensFacing = ANDROID_LENS_FACING_EXTERNAL;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > \tstaticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1);\n> > >\n> > > \tstd::vector lensFocalLenghts = {","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 C4E1660BCA\n\tfor ;\n\tThu, 5 Dec 2019 15:48:32 +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 324F12E5;\n\tThu, 5 Dec 2019 15:48:32 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575557312;\n\tbh=nyDGgdO0XOc3b92WhsrwXFcLVlMMWqx66kgcNUUWsaA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H9sphkqNvSvMvRvJ25Fa9CIepqTxmOEm26kmLPqHyUEO/D1q6x9DmCgYc00etRcoB\n\t/d3b87wKt/t3iHoNEo7Q9N4Mqm2K1FksJ3wBdjgZ1bt7OgMC52UNExrHr4kLdNd9q1\n\t60n4rvIt/wgYDDTyGon11CNiAmOLN0tP2F0HJLNw=","Date":"Thu, 5 Dec 2019 16:48:25 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191205144825.GJ4734@pendragon.ideasonboard.com>","References":"<20191204132106.21582-1-jacopo@jmondi.org>\n\t<20191204132106.21582-10-jacopo@jmondi.org>\n\t<20191204163235.GH7811@pendragon.ideasonboard.com>\n\t<20191205144017.bwx65h4mhsixpeto@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191205144017.bwx65h4mhsixpeto@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 09/10] android: camera_device: Use\n\tCamera properties for static Metadata","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:48:33 -0000"}}]