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

8565 diff mbox series

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@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'...

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@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_;
>  };
>

Hi Laurent, Kieran,

On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> 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@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 ?

If I'm not mistaken Android uses int32_t

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

Wouldn't this require manual pre-calculation as we do today ?

> 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
>

That's my concern too.

I think it's acceptable to perform relocations while building the
static metadata at camera initialization time, but not for run time
usage. Maybe a different class just for static metadata would work
better ?

> 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Heya,

On 03/07/2020 09:49, Jacopo Mondi wrote:
> Hi Laurent, Kieran,
> 
> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
>> 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@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 ?
> 
> If I'm not mistaken Android uses int32_t

Looks like uint32_t is used everywhere, so that's probably the better
type to use.

>>
>> You should reserve() space in tags_ in the CameraMetadata constructor.
>>
> Wouldn't this require manual pre-calculation as we do today ?

Indeed, that's what I'm trying to remove. We could preallocate a size to
reduce likely hood of reallocations or such - but this might change
quite a bit anyway...

> 
>> 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
>>
> 
> That's my concern too.
> 
> I think it's acceptable to perform relocations while building the
> static metadata at camera initialization time, but not for run time
> usage. Maybe a different class just for static metadata would work
> better ?
> 
>> I like the automation this brings though, so maybe we could find a
>> different approach that would still bring the same improvement ?

I need to add more keys to the static data, so my main aim here is to
automate the calculations required throughout. Otherwise, I fear this
will go wrong quickly. I also fear that the calculations might already
be wrong, and could potentially be the cause of crashes I experience
with multi-stream support. However I haven't been able to confirm/deny
that theory yet. (or maybe the valid flag already tracks if we were/were
not able to add entries to the metadata successfully).



I have already been toying with subclassing CameraMetadata to make a
CameraMetadataNull, which would allow programmatically identifying the
sizes required, rather than manually.

(A fake MetaData instance which just tracks how many tags/ how much data
is added)

Equally, I could pull the vector out of the class, and have a wrapper to
addEntry() which tracks the tags, and just keep that in the
getStaticMetadata() function:

Class EntryTagTracker {
  EntryTagTracker(CameraMetadata *md) : md_(md) {};

  bool addEntry(uint32_t tag, const void *data, size_t data_count);
  {
	bool ret = md_->addEntry(tag, data, data_count);
	if (ret)
		tags_.push_back(tag);
	return ret;
  }

  const std::vector<int32_t> &tags() { return tags_; }

private:
  CameraMetadata *md_;
  std::vector<int32_t> tags_;
}


I have a vision forming to try to automate collection of the Request and
Result keys too, which would require a Null metadata object, and calling
(adapted) functions to extract the tags used and start up time.


In fact, pulling all that together, a fake metadata object which
/stores/ the tags, and tracks the size and count would then handle all
the requirements, so I might retry that path in a bit.


>>
>>>  	bool valid_;
>>>  };
>>>
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Kieran

On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:
> Heya,
>
> On 03/07/2020 09:49, Jacopo Mondi wrote:
> > Hi Laurent, Kieran,
> >
> > On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> >> 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@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 ?
> >
> > If I'm not mistaken Android uses int32_t
>
> Looks like uint32_t is used everywhere, so that's probably the better
> type to use.

uint32_t is used by the android metadata library, but tags types is
described as int32

    TYPE_INT32 = 1,

Seems like that they chose INT instead of UINT just to describe the
type, but the tags are actually stored as unsigned ?


>
> >>
> >> You should reserve() space in tags_ in the CameraMetadata constructor.
> >>
> > Wouldn't this require manual pre-calculation as we do today ?
>
> Indeed, that's what I'm trying to remove. We could preallocate a size to
> reduce likely hood of reallocations or such - but this might change
> quite a bit anyway...
>
> >
> >> 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
> >>
> >
> > That's my concern too.
> >
> > I think it's acceptable to perform relocations while building the
> > static metadata at camera initialization time, but not for run time
> > usage. Maybe a different class just for static metadata would work
> > better ?
> >
> >> I like the automation this brings though, so maybe we could find a
> >> different approach that would still bring the same improvement ?
>
> I need to add more keys to the static data, so my main aim here is to
> automate the calculations required throughout. Otherwise, I fear this
> will go wrong quickly. I also fear that the calculations might already
> be wrong, and could potentially be the cause of crashes I experience
> with multi-stream support. However I haven't been able to confirm/deny
> that theory yet. (or maybe the valid flag already tracks if we were/were
> not able to add entries to the metadata successfully).
>

If you're talking about the existing code, when I had not enough space
allocated, I noticed, it segfaults very early :)

