[libcamera-devel] [PATCH][RFC/UNTESTED] android: camera_metadata: Track tags of each entry added

[Laurent Pinchart]

Hi Kieran,

On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:
> Provide automatic tracking of tags added to automatically report the
> keys used for the entry:
>  ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
> 
> This allows automatic addition of added keys without having to manually
> maintain the list in the code base.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/android/camera_device.cpp   | 57 ++++-----------------------------
>  src/android/camera_metadata.cpp |  4 ++-
>  src/android/camera_metadata.h   |  4 +++
>  3 files changed, 13 insertions(+), 52 deletions(-)
> 
> Sending this, completely untested because ... that's how I roll, and I
> wanted to know if this is a reasonable route to reduce maintainance
> burden.
> 
> A next step beyond this is also to consider a two-pass iteration on all
> of the meta-data structures, where the first pass will determine the
> number of entries, and bytes required, while the second pass will
> actually populate the android metadata.
> 
> Anythoughts on this? It would mean processing the entries twice, but
> would stop the guessing game of 'is there enough memory allocated
> here'...
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 5a3b4dfae6a0..de73c31ed3ea 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  				  availableCapabilities.data(),
>  				  availableCapabilities.size());
>  
> -	std::vector<int32_t> availableCharacteristicsKeys = {
> -		ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,
> -		ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> -		ANDROID_CONTROL_AE_AVAILABLE_MODES,
> -		ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,
> -		ANDROID_CONTROL_AE_COMPENSATION_RANGE,
> -		ANDROID_CONTROL_AE_COMPENSATION_STEP,
> -		ANDROID_CONTROL_AF_AVAILABLE_MODES,
> -		ANDROID_CONTROL_AVAILABLE_EFFECTS,
> -		ANDROID_CONTROL_AVAILABLE_SCENE_MODES,
> -		ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,
> -		ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> -		ANDROID_CONTROL_MAX_REGIONS,
> -		ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
> -		ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> -		ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> -		ANDROID_CONTROL_AVAILABLE_MODES,
> -		ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> -		ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> -		ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> -		ANDROID_SENSOR_INFO_SENSITIVITY_RANGE,
> -		ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> -		ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> -		ANDROID_SENSOR_ORIENTATION,
> -		ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
> -		ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> -		ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
> -		ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,
> -		ANDROID_STATISTICS_INFO_MAX_FACE_COUNT,
> -		ANDROID_SYNC_MAX_LATENCY,
> -		ANDROID_FLASH_INFO_AVAILABLE,
> -		ANDROID_LENS_INFO_AVAILABLE_APERTURES,
> -		ANDROID_LENS_FACING,
> -		ANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,
> -		ANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,
> -		ANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,
> -		ANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,
> -		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> -		ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
> -		ANDROID_SCALER_AVAILABLE_FORMATS,
> -		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
> -		ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
> -		ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
> -		ANDROID_SCALER_CROPPING_TYPE,
> -		ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> -		ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> -		ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> -		ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> -	};
> +	/*
> +	 * All tags added to staticMetadata_ to this point are added
> +	 * as keys for the available characteristics.
> +	 */
>  	staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
> -				  availableCharacteristicsKeys.data(),
> -				  availableCharacteristicsKeys.size());
> +				  staticMetadata_->tags().data(),
> +				  staticMetadata_->tags().size());
>  
>  	std::vector<int32_t> availableRequestKeys = {
>  		ANDROID_CONTROL_AE_MODE,
> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> index 47b2e4ef117a..15b569aea52b 100644
> --- a/src/android/camera_metadata.cpp
> +++ b/src/android/camera_metadata.cpp
> @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
>  	if (!valid_)
>  		return false;
>  
> -	if (!add_camera_metadata_entry(metadata_, tag, data, count))
> +	if (!add_camera_metadata_entry(metadata_, tag, data, count)) {
> +		tags_.push_back(tag);
>  		return true;
> +	}
>  
>  	const char *name = get_camera_metadata_tag_name(tag);
>  	if (name)
> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> index 75a9d7066f31..a0e23119e68f 100644
> --- a/src/android/camera_metadata.h
> +++ b/src/android/camera_metadata.h
> @@ -8,6 +8,7 @@
>  #define __ANDROID_CAMERA_METADATA_H__
>  
>  #include <stdint.h>
> +#include <vector>
>  
>  #include <system/camera_metadata.h>
>  
> @@ -20,10 +21,13 @@ public:
>  	bool isValid() { return valid_; }
>  	bool addEntry(uint32_t tag, const void *data, size_t data_count);
>  
> +	const std::vector<int32_t> &tags() { return tags_; }
> +
>  	camera_metadata_t *get();
>  
>  private:
>  	camera_metadata_t *metadata_;
> +	std::vector<int32_t> tags_;

Aren't tags unsigned ?

You should reserve() space in tags_ in the CameraMetadata constructor.

As CameraMetadata is also used to report dynamic metadata, we will
always add tags to the vector, even if they're only used for static
metadata. Not very nice, given that we should try to minimize dynamic
memory allocation during streaming :-S

I like the automation this brings though, so maybe we could find a
different approach that would still bring the same improvement ?

>  	bool valid_;
>  };
>  

-- 
Regards,

Laurent Pinchart