[{"id":11107,"web_url":"https://patchwork.libcamera.org/comment/11107/","msgid":"<20200703005334.GH12562@pendragon.ideasonboard.com>","date":"2020-07-03T00:53:34","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n> Provide automatic tracking of tags added to automatically report the\n> keys used for the entry:\n> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> \n> This allows automatic addition of added keys without having to manually\n> maintain the list in the code base.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/android/camera_device.cpp | 57 ++++-----------------------------\n> src/android/camera_metadata.cpp | 4 ++-\n> src/android/camera_metadata.h | 4 +++\n> 3 files changed, 13 insertions(+), 52 deletions(-)\n> \n> Sending this, completely untested because ... that's how I roll, and I\n> wanted to know if this is a reasonable route to reduce maintainance\n> burden.\n> \n> A next step beyond this is also to consider a two-pass iteration on all\n> of the meta-data structures, where the first pass will determine the\n> number of entries, and bytes required, while the second pass will\n> actually populate the android metadata.\n> \n> Anythoughts on this? It would mean processing the entries twice, but\n> would stop the guessing game of 'is there enough memory allocated\n> here'...\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 5a3b4dfae6a0..de73c31ed3ea 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\t\t\t availableCapabilities.data(),\n> \t\t\t\t availableCapabilities.size());\n> \n> -\tstd::vector availableCharacteristicsKeys = {\n> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> -\t\tANDROID_CONTROL_MAX_REGIONS,\n> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> -\t\tANDROID_SENSOR_ORIENTATION,\n> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> -\t\tANDROID_SYNC_MAX_LATENCY,\n> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n> -\t\tANDROID_LENS_FACING,\n> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> -\t\tANDROID_SCALER_CROPPING_TYPE,\n> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> -\t};\n> +\t/*\n> +\t * All tags added to staticMetadata_ to this point are added\n> +\t * as keys for the available characteristics.\n> +\t */\n> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> -\t\t\t\t availableCharacteristicsKeys.data(),\n> -\t\t\t\t availableCharacteristicsKeys.size());\n> +\t\t\t\t staticMetadata_->tags().data(),\n> +\t\t\t\t staticMetadata_->tags().size());\n> \n> \tstd::vector availableRequestKeys = {\n> \t\tANDROID_CONTROL_AE_MODE,\n> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> index 47b2e4ef117a..15b569aea52b 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> \tif (!valid_)\n> \t\treturn false;\n> \n> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n> +\t\ttags_.push_back(tag);\n> \t\treturn true;\n> +\t}\n> \n> \tconst char *name = get_camera_metadata_tag_name(tag);\n> \tif (name)\n> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> index 75a9d7066f31..a0e23119e68f 100644\n> --- a/src/android/camera_metadata.h\n> +++ b/src/android/camera_metadata.h\n> @@ -8,6 +8,7 @@\n> #define __ANDROID_CAMERA_METADATA_H__\n> \n> #include \n> +#include \n> \n> #include \n> \n> @@ -20,10 +21,13 @@ public:\n> \tbool isValid() { return valid_; }\n> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> \n> +\tconst std::vector &tags() { return tags_; }\n> +\n> \tcamera_metadata_t *get();\n> \n> private:\n> \tcamera_metadata_t *metadata_;\n> +\tstd::vector tags_;\n\nAren't tags unsigned ?\n\nYou should reserve() space in tags_ in the CameraMetadata constructor.\n\nAs CameraMetadata is also used to report dynamic metadata, we will\nalways add tags to the vector, even if they're only used for static\nmetadata. Not very nice, given that we should try to minimize dynamic\nmemory allocation during streaming :-S\n\nI like the automation this brings though, so maybe we could find a\ndifferent approach that would still bring the same improvement ?\n\n> \tbool valid_;\n> };\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EEC40BE905\n\tfor ;\n\tFri, 3 Jul 2020 00:53:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 872F3609C7;\n\tFri, 3 Jul 2020 02:53:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A8DC609C7\n\tfor ;\n\tFri, 3 Jul 2020 02:53:39 +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 7CCB49CB;\n\tFri, 3 Jul 2020 02:53:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"n44VjNGR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593737618;\n\tbh=Kbyye+AjcS3IRP0pcG0/shE/+jTDj2qCp+CCEHzDsaU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n44VjNGRNTFW6JTsznyTUn580tZB3So2IB5k2LHo3gSpEykdq3G73kLbXXwr0nivM\n\t5r96U4S7UVgP5XdkRPfy+40MzyJ9kzZQgw0G0XN9E5fkU0Wx0T2hfUe9KjDGGXAWNd\n\tqEEBZcWadT7WvRr2w02KIox9ah2eRYNUsLc4Gxc0=","Date":"Fri, 3 Jul 2020 03:53:34 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200703005334.GH12562@pendragon.ideasonboard.com>","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11118,"web_url":"https://patchwork.libcamera.org/comment/11118/","msgid":"<20200703084951.due7dv3chcxftv7b@uno.localdomain>","date":"2020-07-03T08:49:51","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Kieran,\n\nOn Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n> Hi Kieran,\n>\n> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n> > Provide automatic tracking of tags added to automatically report the\n> > keys used for the entry:\n> > ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> >\n> > This allows automatic addition of added keys without having to manually\n> > maintain the list in the code base.\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/android/camera_device.cpp | 57 ++++-----------------------------\n> > src/android/camera_metadata.cpp | 4 ++-\n> > src/android/camera_metadata.h | 4 +++\n> > 3 files changed, 13 insertions(+), 52 deletions(-)\n> >\n> > Sending this, completely untested because ... that's how I roll, and I\n> > wanted to know if this is a reasonable route to reduce maintainance\n> > burden.\n> >\n> > A next step beyond this is also to consider a two-pass iteration on all\n> > of the meta-data structures, where the first pass will determine the\n> > number of entries, and bytes required, while the second pass will\n> > actually populate the android metadata.\n> >\n> > Anythoughts on this? It would mean processing the entries twice, but\n> > would stop the guessing game of 'is there enough memory allocated\n> > here'...\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 5a3b4dfae6a0..de73c31ed3ea 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > \t\t\t\t availableCapabilities.data(),\n> > \t\t\t\t availableCapabilities.size());\n> >\n> > -\tstd::vector availableCharacteristicsKeys = {\n> > -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> > -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> > -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n> > -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n> > -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n> > -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n> > -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n> > -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> > -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > -\t\tANDROID_CONTROL_MAX_REGIONS,\n> > -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> > -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> > -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> > -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> > -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> > -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > -\t\tANDROID_SENSOR_ORIENTATION,\n> > -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> > -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n> > -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> > -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> > -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> > -\t\tANDROID_SYNC_MAX_LATENCY,\n> > -\t\tANDROID_FLASH_INFO_AVAILABLE,\n> > -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n> > -\t\tANDROID_LENS_FACING,\n> > -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n> > -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n> > -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n> > -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n> > -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> > -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n> > -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> > -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> > -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> > -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> > -\t\tANDROID_SCALER_CROPPING_TYPE,\n> > -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > -\t};\n> > +\t/*\n> > +\t * All tags added to staticMetadata_ to this point are added\n> > +\t * as keys for the available characteristics.\n> > +\t */\n> > \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > -\t\t\t\t availableCharacteristicsKeys.data(),\n> > -\t\t\t\t availableCharacteristicsKeys.size());\n> > +\t\t\t\t staticMetadata_->tags().data(),\n> > +\t\t\t\t staticMetadata_->tags().size());\n> >\n> > \tstd::vector availableRequestKeys = {\n> > \t\tANDROID_CONTROL_AE_MODE,\n> > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > index 47b2e4ef117a..15b569aea52b 100644\n> > --- a/src/android/camera_metadata.cpp\n> > +++ b/src/android/camera_metadata.cpp\n> > @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> > \tif (!valid_)\n> > \t\treturn false;\n> >\n> > -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> > +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n> > +\t\ttags_.push_back(tag);\n> > \t\treturn true;\n> > +\t}\n> >\n> > \tconst char *name = get_camera_metadata_tag_name(tag);\n> > \tif (name)\n> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > index 75a9d7066f31..a0e23119e68f 100644\n> > --- a/src/android/camera_metadata.h\n> > +++ b/src/android/camera_metadata.h\n> > @@ -8,6 +8,7 @@\n> > #define __ANDROID_CAMERA_METADATA_H__\n> >\n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -20,10 +21,13 @@ public:\n> > \tbool isValid() { return valid_; }\n> > \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> >\n> > +\tconst std::vector &tags() { return tags_; }\n> > +\n> > \tcamera_metadata_t *get();\n> >\n> > private:\n> > \tcamera_metadata_t *metadata_;\n> > +\tstd::vector tags_;\n>\n> Aren't tags unsigned ?\n\nIf I'm not mistaken Android uses int32_t\n\n>\n> You should reserve() space in tags_ in the CameraMetadata constructor.\n>\n\nWouldn't this require manual pre-calculation as we do today ?\n\n> As CameraMetadata is also used to report dynamic metadata, we will\n> always add tags to the vector, even if they're only used for static\n> metadata. Not very nice, given that we should try to minimize dynamic\n> memory allocation during streaming :-S\n>\n\nThat's my concern too.\n\nI think it's acceptable to perform relocations while building the\nstatic metadata at camera initialization time, but not for run time\nusage. Maybe a different class just for static metadata would work\nbetter ?\n\n> I like the automation this brings though, so maybe we could find a\n> different approach that would still bring the same improvement ?\n>\n> > \tbool valid_;\n> > };\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7200ABE905\n\tfor ;\n\tFri, 3 Jul 2020 08:46:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAB7360C57;\n\tFri, 3 Jul 2020 10:46:20 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E622060C50\n\tfor ;\n\tFri, 3 Jul 2020 10:46:19 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id EEF72240013;\n\tFri, 3 Jul 2020 08:46:18 +0000 (UTC)"],"Date":"Fri, 3 Jul 2020 10:49:51 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200703084951.due7dv3chcxftv7b@uno.localdomain>","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200703005334.GH12562@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11123,"web_url":"https://patchwork.libcamera.org/comment/11123/","msgid":"","date":"2020-07-03T09:05:46","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Heya,\n\nOn 03/07/2020 09:49, Jacopo Mondi wrote:\n> Hi Laurent, Kieran,\n> \n> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n>> Hi Kieran,\n>>\n>> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n>>> Provide automatic tracking of tags added to automatically report the\n>>> keys used for the entry:\n>>> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n>>>\n>>> This allows automatic addition of added keys without having to manually\n>>> maintain the list in the code base.\n>>>\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> src/android/camera_device.cpp | 57 ++++-----------------------------\n>>> src/android/camera_metadata.cpp | 4 ++-\n>>> src/android/camera_metadata.h | 4 +++\n>>> 3 files changed, 13 insertions(+), 52 deletions(-)\n>>>\n>>> Sending this, completely untested because ... that's how I roll, and I\n>>> wanted to know if this is a reasonable route to reduce maintainance\n>>> burden.\n>>>\n>>> A next step beyond this is also to consider a two-pass iteration on all\n>>> of the meta-data structures, where the first pass will determine the\n>>> number of entries, and bytes required, while the second pass will\n>>> actually populate the android metadata.\n>>>\n>>> Anythoughts on this? It would mean processing the entries twice, but\n>>> would stop the guessing game of 'is there enough memory allocated\n>>> here'...\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index 5a3b4dfae6a0..de73c31ed3ea 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>>> \t\t\t\t availableCapabilities.data(),\n>>> \t\t\t\t availableCapabilities.size());\n>>>\n>>> -\tstd::vector availableCharacteristicsKeys = {\n>>> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n>>> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n>>> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n>>> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n>>> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n>>> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n>>> -\t\tANDROID_CONTROL_MAX_REGIONS,\n>>> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n>>> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n>>> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n>>> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n>>> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>>> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n>>> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n>>> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n>>> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n>>> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n>>> -\t\tANDROID_SENSOR_ORIENTATION,\n>>> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n>>> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n>>> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n>>> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n>>> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n>>> -\t\tANDROID_SYNC_MAX_LATENCY,\n>>> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n>>> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n>>> -\t\tANDROID_LENS_FACING,\n>>> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n>>> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n>>> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n>>> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n>>> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n>>> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n>>> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n>>> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n>>> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n>>> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n>>> -\t\tANDROID_SCALER_CROPPING_TYPE,\n>>> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n>>> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n>>> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n>>> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n>>> -\t};\n>>> +\t/*\n>>> +\t * All tags added to staticMetadata_ to this point are added\n>>> +\t * as keys for the available characteristics.\n>>> +\t */\n>>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n>>> -\t\t\t\t availableCharacteristicsKeys.data(),\n>>> -\t\t\t\t availableCharacteristicsKeys.size());\n>>> +\t\t\t\t staticMetadata_->tags().data(),\n>>> +\t\t\t\t staticMetadata_->tags().size());\n>>>\n>>> \tstd::vector availableRequestKeys = {\n>>> \t\tANDROID_CONTROL_AE_MODE,\n>>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n>>> index 47b2e4ef117a..15b569aea52b 100644\n>>> --- a/src/android/camera_metadata.cpp\n>>> +++ b/src/android/camera_metadata.cpp\n>>> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n>>> \tif (!valid_)\n>>> \t\treturn false;\n>>>\n>>> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n>>> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n>>> +\t\ttags_.push_back(tag);\n>>> \t\treturn true;\n>>> +\t}\n>>>\n>>> \tconst char *name = get_camera_metadata_tag_name(tag);\n>>> \tif (name)\n>>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n>>> index 75a9d7066f31..a0e23119e68f 100644\n>>> --- a/src/android/camera_metadata.h\n>>> +++ b/src/android/camera_metadata.h\n>>> @@ -8,6 +8,7 @@\n>>> #define __ANDROID_CAMERA_METADATA_H__\n>>>\n>>> #include \n>>> +#include \n>>>\n>>> #include \n>>>\n>>> @@ -20,10 +21,13 @@ public:\n>>> \tbool isValid() { return valid_; }\n>>> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n>>>\n>>> +\tconst std::vector &tags() { return tags_; }\n>>> +\n>>> \tcamera_metadata_t *get();\n>>>\n>>> private:\n>>> \tcamera_metadata_t *metadata_;\n>>> +\tstd::vector tags_;\n>>\n>> Aren't tags unsigned ?\n> \n> If I'm not mistaken Android uses int32_t\n\nLooks like uint32_t is used everywhere, so that's probably the better\ntype to use.\n\n>>\n>> You should reserve() space in tags_ in the CameraMetadata constructor.\n>>\n> Wouldn't this require manual pre-calculation as we do today ?\n\nIndeed, that's what I'm trying to remove. We could preallocate a size to\nreduce likely hood of reallocations or such - but this might change\nquite a bit anyway...\n\n> \n>> As CameraMetadata is also used to report dynamic metadata, we will\n>> always add tags to the vector, even if they're only used for static\n>> metadata. Not very nice, given that we should try to minimize dynamic\n>> memory allocation during streaming :-S\n>>\n> \n> That's my concern too.\n> \n> I think it's acceptable to perform relocations while building the\n> static metadata at camera initialization time, but not for run time\n> usage. Maybe a different class just for static metadata would work\n> better ?\n> \n>> I like the automation this brings though, so maybe we could find a\n>> different approach that would still bring the same improvement ?\n\nI need to add more keys to the static data, so my main aim here is to\nautomate the calculations required throughout. Otherwise, I fear this\nwill go wrong quickly. I also fear that the calculations might already\nbe wrong, and could potentially be the cause of crashes I experience\nwith multi-stream support. However I haven't been able to confirm/deny\nthat theory yet. (or maybe the valid flag already tracks if we were/were\nnot able to add entries to the metadata successfully).\n\n\n\nI have already been toying with subclassing CameraMetadata to make a\nCameraMetadataNull, which would allow programmatically identifying the\nsizes required, rather than manually.\n\n(A fake MetaData instance which just tracks how many tags/ how much data\nis added)\n\nEqually, I could pull the vector out of the class, and have a wrapper to\naddEntry() which tracks the tags, and just keep that in the\ngetStaticMetadata() function:\n\nClass EntryTagTracker {\n EntryTagTracker(CameraMetadata *md) : md_(md) {};\n\n bool addEntry(uint32_t tag, const void *data, size_t data_count);\n {\n\tbool ret = md_->addEntry(tag, data, data_count);\n\tif (ret)\n\t\ttags_.push_back(tag);\n\treturn ret;\n }\n\n const std::vector &tags() { return tags_; }\n\nprivate:\n CameraMetadata *md_;\n std::vector tags_;\n}\n\n\nI have a vision forming to try to automate collection of the Request and\nResult keys too, which would require a Null metadata object, and calling\n(adapted) functions to extract the tags used and start up time.\n\n\nIn fact, pulling all that together, a fake metadata object which\n/stores/ the tags, and tracks the size and count would then handle all\nthe requirements, so I might retry that path in a bit.\n\n\n>>\n>>> \tbool valid_;\n>>> };\n>>>\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 671FABFFE2\n\tfor ;\n\tFri, 3 Jul 2020 09:05:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3553060C56;\n\tFri, 3 Jul 2020 11:05:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A34A160C55\n\tfor ;\n\tFri, 3 Jul 2020 11:05:53 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A49E351B;\n\tFri, 3 Jul 2020 11:05:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Srm07zHE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593767152;\n\tbh=XD6VHYMJjQyjDe+8WmurLqBY+c6hvLlA7oCGPsKfEGM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Srm07zHEicKmMoTcjl6sv0KmfJQMe5NN7h37jj3YtohHoA1jDPrOT+tgNbr3GQkev\n\tGyS6e7LSJasxKF/xUaoNSRI+W+Cry1dXgYHqkDBlfdImrWaH1QEY1o7yc9XSJLouze\n\tHa+UFOywW7DiXwjywxPLNjHr/WyPAe7GHt6obHeE=","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 3 Jul 2020 10:05:46 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200703084951.due7dv3chcxftv7b@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11135,"web_url":"https://patchwork.libcamera.org/comment/11135/","msgid":"<20200703101248.mqor6zt5uken5ciq@uno.localdomain>","date":"2020-07-03T10:12:48","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:\n> Heya,\n>\n> On 03/07/2020 09:49, Jacopo Mondi wrote:\n> > Hi Laurent, Kieran,\n> >\n> > On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n> >> Hi Kieran,\n> >>\n> >> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n> >>> Provide automatic tracking of tags added to automatically report the\n> >>> keys used for the entry:\n> >>> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> >>>\n> >>> This allows automatic addition of added keys without having to manually\n> >>> maintain the list in the code base.\n> >>>\n> >>> Signed-off-by: Kieran Bingham \n> >>> ---\n> >>> src/android/camera_device.cpp | 57 ++++-----------------------------\n> >>> src/android/camera_metadata.cpp | 4 ++-\n> >>> src/android/camera_metadata.h | 4 +++\n> >>> 3 files changed, 13 insertions(+), 52 deletions(-)\n> >>>\n> >>> Sending this, completely untested because ... that's how I roll, and I\n> >>> wanted to know if this is a reasonable route to reduce maintainance\n> >>> burden.\n> >>>\n> >>> A next step beyond this is also to consider a two-pass iteration on all\n> >>> of the meta-data structures, where the first pass will determine the\n> >>> number of entries, and bytes required, while the second pass will\n> >>> actually populate the android metadata.\n> >>>\n> >>> Anythoughts on this? It would mean processing the entries twice, but\n> >>> would stop the guessing game of 'is there enough memory allocated\n> >>> here'...\n> >>>\n> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>> index 5a3b4dfae6a0..de73c31ed3ea 100644\n> >>> --- a/src/android/camera_device.cpp\n> >>> +++ b/src/android/camera_device.cpp\n> >>> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> >>> \t\t\t\t availableCapabilities.data(),\n> >>> \t\t\t\t availableCapabilities.size());\n> >>>\n> >>> -\tstd::vector availableCharacteristicsKeys = {\n> >>> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> >>> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n> >>> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n> >>> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n> >>> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n> >>> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n> >>> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> >>> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> >>> -\t\tANDROID_CONTROL_MAX_REGIONS,\n> >>> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> >>> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> >>> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> >>> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> >>> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> >>> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> >>> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> >>> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> >>> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> >>> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> >>> -\t\tANDROID_SENSOR_ORIENTATION,\n> >>> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> >>> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n> >>> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> >>> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> >>> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> >>> -\t\tANDROID_SYNC_MAX_LATENCY,\n> >>> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n> >>> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n> >>> -\t\tANDROID_LENS_FACING,\n> >>> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n> >>> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n> >>> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n> >>> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n> >>> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> >>> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n> >>> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> >>> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> >>> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> >>> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> >>> -\t\tANDROID_SCALER_CROPPING_TYPE,\n> >>> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> >>> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> >>> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> >>> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> >>> -\t};\n> >>> +\t/*\n> >>> +\t * All tags added to staticMetadata_ to this point are added\n> >>> +\t * as keys for the available characteristics.\n> >>> +\t */\n> >>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> >>> -\t\t\t\t availableCharacteristicsKeys.data(),\n> >>> -\t\t\t\t availableCharacteristicsKeys.size());\n> >>> +\t\t\t\t staticMetadata_->tags().data(),\n> >>> +\t\t\t\t staticMetadata_->tags().size());\n> >>>\n> >>> \tstd::vector availableRequestKeys = {\n> >>> \t\tANDROID_CONTROL_AE_MODE,\n> >>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> >>> index 47b2e4ef117a..15b569aea52b 100644\n> >>> --- a/src/android/camera_metadata.cpp\n> >>> +++ b/src/android/camera_metadata.cpp\n> >>> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> >>> \tif (!valid_)\n> >>> \t\treturn false;\n> >>>\n> >>> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> >>> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n> >>> +\t\ttags_.push_back(tag);\n> >>> \t\treturn true;\n> >>> +\t}\n> >>>\n> >>> \tconst char *name = get_camera_metadata_tag_name(tag);\n> >>> \tif (name)\n> >>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> >>> index 75a9d7066f31..a0e23119e68f 100644\n> >>> --- a/src/android/camera_metadata.h\n> >>> +++ b/src/android/camera_metadata.h\n> >>> @@ -8,6 +8,7 @@\n> >>> #define __ANDROID_CAMERA_METADATA_H__\n> >>>\n> >>> #include \n> >>> +#include \n> >>>\n> >>> #include \n> >>>\n> >>> @@ -20,10 +21,13 @@ public:\n> >>> \tbool isValid() { return valid_; }\n> >>> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> >>>\n> >>> +\tconst std::vector &tags() { return tags_; }\n> >>> +\n> >>> \tcamera_metadata_t *get();\n> >>>\n> >>> private:\n> >>> \tcamera_metadata_t *metadata_;\n> >>> +\tstd::vector tags_;\n> >>\n> >> Aren't tags unsigned ?\n> >\n> > If I'm not mistaken Android uses int32_t\n>\n> Looks like uint32_t is used everywhere, so that's probably the better\n> type to use.\n\nuint32_t is used by the android metadata library, but tags types is\ndescribed as int32\n\n TYPE_INT32 = 1,\n\nSeems like that they chose INT instead of UINT just to describe the\ntype, but the tags are actually stored as unsigned ?\n\n\n>\n> >>\n> >> You should reserve() space in tags_ in the CameraMetadata constructor.\n> >>\n> > Wouldn't this require manual pre-calculation as we do today ?\n>\n> Indeed, that's what I'm trying to remove. We could preallocate a size to\n> reduce likely hood of reallocations or such - but this might change\n> quite a bit anyway...\n>\n> >\n> >> As CameraMetadata is also used to report dynamic metadata, we will\n> >> always add tags to the vector, even if they're only used for static\n> >> metadata. Not very nice, given that we should try to minimize dynamic\n> >> memory allocation during streaming :-S\n> >>\n> >\n> > That's my concern too.\n> >\n> > I think it's acceptable to perform relocations while building the\n> > static metadata at camera initialization time, but not for run time\n> > usage. Maybe a different class just for static metadata would work\n> > better ?\n> >\n> >> I like the automation this brings though, so maybe we could find a\n> >> different approach that would still bring the same improvement ?\n>\n> I need to add more keys to the static data, so my main aim here is to\n> automate the calculations required throughout. Otherwise, I fear this\n> will go wrong quickly. I also fear that the calculations might already\n> be wrong, and could potentially be the cause of crashes I experience\n> with multi-stream support. However I haven't been able to confirm/deny\n> that theory yet. (or maybe the valid flag already tracks if we were/were\n> not able to add entries to the metadata successfully).\n>\n\nIf you're talking about the existing code, when I had not enough space\nallocated, I noticed, it segfaults very early :)\n\n>\n>\n> I have already been toying with subclassing CameraMetadata to make a\n> CameraMetadataNull, which would allow programmatically identifying the\n> sizes required, rather than manually.\n>\n> (A fake MetaData instance which just tracks how many tags/ how much data\n> is added)\n>\n> Equally, I could pull the vector out of the class, and have a wrapper to\n> addEntry() which tracks the tags, and just keep that in the\n> getStaticMetadata() function:\n>\n> Class EntryTagTracker {\n> EntryTagTracker(CameraMetadata *md) : md_(md) {};\n>\n> bool addEntry(uint32_t tag, const void *data, size_t data_count);\n> {\n> \tbool ret = md_->addEntry(tag, data, data_count);\n> \tif (ret)\n> \t\ttags_.push_back(tag);\n> \treturn ret;\n> }\n>\n> const std::vector &tags() { return tags_; }\n>\n> private:\n> CameraMetadata *md_;\n> std::vector tags_;\n> }\n>\n>\n> I have a vision forming to try to automate collection of the Request and\n> Result keys too, which would require a Null metadata object, and calling\n> (adapted) functions to extract the tags used and start up time.\n>\n>\n> In fact, pulling all that together, a fake metadata object which\n> /stores/ the tags, and tracks the size and count would then handle all\n> the requirements, so I might retry that path in a bit.\n>\n\nI don't think that is has to be 'fake'.\n\nIdeally our CameraMetadata wrapper should be changed to store tags in\na temporary container, tracking their number and sizes (the most\nelegant way to do so would be to to provide an addEntry() overloaded\non the type of a vector first argument, so that you can receive an\n\n addEntry(uin32_t tag, std::vector &data)\n\nAdd the raw vector content in a temporary (and unfortunately\nrelocatable) storage space, and add them one by one to an actual\ncamera_metadata_t through an helper function like\n\n storeMetadata(camera_metadata_t *metadata)\n\nNot a 30 minutes job I fear\n\n>\n> >>\n> >>> \tbool valid_;\n> >>> };\n> >>>\n> >>\n> >> --\n> >> Regards,\n> >>\n> >> Laurent Pinchart\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AF257BFFE2\n\tfor ;\n\tFri, 3 Jul 2020 10:09:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5441B60C57;\n\tFri, 3 Jul 2020 12:09:21 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B0DA603AE\n\tfor ;\n\tFri, 3 Jul 2020 12:09:20 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 9F776200008;\n\tFri, 3 Jul 2020 10:09:19 +0000 (UTC)"],"Date":"Fri, 3 Jul 2020 12:12:48 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20200703101248.mqor6zt5uken5ciq@uno.localdomain>","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11145,"web_url":"https://patchwork.libcamera.org/comment/11145/","msgid":"<20200703105158.GF5963@pendragon.ideasonboard.com>","date":"2020-07-03T10:51:58","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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, Jul 03, 2020 at 10:49:51AM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n> > On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n> > > Provide automatic tracking of tags added to automatically report the\n> > > keys used for the entry:\n> > > ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > >\n> > > This allows automatic addition of added keys without having to manually\n> > > maintain the list in the code base.\n> > >\n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/android/camera_device.cpp | 57 ++++-----------------------------\n> > > src/android/camera_metadata.cpp | 4 ++-\n> > > src/android/camera_metadata.h | 4 +++\n> > > 3 files changed, 13 insertions(+), 52 deletions(-)\n> > >\n> > > Sending this, completely untested because ... that's how I roll, and I\n> > > wanted to know if this is a reasonable route to reduce maintainance\n> > > burden.\n> > >\n> > > A next step beyond this is also to consider a two-pass iteration on all\n> > > of the meta-data structures, where the first pass will determine the\n> > > number of entries, and bytes required, while the second pass will\n> > > actually populate the android metadata.\n> > >\n> > > Anythoughts on this? It would mean processing the entries twice, but\n> > > would stop the guessing game of 'is there enough memory allocated\n> > > here'...\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 5a3b4dfae6a0..de73c31ed3ea 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > \t\t\t\t availableCapabilities.data(),\n> > > \t\t\t\t availableCapabilities.size());\n> > >\n> > > -\tstd::vector availableCharacteristicsKeys = {\n> > > -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> > > -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > > -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> > > -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n> > > -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n> > > -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n> > > -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n> > > -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n> > > -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> > > -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > -\t\tANDROID_CONTROL_MAX_REGIONS,\n> > > -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> > > -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> > > -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> > > -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> > > -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> > > -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > > -\t\tANDROID_SENSOR_ORIENTATION,\n> > > -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> > > -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n> > > -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> > > -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> > > -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> > > -\t\tANDROID_SYNC_MAX_LATENCY,\n> > > -\t\tANDROID_FLASH_INFO_AVAILABLE,\n> > > -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n> > > -\t\tANDROID_LENS_FACING,\n> > > -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n> > > -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n> > > -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n> > > -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n> > > -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> > > -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n> > > -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> > > -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> > > -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> > > -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> > > -\t\tANDROID_SCALER_CROPPING_TYPE,\n> > > -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > > -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> > > -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > > -\t};\n> > > +\t/*\n> > > +\t * All tags added to staticMetadata_ to this point are added\n> > > +\t * as keys for the available characteristics.\n> > > +\t */\n> > > \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > > -\t\t\t\t availableCharacteristicsKeys.data(),\n> > > -\t\t\t\t availableCharacteristicsKeys.size());\n> > > +\t\t\t\t staticMetadata_->tags().data(),\n> > > +\t\t\t\t staticMetadata_->tags().size());\n> > >\n> > > \tstd::vector availableRequestKeys = {\n> > > \t\tANDROID_CONTROL_AE_MODE,\n> > > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > > index 47b2e4ef117a..15b569aea52b 100644\n> > > --- a/src/android/camera_metadata.cpp\n> > > +++ b/src/android/camera_metadata.cpp\n> > > @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> > > \tif (!valid_)\n> > > \t\treturn false;\n> > >\n> > > -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> > > +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n> > > +\t\ttags_.push_back(tag);\n> > > \t\treturn true;\n> > > +\t}\n> > >\n> > > \tconst char *name = get_camera_metadata_tag_name(tag);\n> > > \tif (name)\n> > > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > > index 75a9d7066f31..a0e23119e68f 100644\n> > > --- a/src/android/camera_metadata.h\n> > > +++ b/src/android/camera_metadata.h\n> > > @@ -8,6 +8,7 @@\n> > > #define __ANDROID_CAMERA_METADATA_H__\n> > >\n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -20,10 +21,13 @@ public:\n> > > \tbool isValid() { return valid_; }\n> > > \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> > >\n> > > +\tconst std::vector &tags() { return tags_; }\n> > > +\n> > > \tcamera_metadata_t *get();\n> > >\n> > > private:\n> > > \tcamera_metadata_t *metadata_;\n> > > +\tstd::vector tags_;\n> >\n> > Aren't tags unsigned ?\n> \n> If I'm not mistaken Android uses int32_t\n\nThen we should fix addEntry(), it uses a uint32_t.\n\n> > You should reserve() space in tags_ in the CameraMetadata constructor.\n> \n> Wouldn't this require manual pre-calculation as we do today ?\n\nIt would, but we already do that pre-calculation :-) As the value is\nalready passed to the constructor, we could use it.\n\n> > As CameraMetadata is also used to report dynamic metadata, we will\n> > always add tags to the vector, even if they're only used for static\n> > metadata. Not very nice, given that we should try to minimize dynamic\n> > memory allocation during streaming :-S\n> \n> That's my concern too.\n> \n> I think it's acceptable to perform relocations while building the\n> static metadata at camera initialization time, but not for run time\n> usage. Maybe a different class just for static metadata would work\n> better ?\n\nOr an extra parameter to the constructor to tell if the metadata is\nstatic or dynamic ? (This should then be a CameraMetadataType enum, not\na bool). In the dynamic case, we would skip the reserve() and\npush_back().\n\n> > I like the automation this brings though, so maybe we could find a\n> > different approach that would still bring the same improvement ?\n> >\n> > > \tbool valid_;\n> > > };\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AC81DBFFE2\n\tfor ;\n\tFri, 3 Jul 2020 10:52:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E4BB60C55;\n\tFri, 3 Jul 2020 12:52:04 +0200 (CEST)","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 DECF2603AE\n\tfor ;\n\tFri, 3 Jul 2020 12:52:02 +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 4A7F451B;\n\tFri, 3 Jul 2020 12:52:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"u/eLY4Rx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593773522;\n\tbh=j1MmFM1nR1goYLpKkpBtvxTxGDsxVnymjfwbsLDA5Z4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u/eLY4RxLNr7uYambAVrrbF0nqboOCw+aXGiVzh4ctt0PECY/d/bzj8WM0SSYYBnd\n\tQ/y4vHYF65KnVWRhN9jehW66P+X7yz29rDJcFmlfDhJI+AVRsSUjpm2mPRSKlFgQm+\n\tefIsAvzQHTZsIcXAt2ch4eHogZ+gu9mBXP7UVr+c=","Date":"Fri, 3 Jul 2020 13:51:58 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200703105158.GF5963@pendragon.ideasonboard.com>","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200703084951.due7dv3chcxftv7b@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11147,"web_url":"https://patchwork.libcamera.org/comment/11147/","msgid":"<20200703105738.GG5963@pendragon.ideasonboard.com>","date":"2020-07-03T10:57:38","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:\n> > On 03/07/2020 09:49, Jacopo Mondi wrote:\n> > > On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n> > >> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n> > >>> Provide automatic tracking of tags added to automatically report the\n> > >>> keys used for the entry:\n> > >>> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > >>>\n> > >>> This allows automatic addition of added keys without having to manually\n> > >>> maintain the list in the code base.\n> > >>>\n> > >>> Signed-off-by: Kieran Bingham \n> > >>> ---\n> > >>> src/android/camera_device.cpp | 57 ++++-----------------------------\n> > >>> src/android/camera_metadata.cpp | 4 ++-\n> > >>> src/android/camera_metadata.h | 4 +++\n> > >>> 3 files changed, 13 insertions(+), 52 deletions(-)\n> > >>>\n> > >>> Sending this, completely untested because ... that's how I roll, and I\n> > >>> wanted to know if this is a reasonable route to reduce maintainance\n> > >>> burden.\n> > >>>\n> > >>> A next step beyond this is also to consider a two-pass iteration on all\n> > >>> of the meta-data structures, where the first pass will determine the\n> > >>> number of entries, and bytes required, while the second pass will\n> > >>> actually populate the android metadata.\n> > >>>\n> > >>> Anythoughts on this? It would mean processing the entries twice, but\n> > >>> would stop the guessing game of 'is there enough memory allocated\n> > >>> here'...\n> > >>>\n> > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > >>> index 5a3b4dfae6a0..de73c31ed3ea 100644\n> > >>> --- a/src/android/camera_device.cpp\n> > >>> +++ b/src/android/camera_device.cpp\n> > >>> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > >>> \t\t\t\t availableCapabilities.data(),\n> > >>> \t\t\t\t availableCapabilities.size());\n> > >>>\n> > >>> -\tstd::vector availableCharacteristicsKeys = {\n> > >>> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> > >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> > >>> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n> > >>> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n> > >>> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n> > >>> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n> > >>> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n> > >>> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> > >>> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > >>> -\t\tANDROID_CONTROL_MAX_REGIONS,\n> > >>> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> > >>> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > >>> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > >>> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> > >>> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> > >>> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > >>> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > >>> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> > >>> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> > >>> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > >>> -\t\tANDROID_SENSOR_ORIENTATION,\n> > >>> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> > >>> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n> > >>> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> > >>> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> > >>> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> > >>> -\t\tANDROID_SYNC_MAX_LATENCY,\n> > >>> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n> > >>> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n> > >>> -\t\tANDROID_LENS_FACING,\n> > >>> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n> > >>> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n> > >>> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n> > >>> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n> > >>> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> > >>> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n> > >>> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> > >>> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> > >>> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> > >>> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> > >>> -\t\tANDROID_SCALER_CROPPING_TYPE,\n> > >>> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > >>> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > >>> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> > >>> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > >>> -\t};\n> > >>> +\t/*\n> > >>> +\t * All tags added to staticMetadata_ to this point are added\n> > >>> +\t * as keys for the available characteristics.\n> > >>> +\t */\n> > >>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > >>> -\t\t\t\t availableCharacteristicsKeys.data(),\n> > >>> -\t\t\t\t availableCharacteristicsKeys.size());\n> > >>> +\t\t\t\t staticMetadata_->tags().data(),\n> > >>> +\t\t\t\t staticMetadata_->tags().size());\n> > >>>\n> > >>> \tstd::vector availableRequestKeys = {\n> > >>> \t\tANDROID_CONTROL_AE_MODE,\n> > >>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > >>> index 47b2e4ef117a..15b569aea52b 100644\n> > >>> --- a/src/android/camera_metadata.cpp\n> > >>> +++ b/src/android/camera_metadata.cpp\n> > >>> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> > >>> \tif (!valid_)\n> > >>> \t\treturn false;\n> > >>>\n> > >>> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> > >>> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n> > >>> +\t\ttags_.push_back(tag);\n> > >>> \t\treturn true;\n> > >>> +\t}\n> > >>>\n> > >>> \tconst char *name = get_camera_metadata_tag_name(tag);\n> > >>> \tif (name)\n> > >>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > >>> index 75a9d7066f31..a0e23119e68f 100644\n> > >>> --- a/src/android/camera_metadata.h\n> > >>> +++ b/src/android/camera_metadata.h\n> > >>> @@ -8,6 +8,7 @@\n> > >>> #define __ANDROID_CAMERA_METADATA_H__\n> > >>>\n> > >>> #include \n> > >>> +#include \n> > >>>\n> > >>> #include \n> > >>>\n> > >>> @@ -20,10 +21,13 @@ public:\n> > >>> \tbool isValid() { return valid_; }\n> > >>> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> > >>>\n> > >>> +\tconst std::vector &tags() { return tags_; }\n> > >>> +\n> > >>> \tcamera_metadata_t *get();\n> > >>>\n> > >>> private:\n> > >>> \tcamera_metadata_t *metadata_;\n> > >>> +\tstd::vector tags_;\n> > >>\n> > >> Aren't tags unsigned ?\n> > >\n> > > If I'm not mistaken Android uses int32_t\n> >\n> > Looks like uint32_t is used everywhere, so that's probably the better\n> > type to use.\n> \n> uint32_t is used by the android metadata library, but tags types is\n> described as int32\n> \n> TYPE_INT32 = 1,\n> \n> Seems like that they chose INT instead of UINT just to describe the\n> type, but the tags are actually stored as unsigned ?\n\nThat's for the value, not the tag itself, isn't it ?\n\n> > >>\n> > >> You should reserve() space in tags_ in the CameraMetadata constructor.\n> > >>\n> > > Wouldn't this require manual pre-calculation as we do today ?\n> >\n> > Indeed, that's what I'm trying to remove. We could preallocate a size to\n> > reduce likely hood of reallocations or such - but this might change\n> > quite a bit anyway...\n> >\n> > >> As CameraMetadata is also used to report dynamic metadata, we will\n> > >> always add tags to the vector, even if they're only used for static\n> > >> metadata. Not very nice, given that we should try to minimize dynamic\n> > >> memory allocation during streaming :-S\n> > >\n> > > That's my concern too.\n> > >\n> > > I think it's acceptable to perform relocations while building the\n> > > static metadata at camera initialization time, but not for run time\n> > > usage. Maybe a different class just for static metadata would work\n> > > better ?\n> > >\n> > >> I like the automation this brings though, so maybe we could find a\n> > >> different approach that would still bring the same improvement ?\n> >\n> > I need to add more keys to the static data, so my main aim here is to\n> > automate the calculations required throughout. Otherwise, I fear this\n> > will go wrong quickly. I also fear that the calculations might already\n> > be wrong, and could potentially be the cause of crashes I experience\n> > with multi-stream support. However I haven't been able to confirm/deny\n> > that theory yet. (or maybe the valid flag already tracks if we were/were\n> > not able to add entries to the metadata successfully).\n> \n> If you're talking about the existing code, when I had not enough space\n> allocated, I noticed, it segfaults very early :)\n\nNot the best way to notice though :-)\n\n> > I have already been toying with subclassing CameraMetadata to make a\n> > CameraMetadataNull, which would allow programmatically identifying the\n> > sizes required, rather than manually.\n> >\n> > (A fake MetaData instance which just tracks how many tags/ how much data\n> > is added)\n> >\n> > Equally, I could pull the vector out of the class, and have a wrapper to\n> > addEntry() which tracks the tags, and just keep that in the\n> > getStaticMetadata() function:\n> >\n> > Class EntryTagTracker {\n> > EntryTagTracker(CameraMetadata *md) : md_(md) {};\n> >\n> > bool addEntry(uint32_t tag, const void *data, size_t data_count);\n> > {\n> > \tbool ret = md_->addEntry(tag, data, data_count);\n> > \tif (ret)\n> > \t\ttags_.push_back(tag);\n> > \treturn ret;\n> > }\n> >\n> > const std::vector &tags() { return tags_; }\n> >\n> > private:\n> > CameraMetadata *md_;\n> > std::vector tags_;\n> > }\n> >\n> >\n> > I have a vision forming to try to automate collection of the Request and\n> > Result keys too, which would require a Null metadata object, and calling\n> > (adapted) functions to extract the tags used and start up time.\n> >\n> >\n> > In fact, pulling all that together, a fake metadata object which\n> > /stores/ the tags, and tracks the size and count would then handle all\n> > the requirements, so I might retry that path in a bit.\n> \n> I don't think that is has to be 'fake'.\n> \n> Ideally our CameraMetadata wrapper should be changed to store tags in\n> a temporary container, tracking their number and sizes (the most\n> elegant way to do so would be to to provide an addEntry() overloaded\n> on the type of a vector first argument, so that you can receive an\n> \n> addEntry(uin32_t tag, std::vector &data)\n> \n> Add the raw vector content in a temporary (and unfortunately\n> relocatable) storage space, and add them one by one to an actual\n> camera_metadata_t through an helper function like\n> \n> storeMetadata(camera_metadata_t *metadata)\n> \n> Not a 30 minutes job I fear\n\nFor static metadata this is completely fine, we can make it more complex\n(in the CPU usage sense) with additional memory allocation to achieve a\nsimpler API. We would store data in custom containers and generate an\nAndroid metadata buffer at the end. That would be my preference, but it\nwill take a bit of time to develop. It could even allow us to ditch the\nAndroid metadata library.\n\nFor dynamic metadata, it's a bit of a different story, as we want to\nminimize the memory allocations.\n\n> > >>> \tbool valid_;\n> > >>> };","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EAE1FBE905\n\tfor ;\n\tFri, 3 Jul 2020 10:57:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7972060C58;\n\tFri, 3 Jul 2020 12:57:43 +0200 (CEST)","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 4417B603AE\n\tfor ;\n\tFri, 3 Jul 2020 12:57:42 +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 AF66251B;\n\tFri, 3 Jul 2020 12:57:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VtE8jZ7z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593773861;\n\tbh=Q7DJW2XuBt3Mp6Q2mTYbmWL3V4tjiS9w6QqQl3hxh2g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VtE8jZ7zc9rg9V+Nw52klpme5cNyd03Ns1sHDRSGGcrNLUxzfB0Sr4+KrP61jFFHQ\n\tELP7VZbNIHHo33lwGQw0CVR97mKNVXuL9RDsh5T7JY88wJcY4dUiIwJ073msHbQ452\n\tqrZX5Dux0i7ovZRidhPuuJNcTK2ws41bKh4nHsso=","Date":"Fri, 3 Jul 2020 13:57:38 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200703105738.GG5963@pendragon.ideasonboard.com>","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>\n\t\n\t<20200703101248.mqor6zt5uken5ciq@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200703101248.mqor6zt5uken5ciq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11152,"web_url":"https://patchwork.libcamera.org/comment/11152/","msgid":"","date":"2020-07-03T11:20:34","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 03/07/2020 11:51, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> On Fri, Jul 03, 2020 at 10:49:51AM +0200, Jacopo Mondi wrote:\n>> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n>>> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n>>>> Provide automatic tracking of tags added to automatically report the\n>>>> keys used for the entry:\n>>>> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n>>>>\n>>>> This allows automatic addition of added keys without having to manually\n>>>> maintain the list in the code base.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> src/android/camera_device.cpp | 57 ++++-----------------------------\n>>>> src/android/camera_metadata.cpp | 4 ++-\n>>>> src/android/camera_metadata.h | 4 +++\n>>>> 3 files changed, 13 insertions(+), 52 deletions(-)\n>>>>\n>>>> Sending this, completely untested because ... that's how I roll, and I\n>>>> wanted to know if this is a reasonable route to reduce maintainance\n>>>> burden.\n>>>>\n>>>> A next step beyond this is also to consider a two-pass iteration on all\n>>>> of the meta-data structures, where the first pass will determine the\n>>>> number of entries, and bytes required, while the second pass will\n>>>> actually populate the android metadata.\n>>>>\n>>>> Anythoughts on this? It would mean processing the entries twice, but\n>>>> would stop the guessing game of 'is there enough memory allocated\n>>>> here'...\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 5a3b4dfae6a0..de73c31ed3ea 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>>>> \t\t\t\t availableCapabilities.data(),\n>>>> \t\t\t\t availableCapabilities.size());\n>>>>\n>>>> -\tstd::vector availableCharacteristicsKeys = {\n>>>> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n>>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n>>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n>>>> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n>>>> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n>>>> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n>>>> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n>>>> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n>>>> -\t\tANDROID_CONTROL_MAX_REGIONS,\n>>>> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n>>>> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n>>>> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n>>>> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n>>>> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>>>> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n>>>> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n>>>> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n>>>> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n>>>> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n>>>> -\t\tANDROID_SENSOR_ORIENTATION,\n>>>> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n>>>> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n>>>> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n>>>> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n>>>> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n>>>> -\t\tANDROID_SYNC_MAX_LATENCY,\n>>>> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n>>>> -\t\tANDROID_LENS_FACING,\n>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n>>>> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n>>>> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n>>>> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n>>>> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n>>>> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n>>>> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n>>>> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n>>>> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n>>>> -\t\tANDROID_SCALER_CROPPING_TYPE,\n>>>> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n>>>> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n>>>> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n>>>> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n>>>> -\t};\n>>>> +\t/*\n>>>> +\t * All tags added to staticMetadata_ to this point are added\n>>>> +\t * as keys for the available characteristics.\n>>>> +\t */\n>>>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n>>>> -\t\t\t\t availableCharacteristicsKeys.data(),\n>>>> -\t\t\t\t availableCharacteristicsKeys.size());\n>>>> +\t\t\t\t staticMetadata_->tags().data(),\n>>>> +\t\t\t\t staticMetadata_->tags().size());\n>>>>\n>>>> \tstd::vector availableRequestKeys = {\n>>>> \t\tANDROID_CONTROL_AE_MODE,\n>>>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n>>>> index 47b2e4ef117a..15b569aea52b 100644\n>>>> --- a/src/android/camera_metadata.cpp\n>>>> +++ b/src/android/camera_metadata.cpp\n>>>> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n>>>> \tif (!valid_)\n>>>> \t\treturn false;\n>>>>\n>>>> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n>>>> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n>>>> +\t\ttags_.push_back(tag);\n>>>> \t\treturn true;\n>>>> +\t}\n>>>>\n>>>> \tconst char *name = get_camera_metadata_tag_name(tag);\n>>>> \tif (name)\n>>>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n>>>> index 75a9d7066f31..a0e23119e68f 100644\n>>>> --- a/src/android/camera_metadata.h\n>>>> +++ b/src/android/camera_metadata.h\n>>>> @@ -8,6 +8,7 @@\n>>>> #define __ANDROID_CAMERA_METADATA_H__\n>>>>\n>>>> #include \n>>>> +#include \n>>>>\n>>>> #include \n>>>>\n>>>> @@ -20,10 +21,13 @@ public:\n>>>> \tbool isValid() { return valid_; }\n>>>> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n>>>>\n>>>> +\tconst std::vector &tags() { return tags_; }\n>>>> +\n>>>> \tcamera_metadata_t *get();\n>>>>\n>>>> private:\n>>>> \tcamera_metadata_t *metadata_;\n>>>> +\tstd::vector tags_;\n>>>\n>>> Aren't tags unsigned ?\n>>\n>> If I'm not mistaken Android uses int32_t\n> \n> Then we should fix addEntry(), it uses a uint32_t.\n> \n>>> You should reserve() space in tags_ in the CameraMetadata constructor.\n>>\n>> Wouldn't this require manual pre-calculation as we do today ?\n> \n> It would, but we already do that pre-calculation :-) As the value is\n> already passed to the constructor, we could use it.\n\nOk, I see the point, but this is part of a (not yet existing series) to\n/remove/ those pre-calculations ;-)\n\nIf I get back to this I'll add the reserve, but it would (hopefully) get\nremoved again in the same series.\n\n--\nKieran\n\n\n\n> \n>>> As CameraMetadata is also used to report dynamic metadata, we will\n>>> always add tags to the vector, even if they're only used for static\n>>> metadata. Not very nice, given that we should try to minimize dynamic\n>>> memory allocation during streaming :-S\n>>\n>> That's my concern too.\n>>\n>> I think it's acceptable to perform relocations while building the\n>> static metadata at camera initialization time, but not for run time\n>> usage. Maybe a different class just for static metadata would work\n>> better ?\n> \n> Or an extra parameter to the constructor to tell if the metadata is\n> static or dynamic ? (This should then be a CameraMetadataType enum, not\n> a bool). In the dynamic case, we would skip the reserve() and\n> push_back().\n> \n>>> I like the automation this brings though, so maybe we could find a\n>>> different approach that would still bring the same improvement ?\n>>>\n>>>> \tbool valid_;\n>>>> };\n>>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E0B8DBFFE2\n\tfor ;\n\tFri, 3 Jul 2020 11:20:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5802660C58;\n\tFri, 3 Jul 2020 13:20:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD781603AD\n\tfor ;\n\tFri, 3 Jul 2020 13:20:38 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 06D8051B;\n\tFri, 3 Jul 2020 13:20:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"t3hL+fCo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593775238;\n\tbh=fq/lquS2vgg/jHX3UNhVXb3KRj+ynMHrx9WcKyGC3uQ=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=t3hL+fCo6DYxFr6XN6JmaRGrrFaJ/UnwFpQFXfdmILHkgbJF2FkO+LjE8gBzd1cku\n\t8gyWUl9WN6CobKyeSr/ar+B9IILkD/izRq2WmEe/leI0tZQ8TYvQ7QdqZG439CJIXo\n\tzluDjiPcCQLV727cMr8IyU72WFJOM1UDpOdOe/u8=","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>\n\t<20200703105158.GF5963@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 3 Jul 2020 12:20:34 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200703105158.GF5963@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11153,"web_url":"https://patchwork.libcamera.org/comment/11153/","msgid":"<20200703114426.db2gs2ktxms4rkez@uno.localdomain>","date":"2020-07-03T11:44:26","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:\n> > On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:\n> > > On 03/07/2020 09:49, Jacopo Mondi wrote:\n> > > > On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n> > > >> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n> > > >>> Provide automatic tracking of tags added to automatically report the\n> > > >>> keys used for the entry:\n> > > >>> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > > >>>\n> > > >>> This allows automatic addition of added keys without having to manually\n> > > >>> maintain the list in the code base.\n> > > >>>\n> > > >>> Signed-off-by: Kieran Bingham \n> > > >>> ---\n> > > >>> src/android/camera_device.cpp | 57 ++++-----------------------------\n> > > >>> src/android/camera_metadata.cpp | 4 ++-\n> > > >>> src/android/camera_metadata.h | 4 +++\n> > > >>> 3 files changed, 13 insertions(+), 52 deletions(-)\n> > > >>>\n> > > >>> Sending this, completely untested because ... that's how I roll, and I\n> > > >>> wanted to know if this is a reasonable route to reduce maintainance\n> > > >>> burden.\n> > > >>>\n> > > >>> A next step beyond this is also to consider a two-pass iteration on all\n> > > >>> of the meta-data structures, where the first pass will determine the\n> > > >>> number of entries, and bytes required, while the second pass will\n> > > >>> actually populate the android metadata.\n> > > >>>\n> > > >>> Anythoughts on this? It would mean processing the entries twice, but\n> > > >>> would stop the guessing game of 'is there enough memory allocated\n> > > >>> here'...\n> > > >>>\n> > > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > >>> index 5a3b4dfae6a0..de73c31ed3ea 100644\n> > > >>> --- a/src/android/camera_device.cpp\n> > > >>> +++ b/src/android/camera_device.cpp\n> > > >>> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > >>> \t\t\t\t availableCapabilities.data(),\n> > > >>> \t\t\t\t availableCapabilities.size());\n> > > >>>\n> > > >>> -\tstd::vector availableCharacteristicsKeys = {\n> > > >>> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> > > >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > > >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > > >>> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> > > >>> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n> > > >>> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n> > > >>> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n> > > >>> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n> > > >>> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n> > > >>> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> > > >>> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> > > >>> -\t\tANDROID_CONTROL_MAX_REGIONS,\n> > > >>> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> > > >>> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > >>> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > > >>> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> > > >>> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> > > >>> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > >>> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > >>> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> > > >>> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> > > >>> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> > > >>> -\t\tANDROID_SENSOR_ORIENTATION,\n> > > >>> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> > > >>> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n> > > >>> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> > > >>> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> > > >>> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> > > >>> -\t\tANDROID_SYNC_MAX_LATENCY,\n> > > >>> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n> > > >>> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n> > > >>> -\t\tANDROID_LENS_FACING,\n> > > >>> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n> > > >>> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n> > > >>> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n> > > >>> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n> > > >>> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> > > >>> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n> > > >>> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> > > >>> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> > > >>> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> > > >>> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> > > >>> -\t\tANDROID_SCALER_CROPPING_TYPE,\n> > > >>> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > >>> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> > > >>> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> > > >>> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> > > >>> -\t};\n> > > >>> +\t/*\n> > > >>> +\t * All tags added to staticMetadata_ to this point are added\n> > > >>> +\t * as keys for the available characteristics.\n> > > >>> +\t */\n> > > >>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> > > >>> -\t\t\t\t availableCharacteristicsKeys.data(),\n> > > >>> -\t\t\t\t availableCharacteristicsKeys.size());\n> > > >>> +\t\t\t\t staticMetadata_->tags().data(),\n> > > >>> +\t\t\t\t staticMetadata_->tags().size());\n> > > >>>\n> > > >>> \tstd::vector availableRequestKeys = {\n> > > >>> \t\tANDROID_CONTROL_AE_MODE,\n> > > >>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > > >>> index 47b2e4ef117a..15b569aea52b 100644\n> > > >>> --- a/src/android/camera_metadata.cpp\n> > > >>> +++ b/src/android/camera_metadata.cpp\n> > > >>> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> > > >>> \tif (!valid_)\n> > > >>> \t\treturn false;\n> > > >>>\n> > > >>> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> > > >>> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n> > > >>> +\t\ttags_.push_back(tag);\n> > > >>> \t\treturn true;\n> > > >>> +\t}\n> > > >>>\n> > > >>> \tconst char *name = get_camera_metadata_tag_name(tag);\n> > > >>> \tif (name)\n> > > >>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > > >>> index 75a9d7066f31..a0e23119e68f 100644\n> > > >>> --- a/src/android/camera_metadata.h\n> > > >>> +++ b/src/android/camera_metadata.h\n> > > >>> @@ -8,6 +8,7 @@\n> > > >>> #define __ANDROID_CAMERA_METADATA_H__\n> > > >>>\n> > > >>> #include \n> > > >>> +#include \n> > > >>>\n> > > >>> #include \n> > > >>>\n> > > >>> @@ -20,10 +21,13 @@ public:\n> > > >>> \tbool isValid() { return valid_; }\n> > > >>> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> > > >>>\n> > > >>> +\tconst std::vector &tags() { return tags_; }\n> > > >>> +\n> > > >>> \tcamera_metadata_t *get();\n> > > >>>\n> > > >>> private:\n> > > >>> \tcamera_metadata_t *metadata_;\n> > > >>> +\tstd::vector tags_;\n> > > >>\n> > > >> Aren't tags unsigned ?\n> > > >\n> > > > If I'm not mistaken Android uses int32_t\n> > >\n> > > Looks like uint32_t is used everywhere, so that's probably the better\n> > > type to use.\n> >\n> > uint32_t is used by the android metadata library, but tags types is\n> > described as int32\n> >\n> > TYPE_INT32 = 1,\n> >\n> > Seems like that they chose INT instead of UINT just to describe the\n> > type, but the tags are actually stored as unsigned ?\n>\n> That's for the value, not the tag itself, isn't it ?\n>\n\nright! I confused ht two /(0.0)\\\n\n> > > >>\n> > > >> You should reserve() space in tags_ in the CameraMetadata constructor.\n> > > >>\n> > > > Wouldn't this require manual pre-calculation as we do today ?\n> > >\n> > > Indeed, that's what I'm trying to remove. We could preallocate a size to\n> > > reduce likely hood of reallocations or such - but this might change\n> > > quite a bit anyway...\n> > >\n> > > >> As CameraMetadata is also used to report dynamic metadata, we will\n> > > >> always add tags to the vector, even if they're only used for static\n> > > >> metadata. Not very nice, given that we should try to minimize dynamic\n> > > >> memory allocation during streaming :-S\n> > > >\n> > > > That's my concern too.\n> > > >\n> > > > I think it's acceptable to perform relocations while building the\n> > > > static metadata at camera initialization time, but not for run time\n> > > > usage. Maybe a different class just for static metadata would work\n> > > > better ?\n> > > >\n> > > >> I like the automation this brings though, so maybe we could find a\n> > > >> different approach that would still bring the same improvement ?\n> > >\n> > > I need to add more keys to the static data, so my main aim here is to\n> > > automate the calculations required throughout. Otherwise, I fear this\n> > > will go wrong quickly. I also fear that the calculations might already\n> > > be wrong, and could potentially be the cause of crashes I experience\n> > > with multi-stream support. However I haven't been able to confirm/deny\n> > > that theory yet. (or maybe the valid flag already tracks if we were/were\n> > > not able to add entries to the metadata successfully).\n> >\n> > If you're talking about the existing code, when I had not enough space\n> > allocated, I noticed, it segfaults very early :)\n>\n> Not the best way to notice though :-)\n>\n\nEffective, for sure.\n\nWhat I wanted to point out was to help Kieran in ruling out a wrong\nspace allocation issue, which when happened to me was quite noticeable!\n\n> > > I have already been toying with subclassing CameraMetadata to make a\n> > > CameraMetadataNull, which would allow programmatically identifying the\n> > > sizes required, rather than manually.\n> > >\n> > > (A fake MetaData instance which just tracks how many tags/ how much data\n> > > is added)\n> > >\n> > > Equally, I could pull the vector out of the class, and have a wrapper to\n> > > addEntry() which tracks the tags, and just keep that in the\n> > > getStaticMetadata() function:\n> > >\n> > > Class EntryTagTracker {\n> > > EntryTagTracker(CameraMetadata *md) : md_(md) {};\n> > >\n> > > bool addEntry(uint32_t tag, const void *data, size_t data_count);\n> > > {\n> > > \tbool ret = md_->addEntry(tag, data, data_count);\n> > > \tif (ret)\n> > > \t\ttags_.push_back(tag);\n> > > \treturn ret;\n> > > }\n> > >\n> > > const std::vector &tags() { return tags_; }\n> > >\n> > > private:\n> > > CameraMetadata *md_;\n> > > std::vector tags_;\n> > > }\n> > >\n> > >\n> > > I have a vision forming to try to automate collection of the Request and\n> > > Result keys too, which would require a Null metadata object, and calling\n> > > (adapted) functions to extract the tags used and start up time.\n> > >\n> > >\n> > > In fact, pulling all that together, a fake metadata object which\n> > > /stores/ the tags, and tracks the size and count would then handle all\n> > > the requirements, so I might retry that path in a bit.\n> >\n> > I don't think that is has to be 'fake'.\n> >\n> > Ideally our CameraMetadata wrapper should be changed to store tags in\n> > a temporary container, tracking their number and sizes (the most\n> > elegant way to do so would be to to provide an addEntry() overloaded\n> > on the type of a vector first argument, so that you can receive an\n> >\n> > addEntry(uin32_t tag, std::vector &data)\n> >\n> > Add the raw vector content in a temporary (and unfortunately\n> > relocatable) storage space, and add them one by one to an actual\n> > camera_metadata_t through an helper function like\n> >\n> > storeMetadata(camera_metadata_t *metadata)\n> >\n> > Not a 30 minutes job I fear\n>\n> For static metadata this is completely fine, we can make it more complex\n> (in the CPU usage sense) with additional memory allocation to achieve a\n> simpler API. We would store data in custom containers and generate an\n> Android metadata buffer at the end. That would be my preference, but it\n> will take a bit of time to develop. It could even allow us to ditch the\n> Android metadata library.\n>\n> For dynamic metadata, it's a bit of a different story, as we want to\n> minimize the memory allocations.\n\nyes this should be considered for static metadata only. I'll let\nKieran consider if that's wroth the effort at this point..\n\n>\n> > > >>> \tbool valid_;\n> > > >>> };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BCFE9BE905\n\tfor ;\n\tFri, 3 Jul 2020 11:40:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6298D603B1;\n\tFri, 3 Jul 2020 13:40:58 +0200 (CEST)","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 17CED603AE\n\tfor ;\n\tFri, 3 Jul 2020 13:40:58 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 7362624000E;\n\tFri, 3 Jul 2020 11:40:57 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 3 Jul 2020 13:44:26 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200703114426.db2gs2ktxms4rkez@uno.localdomain>","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>\n\t\n\t<20200703101248.mqor6zt5uken5ciq@uno.localdomain>\n\t<20200703105738.GG5963@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200703105738.GG5963@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11154,"web_url":"https://patchwork.libcamera.org/comment/11154/","msgid":"<20200703114721.GK5963@pendragon.ideasonboard.com>","date":"2020-07-03T11:47:21","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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, Jul 03, 2020 at 01:44:26PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:\n> > On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:\n> >> On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:\n> >>> On 03/07/2020 09:49, Jacopo Mondi wrote:\n> >>>> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n> >>>>> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n> >>>>>> Provide automatic tracking of tags added to automatically report the\n> >>>>>> keys used for the entry:\n> >>>>>> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> >>>>>>\n> >>>>>> This allows automatic addition of added keys without having to manually\n> >>>>>> maintain the list in the code base.\n> >>>>>>\n> >>>>>> Signed-off-by: Kieran Bingham \n> >>>>>> ---\n> >>>>>> src/android/camera_device.cpp | 57 ++++-----------------------------\n> >>>>>> src/android/camera_metadata.cpp | 4 ++-\n> >>>>>> src/android/camera_metadata.h | 4 +++\n> >>>>>> 3 files changed, 13 insertions(+), 52 deletions(-)\n> >>>>>>\n> >>>>>> Sending this, completely untested because ... that's how I roll, and I\n> >>>>>> wanted to know if this is a reasonable route to reduce maintainance\n> >>>>>> burden.\n> >>>>>>\n> >>>>>> A next step beyond this is also to consider a two-pass iteration on all\n> >>>>>> of the meta-data structures, where the first pass will determine the\n> >>>>>> number of entries, and bytes required, while the second pass will\n> >>>>>> actually populate the android metadata.\n> >>>>>>\n> >>>>>> Anythoughts on this? It would mean processing the entries twice, but\n> >>>>>> would stop the guessing game of 'is there enough memory allocated\n> >>>>>> here'...\n> >>>>>>\n> >>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>>>> index 5a3b4dfae6a0..de73c31ed3ea 100644\n> >>>>>> --- a/src/android/camera_device.cpp\n> >>>>>> +++ b/src/android/camera_device.cpp\n> >>>>>> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> >>>>>> \t\t\t\t availableCapabilities.data(),\n> >>>>>> \t\t\t\t availableCapabilities.size());\n> >>>>>>\n> >>>>>> -\tstd::vector availableCharacteristicsKeys = {\n> >>>>>> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> >>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> >>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> >>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> >>>>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n> >>>>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n> >>>>>> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n> >>>>>> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n> >>>>>> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n> >>>>>> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> >>>>>> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> >>>>>> -\t\tANDROID_CONTROL_MAX_REGIONS,\n> >>>>>> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> >>>>>> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> >>>>>> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> >>>>>> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> >>>>>> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> >>>>>> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> >>>>>> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> >>>>>> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> >>>>>> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> >>>>>> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> >>>>>> -\t\tANDROID_SENSOR_ORIENTATION,\n> >>>>>> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> >>>>>> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n> >>>>>> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> >>>>>> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> >>>>>> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> >>>>>> -\t\tANDROID_SYNC_MAX_LATENCY,\n> >>>>>> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n> >>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n> >>>>>> -\t\tANDROID_LENS_FACING,\n> >>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n> >>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n> >>>>>> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n> >>>>>> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n> >>>>>> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> >>>>>> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n> >>>>>> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> >>>>>> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> >>>>>> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> >>>>>> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> >>>>>> -\t\tANDROID_SCALER_CROPPING_TYPE,\n> >>>>>> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> >>>>>> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> >>>>>> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> >>>>>> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> >>>>>> -\t};\n> >>>>>> +\t/*\n> >>>>>> +\t * All tags added to staticMetadata_ to this point are added\n> >>>>>> +\t * as keys for the available characteristics.\n> >>>>>> +\t */\n> >>>>>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> >>>>>> -\t\t\t\t availableCharacteristicsKeys.data(),\n> >>>>>> -\t\t\t\t availableCharacteristicsKeys.size());\n> >>>>>> +\t\t\t\t staticMetadata_->tags().data(),\n> >>>>>> +\t\t\t\t staticMetadata_->tags().size());\n> >>>>>>\n> >>>>>> \tstd::vector availableRequestKeys = {\n> >>>>>> \t\tANDROID_CONTROL_AE_MODE,\n> >>>>>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> >>>>>> index 47b2e4ef117a..15b569aea52b 100644\n> >>>>>> --- a/src/android/camera_metadata.cpp\n> >>>>>> +++ b/src/android/camera_metadata.cpp\n> >>>>>> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> >>>>>> \tif (!valid_)\n> >>>>>> \t\treturn false;\n> >>>>>>\n> >>>>>> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> >>>>>> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n> >>>>>> +\t\ttags_.push_back(tag);\n> >>>>>> \t\treturn true;\n> >>>>>> +\t}\n> >>>>>>\n> >>>>>> \tconst char *name = get_camera_metadata_tag_name(tag);\n> >>>>>> \tif (name)\n> >>>>>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> >>>>>> index 75a9d7066f31..a0e23119e68f 100644\n> >>>>>> --- a/src/android/camera_metadata.h\n> >>>>>> +++ b/src/android/camera_metadata.h\n> >>>>>> @@ -8,6 +8,7 @@\n> >>>>>> #define __ANDROID_CAMERA_METADATA_H__\n> >>>>>>\n> >>>>>> #include \n> >>>>>> +#include \n> >>>>>>\n> >>>>>> #include \n> >>>>>>\n> >>>>>> @@ -20,10 +21,13 @@ public:\n> >>>>>> \tbool isValid() { return valid_; }\n> >>>>>> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> >>>>>>\n> >>>>>> +\tconst std::vector &tags() { return tags_; }\n> >>>>>> +\n> >>>>>> \tcamera_metadata_t *get();\n> >>>>>>\n> >>>>>> private:\n> >>>>>> \tcamera_metadata_t *metadata_;\n> >>>>>> +\tstd::vector tags_;\n> >>>>>\n> >>>>> Aren't tags unsigned ?\n> >>>>\n> >>>> If I'm not mistaken Android uses int32_t\n> >>>\n> >>> Looks like uint32_t is used everywhere, so that's probably the better\n> >>> type to use.\n> >>\n> >> uint32_t is used by the android metadata library, but tags types is\n> >> described as int32\n> >>\n> >> TYPE_INT32 = 1,\n> >>\n> >> Seems like that they chose INT instead of UINT just to describe the\n> >> type, but the tags are actually stored as unsigned ?\n> >\n> > That's for the value, not the tag itself, isn't it ?\n> >\n> \n> right! I confused ht two /(0.0)\\\n> \n> >>>>>\n> >>>>> You should reserve() space in tags_ in the CameraMetadata constructor.\n> >>>>>\n> >>>> Wouldn't this require manual pre-calculation as we do today ?\n> >>>\n> >>> Indeed, that's what I'm trying to remove. We could preallocate a size to\n> >>> reduce likely hood of reallocations or such - but this might change\n> >>> quite a bit anyway...\n> >>>\n> >>>>> As CameraMetadata is also used to report dynamic metadata, we will\n> >>>>> always add tags to the vector, even if they're only used for static\n> >>>>> metadata. Not very nice, given that we should try to minimize dynamic\n> >>>>> memory allocation during streaming :-S\n> >>>>\n> >>>> That's my concern too.\n> >>>>\n> >>>> I think it's acceptable to perform relocations while building the\n> >>>> static metadata at camera initialization time, but not for run time\n> >>>> usage. Maybe a different class just for static metadata would work\n> >>>> better ?\n> >>>>\n> >>>>> I like the automation this brings though, so maybe we could find a\n> >>>>> different approach that would still bring the same improvement ?\n> >>>\n> >>> I need to add more keys to the static data, so my main aim here is to\n> >>> automate the calculations required throughout. Otherwise, I fear this\n> >>> will go wrong quickly. I also fear that the calculations might already\n> >>> be wrong, and could potentially be the cause of crashes I experience\n> >>> with multi-stream support. However I haven't been able to confirm/deny\n> >>> that theory yet. (or maybe the valid flag already tracks if we were/were\n> >>> not able to add entries to the metadata successfully).\n> >>\n> >> If you're talking about the existing code, when I had not enough space\n> >> allocated, I noticed, it segfaults very early :)\n> >\n> > Not the best way to notice though :-)\n> >\n> \n> Effective, for sure.\n> \n> What I wanted to point out was to help Kieran in ruling out a wrong\n> space allocation issue, which when happened to me was quite noticeable!\n> \n> >>> I have already been toying with subclassing CameraMetadata to make a\n> >>> CameraMetadataNull, which would allow programmatically identifying the\n> >>> sizes required, rather than manually.\n> >>>\n> >>> (A fake MetaData instance which just tracks how many tags/ how much data\n> >>> is added)\n> >>>\n> >>> Equally, I could pull the vector out of the class, and have a wrapper to\n> >>> addEntry() which tracks the tags, and just keep that in the\n> >>> getStaticMetadata() function:\n> >>>\n> >>> Class EntryTagTracker {\n> >>> EntryTagTracker(CameraMetadata *md) : md_(md) {};\n> >>>\n> >>> bool addEntry(uint32_t tag, const void *data, size_t data_count);\n> >>> {\n> >>> \tbool ret = md_->addEntry(tag, data, data_count);\n> >>> \tif (ret)\n> >>> \t\ttags_.push_back(tag);\n> >>> \treturn ret;\n> >>> }\n> >>>\n> >>> const std::vector &tags() { return tags_; }\n> >>>\n> >>> private:\n> >>> CameraMetadata *md_;\n> >>> std::vector tags_;\n> >>> }\n> >>>\n> >>>\n> >>> I have a vision forming to try to automate collection of the Request and\n> >>> Result keys too, which would require a Null metadata object, and calling\n> >>> (adapted) functions to extract the tags used and start up time.\n> >>>\n> >>>\n> >>> In fact, pulling all that together, a fake metadata object which\n> >>> /stores/ the tags, and tracks the size and count would then handle all\n> >>> the requirements, so I might retry that path in a bit.\n> >>\n> >> I don't think that is has to be 'fake'.\n> >>\n> >> Ideally our CameraMetadata wrapper should be changed to store tags in\n> >> a temporary container, tracking their number and sizes (the most\n> >> elegant way to do so would be to to provide an addEntry() overloaded\n> >> on the type of a vector first argument, so that you can receive an\n> >>\n> >> addEntry(uin32_t tag, std::vector &data)\n> >>\n> >> Add the raw vector content in a temporary (and unfortunately\n> >> relocatable) storage space, and add them one by one to an actual\n> >> camera_metadata_t through an helper function like\n> >>\n> >> storeMetadata(camera_metadata_t *metadata)\n> >>\n> >> Not a 30 minutes job I fear\n> >\n> > For static metadata this is completely fine, we can make it more complex\n> > (in the CPU usage sense) with additional memory allocation to achieve a\n> > simpler API. We would store data in custom containers and generate an\n> > Android metadata buffer at the end. That would be my preference, but it\n> > will take a bit of time to develop. It could even allow us to ditch the\n> > Android metadata library.\n> >\n> > For dynamic metadata, it's a bit of a different story, as we want to\n> > minimize the memory allocations.\n> \n> yes this should be considered for static metadata only. I'll let\n> Kieran consider if that's wroth the effort at this point..\n\nAnother point to consider on this topic is the inclusion of the Android\nmetadata library in the libcamera sources. It's a small library,\navailable as a shared object on both Android and Chrome OS, but we have\npulled its source in libcamera in order to allow compile-tests without\ndepending on a Chrome OS or Android environment. There's no urgency at\nthis point, but I'd like to fix this. One way is to link against an\nexternal library, at the expense of restricting compile-tests, but\nanother way would be to stop using it completely if we find a better\nAPI.\n\n> >>>>>> \tbool valid_;\n> >>>>>> };","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 75FA6BFFE2\n\tfor ;\n\tFri, 3 Jul 2020 11:47:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1403F60C57;\n\tFri, 3 Jul 2020 13:47:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 357BB603AE\n\tfor ;\n\tFri, 3 Jul 2020 13:47:26 +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 94EE551B;\n\tFri, 3 Jul 2020 13:47:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"S99RvlVW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593776845;\n\tbh=Y5xhtnTYbffrSm9WQWaaaqWvynOMtqmBn6ECaorjuNE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S99RvlVW6hQ3ZiMHlkFv8+XsxSUI0fT7Dr5Jru8Hjt8WNyrUws2cUMv1yOprz+Z6P\n\tioUQr2Nilg4DCJXedIIh8KQp8zttBFCil+kZ/RYLMjQDQAQp6WgrWMDxEFSCkpkx69\n\tlErMOJ8gjSwb0fTYs9cI8+ue6s2gJpSmsXWGHL2o=","Date":"Fri, 3 Jul 2020 14:47:21 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200703114721.GK5963@pendragon.ideasonboard.com>","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>\n\t\n\t<20200703101248.mqor6zt5uken5ciq@uno.localdomain>\n\t<20200703105738.GG5963@pendragon.ideasonboard.com>\n\t<20200703114426.db2gs2ktxms4rkez@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200703114426.db2gs2ktxms4rkez@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11519,"web_url":"https://patchwork.libcamera.org/comment/11519/","msgid":"","date":"2020-07-23T09:26:58","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 03/07/2020 12:47, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> On Fri, Jul 03, 2020 at 01:44:26PM +0200, Jacopo Mondi wrote:\n>> On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:\n>>> On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:\n>>>> On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:\n>>>>> On 03/07/2020 09:49, Jacopo Mondi wrote:\n>>>>>> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n>>>>>>> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n>>>>>>>> Provide automatic tracking of tags added to automatically report the\n>>>>>>>> keys used for the entry:\n>>>>>>>> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n>>>>>>>>\n>>>>>>>> This allows automatic addition of added keys without having to manually\n>>>>>>>> maintain the list in the code base.\n>>>>>>>>\n>>>>>>>> Signed-off-by: Kieran Bingham \n>>>>>>>> ---\n>>>>>>>> src/android/camera_device.cpp | 57 ++++-----------------------------\n>>>>>>>> src/android/camera_metadata.cpp | 4 ++-\n>>>>>>>> src/android/camera_metadata.h | 4 +++\n>>>>>>>> 3 files changed, 13 insertions(+), 52 deletions(-)\n>>>>>>>>\n>>>>>>>> Sending this, completely untested because ... that's how I roll, and I\n>>>>>>>> wanted to know if this is a reasonable route to reduce maintainance\n>>>>>>>> burden.\n>>>>>>>>\n>>>>>>>> A next step beyond this is also to consider a two-pass iteration on all\n>>>>>>>> of the meta-data structures, where the first pass will determine the\n>>>>>>>> number of entries, and bytes required, while the second pass will\n>>>>>>>> actually populate the android metadata.\n>>>>>>>>\n>>>>>>>> Anythoughts on this? It would mean processing the entries twice, but\n>>>>>>>> would stop the guessing game of 'is there enough memory allocated\n>>>>>>>> here'...\n>>>>>>>>\n>>>>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>>>>>> index 5a3b4dfae6a0..de73c31ed3ea 100644\n>>>>>>>> --- a/src/android/camera_device.cpp\n>>>>>>>> +++ b/src/android/camera_device.cpp\n>>>>>>>> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>>>>>>>> \t\t\t\t availableCapabilities.data(),\n>>>>>>>> \t\t\t\t availableCapabilities.size());\n>>>>>>>>\n>>>>>>>> -\tstd::vector availableCharacteristicsKeys = {\n>>>>>>>> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n>>>>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n>>>>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n>>>>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n>>>>>>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n>>>>>>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n>>>>>>>> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n>>>>>>>> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n>>>>>>>> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n>>>>>>>> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n>>>>>>>> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n>>>>>>>> -\t\tANDROID_CONTROL_MAX_REGIONS,\n>>>>>>>> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n>>>>>>>> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n>>>>>>>> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n>>>>>>>> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n>>>>>>>> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>>>>>>>> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n>>>>>>>> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n>>>>>>>> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n>>>>>>>> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n>>>>>>>> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n>>>>>>>> -\t\tANDROID_SENSOR_ORIENTATION,\n>>>>>>>> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n>>>>>>>> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n>>>>>>>> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n>>>>>>>> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n>>>>>>>> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n>>>>>>>> -\t\tANDROID_SYNC_MAX_LATENCY,\n>>>>>>>> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n>>>>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n>>>>>>>> -\t\tANDROID_LENS_FACING,\n>>>>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n>>>>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n>>>>>>>> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n>>>>>>>> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n>>>>>>>> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n>>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n>>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n>>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n>>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n>>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n>>>>>>>> -\t\tANDROID_SCALER_CROPPING_TYPE,\n>>>>>>>> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n>>>>>>>> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n>>>>>>>> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n>>>>>>>> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n>>>>>>>> -\t};\n>>>>>>>> +\t/*\n>>>>>>>> +\t * All tags added to staticMetadata_ to this point are added\n>>>>>>>> +\t * as keys for the available characteristics.\n>>>>>>>> +\t */\n>>>>>>>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n>>>>>>>> -\t\t\t\t availableCharacteristicsKeys.data(),\n>>>>>>>> -\t\t\t\t availableCharacteristicsKeys.size());\n>>>>>>>> +\t\t\t\t staticMetadata_->tags().data(),\n>>>>>>>> +\t\t\t\t staticMetadata_->tags().size());\n>>>>>>>>\n>>>>>>>> \tstd::vector availableRequestKeys = {\n>>>>>>>> \t\tANDROID_CONTROL_AE_MODE,\n>>>>>>>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n>>>>>>>> index 47b2e4ef117a..15b569aea52b 100644\n>>>>>>>> --- a/src/android/camera_metadata.cpp\n>>>>>>>> +++ b/src/android/camera_metadata.cpp\n>>>>>>>> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n>>>>>>>> \tif (!valid_)\n>>>>>>>> \t\treturn false;\n>>>>>>>>\n>>>>>>>> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n>>>>>>>> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n>>>>>>>> +\t\ttags_.push_back(tag);\n>>>>>>>> \t\treturn true;\n>>>>>>>> +\t}\n>>>>>>>>\n>>>>>>>> \tconst char *name = get_camera_metadata_tag_name(tag);\n>>>>>>>> \tif (name)\n>>>>>>>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n>>>>>>>> index 75a9d7066f31..a0e23119e68f 100644\n>>>>>>>> --- a/src/android/camera_metadata.h\n>>>>>>>> +++ b/src/android/camera_metadata.h\n>>>>>>>> @@ -8,6 +8,7 @@\n>>>>>>>> #define __ANDROID_CAMERA_METADATA_H__\n>>>>>>>>\n>>>>>>>> #include \n>>>>>>>> +#include \n>>>>>>>>\n>>>>>>>> #include \n>>>>>>>>\n>>>>>>>> @@ -20,10 +21,13 @@ public:\n>>>>>>>> \tbool isValid() { return valid_; }\n>>>>>>>> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n>>>>>>>>\n>>>>>>>> +\tconst std::vector &tags() { return tags_; }\n>>>>>>>> +\n>>>>>>>> \tcamera_metadata_t *get();\n>>>>>>>>\n>>>>>>>> private:\n>>>>>>>> \tcamera_metadata_t *metadata_;\n>>>>>>>> +\tstd::vector tags_;\n>>>>>>>\n>>>>>>> Aren't tags unsigned ?\n>>>>>>\n>>>>>> If I'm not mistaken Android uses int32_t\n>>>>>\n>>>>> Looks like uint32_t is used everywhere, so that's probably the better\n>>>>> type to use.\n>>>>\n>>>> uint32_t is used by the android metadata library, but tags types is\n>>>> described as int32\n>>>>\n>>>> TYPE_INT32 = 1,\n>>>>\n>>>> Seems like that they chose INT instead of UINT just to describe the\n>>>> type, but the tags are actually stored as unsigned ?\n>>>\n>>> That's for the value, not the tag itself, isn't it ?\n>>>\n>>\n>> right! I confused ht two /(0.0)\\\n>>\n>>>>>>>\n>>>>>>> You should reserve() space in tags_ in the CameraMetadata constructor.\n>>>>>>>\n>>>>>> Wouldn't this require manual pre-calculation as we do today ?\n>>>>>\n>>>>> Indeed, that's what I'm trying to remove. We could preallocate a size to\n>>>>> reduce likely hood of reallocations or such - but this might change\n>>>>> quite a bit anyway...\n>>>>>\n>>>>>>> As CameraMetadata is also used to report dynamic metadata, we will\n>>>>>>> always add tags to the vector, even if they're only used for static\n>>>>>>> metadata. Not very nice, given that we should try to minimize dynamic\n>>>>>>> memory allocation during streaming :-S\n>>>>>>\n>>>>>> That's my concern too.\n>>>>>>\n>>>>>> I think it's acceptable to perform relocations while building the\n>>>>>> static metadata at camera initialization time, but not for run time\n>>>>>> usage. Maybe a different class just for static metadata would work\n>>>>>> better ?\n>>>>>>\n>>>>>>> I like the automation this brings though, so maybe we could find a\n>>>>>>> different approach that would still bring the same improvement ?\n>>>>>\n>>>>> I need to add more keys to the static data, so my main aim here is to\n>>>>> automate the calculations required throughout. Otherwise, I fear this\n>>>>> will go wrong quickly. I also fear that the calculations might already\n>>>>> be wrong, and could potentially be the cause of crashes I experience\n>>>>> with multi-stream support. However I haven't been able to confirm/deny\n>>>>> that theory yet. (or maybe the valid flag already tracks if we were/were\n>>>>> not able to add entries to the metadata successfully).\n>>>>\n>>>> If you're talking about the existing code, when I had not enough space\n>>>> allocated, I noticed, it segfaults very early :)\n>>>\n>>> Not the best way to notice though :-)\n>>>\n>>\n>> Effective, for sure.\n>>\n>> What I wanted to point out was to help Kieran in ruling out a wrong\n>> space allocation issue, which when happened to me was quite noticeable!\n>>\n>>>>> I have already been toying with subclassing CameraMetadata to make a\n>>>>> CameraMetadataNull, which would allow programmatically identifying the\n>>>>> sizes required, rather than manually.\n>>>>>\n>>>>> (A fake MetaData instance which just tracks how many tags/ how much data\n>>>>> is added)\n>>>>>\n>>>>> Equally, I could pull the vector out of the class, and have a wrapper to\n>>>>> addEntry() which tracks the tags, and just keep that in the\n>>>>> getStaticMetadata() function:\n>>>>>\n>>>>> Class EntryTagTracker {\n>>>>> EntryTagTracker(CameraMetadata *md) : md_(md) {};\n>>>>>\n>>>>> bool addEntry(uint32_t tag, const void *data, size_t data_count);\n>>>>> {\n>>>>> \tbool ret = md_->addEntry(tag, data, data_count);\n>>>>> \tif (ret)\n>>>>> \t\ttags_.push_back(tag);\n>>>>> \treturn ret;\n>>>>> }\n>>>>>\n>>>>> const std::vector &tags() { return tags_; }\n>>>>>\n>>>>> private:\n>>>>> CameraMetadata *md_;\n>>>>> std::vector tags_;\n>>>>> }\n>>>>>\n>>>>>\n>>>>> I have a vision forming to try to automate collection of the Request and\n>>>>> Result keys too, which would require a Null metadata object, and calling\n>>>>> (adapted) functions to extract the tags used and start up time.\n>>>>>\n>>>>>\n>>>>> In fact, pulling all that together, a fake metadata object which\n>>>>> /stores/ the tags, and tracks the size and count would then handle all\n>>>>> the requirements, so I might retry that path in a bit.\n>>>>\n>>>> I don't think that is has to be 'fake'.\n>>>>\n>>>> Ideally our CameraMetadata wrapper should be changed to store tags in\n>>>> a temporary container, tracking their number and sizes (the most\n>>>> elegant way to do so would be to to provide an addEntry() overloaded\n>>>> on the type of a vector first argument, so that you can receive an\n>>>>\n>>>> addEntry(uin32_t tag, std::vector &data)\n>>>>\n>>>> Add the raw vector content in a temporary (and unfortunately\n>>>> relocatable) storage space, and add them one by one to an actual\n>>>> camera_metadata_t through an helper function like\n>>>>\n>>>> storeMetadata(camera_metadata_t *metadata)\n>>>>\n>>>> Not a 30 minutes job I fear\n>>>\n>>> For static metadata this is completely fine, we can make it more complex\n>>> (in the CPU usage sense) with additional memory allocation to achieve a\n>>> simpler API. We would store data in custom containers and generate an\n>>> Android metadata buffer at the end. That would be my preference, but it\n>>> will take a bit of time to develop. It could even allow us to ditch the\n>>> Android metadata library.\n>>>\n>>> For dynamic metadata, it's a bit of a different story, as we want to\n>>> minimize the memory allocations.\n>>\n>> yes this should be considered for static metadata only. I'll let\n>> Kieran consider if that's wroth the effort at this point..\n\nI think I got confused where the feeling of this RFC went.\n\nI need to dynamically add keys to requests.\n\nFor example, the JPEG result size would only be added to requests which\nperformed a JPEG encode.\n\n\nSo we can not with a fixed value determine in advance how many entries\nwe will add, without going through the process of executing the code to\ndecide.\n\nWe could determine a 'larger/large enough' value in advance perhaps.\nWould that be more to your liking?\n\nThat maximum size can be determined during the stage where the available\nkeys are initialised anyway.\n\n\nOtherwise, I can not see a route forwards without making some kind of\ndynamic allocation ...\n\n\n\n> Another point to consider on this topic is the inclusion of the Android\n> metadata library in the libcamera sources. It's a small library,\n> available as a shared object on both Android and Chrome OS, but we have\n> pulled its source in libcamera in order to allow compile-tests without\n> depending on a Chrome OS or Android environment. There's no urgency at\n> this point, but I'd like to fix this. One way is to link against an\n> external library, at the expense of restricting compile-tests, but\n> another way would be to stop using it completely if we find a better\n> API.\n> \n>>>>>>>> \tbool valid_;\n>>>>>>>> };\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DE7BFBD878\n\tfor ;\n\tThu, 23 Jul 2020 09:27:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A70C60E78;\n\tThu, 23 Jul 2020 11:27:04 +0200 (CEST)","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 557596039B\n\tfor ;\n\tThu, 23 Jul 2020 11:27:02 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86D7C279;\n\tThu, 23 Jul 2020 11:27:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DeNXYive\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595496421;\n\tbh=D8cy783GVpXN4HVEz9QxJ2xJkwZVmZN67wC8vvDGnmk=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=DeNXYive9bU5+BsMMj3EMUkPZravB+/jRe+WrH6Hq6MRHrkB58iIucM+S5qS0WcVH\n\t1BcKZeF66CtdOcz8Itnk139YLr/+Tuk9JeCkx9RFQI7GiZvRTZwQMeXGjb5TxUeZYm\n\tPdh2Jtistu+WgJOQSKPoRNHkX8GpBWwa2LKmktoo=","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>\n\t\n\t<20200703101248.mqor6zt5uken5ciq@uno.localdomain>\n\t<20200703105738.GG5963@pendragon.ideasonboard.com>\n\t<20200703114426.db2gs2ktxms4rkez@uno.localdomain>\n\t<20200703114721.GK5963@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 23 Jul 2020 10:26:58 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200703114721.GK5963@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11521,"web_url":"https://patchwork.libcamera.org/comment/11521/","msgid":"<20200723102754.GA5920@pendragon.ideasonboard.com>","date":"2020-07-23T10:27:54","subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jul 23, 2020 at 10:26:58AM +0100, Kieran Bingham wrote:\n> On 03/07/2020 12:47, Laurent Pinchart wrote:\n> > On Fri, Jul 03, 2020 at 01:44:26PM +0200, Jacopo Mondi wrote:\n> >> On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:\n> >>> On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:\n> >>>> On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:\n> >>>>> On 03/07/2020 09:49, Jacopo Mondi wrote:\n> >>>>>> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:\n> >>>>>>> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:\n> >>>>>>>> Provide automatic tracking of tags added to automatically report the\n> >>>>>>>> keys used for the entry:\n> >>>>>>>> ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> >>>>>>>>\n> >>>>>>>> This allows automatic addition of added keys without having to manually\n> >>>>>>>> maintain the list in the code base.\n> >>>>>>>>\n> >>>>>>>> Signed-off-by: Kieran Bingham \n> >>>>>>>> ---\n> >>>>>>>> src/android/camera_device.cpp | 57 ++++-----------------------------\n> >>>>>>>> src/android/camera_metadata.cpp | 4 ++-\n> >>>>>>>> src/android/camera_metadata.h | 4 +++\n> >>>>>>>> 3 files changed, 13 insertions(+), 52 deletions(-)\n> >>>>>>>>\n> >>>>>>>> Sending this, completely untested because ... that's how I roll, and I\n> >>>>>>>> wanted to know if this is a reasonable route to reduce maintainance\n> >>>>>>>> burden.\n> >>>>>>>>\n> >>>>>>>> A next step beyond this is also to consider a two-pass iteration on all\n> >>>>>>>> of the meta-data structures, where the first pass will determine the\n> >>>>>>>> number of entries, and bytes required, while the second pass will\n> >>>>>>>> actually populate the android metadata.\n> >>>>>>>>\n> >>>>>>>> Anythoughts on this? It would mean processing the entries twice, but\n> >>>>>>>> would stop the guessing game of 'is there enough memory allocated\n> >>>>>>>> here'...\n> >>>>>>>>\n> >>>>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>>>>>> index 5a3b4dfae6a0..de73c31ed3ea 100644\n> >>>>>>>> --- a/src/android/camera_device.cpp\n> >>>>>>>> +++ b/src/android/camera_device.cpp\n> >>>>>>>> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> >>>>>>>> \t\t\t\t availableCapabilities.data(),\n> >>>>>>>> \t\t\t\t availableCapabilities.size());\n> >>>>>>>>\n> >>>>>>>> -\tstd::vector availableCharacteristicsKeys = {\n> >>>>>>>> -\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n> >>>>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> >>>>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n> >>>>>>>> -\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> >>>>>>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n> >>>>>>>> -\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n> >>>>>>>> -\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n> >>>>>>>> -\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n> >>>>>>>> -\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n> >>>>>>>> -\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n> >>>>>>>> -\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n> >>>>>>>> -\t\tANDROID_CONTROL_MAX_REGIONS,\n> >>>>>>>> -\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> >>>>>>>> -\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> >>>>>>>> -\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> >>>>>>>> -\t\tANDROID_CONTROL_AVAILABLE_MODES,\n> >>>>>>>> -\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> >>>>>>>> -\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> >>>>>>>> -\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> >>>>>>>> -\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> >>>>>>>> -\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> >>>>>>>> -\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> >>>>>>>> -\t\tANDROID_SENSOR_ORIENTATION,\n> >>>>>>>> -\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> >>>>>>>> -\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n> >>>>>>>> -\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> >>>>>>>> -\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> >>>>>>>> -\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n> >>>>>>>> -\t\tANDROID_SYNC_MAX_LATENCY,\n> >>>>>>>> -\t\tANDROID_FLASH_INFO_AVAILABLE,\n> >>>>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n> >>>>>>>> -\t\tANDROID_LENS_FACING,\n> >>>>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n> >>>>>>>> -\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n> >>>>>>>> -\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n> >>>>>>>> -\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n> >>>>>>>> -\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> >>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n> >>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n> >>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> >>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n> >>>>>>>> -\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> >>>>>>>> -\t\tANDROID_SCALER_CROPPING_TYPE,\n> >>>>>>>> -\t\tANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> >>>>>>>> -\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n> >>>>>>>> -\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n> >>>>>>>> -\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n> >>>>>>>> -\t};\n> >>>>>>>> +\t/*\n> >>>>>>>> +\t * All tags added to staticMetadata_ to this point are added\n> >>>>>>>> +\t * as keys for the available characteristics.\n> >>>>>>>> +\t */\n> >>>>>>>> \tstaticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,\n> >>>>>>>> -\t\t\t\t availableCharacteristicsKeys.data(),\n> >>>>>>>> -\t\t\t\t availableCharacteristicsKeys.size());\n> >>>>>>>> +\t\t\t\t staticMetadata_->tags().data(),\n> >>>>>>>> +\t\t\t\t staticMetadata_->tags().size());\n> >>>>>>>>\n> >>>>>>>> \tstd::vector availableRequestKeys = {\n> >>>>>>>> \t\tANDROID_CONTROL_AE_MODE,\n> >>>>>>>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> >>>>>>>> index 47b2e4ef117a..15b569aea52b 100644\n> >>>>>>>> --- a/src/android/camera_metadata.cpp\n> >>>>>>>> +++ b/src/android/camera_metadata.cpp\n> >>>>>>>> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> >>>>>>>> \tif (!valid_)\n> >>>>>>>> \t\treturn false;\n> >>>>>>>>\n> >>>>>>>> -\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> >>>>>>>> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count)) {\n> >>>>>>>> +\t\ttags_.push_back(tag);\n> >>>>>>>> \t\treturn true;\n> >>>>>>>> +\t}\n> >>>>>>>>\n> >>>>>>>> \tconst char *name = get_camera_metadata_tag_name(tag);\n> >>>>>>>> \tif (name)\n> >>>>>>>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> >>>>>>>> index 75a9d7066f31..a0e23119e68f 100644\n> >>>>>>>> --- a/src/android/camera_metadata.h\n> >>>>>>>> +++ b/src/android/camera_metadata.h\n> >>>>>>>> @@ -8,6 +8,7 @@\n> >>>>>>>> #define __ANDROID_CAMERA_METADATA_H__\n> >>>>>>>>\n> >>>>>>>> #include \n> >>>>>>>> +#include \n> >>>>>>>>\n> >>>>>>>> #include \n> >>>>>>>>\n> >>>>>>>> @@ -20,10 +21,13 @@ public:\n> >>>>>>>> \tbool isValid() { return valid_; }\n> >>>>>>>> \tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> >>>>>>>>\n> >>>>>>>> +\tconst std::vector &tags() { return tags_; }\n> >>>>>>>> +\n> >>>>>>>> \tcamera_metadata_t *get();\n> >>>>>>>>\n> >>>>>>>> private:\n> >>>>>>>> \tcamera_metadata_t *metadata_;\n> >>>>>>>> +\tstd::vector tags_;\n> >>>>>>>\n> >>>>>>> Aren't tags unsigned ?\n> >>>>>>\n> >>>>>> If I'm not mistaken Android uses int32_t\n> >>>>>\n> >>>>> Looks like uint32_t is used everywhere, so that's probably the better\n> >>>>> type to use.\n> >>>>\n> >>>> uint32_t is used by the android metadata library, but tags types is\n> >>>> described as int32\n> >>>>\n> >>>> TYPE_INT32 = 1,\n> >>>>\n> >>>> Seems like that they chose INT instead of UINT just to describe the\n> >>>> type, but the tags are actually stored as unsigned ?\n> >>>\n> >>> That's for the value, not the tag itself, isn't it ?\n> >>>\n> >>\n> >> right! I confused ht two /(0.0)\\\n> >>\n> >>>>>>>\n> >>>>>>> You should reserve() space in tags_ in the CameraMetadata constructor.\n> >>>>>>>\n> >>>>>> Wouldn't this require manual pre-calculation as we do today ?\n> >>>>>\n> >>>>> Indeed, that's what I'm trying to remove. We could preallocate a size to\n> >>>>> reduce likely hood of reallocations or such - but this might change\n> >>>>> quite a bit anyway...\n> >>>>>\n> >>>>>>> As CameraMetadata is also used to report dynamic metadata, we will\n> >>>>>>> always add tags to the vector, even if they're only used for static\n> >>>>>>> metadata. Not very nice, given that we should try to minimize dynamic\n> >>>>>>> memory allocation during streaming :-S\n> >>>>>>\n> >>>>>> That's my concern too.\n> >>>>>>\n> >>>>>> I think it's acceptable to perform relocations while building the\n> >>>>>> static metadata at camera initialization time, but not for run time\n> >>>>>> usage. Maybe a different class just for static metadata would work\n> >>>>>> better ?\n> >>>>>>\n> >>>>>>> I like the automation this brings though, so maybe we could find a\n> >>>>>>> different approach that would still bring the same improvement ?\n> >>>>>\n> >>>>> I need to add more keys to the static data, so my main aim here is to\n> >>>>> automate the calculations required throughout. Otherwise, I fear this\n> >>>>> will go wrong quickly. I also fear that the calculations might already\n> >>>>> be wrong, and could potentially be the cause of crashes I experience\n> >>>>> with multi-stream support. However I haven't been able to confirm/deny\n> >>>>> that theory yet. (or maybe the valid flag already tracks if we were/were\n> >>>>> not able to add entries to the metadata successfully).\n> >>>>\n> >>>> If you're talking about the existing code, when I had not enough space\n> >>>> allocated, I noticed, it segfaults very early :)\n> >>>\n> >>> Not the best way to notice though :-)\n> >>>\n> >>\n> >> Effective, for sure.\n> >>\n> >> What I wanted to point out was to help Kieran in ruling out a wrong\n> >> space allocation issue, which when happened to me was quite noticeable!\n> >>\n> >>>>> I have already been toying with subclassing CameraMetadata to make a\n> >>>>> CameraMetadataNull, which would allow programmatically identifying the\n> >>>>> sizes required, rather than manually.\n> >>>>>\n> >>>>> (A fake MetaData instance which just tracks how many tags/ how much data\n> >>>>> is added)\n> >>>>>\n> >>>>> Equally, I could pull the vector out of the class, and have a wrapper to\n> >>>>> addEntry() which tracks the tags, and just keep that in the\n> >>>>> getStaticMetadata() function:\n> >>>>>\n> >>>>> Class EntryTagTracker {\n> >>>>> EntryTagTracker(CameraMetadata *md) : md_(md) {};\n> >>>>>\n> >>>>> bool addEntry(uint32_t tag, const void *data, size_t data_count);\n> >>>>> {\n> >>>>> \tbool ret = md_->addEntry(tag, data, data_count);\n> >>>>> \tif (ret)\n> >>>>> \t\ttags_.push_back(tag);\n> >>>>> \treturn ret;\n> >>>>> }\n> >>>>>\n> >>>>> const std::vector &tags() { return tags_; }\n> >>>>>\n> >>>>> private:\n> >>>>> CameraMetadata *md_;\n> >>>>> std::vector tags_;\n> >>>>> }\n> >>>>>\n> >>>>>\n> >>>>> I have a vision forming to try to automate collection of the Request and\n> >>>>> Result keys too, which would require a Null metadata object, and calling\n> >>>>> (adapted) functions to extract the tags used and start up time.\n> >>>>>\n> >>>>>\n> >>>>> In fact, pulling all that together, a fake metadata object which\n> >>>>> /stores/ the tags, and tracks the size and count would then handle all\n> >>>>> the requirements, so I might retry that path in a bit.\n> >>>>\n> >>>> I don't think that is has to be 'fake'.\n> >>>>\n> >>>> Ideally our CameraMetadata wrapper should be changed to store tags in\n> >>>> a temporary container, tracking their number and sizes (the most\n> >>>> elegant way to do so would be to to provide an addEntry() overloaded\n> >>>> on the type of a vector first argument, so that you can receive an\n> >>>>\n> >>>> addEntry(uin32_t tag, std::vector &data)\n> >>>>\n> >>>> Add the raw vector content in a temporary (and unfortunately\n> >>>> relocatable) storage space, and add them one by one to an actual\n> >>>> camera_metadata_t through an helper function like\n> >>>>\n> >>>> storeMetadata(camera_metadata_t *metadata)\n> >>>>\n> >>>> Not a 30 minutes job I fear\n> >>>\n> >>> For static metadata this is completely fine, we can make it more complex\n> >>> (in the CPU usage sense) with additional memory allocation to achieve a\n> >>> simpler API. We would store data in custom containers and generate an\n> >>> Android metadata buffer at the end. That would be my preference, but it\n> >>> will take a bit of time to develop. It could even allow us to ditch the\n> >>> Android metadata library.\n> >>>\n> >>> For dynamic metadata, it's a bit of a different story, as we want to\n> >>> minimize the memory allocations.\n> >>\n> >> yes this should be considered for static metadata only. I'll let\n> >> Kieran consider if that's wroth the effort at this point..\n> \n> I think I got confused where the feeling of this RFC went.\n> \n> I need to dynamically add keys to requests.\n> \n> For example, the JPEG result size would only be added to requests which\n> performed a JPEG encode.\n> \n> So we can not with a fixed value determine in advance how many entries\n> we will add, without going through the process of executing the code to\n> decide.\n> \n> We could determine a 'larger/large enough' value in advance perhaps.\n> Would that be more to your liking?\n> \n> That maximum size can be determined during the stage where the available\n> keys are initialised anyway.\n> \n> Otherwise, I can not see a route forwards without making some kind of\n> dynamic allocation ...\n\nI see three options:\n\n- Adding metadata to a dynamic container (e.g. std::map) and generating\n the Android metadata blob at the end\n\n- Running a first pass to compute the size of the metadata buffer and a\n second pass to fill it.\n\n- Calculating a worst case size at initialization time and using it to\n allocate metadata buffers.\n\nThe first option requires dynamic allocation, the second option uses\nmore CPU time, and the third option uses more memory than strictly\nnecessary. The code structure would likely also be more or less clean\ndepending on which option is picked.\n\nAs stated before, for static metadata, extra CPU time and dynamic memory\nallocation isn't much of an issue. I'd prioritize ease of use there, and\ncode sharing with the dynamic metadata case. For dynamic metadata, it\nwould be better to minimize dynamic allocations as much as possible.\n\nLet's also not forget that some metadata tags store a variable number of\nentries. That's mostly used for static metadata and controls, but a few\ndynamic metadata tags are also affected (android.request.outputStreams,\nand several of the android.statistics.* and android.tonemap.* tags). The\nnumber of entries should however be either bounded (e.g.\nandroid.statistics.faceIds is bounded by\nandroid.statistics.info.maxFaceCount) or constant for a given\nconfiguration (e.g. the number of entries in\nandroid.statistics.histogram doesn't vary with each frame).\n\n> > Another point to consider on this topic is the inclusion of the Android\n> > metadata library in the libcamera sources. It's a small library,\n> > available as a shared object on both Android and Chrome OS, but we have\n> > pulled its source in libcamera in order to allow compile-tests without\n> > depending on a Chrome OS or Android environment. There's no urgency at\n> > this point, but I'd like to fix this. One way is to link against an\n> > external library, at the expense of restricting compile-tests, but\n> > another way would be to stop using it completely if we find a better\n> > API.\n> > \n> >>>>>>>> \tbool valid_;\n> >>>>>>>> };","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 69588BD878\n\tfor ;\n\tThu, 23 Jul 2020 10:28:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D809060E78;\n\tThu, 23 Jul 2020 12:28:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6EB060939\n\tfor ;\n\tThu, 23 Jul 2020 12:28:01 +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 6B7B4279;\n\tThu, 23 Jul 2020 12:28:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nk7axYb1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595500080;\n\tbh=FHabzw1mcgv1cKwGkh5EEE1A9Tz7FWr4VKefV73XACw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nk7axYb1BiYspRcAs0Vu555myqx5jj+Bwun1TGLFMSHAB4ON8qMf5Fwp0PbCd9ScC\n\toqaumYGGCttqvh2MtrWsBVOaEnTWFqpCxuXo1914iwBtK/V6q0g0E+9dLiPs5pLyR1\n\tmARcMAp6jUecPqfYQqyaEYjNOjF2h/e6zz8hQFaA=","Date":"Thu, 23 Jul 2020 13:27:54 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200723102754.GA5920@pendragon.ideasonboard.com>","References":"<20200702214009.2129404-1-kieran.bingham@ideasonboard.com>\n\t<20200703005334.GH12562@pendragon.ideasonboard.com>\n\t<20200703084951.due7dv3chcxftv7b@uno.localdomain>\n\t\n\t<20200703101248.mqor6zt5uken5ciq@uno.localdomain>\n\t<20200703105738.GG5963@pendragon.ideasonboard.com>\n\t<20200703114426.db2gs2ktxms4rkez@uno.localdomain>\n\t<20200703114721.GK5963@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH][RFC/UNTESTED] android:\n\tcamera_metadata: Track tags of each entry added","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]