>
>
> I have already been toying with subclassing CameraMetadata to make a
> CameraMetadataNull, which would allow programmatically identifying the
> sizes required, rather than manually.
>
> (A fake MetaData instance which just tracks how many tags/ how much data
> is added)
>
> Equally, I could pull the vector out of the class, and have a wrapper to
> addEntry() which tracks the tags, and just keep that in the
> getStaticMetadata() function:
>
> Class EntryTagTracker {
>   EntryTagTracker(CameraMetadata *md) : md_(md) {};
>
>   bool addEntry(uint32_t tag, const void *data, size_t data_count);
>   {
> 	bool ret = md_->addEntry(tag, data, data_count);
> 	if (ret)
> 		tags_.push_back(tag);
> 	return ret;
>   }
>
>   const std::vector<int32_t> &tags() { return tags_; }
>
> private:
>   CameraMetadata *md_;
>   std::vector<int32_t> tags_;
> }
>
>
> I have a vision forming to try to automate collection of the Request and
> Result keys too, which would require a Null metadata object, and calling
> (adapted) functions to extract the tags used and start up time.
>
>
> In fact, pulling all that together, a fake metadata object which
> /stores/ the tags, and tracks the size and count would then handle all
> the requirements, so I might retry that path in a bit.
>

I don't think that is has to be 'fake'.

Ideally our CameraMetadata wrapper should be changed to store tags in
a temporary container, tracking their number and sizes (the most
elegant way to do so would be to to provide an addEntry() overloaded
on the type of a vector<T> first argument, so that you can receive an

        addEntry(uin32_t tag, std::vector<T> &data)

Add the raw vector content in a temporary (and unfortunately
relocatable) storage space, and add them one by one to an actual
camera_metadata_t through an helper function like

        storeMetadata(camera_metadata_t *metadata)

Not a 30 minutes job I fear

>
> >>
> >>>  	bool valid_;
> >>>  };
> >>>
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran

Hi Jacopo,

On Fri, Jul 03, 2020 at 10:49:51AM +0200, Jacopo Mondi wrote:
> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> > 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@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 ?
> 
> If I'm not mistaken Android uses int32_t

Then we should fix addEntry(), it uses a uint32_t.

> > You should reserve() space in tags_ in the CameraMetadata constructor.
> 
> Wouldn't this require manual pre-calculation as we do today ?

It would, but we already do that pre-calculation :-) As the value is
already passed to the constructor, we could use it.

> > 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
> 
> That's my concern too.
> 
> I think it's acceptable to perform relocations while building the
> static metadata at camera initialization time, but not for run time
> usage. Maybe a different class just for static metadata would work
> better ?

Or an extra parameter to the constructor to tell if the metadata is
static or dynamic ? (This should then be a CameraMetadataType enum, not
a bool). In the dynamic case, we would skip the reserve() and
push_back().

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

Hello,

On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:
> > On 03/07/2020 09:49, Jacopo Mondi wrote:
> > > On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> > >> 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@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 ?
> > >
> > > If I'm not mistaken Android uses int32_t
> >
> > Looks like uint32_t is used everywhere, so that's probably the better
> > type to use.
> 
> uint32_t is used by the android metadata library, but tags types is
> described as int32
> 
>     TYPE_INT32 = 1,
> 
> Seems like that they chose INT instead of UINT just to describe the
> type, but the tags are actually stored as unsigned ?

That's for the value, not the tag itself, isn't it ?

> > >>
> > >> You should reserve() space in tags_ in the CameraMetadata constructor.
> > >>
> > > Wouldn't this require manual pre-calculation as we do today ?
> >
> > Indeed, that's what I'm trying to remove. We could preallocate a size to
> > reduce likely hood of reallocations or such - but this might change
> > quite a bit anyway...
> >
> > >> 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
> > >
> > > That's my concern too.
> > >
> > > I think it's acceptable to perform relocations while building the
> > > static metadata at camera initialization time, but not for run time
> > > usage. Maybe a different class just for static metadata would work
> > > better ?
> > >
> > >> I like the automation this brings though, so maybe we could find a
> > >> different approach that would still bring the same improvement ?
> >
> > I need to add more keys to the static data, so my main aim here is to
> > automate the calculations required throughout. Otherwise, I fear this
> > will go wrong quickly. I also fear that the calculations might already
> > be wrong, and could potentially be the cause of crashes I experience
> > with multi-stream support. However I haven't been able to confirm/deny
> > that theory yet. (or maybe the valid flag already tracks if we were/were
> > not able to add entries to the metadata successfully).
> 
> If you're talking about the existing code, when I had not enough space
> allocated, I noticed, it segfaults very early :)

Not the best way to notice though :-)

> > I have already been toying with subclassing CameraMetadata to make a
> > CameraMetadataNull, which would allow programmatically identifying the
> > sizes required, rather than manually.
> >
> > (A fake MetaData instance which just tracks how many tags/ how much data
> > is added)
> >
> > Equally, I could pull the vector out of the class, and have a wrapper to
> > addEntry() which tracks the tags, and just keep that in the
> > getStaticMetadata() function:
> >
> > Class EntryTagTracker {
> >   EntryTagTracker(CameraMetadata *md) : md_(md) {};
> >
> >   bool addEntry(uint32_t tag, const void *data, size_t data_count);
> >   {
> > 	bool ret = md_->addEntry(tag, data, data_count);
> > 	if (ret)
> > 		tags_.push_back(tag);
> > 	return ret;
> >   }
> >
> >   const std::vector<int32_t> &tags() { return tags_; }
> >
> > private:
> >   CameraMetadata *md_;
> >   std::vector<int32_t> tags_;
> > }
> >
> >
> > I have a vision forming to try to automate collection of the Request and
> > Result keys too, which would require a Null metadata object, and calling
> > (adapted) functions to extract the tags used and start up time.
> >
> >
> > In fact, pulling all that together, a fake metadata object which
> > /stores/ the tags, and tracks the size and count would then handle all
> > the requirements, so I might retry that path in a bit.
> 
> I don't think that is has to be 'fake'.
> 
> Ideally our CameraMetadata wrapper should be changed to store tags in
> a temporary container, tracking their number and sizes (the most
> elegant way to do so would be to to provide an addEntry() overloaded
> on the type of a vector<T> first argument, so that you can receive an
> 
>         addEntry(uin32_t tag, std::vector<T> &data)
> 
> Add the raw vector content in a temporary (and unfortunately
> relocatable) storage space, and add them one by one to an actual
> camera_metadata_t through an helper function like
> 
>         storeMetadata(camera_metadata_t *metadata)
> 
> Not a 30 minutes job I fear

For static metadata this is completely fine, we can make it more complex
(in the CPU usage sense) with additional memory allocation to achieve a
simpler API. We would store data in custom containers and generate an
Android metadata buffer at the end. That would be my preference, but it
will take a bit of time to develop. It could even allow us to ditch the
Android metadata library.

For dynamic metadata, it's a bit of a different story, as we want to
minimize the memory allocations.

> > >>>  	bool valid_;
> > >>>  };

Hi Laurent,

On 03/07/2020 11:51, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Fri, Jul 03, 2020 at 10:49:51AM +0200, Jacopo Mondi wrote:
>> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
>>> 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@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 ?
>>
>> If I'm not mistaken Android uses int32_t
> 
> Then we should fix addEntry(), it uses a uint32_t.
> 
>>> You should reserve() space in tags_ in the CameraMetadata constructor.
>>
>> Wouldn't this require manual pre-calculation as we do today ?
> 
> It would, but we already do that pre-calculation :-) As the value is
> already passed to the constructor, we could use it.

Ok, I see the point, but this is part of a (not yet existing series) to
/remove/ those pre-calculations ;-)

If I get back to this I'll add the reserve, but it would (hopefully) get
removed again in the same series.

--
Kieran



> 
>>> 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
>>
>> That's my concern too.
>>
>> I think it's acceptable to perform relocations while building the
>> static metadata at camera initialization time, but not for run time
>> usage. Maybe a different class just for static metadata would work
>> better ?
> 
> Or an extra parameter to the constructor to tell if the metadata is
> static or dynamic ? (This should then be a CameraMetadataType enum, not
> a bool). In the dynamic case, we would skip the reserve() and
> push_back().
> 
>>> I like the automation this brings though, so maybe we could find a
>>> different approach that would still bring the same improvement ?
>>>
>>>>  	bool valid_;
>>>>  };
>>>>
>

Hi Laurent,

On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:
> > On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:
> > > On 03/07/2020 09:49, Jacopo Mondi wrote:
> > > > On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> > > >> 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@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 ?
> > > >
> > > > If I'm not mistaken Android uses int32_t
> > >
> > > Looks like uint32_t is used everywhere, so that's probably the better
> > > type to use.
> >
> > uint32_t is used by the android metadata library, but tags types is
> > described as int32
> >
> >     TYPE_INT32 = 1,
> >
> > Seems like that they chose INT instead of UINT just to describe the
> > type, but the tags are actually stored as unsigned ?
>
> That's for the value, not the tag itself, isn't it ?
>

right! I confused ht two /(0.0)\

> > > >>
> > > >> You should reserve() space in tags_ in the CameraMetadata constructor.
> > > >>
> > > > Wouldn't this require manual pre-calculation as we do today ?
> > >
> > > Indeed, that's what I'm trying to remove. We could preallocate a size to
> > > reduce likely hood of reallocations or such - but this might change
> > > quite a bit anyway...
> > >
> > > >> 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
> > > >
> > > > That's my concern too.
> > > >
> > > > I think it's acceptable to perform relocations while building the
> > > > static metadata at camera initialization time, but not for run time
> > > > usage. Maybe a different class just for static metadata would work
> > > > better ?
> > > >
> > > >> I like the automation this brings though, so maybe we could find a
> > > >> different approach that would still bring the same improvement ?
> > >
> > > I need to add more keys to the static data, so my main aim here is to
> > > automate the calculations required throughout. Otherwise, I fear this
> > > will go wrong quickly. I also fear that the calculations might already
> > > be wrong, and could potentially be the cause of crashes I experience
> > > with multi-stream support. However I haven't been able to confirm/deny
> > > that theory yet. (or maybe the valid flag already tracks if we were/were
> > > not able to add entries to the metadata successfully).
> >
> > If you're talking about the existing code, when I had not enough space
> > allocated, I noticed, it segfaults very early :)
>
> Not the best way to notice though :-)
>

Effective, for sure.

What I wanted to point out was to help Kieran in ruling out a wrong
space allocation issue, which when happened to me was quite noticeable!

> > > I have already been toying with subclassing CameraMetadata to make a
> > > CameraMetadataNull, which would allow programmatically identifying the
> > > sizes required, rather than manually.
> > >
> > > (A fake MetaData instance which just tracks how many tags/ how much data
> > > is added)
> > >
> > > Equally, I could pull the vector out of the class, and have a wrapper to
> > > addEntry() which tracks the tags, and just keep that in the
> > > getStaticMetadata() function:
> > >
> > > Class EntryTagTracker {
> > >   EntryTagTracker(CameraMetadata *md) : md_(md) {};
> > >
> > >   bool addEntry(uint32_t tag, const void *data, size_t data_count);
> > >   {
> > > 	bool ret = md_->addEntry(tag, data, data_count);
> > > 	if (ret)
> > > 		tags_.push_back(tag);
> > > 	return ret;
> > >   }
> > >
> > >   const std::vector<int32_t> &tags() { return tags_; }
> > >
> > > private:
> > >   CameraMetadata *md_;
> > >   std::vector<int32_t> tags_;
> > > }
> > >
> > >
> > > I have a vision forming to try to automate collection of the Request and
> > > Result keys too, which would require a Null metadata object, and calling
> > > (adapted) functions to extract the tags used and start up time.
> > >
> > >
> > > In fact, pulling all that together, a fake metadata object which
> > > /stores/ the tags, and tracks the size and count would then handle all
> > > the requirements, so I might retry that path in a bit.
> >
> > I don't think that is has to be 'fake'.
> >
> > Ideally our CameraMetadata wrapper should be changed to store tags in
> > a temporary container, tracking their number and sizes (the most
> > elegant way to do so would be to to provide an addEntry() overloaded
> > on the type of a vector<T> first argument, so that you can receive an
> >
> >         addEntry(uin32_t tag, std::vector<T> &data)
> >
> > Add the raw vector content in a temporary (and unfortunately
> > relocatable) storage space, and add them one by one to an actual
> > camera_metadata_t through an helper function like
> >
> >         storeMetadata(camera_metadata_t *metadata)
> >
> > Not a 30 minutes job I fear
>
> For static metadata this is completely fine, we can make it more complex
> (in the CPU usage sense) with additional memory allocation to achieve a
> simpler API. We would store data in custom containers and generate an
> Android metadata buffer at the end. That would be my preference, but it
> will take a bit of time to develop. It could even allow us to ditch the
> Android metadata library.
>
> For dynamic metadata, it's a bit of a different story, as we want to
> minimize the memory allocations.

yes this should be considered for static metadata only. I'll let
Kieran consider if that's wroth the effort at this point..

>
> > > >>>  	bool valid_;
> > > >>>  };
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Fri, Jul 03, 2020 at 01:44:26PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:
> >> On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:
> >>> On 03/07/2020 09:49, Jacopo Mondi wrote:
> >>>> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> >>>>> 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@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 ?
> >>>>
> >>>> If I'm not mistaken Android uses int32_t
> >>>
> >>> Looks like uint32_t is used everywhere, so that's probably the better
> >>> type to use.
> >>
> >> uint32_t is used by the android metadata library, but tags types is
> >> described as int32
> >>
> >>     TYPE_INT32 = 1,
> >>
> >> Seems like that they chose INT instead of UINT just to describe the
> >> type, but the tags are actually stored as unsigned ?
> >
> > That's for the value, not the tag itself, isn't it ?
> >
> 
> right! I confused ht two /(0.0)\
> 
> >>>>>
> >>>>> You should reserve() space in tags_ in the CameraMetadata constructor.
> >>>>>
> >>>> Wouldn't this require manual pre-calculation as we do today ?
> >>>
> >>> Indeed, that's what I'm trying to remove. We could preallocate a size to
> >>> reduce likely hood of reallocations or such - but this might change
> >>> quite a bit anyway...
> >>>
> >>>>> 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
> >>>>
> >>>> That's my concern too.
> >>>>
> >>>> I think it's acceptable to perform relocations while building the
> >>>> static metadata at camera initialization time, but not for run time
> >>>> usage. Maybe a different class just for static metadata would work
> >>>> better ?
> >>>>
> >>>>> I like the automation this brings though, so maybe we could find a
> >>>>> different approach that would still bring the same improvement ?
> >>>
> >>> I need to add more keys to the static data, so my main aim here is to
> >>> automate the calculations required throughout. Otherwise, I fear this
> >>> will go wrong quickly. I also fear that the calculations might already
> >>> be wrong, and could potentially be the cause of crashes I experience
> >>> with multi-stream support. However I haven't been able to confirm/deny
> >>> that theory yet. (or maybe the valid flag already tracks if we were/were
> >>> not able to add entries to the metadata successfully).
> >>
> >> If you're talking about the existing code, when I had not enough space
> >> allocated, I noticed, it segfaults very early :)
> >
> > Not the best way to notice though :-)
> >
> 
> Effective, for sure.
> 
> What I wanted to point out was to help Kieran in ruling out a wrong
> space allocation issue, which when happened to me was quite noticeable!
> 
> >>> I have already been toying with subclassing CameraMetadata to make a
> >>> CameraMetadataNull, which would allow programmatically identifying the
> >>> sizes required, rather than manually.
> >>>
> >>> (A fake MetaData instance which just tracks how many tags/ how much data
> >>> is added)
> >>>
> >>> Equally, I could pull the vector out of the class, and have a wrapper to
> >>> addEntry() which tracks the tags, and just keep that in the
> >>> getStaticMetadata() function:
> >>>
> >>> Class EntryTagTracker {
> >>>   EntryTagTracker(CameraMetadata *md) : md_(md) {};
> >>>
> >>>   bool addEntry(uint32_t tag, const void *data, size_t data_count);
> >>>   {
> >>> 	bool ret = md_->addEntry(tag, data, data_count);
> >>> 	if (ret)
> >>> 		tags_.push_back(tag);
> >>> 	return ret;
> >>>   }
> >>>
> >>>   const std::vector<int32_t> &tags() { return tags_; }
> >>>
> >>> private:
> >>>   CameraMetadata *md_;
> >>>   std::vector<int32_t> tags_;
> >>> }
> >>>
> >>>
> >>> I have a vision forming to try to automate collection of the Request and
> >>> Result keys too, which would require a Null metadata object, and calling
> >>> (adapted) functions to extract the tags used and start up time.
> >>>
> >>>
> >>> In fact, pulling all that together, a fake metadata object which
> >>> /stores/ the tags, and tracks the size and count would then handle all
> >>> the requirements, so I might retry that path in a bit.
> >>
> >> I don't think that is has to be 'fake'.
> >>
> >> Ideally our CameraMetadata wrapper should be changed to store tags in
> >> a temporary container, tracking their number and sizes (the most
> >> elegant way to do so would be to to provide an addEntry() overloaded
> >> on the type of a vector<T> first argument, so that you can receive an
> >>
> >>         addEntry(uin32_t tag, std::vector<T> &data)
> >>
> >> Add the raw vector content in a temporary (and unfortunately
> >> relocatable) storage space, and add them one by one to an actual
> >> camera_metadata_t through an helper function like
> >>
> >>         storeMetadata(camera_metadata_t *metadata)
> >>
> >> Not a 30 minutes job I fear
> >
> > For static metadata this is completely fine, we can make it more complex
> > (in the CPU usage sense) with additional memory allocation to achieve a
> > simpler API. We would store data in custom containers and generate an
> > Android metadata buffer at the end. That would be my preference, but it
> > will take a bit of time to develop. It could even allow us to ditch the
> > Android metadata library.
> >
> > For dynamic metadata, it's a bit of a different story, as we want to
> > minimize the memory allocations.
> 
> yes this should be considered for static metadata only. I'll let
> Kieran consider if that's wroth the effort at this point..

Another point to consider on this topic is the inclusion of the Android
metadata library in the libcamera sources. It's a small library,
available as a shared object on both Android and Chrome OS, but we have
pulled its source in libcamera in order to allow compile-tests without
depending on a Chrome OS or Android environment. There's no urgency at
this point, but I'd like to fix this. One way is to link against an
external library, at the expense of restricting compile-tests, but
another way would be to stop using it completely if we find a better
API.

> >>>>>>  	bool valid_;
> >>>>>>  };

Hi Laurent,

On 03/07/2020 12:47, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Fri, Jul 03, 2020 at 01:44:26PM +0200, Jacopo Mondi wrote:
>> On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:
>>> On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:
>>>> On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:
>>>>> On 03/07/2020 09:49, Jacopo Mondi wrote:
>>>>>> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
>>>>>>> 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@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 ?
>>>>>>
>>>>>> If I'm not mistaken Android uses int32_t
>>>>>
>>>>> Looks like uint32_t is used everywhere, so that's probably the better
>>>>> type to use.
>>>>
>>>> uint32_t is used by the android metadata library, but tags types is
>>>> described as int32
>>>>
>>>>     TYPE_INT32 = 1,
>>>>
>>>> Seems like that they chose INT instead of UINT just to describe the
>>>> type, but the tags are actually stored as unsigned ?
>>>
>>> That's for the value, not the tag itself, isn't it ?
>>>
>>
>> right! I confused ht two /(0.0)\
>>
>>>>>>>
>>>>>>> You should reserve() space in tags_ in the CameraMetadata constructor.
>>>>>>>
>>>>>> Wouldn't this require manual pre-calculation as we do today ?
>>>>>
>>>>> Indeed, that's what I'm trying to remove. We could preallocate a size to
>>>>> reduce likely hood of reallocations or such - but this might change
>>>>> quite a bit anyway...
>>>>>
>>>>>>> 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
>>>>>>
>>>>>> That's my concern too.
>>>>>>
>>>>>> I think it's acceptable to perform relocations while building the
>>>>>> static metadata at camera initialization time, but not for run time
>>>>>> usage. Maybe a different class just for static metadata would work
>>>>>> better ?
>>>>>>
>>>>>>> I like the automation this brings though, so maybe we could find a
>>>>>>> different approach that would still bring the same improvement ?
>>>>>
>>>>> I need to add more keys to the static data, so my main aim here is to
>>>>> automate the calculations required throughout. Otherwise, I fear this
>>>>> will go wrong quickly. I also fear that the calculations might already
>>>>> be wrong, and could potentially be the cause of crashes I experience
>>>>> with multi-stream support. However I haven't been able to confirm/deny
>>>>> that theory yet. (or maybe the valid flag already tracks if we were/were
>>>>> not able to add entries to the metadata successfully).
>>>>
>>>> If you're talking about the existing code, when I had not enough space
>>>> allocated, I noticed, it segfaults very early :)
>>>
>>> Not the best way to notice though :-)
>>>
>>
>> Effective, for sure.
>>
>> What I wanted to point out was to help Kieran in ruling out a wrong
>> space allocation issue, which when happened to me was quite noticeable!
>>
>>>>> I have already been toying with subclassing CameraMetadata to make a
>>>>> CameraMetadataNull, which would allow programmatically identifying the
>>>>> sizes required, rather than manually.
>>>>>
>>>>> (A fake MetaData instance which just tracks how many tags/ how much data
>>>>> is added)
>>>>>
>>>>> Equally, I could pull the vector out of the class, and have a wrapper to
>>>>> addEntry() which tracks the tags, and just keep that in the
>>>>> getStaticMetadata() function:
>>>>>
>>>>> Class EntryTagTracker {
>>>>>   EntryTagTracker(CameraMetadata *md) : md_(md) {};
>>>>>
>>>>>   bool addEntry(uint32_t tag, const void *data, size_t data_count);
>>>>>   {
>>>>> 	bool ret = md_->addEntry(tag, data, data_count);
>>>>> 	if (ret)
>>>>> 		tags_.push_back(tag);
>>>>> 	return ret;
>>>>>   }
>>>>>
>>>>>   const std::vector<int32_t> &tags() { return tags_; }
>>>>>
>>>>> private:
>>>>>   CameraMetadata *md_;
>>>>>   std::vector<int32_t> tags_;
>>>>> }
>>>>>
>>>>>
>>>>> I have a vision forming to try to automate collection of the Request and
>>>>> Result keys too, which would require a Null metadata object, and calling
>>>>> (adapted) functions to extract the tags used and start up time.
>>>>>
>>>>>
>>>>> In fact, pulling all that together, a fake metadata object which
>>>>> /stores/ the tags, and tracks the size and count would then handle all
>>>>> the requirements, so I might retry that path in a bit.
>>>>
>>>> I don't think that is has to be 'fake'.
>>>>
>>>> Ideally our CameraMetadata wrapper should be changed to store tags in
>>>> a temporary container, tracking their number and sizes (the most
>>>> elegant way to do so would be to to provide an addEntry() overloaded
>>>> on the type of a vector<T> first argument, so that you can receive an
>>>>
>>>>         addEntry(uin32_t tag, std::vector<T> &data)
>>>>
>>>> Add the raw vector content in a temporary (and unfortunately
>>>> relocatable) storage space, and add them one by one to an actual
>>>> camera_metadata_t through an helper function like
>>>>
>>>>         storeMetadata(camera_metadata_t *metadata)
>>>>
>>>> Not a 30 minutes job I fear
>>>
>>> For static metadata this is completely fine, we can make it more complex
>>> (in the CPU usage sense) with additional memory allocation to achieve a
>>> simpler API. We would store data in custom containers and generate an
>>> Android metadata buffer at the end. That would be my preference, but it
>>> will take a bit of time to develop. It could even allow us to ditch the
>>> Android metadata library.
>>>
>>> For dynamic metadata, it's a bit of a different story, as we want to
>>> minimize the memory allocations.
>>
>> yes this should be considered for static metadata only. I'll let
>> Kieran consider if that's wroth the effort at this point..

I think I got confused where the feeling of this RFC went.

I need to dynamically add keys to requests.

For example, the JPEG result size would only be added to requests which
performed a JPEG encode.


So we can not with a fixed value determine in advance how many entries
we will add, without going through the process of executing the code to
decide.

We could determine a 'larger/large enough' value in advance perhaps.
Would that be more to  your liking?

That maximum size can be determined during the stage where the available
keys are initialised anyway.


Otherwise, I can not see a route forwards without making some kind of
dynamic allocation ...



> Another point to consider on this topic is the inclusion of the Android
> metadata library in the libcamera sources. It's a small library,
> available as a shared object on both Android and Chrome OS, but we have
> pulled its source in libcamera in order to allow compile-tests without
> depending on a Chrome OS or Android environment. There's no urgency at
> this point, but I'd like to fix this. One way is to link against an
> external library, at the expense of restricting compile-tests, but
> another way would be to stop using it completely if we find a better
> API.
> 
>>>>>>>>  	bool valid_;
>>>>>>>>  };
>

Hi Kieran,

On Thu, Jul 23, 2020 at 10:26:58AM +0100, Kieran Bingham wrote:
> On 03/07/2020 12:47, Laurent Pinchart wrote:
> > On Fri, Jul 03, 2020 at 01:44:26PM +0200, Jacopo Mondi wrote:
> >> On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:
> >>> On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:
> >>>> On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:
> >>>>> On 03/07/2020 09:49, Jacopo Mondi wrote:
> >>>>>> On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> >>>>>>> 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@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 ?
> >>>>>>
> >>>>>> If I'm not mistaken Android uses int32_t
> >>>>>
> >>>>> Looks like uint32_t is used everywhere, so that's probably the better
> >>>>> type to use.
> >>>>
> >>>> uint32_t is used by the android metadata library, but tags types is
> >>>> described as int32
> >>>>
> >>>>     TYPE_INT32 = 1,
> >>>>
> >>>> Seems like that they chose INT instead of UINT just to describe the
> >>>> type, but the tags are actually stored as unsigned ?
> >>>
> >>> That's for the value, not the tag itself, isn't it ?
> >>>
> >>
> >> right! I confused ht two /(0.0)\
> >>
> >>>>>>>
> >>>>>>> You should reserve() space in tags_ in the CameraMetadata constructor.
> >>>>>>>
> >>>>>> Wouldn't this require manual pre-calculation as we do today ?
> >>>>>
> >>>>> Indeed, that's what I'm trying to remove. We could preallocate a size to
> >>>>> reduce likely hood of reallocations or such - but this might change
> >>>>> quite a bit anyway...
> >>>>>
> >>>>>>> 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
> >>>>>>
> >>>>>> That's my concern too.
> >>>>>>
> >>>>>> I think it's acceptable to perform relocations while building the
> >>>>>> static metadata at camera initialization time, but not for run time
> >>>>>> usage. Maybe a different class just for static metadata would work
> >>>>>> better ?
> >>>>>>
> >>>>>>> I like the automation this brings though, so maybe we could find a
> >>>>>>> different approach that would still bring the same improvement ?
> >>>>>
> >>>>> I need to add more keys to the static data, so my main aim here is to
> >>>>> automate the calculations required throughout. Otherwise, I fear this
> >>>>> will go wrong quickly. I also fear that the calculations might already
> >>>>> be wrong, and could potentially be the cause of crashes I experience
> >>>>> with multi-stream support. However I haven't been able to confirm/deny
> >>>>> that theory yet. (or maybe the valid flag already tracks if we were/were
> >>>>> not able to add entries to the metadata successfully).
> >>>>
> >>>> If you're talking about the existing code, when I had not enough space
> >>>> allocated, I noticed, it segfaults very early :)
> >>>
> >>> Not the best way to notice though :-)
> >>>
> >>
> >> Effective, for sure.
> >>
> >> What I wanted to point out was to help Kieran in ruling out a wrong
> >> space allocation issue, which when happened to me was quite noticeable!
> >>
> >>>>> I have already been toying with subclassing CameraMetadata to make a
> >>>>> CameraMetadataNull, which would allow programmatically identifying the
> >>>>> sizes required, rather than manually.
> >>>>>
> >>>>> (A fake MetaData instance which just tracks how many tags/ how much data
> >>>>> is added)
> >>>>>
> >>>>> Equally, I could pull the vector out of the class, and have a wrapper to
> >>>>> addEntry() which tracks the tags, and just keep that in the
> >>>>> getStaticMetadata() function:
> >>>>>
> >>>>> Class EntryTagTracker {
> >>>>>   EntryTagTracker(CameraMetadata *md) : md_(md) {};
> >>>>>
> >>>>>   bool addEntry(uint32_t tag, const void *data, size_t data_count);
> >>>>>   {
> >>>>> 	bool ret = md_->addEntry(tag, data, data_count);
> >>>>> 	if (ret)
> >>>>> 		tags_.push_back(tag);
> >>>>> 	return ret;
> >>>>>   }
> >>>>>
> >>>>>   const std::vector<int32_t> &tags() { return tags_; }
> >>>>>
> >>>>> private:
> >>>>>   CameraMetadata *md_;
> >>>>>   std::vector<int32_t> tags_;
> >>>>> }
> >>>>>
> >>>>>
> >>>>> I have a vision forming to try to automate collection of the Request and
> >>>>> Result keys too, which would require a Null metadata object, and calling
> >>>>> (adapted) functions to extract the tags used and start up time.
> >>>>>
> >>>>>
> >>>>> In fact, pulling all that together, a fake metadata object which
> >>>>> /stores/ the tags, and tracks the size and count would then handle all
> >>>>> the requirements, so I might retry that path in a bit.
> >>>>
> >>>> I don't think that is has to be 'fake'.
> >>>>
> >>>> Ideally our CameraMetadata wrapper should be changed to store tags in
> >>>> a temporary container, tracking their number and sizes (the most
> >>>> elegant way to do so would be to to provide an addEntry() overloaded
> >>>> on the type of a vector<T> first argument, so that you can receive an
> >>>>
> >>>>         addEntry(uin32_t tag, std::vector<T> &data)
> >>>>
> >>>> Add the raw vector content in a temporary (and unfortunately
> >>>> relocatable) storage space, and add them one by one to an actual
> >>>> camera_metadata_t through an helper function like
> >>>>
> >>>>         storeMetadata(camera_metadata_t *metadata)
> >>>>
> >>>> Not a 30 minutes job I fear
> >>>
> >>> For static metadata this is completely fine, we can make it more complex
> >>> (in the CPU usage sense) with additional memory allocation to achieve a
> >>> simpler API. We would store data in custom containers and generate an
> >>> Android metadata buffer at the end. That would be my preference, but it
> >>> will take a bit of time to develop. It could even allow us to ditch the
> >>> Android metadata library.
> >>>
> >>> For dynamic metadata, it's a bit of a different story, as we want to
> >>> minimize the memory allocations.
> >>
> >> yes this should be considered for static metadata only. I'll let
> >> Kieran consider if that's wroth the effort at this point..
> 
> I think I got confused where the feeling of this RFC went.
> 
> I need to dynamically add keys to requests.
> 
> For example, the JPEG result size would only be added to requests which
> performed a JPEG encode.
> 
> So we can not with a fixed value determine in advance how many entries
> we will add, without going through the process of executing the code to
> decide.
> 
> We could determine a 'larger/large enough' value in advance perhaps.
> Would that be more to  your liking?
> 
> That maximum size can be determined during the stage where the available
> keys are initialised anyway.
> 
> Otherwise, I can not see a route forwards without making some kind of
> dynamic allocation ...

I see three options:

- Adding metadata to a dynamic container (e.g. std::map) and generating
  the Android metadata blob at the end

- Running a first pass to compute the size of the metadata buffer and a
  second pass to fill it.

- Calculating a worst case size at initialization time and using it to
  allocate metadata buffers.

The first option requires dynamic allocation, the second option uses
more CPU time, and the third option uses more memory than strictly
necessary. The code structure would likely also be more or less clean
depending on which option is picked.

As stated before, for static metadata, extra CPU time and dynamic memory
allocation isn't much of an issue. I'd prioritize ease of use there, and
code sharing with the dynamic metadata case. For dynamic metadata, it
would be better to minimize dynamic allocations as much as possible.

Let's also not forget that some metadata tags store a variable number of
entries. That's mostly used for static metadata and controls, but a few
dynamic metadata tags are also affected (android.request.outputStreams,
and several of the android.statistics.* and android.tonemap.* tags). The
number of entries should however be either bounded (e.g.
android.statistics.faceIds is bounded by
android.statistics.info.maxFaceCount) or constant for a given
configuration (e.g. the number of entries in
android.statistics.histogram doesn't vary with each frame).

> > Another point to consider on this topic is the inclusion of the Android
> > metadata library in the libcamera sources. It's a small library,
> > available as a shared object on both Android and Chrome OS, but we have
> > pulled its source in libcamera in order to allow compile-tests without
> > depending on a Chrome OS or Android environment. There's no urgency at
> > this point, but I'd like to fix this. One way is to link against an
> > external library, at the expense of restricting compile-tests, but
> > another way would be to stop using it completely if we find a better
> > API.
> > 
> >>>>>>>>  	bool valid_;
> >>>>>>>>  };