[libcamera-devel,RFC] android: camera_metadata: Auto-resize CameraMetadata

12206 diff mbox series

This patch depends on the series "FULL hardware level fixes".

Previously we had to manually declare the size of CameraMetadata on
allocation, and its count not be changed after construction. Change
CameraMetadata's behavior so that the user can simply add entries, and
the CameraMetadata will auto-resize (double the size) as necessary. At
the same time, make addEntry() also serve the purpose of updateEntry(),
and remove updateEntry(). Also remove everything involved with
calculating the initial size for any CameraMetadata instances.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
1 - What do you (plural) think about merging updateEntry() into
    addEntry()? I thought that with the dynamic allocation it would be
    convenient to use one function. Is there any reason that we should
    keep them separate, or is it fine to merge them?
2 - How can I get logging working in the CameraMetadata header file? The
    last time I did that was in ipa_data_serializer.h and that wasn't very
    pretty either...

(I haven't tested it on CTS yet; this is just RFC for the API and
implementation)
---
 src/android/camera_device.cpp   | 79 +++++----------------------------
 src/android/camera_device.h     |  1 -
 src/android/camera_metadata.cpp | 61 +++++++++----------------
 src/android/camera_metadata.h   | 39 +++++++++++++++-
 4 files changed, 71 insertions(+), 109 deletions(-)

Hi Paul,

On 06/05/2021 11:41, Paul Elder wrote:
> This patch depends on the series "FULL hardware level fixes".
> 
> Previously we had to manually declare the size of CameraMetadata on
> allocation, and its count not be changed after construction. Change

count could not

> CameraMetadata's behavior so that the user can simply add entries, and
> the CameraMetadata will auto-resize (double the size) as necessary. At
> the same time, make addEntry() also serve the purpose of updateEntry(),
> and remove updateEntry(). Also remove everything involved with
> calculating the initial size for any CameraMetadata instances.

Oh that sweet music to my ears ...


> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> 1 - What do you (plural) think about merging updateEntry() into
>     addEntry()? I thought that with the dynamic allocation it would be
>     convenient to use one function. Is there any reason that we should
>     keep them separate, or is it fine to merge them?

What's the current distinction?
Presumably addEntry requires that the entry doesn't yet exist, whereas
updateEntry would modify an existing entry?

For naming, if we were merging both, wouldn't it be better to call it
'->set(id, value)'?


> 2 - How can I get logging working in the CameraMetadata header file? The
>     last time I did that was in ipa_data_serializer.h and that wasn't very
>     pretty either...
> 
> (I haven't tested it on CTS yet; this is just RFC for the API and
> implementation)
> ---
>  src/android/camera_device.cpp   | 79 +++++----------------------------
>  src/android/camera_device.h     |  1 -
>  src/android/camera_metadata.cpp | 61 +++++++++----------------
>  src/android/camera_metadata.h   | 39 +++++++++++++++-
>  4 files changed, 71 insertions(+), 109 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 114348a6..426e3fcd 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -772,53 +772,6 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
>  	callbacks_ = callbacks;
>  }
>  
> -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
> -{
> -	/*
> -	 * \todo Keep this in sync with the actual number of entries.
> -	 * Currently: 63 entries, 1014 bytes of static metadata
> -	 */
> -	uint32_t numEntries = 63;
> -	uint32_t byteSize = 1014;
> -
> -	// do i need to add for entries in the available keys?
> -	// +1, +4 for EDGE_AVAILABLE_EDGE_MODES
> -	// +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES
> -	// +1, +4 for BLACK_LEVEL_PATTERN
> -	// +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES
> -	// +1, +4 for TONEMAP_MAX_CURVE_POINTS
> -	// +4x9 = 36 for the new result tags
> -
> -	// +36 for new request keys
> -
> -	/*
> -	 * Calculate space occupation in bytes for dynamically built metadata
> -	 * entries.
> -	 *
> -	 * Each stream configuration entry requires 48 bytes:
> -	 * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
> -	 * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
> -	 */
> -	byteSize += streamConfigurations_.size() * 48;
> -
> -	/*
> -	 * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes
> -	 * 2 32bits integers for the (0, 0) thumbnail size
> -	 *
> -	 * This is a worst case estimates as different configurations with the
> -	 * same aspect ratio will generate the same size.
> -	 */
> -	for (const auto &entry : streamConfigurations_) {
> -		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> -			continue;
> -
> -		byteSize += 8;
> -	}
> -	byteSize += 8;
> -
> -	return std::make_tuple(numEntries, byteSize);
> -}
> -
>  /*
>   * Return static information for the camera.
>   */
> @@ -827,15 +780,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	if (staticMetadata_)
>  		return staticMetadata_->get();
>  
> -	/*
> -	 * The here reported metadata are enough to implement a basic capture
> -	 * example application, but a real camera implementation will require
> -	 * more.
> -	 */
> -	uint32_t numEntries;
> -	uint32_t byteSize;
> -	std::tie(numEntries, byteSize) = calculateStaticMetadataSize();
> -	staticMetadata_ = std::make_unique<CameraMetadata>(numEntries, byteSize);
> +	staticMetadata_ = std::make_unique<CameraMetadata>(32, 512);

Presumably the initial count of entries is cheap to allocate, but the
storage size is more expensive. .. is there a distinction on the two?

I presume I'll see later ...


>  	if (!staticMetadata_->isValid()) {
>  		LOG(HAL, Error) << "Failed to allocate static metadata";
>  		staticMetadata_.reset();
> @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()
>  				  &entry);
>  
>  	uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;
> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>  
>  	/*
>  	 * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata
>  	 * has been assembled as {{min, max} {max, max}}.
>  	 */
> -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> +	previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
>  				     entry.data.i32 + 2, 2);
>  
>  	return previewTemplate;
> @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateStill()
>  	 * HIGH_QUALITY.
>  	 */
>  	uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;
> -	previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,
> +	previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,
>  				     &noiseReduction, 1);
>  
>  	uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;
> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>  
>  	uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;
> -	previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> +	previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
>  
>  	uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;
> -	previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> +	previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
>  
>  	return previewTemplate;
>  }
> @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateManual()
>  		return nullptr;
>  
>  	uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;
> -	previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> +	previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
>  
>  	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> +	previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
>  
>  	uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;
> -	previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> +	previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
>  
>  	uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;
> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>  
>  	/* \todo get this from available filter densities */
>  	float filterDensity = 0.0f;
> @@ -1867,7 +1812,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
>  		return nullptr;
>  	}
>  
> -	requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> +	requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
>  				     &captureIntent, 1);
>  
>  	requestTemplates_[type] = std::move(requestTemplate);
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 8edbcdfd..88aab012 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -98,7 +98,6 @@ private:
>  	std::vector<libcamera::Size>
>  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
>  
> -	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> index 6f1bcdbe..e0b314ee 100644
> --- a/src/android/camera_metadata.cpp
> +++ b/src/android/camera_metadata.cpp
> @@ -63,49 +63,32 @@ bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) c
>  	return true;
>  }
>  
> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
> +bool CameraMetadata::resize(size_t count, size_t size)
>  {
>  	if (!valid_)
>  		return false;
>  
> -	if (!add_camera_metadata_entry(metadata_, tag, data, count))
> -		return true;
> -
> -	const char *name = get_camera_metadata_tag_name(tag);
> -	if (name)
> -		LOG(CameraMetadata, Error)
> -			<< "Failed to add tag " << name;
> -	else
> -		LOG(CameraMetadata, Error)
> -			<< "Failed to add unknown tag " << tag;
> -
> -	valid_ = false;
> -
> -	return false;
> -}
> -
> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
> -{
> -	if (!valid_)
> -		return false;
> -
> -	camera_metadata_entry_t entry;
> -	int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> -	if (ret) {
> -		const char *name = get_camera_metadata_tag_name(tag);
> -		LOG(CameraMetadata, Error)
> -			<< "Failed to update tag "
> -			<< (name ? name : "<unknown>") << ": not present";
> -		return false;
> -	}
> -
> -	ret = update_camera_metadata_entry(metadata_, entry.index, data,
> -					   count, nullptr);
> -	if (ret) {
> -		const char *name = get_camera_metadata_tag_name(tag);
> -		LOG(CameraMetadata, Error)
> -			<< "Failed to update tag " << (name ? name : "<unknown>");
> -		return false;
> +	size_t currentEntryCount = get_camera_metadata_entry_count(metadata_);
> +	size_t currentEntryCapacity = get_camera_metadata_entry_capacity(metadata_);
> +	size_t newEntryCapacity = currentEntryCapacity < currentEntryCount + count ?
> +				  currentEntryCapacity * 2 : currentEntryCapacity;
> +
> +	size_t currentDataCount = get_camera_metadata_data_count(metadata_);
> +	size_t currentDataCapacity = get_camera_metadata_data_capacity(metadata_);
> +	size_t newDataCapacity = currentDataCapacity < currentDataCount + size ?
> +				 currentDataCapacity * 2 : currentDataCapacity;
> +
> +	if (newEntryCapacity > currentEntryCapacity ||
> +	    newDataCapacity > currentDataCapacity) {
> +		camera_metadata_t *oldMetadata = metadata_;
> +		metadata_ = allocate_camera_metadata(newEntryCapacity, newDataCapacity);
> +		if (!metadata_) {
> +			metadata_ = oldMetadata;
> +			return false;
> +		}
> +
> +		append_camera_metadata(metadata_, oldMetadata);
> +		free_camera_metadata(oldMetadata);

Ok - all that looked simpler than I expected.
Maybe there are optimisations on how or when to resize, but I think this
sounds quite reasonable for now?

Given that we can already pre-reserve if we believe we know the initial
sizes...


>  	}
>  
>  	return true;
> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> index d653e2f0..c35ea1b7 100644
> --- a/src/android/camera_metadata.h
> +++ b/src/android/camera_metadata.h
> @@ -7,6 +7,7 @@
>  #ifndef __ANDROID_CAMERA_METADATA_H__
>  #define __ANDROID_CAMERA_METADATA_H__
>  
> +#include <iostream>
>  #include <stdint.h>
>  
>  #include <system/camera_metadata.h>
> @@ -23,9 +24,43 @@ public:
>  	CameraMetadata &operator=(const CameraMetadata &other);
>  
>  	bool isValid() const { return valid_; }
> +	bool resize(size_t count, size_t size);
>  	bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
> -	bool addEntry(uint32_t tag, const void *data, size_t data_count);
> -	bool updateEntry(uint32_t tag, const void *data, size_t data_count);
> +
> +	template<typename T>
> +	bool addEntry(uint32_t tag, const T *data, size_t count)
> +	{
> +		if (!valid_)
> +			return false;
> +
> +		camera_metadata_entry_t entry;
> +		int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> +		if (ret) {
> +			if (!resize(1, count * sizeof(T))) {

Always calling resize feels a bit odd. Especially as resize can return
true without actually resizing...

> +				std::cerr << "Failed to resize";

if you're using std::cerr, you need std::endl on the end :-)
But I get that this will change.

In the code you've removed, there was:
	LOG(CameraMetadata, Error)
			<< "Failed to add tag " << name;

Can't you use that existing LOG infrastructure?


> +				return false;
> +			}
> +
> +			if (!add_camera_metadata_entry(metadata_, tag, data, count))
> +				return true;
> +
> +			const char *name = get_camera_metadata_tag_name(tag);
> +			std::cerr << "Failed to add tag " << (name ? name : "<unknown>");
> +
> +			valid_ = false;
> +
> +			return false;
> +		}
> +
> +		if (!update_camera_metadata_entry(metadata_, entry.index, data,
> +						  count, nullptr))
> +			return true;
> +
> +		const char *name = get_camera_metadata_tag_name(tag);
> +		std::cerr << "Failed to update tag " << (name ? name : "<unknown>");
> +
> +		return false;
> +	}
>  
>  	camera_metadata_t *get();
>  	const camera_metadata_t *get() const;
>

Hi Paul,

On 06/05/2021 12:05, Kieran Bingham wrote:
> Hi Paul,
> 
> On 06/05/2021 11:41, Paul Elder wrote:
>> This patch depends on the series "FULL hardware level fixes".
>>
>> Previously we had to manually declare the size of CameraMetadata on
>> allocation, and its count not be changed after construction. Change
> 
> count could not
> 
>> CameraMetadata's behavior so that the user can simply add entries, and
>> the CameraMetadata will auto-resize (double the size) as necessary. At
>> the same time, make addEntry() also serve the purpose of updateEntry(),
>> and remove updateEntry(). Also remove everything involved with
>> calculating the initial size for any CameraMetadata instances.
> 
> Oh that sweet music to my ears ...
> 
> 
>>
>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>>
>> ---
>> 1 - What do you (plural) think about merging updateEntry() into
>>     addEntry()? I thought that with the dynamic allocation it would be
>>     convenient to use one function. Is there any reason that we should
>>     keep them separate, or is it fine to merge them?
> 
> What's the current distinction?
> Presumably addEntry requires that the entry doesn't yet exist, whereas
> updateEntry would modify an existing entry?
> 
> For naming, if we were merging both, wouldn't it be better to call it
> '->set(id, value)'?
> 
> 
>> 2 - How can I get logging working in the CameraMetadata header file? The
>>     last time I did that was in ipa_data_serializer.h and that wasn't very
>>     pretty either...


Looked like there was already a LOG usage here, so you shoudl be able to
use that I think.

One last set of thoughts.

It might be interesting to keep a flag 'resized' so you know if a resize
did occur, and a mechanism to print out what the (final) storages were,
so that even if it were a manual process, someone could hardcode in a
better / optimal reservation at the start which would prevent resizes
occurring in the first place.

--
Kieran



>>
>> (I haven't tested it on CTS yet; this is just RFC for the API and
>> implementation)
>> ---
>>  src/android/camera_device.cpp   | 79 +++++----------------------------
>>  src/android/camera_device.h     |  1 -
>>  src/android/camera_metadata.cpp | 61 +++++++++----------------
>>  src/android/camera_metadata.h   | 39 +++++++++++++++-
>>  4 files changed, 71 insertions(+), 109 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 114348a6..426e3fcd 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -772,53 +772,6 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
>>  	callbacks_ = callbacks;
>>  }
>>  
>> -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
>> -{
>> -	/*
>> -	 * \todo Keep this in sync with the actual number of entries.
>> -	 * Currently: 63 entries, 1014 bytes of static metadata
>> -	 */
>> -	uint32_t numEntries = 63;
>> -	uint32_t byteSize = 1014;
>> -
>> -	// do i need to add for entries in the available keys?
>> -	// +1, +4 for EDGE_AVAILABLE_EDGE_MODES
>> -	// +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES
>> -	// +1, +4 for BLACK_LEVEL_PATTERN
>> -	// +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES
>> -	// +1, +4 for TONEMAP_MAX_CURVE_POINTS
>> -	// +4x9 = 36 for the new result tags
>> -
>> -	// +36 for new request keys
>> -
>> -	/*
>> -	 * Calculate space occupation in bytes for dynamically built metadata
>> -	 * entries.
>> -	 *
>> -	 * Each stream configuration entry requires 48 bytes:
>> -	 * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
>> -	 * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
>> -	 */
>> -	byteSize += streamConfigurations_.size() * 48;
>> -
>> -	/*
>> -	 * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes
>> -	 * 2 32bits integers for the (0, 0) thumbnail size
>> -	 *
>> -	 * This is a worst case estimates as different configurations with the
>> -	 * same aspect ratio will generate the same size.
>> -	 */
>> -	for (const auto &entry : streamConfigurations_) {
>> -		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
>> -			continue;
>> -
>> -		byteSize += 8;
>> -	}
>> -	byteSize += 8;
>> -
>> -	return std::make_tuple(numEntries, byteSize);
>> -}
>> -
>>  /*
>>   * Return static information for the camera.
>>   */
>> @@ -827,15 +780,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>>  	if (staticMetadata_)
>>  		return staticMetadata_->get();
>>  
>> -	/*
>> -	 * The here reported metadata are enough to implement a basic capture
>> -	 * example application, but a real camera implementation will require
>> -	 * more.
>> -	 */
>> -	uint32_t numEntries;
>> -	uint32_t byteSize;
>> -	std::tie(numEntries, byteSize) = calculateStaticMetadataSize();
>> -	staticMetadata_ = std::make_unique<CameraMetadata>(numEntries, byteSize);
>> +	staticMetadata_ = std::make_unique<CameraMetadata>(32, 512);
> 
> Presumably the initial count of entries is cheap to allocate, but the
> storage size is more expensive. .. is there a distinction on the two?
> 
> I presume I'll see later ...
> 
> 
>>  	if (!staticMetadata_->isValid()) {
>>  		LOG(HAL, Error) << "Failed to allocate static metadata";
>>  		staticMetadata_.reset();
>> @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()
>>  				  &entry);
>>  
>>  	uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;
>> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>>  
>>  	/*
>>  	 * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata
>>  	 * has been assembled as {{min, max} {max, max}}.
>>  	 */
>> -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
>> +	previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
>>  				     entry.data.i32 + 2, 2);
>>  
>>  	return previewTemplate;
>> @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateStill()
>>  	 * HIGH_QUALITY.
>>  	 */
>>  	uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;
>> -	previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,
>> +	previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,
>>  				     &noiseReduction, 1);
>>  
>>  	uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;
>> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>>  
>>  	uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;
>> -	previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
>> +	previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
>>  
>>  	uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;
>> -	previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
>> +	previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
>>  
>>  	return previewTemplate;
>>  }
>> @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateManual()
>>  		return nullptr;
>>  
>>  	uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;
>> -	previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
>> +	previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
>>  
>>  	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
>> -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
>> +	previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
>>  
>>  	uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;
>> -	previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
>> +	previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
>>  
>>  	uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;
>> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>>  
>>  	/* \todo get this from available filter densities */
>>  	float filterDensity = 0.0f;
>> @@ -1867,7 +1812,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
>>  		return nullptr;
>>  	}
>>  
>> -	requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,
>> +	requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
>>  				     &captureIntent, 1);
>>  
>>  	requestTemplates_[type] = std::move(requestTemplate);
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 8edbcdfd..88aab012 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -98,7 +98,6 @@ private:
>>  	std::vector<libcamera::Size>
>>  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
>>  
>> -	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
>>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
>>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
>> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
>> index 6f1bcdbe..e0b314ee 100644
>> --- a/src/android/camera_metadata.cpp
>> +++ b/src/android/camera_metadata.cpp
>> @@ -63,49 +63,32 @@ bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) c
>>  	return true;
>>  }
>>  
>> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
>> +bool CameraMetadata::resize(size_t count, size_t size)
>>  {
>>  	if (!valid_)
>>  		return false;
>>  
>> -	if (!add_camera_metadata_entry(metadata_, tag, data, count))
>> -		return true;
>> -
>> -	const char *name = get_camera_metadata_tag_name(tag);
>> -	if (name)
>> -		LOG(CameraMetadata, Error)
>> -			<< "Failed to add tag " << name;
>> -	else
>> -		LOG(CameraMetadata, Error)
>> -			<< "Failed to add unknown tag " << tag;
>> -
>> -	valid_ = false;
>> -
>> -	return false;
>> -}
>> -
>> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
>> -{
>> -	if (!valid_)
>> -		return false;
>> -
>> -	camera_metadata_entry_t entry;
>> -	int ret = find_camera_metadata_entry(metadata_, tag, &entry);
>> -	if (ret) {
>> -		const char *name = get_camera_metadata_tag_name(tag);
>> -		LOG(CameraMetadata, Error)
>> -			<< "Failed to update tag "
>> -			<< (name ? name : "<unknown>") << ": not present";
>> -		return false;
>> -	}
>> -
>> -	ret = update_camera_metadata_entry(metadata_, entry.index, data,
>> -					   count, nullptr);
>> -	if (ret) {
>> -		const char *name = get_camera_metadata_tag_name(tag);
>> -		LOG(CameraMetadata, Error)
>> -			<< "Failed to update tag " << (name ? name : "<unknown>");
>> -		return false;
>> +	size_t currentEntryCount = get_camera_metadata_entry_count(metadata_);
>> +	size_t currentEntryCapacity = get_camera_metadata_entry_capacity(metadata_);
>> +	size_t newEntryCapacity = currentEntryCapacity < currentEntryCount + count ?
>> +				  currentEntryCapacity * 2 : currentEntryCapacity;
>> +
>> +	size_t currentDataCount = get_camera_metadata_data_count(metadata_);
>> +	size_t currentDataCapacity = get_camera_metadata_data_capacity(metadata_);
>> +	size_t newDataCapacity = currentDataCapacity < currentDataCount + size ?
>> +				 currentDataCapacity * 2 : currentDataCapacity;
>> +
>> +	if (newEntryCapacity > currentEntryCapacity ||
>> +	    newDataCapacity > currentDataCapacity) {
>> +		camera_metadata_t *oldMetadata = metadata_;
>> +		metadata_ = allocate_camera_metadata(newEntryCapacity, newDataCapacity);
>> +		if (!metadata_) {
>> +			metadata_ = oldMetadata;
>> +			return false;
>> +		}
>> +
>> +		append_camera_metadata(metadata_, oldMetadata);
>> +		free_camera_metadata(oldMetadata);
> 
> Ok - all that looked simpler than I expected.
> Maybe there are optimisations on how or when to resize, but I think this
> sounds quite reasonable for now?
> 
> Given that we can already pre-reserve if we believe we know the initial
> sizes...
> 
> 
>>  	}
>>  
>>  	return true;
>> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
>> index d653e2f0..c35ea1b7 100644
>> --- a/src/android/camera_metadata.h
>> +++ b/src/android/camera_metadata.h
>> @@ -7,6 +7,7 @@
>>  #ifndef __ANDROID_CAMERA_METADATA_H__
>>  #define __ANDROID_CAMERA_METADATA_H__
>>  
>> +#include <iostream>
>>  #include <stdint.h>
>>  
>>  #include <system/camera_metadata.h>
>> @@ -23,9 +24,43 @@ public:
>>  	CameraMetadata &operator=(const CameraMetadata &other);
>>  
>>  	bool isValid() const { return valid_; }
>> +	bool resize(size_t count, size_t size);
>>  	bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
>> -	bool addEntry(uint32_t tag, const void *data, size_t data_count);
>> -	bool updateEntry(uint32_t tag, const void *data, size_t data_count);
>> +
>> +	template<typename T>
>> +	bool addEntry(uint32_t tag, const T *data, size_t count)
>> +	{
>> +		if (!valid_)
>> +			return false;
>> +
>> +		camera_metadata_entry_t entry;
>> +		int ret = find_camera_metadata_entry(metadata_, tag, &entry);
>> +		if (ret) {
>> +			if (!resize(1, count * sizeof(T))) {
> 
> Always calling resize feels a bit odd. Especially as resize can return
> true without actually resizing...
> 
>> +				std::cerr << "Failed to resize";
> 
> if you're using std::cerr, you need std::endl on the end :-)
> But I get that this will change.
> 
> In the code you've removed, there was:
> 	LOG(CameraMetadata, Error)
> 			<< "Failed to add tag " << name;
> 
> Can't you use that existing LOG infrastructure?
> 
> 
>> +				return false;
>> +			}
>> +
>> +			if (!add_camera_metadata_entry(metadata_, tag, data, count))
>> +				return true;
>> +
>> +			const char *name = get_camera_metadata_tag_name(tag);
>> +			std::cerr << "Failed to add tag " << (name ? name : "<unknown>");
>> +
>> +			valid_ = false;
>> +
>> +			return false;
>> +		}
>> +
>> +		if (!update_camera_metadata_entry(metadata_, entry.index, data,
>> +						  count, nullptr))
>> +			return true;
>> +
>> +		const char *name = get_camera_metadata_tag_name(tag);
>> +		std::cerr << "Failed to update tag " << (name ? name : "<unknown>");
>> +
>> +		return false;
>> +	}
>>  
>>  	camera_metadata_t *get();
>>  	const camera_metadata_t *get() const;
>>
>

Hello,

On Thu, May 06, 2021 at 12:14:27PM +0100, Kieran Bingham wrote:
> On 06/05/2021 12:05, Kieran Bingham wrote:
> > On 06/05/2021 11:41, Paul Elder wrote:
> >> This patch depends on the series "FULL hardware level fixes".
> >>
> >> Previously we had to manually declare the size of CameraMetadata on
> >> allocation, and its count not be changed after construction. Change
> > 
> > count could not
> > 
> >> CameraMetadata's behavior so that the user can simply add entries, and
> >> the CameraMetadata will auto-resize (double the size) as necessary. At
> >> the same time, make addEntry() also serve the purpose of updateEntry(),
> >> and remove updateEntry(). Also remove everything involved with
> >> calculating the initial size for any CameraMetadata instances.
> > 
> > Oh that sweet music to my ears ...
> > 
> >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >>
> >> ---
> >> 1 - What do you (plural) think about merging updateEntry() into
> >>     addEntry()? I thought that with the dynamic allocation it would be
> >>     convenient to use one function. Is there any reason that we should
> >>     keep them separate, or is it fine to merge them?
> > 
> > What's the current distinction?
> > Presumably addEntry requires that the entry doesn't yet exist, whereas
> > updateEntry would modify an existing entry?

There may be a performance difference on the backend if we keep them
separate, so please have a look at that. If we merge them, I'd call the
resulting function setEntry(). We could even experiment with some sort
of operator[] syntax.

> > For naming, if we were merging both, wouldn't it be better to call it
> > '->set(id, value)'?
> > 
> >> 2 - How can I get logging working in the CameraMetadata header file? The
> >>     last time I did that was in ipa_data_serializer.h and that wasn't very
> >>     pretty either...
> 
> Looked like there was already a LOG usage here, so you shoudl be able to
> use that I think.
> 
> One last set of thoughts.
> 
> It might be interesting to keep a flag 'resized' so you know if a resize
> did occur, and a mechanism to print out what the (final) storages were,
> so that even if it were a manual process, someone could hardcode in a
> better / optimal reservation at the start which would prevent resizes
> occurring in the first place.

Make sense, instrumentation is important. We don't have to implement it
all yet, but designing the API accordingly is important.

> >> (I haven't tested it on CTS yet; this is just RFC for the API and
> >> implementation)
> >> ---
> >>  src/android/camera_device.cpp   | 79 +++++----------------------------
> >>  src/android/camera_device.h     |  1 -
> >>  src/android/camera_metadata.cpp | 61 +++++++++----------------
> >>  src/android/camera_metadata.h   | 39 +++++++++++++++-
> >>  4 files changed, 71 insertions(+), 109 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index 114348a6..426e3fcd 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -772,53 +772,6 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
> >>  	callbacks_ = callbacks;
> >>  }
> >>  
> >> -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
> >> -{
> >> -	/*
> >> -	 * \todo Keep this in sync with the actual number of entries.
> >> -	 * Currently: 63 entries, 1014 bytes of static metadata
> >> -	 */
> >> -	uint32_t numEntries = 63;
> >> -	uint32_t byteSize = 1014;
> >> -
> >> -	// do i need to add for entries in the available keys?
> >> -	// +1, +4 for EDGE_AVAILABLE_EDGE_MODES
> >> -	// +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES
> >> -	// +1, +4 for BLACK_LEVEL_PATTERN
> >> -	// +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES
> >> -	// +1, +4 for TONEMAP_MAX_CURVE_POINTS
> >> -	// +4x9 = 36 for the new result tags
> >> -
> >> -	// +36 for new request keys
> >> -
> >> -	/*
> >> -	 * Calculate space occupation in bytes for dynamically built metadata
> >> -	 * entries.
> >> -	 *
> >> -	 * Each stream configuration entry requires 48 bytes:
> >> -	 * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
> >> -	 * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
> >> -	 */
> >> -	byteSize += streamConfigurations_.size() * 48;
> >> -
> >> -	/*
> >> -	 * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes
> >> -	 * 2 32bits integers for the (0, 0) thumbnail size
> >> -	 *
> >> -	 * This is a worst case estimates as different configurations with the
> >> -	 * same aspect ratio will generate the same size.
> >> -	 */
> >> -	for (const auto &entry : streamConfigurations_) {
> >> -		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> >> -			continue;
> >> -
> >> -		byteSize += 8;
> >> -	}
> >> -	byteSize += 8;
> >> -
> >> -	return std::make_tuple(numEntries, byteSize);
> >> -}
> >> -
> >>  /*
> >>   * Return static information for the camera.
> >>   */
> >> @@ -827,15 +780,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >>  	if (staticMetadata_)
> >>  		return staticMetadata_->get();
> >>  
> >> -	/*
> >> -	 * The here reported metadata are enough to implement a basic capture
> >> -	 * example application, but a real camera implementation will require
> >> -	 * more.
> >> -	 */
> >> -	uint32_t numEntries;
> >> -	uint32_t byteSize;
> >> -	std::tie(numEntries, byteSize) = calculateStaticMetadataSize();
> >> -	staticMetadata_ = std::make_unique<CameraMetadata>(numEntries, byteSize);
> >> +	staticMetadata_ = std::make_unique<CameraMetadata>(32, 512);
> > 
> > Presumably the initial count of entries is cheap to allocate, but the
> > storage size is more expensive. .. is there a distinction on the two?

Compared to memory requirements related to image storage, neither should
be very expensive.

> > I presume I'll see later ...
> > 
> >>  	if (!staticMetadata_->isValid()) {
> >>  		LOG(HAL, Error) << "Failed to allocate static metadata";
> >>  		staticMetadata_.reset();
> >> @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()
> >>  				  &entry);
> >>  
> >>  	uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;
> >> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >>  
> >>  	/*
> >>  	 * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata
> >>  	 * has been assembled as {{min, max} {max, max}}.
> >>  	 */
> >> -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> >> +	previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> >>  				     entry.data.i32 + 2, 2);
> >>  
> >>  	return previewTemplate;
> >> @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateStill()
> >>  	 * HIGH_QUALITY.
> >>  	 */
> >>  	uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;
> >> -	previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,
> >> +	previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> >>  				     &noiseReduction, 1);
> >>  
> >>  	uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;
> >> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >>  
> >>  	uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;
> >> -	previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> >> +	previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> >>  
> >>  	uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;
> >> -	previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> >> +	previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> >>  
> >>  	return previewTemplate;
> >>  }
> >> @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateManual()
> >>  		return nullptr;
> >>  
> >>  	uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;
> >> -	previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> >> +	previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> >>  
> >>  	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> >> -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> >> +	previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> >>  
> >>  	uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;
> >> -	previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> >> +	previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> >>  
> >>  	uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;
> >> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >>  
> >>  	/* \todo get this from available filter densities */
> >>  	float filterDensity = 0.0f;
> >> @@ -1867,7 +1812,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> >>  		return nullptr;
> >>  	}
> >>  
> >> -	requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> >> +	requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> >>  				     &captureIntent, 1);
> >>  
> >>  	requestTemplates_[type] = std::move(requestTemplate);
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index 8edbcdfd..88aab012 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -98,7 +98,6 @@ private:
> >>  	std::vector<libcamera::Size>
> >>  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
> >>  
> >> -	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> >>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> >>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> >> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> >> index 6f1bcdbe..e0b314ee 100644
> >> --- a/src/android/camera_metadata.cpp
> >> +++ b/src/android/camera_metadata.cpp
> >> @@ -63,49 +63,32 @@ bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) c
> >>  	return true;
> >>  }
> >>  
> >> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
> >> +bool CameraMetadata::resize(size_t count, size_t size)
> >>  {
> >>  	if (!valid_)
> >>  		return false;
> >>  
> >> -	if (!add_camera_metadata_entry(metadata_, tag, data, count))
> >> -		return true;
> >> -
> >> -	const char *name = get_camera_metadata_tag_name(tag);
> >> -	if (name)
> >> -		LOG(CameraMetadata, Error)
> >> -			<< "Failed to add tag " << name;
> >> -	else
> >> -		LOG(CameraMetadata, Error)
> >> -			<< "Failed to add unknown tag " << tag;
> >> -
> >> -	valid_ = false;
> >> -
> >> -	return false;
> >> -}
> >> -
> >> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
> >> -{
> >> -	if (!valid_)
> >> -		return false;
> >> -
> >> -	camera_metadata_entry_t entry;
> >> -	int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> >> -	if (ret) {
> >> -		const char *name = get_camera_metadata_tag_name(tag);
> >> -		LOG(CameraMetadata, Error)
> >> -			<< "Failed to update tag "
> >> -			<< (name ? name : "<unknown>") << ": not present";
> >> -		return false;
> >> -	}
> >> -
> >> -	ret = update_camera_metadata_entry(metadata_, entry.index, data,
> >> -					   count, nullptr);
> >> -	if (ret) {
> >> -		const char *name = get_camera_metadata_tag_name(tag);
> >> -		LOG(CameraMetadata, Error)
> >> -			<< "Failed to update tag " << (name ? name : "<unknown>");
> >> -		return false;
> >> +	size_t currentEntryCount = get_camera_metadata_entry_count(metadata_);
> >> +	size_t currentEntryCapacity = get_camera_metadata_entry_capacity(metadata_);
> >> +	size_t newEntryCapacity = currentEntryCapacity < currentEntryCount + count ?
> >> +				  currentEntryCapacity * 2 : currentEntryCapacity;
> >> +
> >> +	size_t currentDataCount = get_camera_metadata_data_count(metadata_);
> >> +	size_t currentDataCapacity = get_camera_metadata_data_capacity(metadata_);
> >> +	size_t newDataCapacity = currentDataCapacity < currentDataCount + size ?
> >> +				 currentDataCapacity * 2 : currentDataCapacity;
> >> +
> >> +	if (newEntryCapacity > currentEntryCapacity ||
> >> +	    newDataCapacity > currentDataCapacity) {
> >> +		camera_metadata_t *oldMetadata = metadata_;
> >> +		metadata_ = allocate_camera_metadata(newEntryCapacity, newDataCapacity);
> >> +		if (!metadata_) {
> >> +			metadata_ = oldMetadata;
> >> +			return false;
> >> +		}
> >> +
> >> +		append_camera_metadata(metadata_, oldMetadata);
> >> +		free_camera_metadata(oldMetadata);
> > 
> > Ok - all that looked simpler than I expected.
> > Maybe there are optimisations on how or when to resize, but I think this
> > sounds quite reasonable for now?
> > 
> > Given that we can already pre-reserve if we believe we know the initial
> > sizes...
> > 
> >>  	}
> >>  
> >>  	return true;
> >> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> >> index d653e2f0..c35ea1b7 100644
> >> --- a/src/android/camera_metadata.h
> >> +++ b/src/android/camera_metadata.h
> >> @@ -7,6 +7,7 @@
> >>  #ifndef __ANDROID_CAMERA_METADATA_H__
> >>  #define __ANDROID_CAMERA_METADATA_H__
> >>  
> >> +#include <iostream>
> >>  #include <stdint.h>
> >>  
> >>  #include <system/camera_metadata.h>
> >> @@ -23,9 +24,43 @@ public:
> >>  	CameraMetadata &operator=(const CameraMetadata &other);
> >>  
> >>  	bool isValid() const { return valid_; }
> >> +	bool resize(size_t count, size_t size);
> >>  	bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
> >> -	bool addEntry(uint32_t tag, const void *data, size_t data_count);
> >> -	bool updateEntry(uint32_t tag, const void *data, size_t data_count);
> >> +
> >> +	template<typename T>
> >> +	bool addEntry(uint32_t tag, const T *data, size_t count)

Think of a template as a function that gets duplicated for every type T
that is used. This will cause lots of duplication. You should gather the
type-depend code in the beginning of the function, and then move
everything else to a private function that isn't type-dependent.

I would also consider using a Span<const T> instead of passing data and
count separately, as well as adding a wrapper for the common case of
single-element entries

	template<typename T>
	bool addEntry(uint32_t tag, const T &data)
	{
		return addEntry(Span<const T>{&data, 1});
	}

You may also change the return type to void, if nobody checks the return
value.

> >> +	{
> >> +		if (!valid_)
> >> +			return false;
> >> +
> >> +		camera_metadata_entry_t entry;
> >> +		int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> >> +		if (ret) {
> >> +			if (!resize(1, count * sizeof(T))) {
> > 
> > Always calling resize feels a bit odd. Especially as resize can return
> > true without actually resizing...
> > 
> >> +				std::cerr << "Failed to resize";
> > 
> > if you're using std::cerr, you need std::endl on the end :-)
> > But I get that this will change.
> > 
> > In the code you've removed, there was:
> > 	LOG(CameraMetadata, Error)
> > 			<< "Failed to add tag " << name;
> > 
> > Can't you use that existing LOG infrastructure?
> > 
> >> +				return false;
> >> +			}
> >> +
> >> +			if (!add_camera_metadata_entry(metadata_, tag, data, count))
> >> +				return true;
> >> +
> >> +			const char *name = get_camera_metadata_tag_name(tag);
> >> +			std::cerr << "Failed to add tag " << (name ? name : "<unknown>");
> >> +
> >> +			valid_ = false;
> >> +
> >> +			return false;
> >> +		}
> >> +
> >> +		if (!update_camera_metadata_entry(metadata_, entry.index, data,
> >> +						  count, nullptr))
> >> +			return true;
> >> +
> >> +		const char *name = get_camera_metadata_tag_name(tag);
> >> +		std::cerr << "Failed to update tag " << (name ? name : "<unknown>");
> >> +
> >> +		return false;
> >> +	}
> >>  
> >>  	camera_metadata_t *get();
> >>  	const camera_metadata_t *get() const;

Hi Paul,

On Thu, May 6, 2021 at 9:20 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hello,
>
> On Thu, May 06, 2021 at 12:14:27PM +0100, Kieran Bingham wrote:
> > On 06/05/2021 12:05, Kieran Bingham wrote:
> > > On 06/05/2021 11:41, Paul Elder wrote:
> > >> This patch depends on the series "FULL hardware level fixes".
> > >>
> > >> Previously we had to manually declare the size of CameraMetadata on
> > >> allocation, and its count not be changed after construction. Change
> > >
> > > count could not
> > >
> > >> CameraMetadata's behavior so that the user can simply add entries, and
> > >> the CameraMetadata will auto-resize (double the size) as necessary. At
> > >> the same time, make addEntry() also serve the purpose of
> updateEntry(),
> > >> and remove updateEntry(). Also remove everything involved with
> > >> calculating the initial size for any CameraMetadata instances.
> > >
> > > Oh that sweet music to my ears ...
> > >
> > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >>
> > >> ---
> > >> 1 - What do you (plural) think about merging updateEntry() into
> > >>     addEntry()? I thought that with the dynamic allocation it would be
> > >>     convenient to use one function. Is there any reason that we should
> > >>     keep them separate, or is it fine to merge them?
> > >
> > > What's the current distinction?
> > > Presumably addEntry requires that the entry doesn't yet exist, whereas
> > > updateEntry would modify an existing entry?
>
> There may be a performance difference on the backend if we keep them
> separate, so please have a look at that. If we merge them, I'd call the
> resulting function setEntry(). We could even experiment with some sort
> of operator[] syntax.
>
> > > For naming, if we were merging both, wouldn't it be better to call it
> > > '->set(id, value)'?
> > >
> > >> 2 - How can I get logging working in the CameraMetadata header file?
> The
> > >>     last time I did that was in ipa_data_serializer.h and that wasn't
> very
> > >>     pretty either...
> >
> > Looked like there was already a LOG usage here, so you shoudl be able to
> > use that I think.
> >
> > One last set of thoughts.
> >
> > It might be interesting to keep a flag 'resized' so you know if a resize
> > did occur, and a mechanism to print out what the (final) storages were,
> > so that even if it were a manual process, someone could hardcode in a
> > better / optimal reservation at the start which would prevent resizes
> > occurring in the first place.
>
> Make sense, instrumentation is important. We don't have to implement it
> all yet, but designing the API accordingly is important.
>
> > >> (I haven't tested it on CTS yet; this is just RFC for the API and
> > >> implementation)
> > >> ---
> > >>  src/android/camera_device.cpp   | 79
> +++++----------------------------
> > >>  src/android/camera_device.h     |  1 -
> > >>  src/android/camera_metadata.cpp | 61 +++++++++----------------
> > >>  src/android/camera_metadata.h   | 39 +++++++++++++++-
> > >>  4 files changed, 71 insertions(+), 109 deletions(-)
> > >>
> > >> diff --git a/src/android/camera_device.cpp
> b/src/android/camera_device.cpp
> > >> index 114348a6..426e3fcd 100644
> > >> --- a/src/android/camera_device.cpp
> > >> +++ b/src/android/camera_device.cpp
> > >> @@ -772,53 +772,6 @@ void CameraDevice::setCallbacks(const
> camera3_callback_ops_t *callbacks)
> > >>    callbacks_ = callbacks;
> > >>  }
> > >>
> > >> -std::tuple<uint32_t, uint32_t>
> CameraDevice::calculateStaticMetadataSize()
> > >> -{
> > >> -  /*
> > >> -   * \todo Keep this in sync with the actual number of entries.
> > >> -   * Currently: 63 entries, 1014 bytes of static metadata
> > >> -   */
> > >> -  uint32_t numEntries = 63;
> > >> -  uint32_t byteSize = 1014;
> > >> -
> > >> -  // do i need to add for entries in the available keys?
> > >> -  // +1, +4 for EDGE_AVAILABLE_EDGE_MODES
> > >> -  // +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES
> > >> -  // +1, +4 for BLACK_LEVEL_PATTERN
> > >> -  // +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES
> > >> -  // +1, +4 for TONEMAP_MAX_CURVE_POINTS
> > >> -  // +4x9 = 36 for the new result tags
> > >> -
> > >> -  // +36 for new request keys
> > >> -
> > >> -  /*
> > >> -   * Calculate space occupation in bytes for dynamically built
> metadata
> > >> -   * entries.
> > >> -   *
> > >> -   * Each stream configuration entry requires 48 bytes:
> > >> -   * 4 32bits integers for
> ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
> > >> -   * 4 64bits integers for
> ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
> > >> -   */
> > >> -  byteSize += streamConfigurations_.size() * 48;
> > >> -
> > >> -  /*
> > >> -   * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail
> sizes
> > >> -   * 2 32bits integers for the (0, 0) thumbnail size
> > >> -   *
> > >> -   * This is a worst case estimates as different configurations with
> the
> > >> -   * same aspect ratio will generate the same size.
> > >> -   */
> > >> -  for (const auto &entry : streamConfigurations_) {
> > >> -          if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> > >> -                  continue;
> > >> -
> > >> -          byteSize += 8;
> > >> -  }
> > >> -  byteSize += 8;
> > >> -
> > >> -  return std::make_tuple(numEntries, byteSize);
> > >> -}
> > >> -
> > >>  /*
> > >>   * Return static information for the camera.
> > >>   */
> > >> @@ -827,15 +780,7 @@ const camera_metadata_t
> *CameraDevice::getStaticMetadata()
> > >>    if (staticMetadata_)
> > >>            return staticMetadata_->get();
> > >>
> > >> -  /*
> > >> -   * The here reported metadata are enough to implement a basic
> capture
> > >> -   * example application, but a real camera implementation will
> require
> > >> -   * more.
> > >> -   */
> > >> -  uint32_t numEntries;
> > >> -  uint32_t byteSize;
> > >> -  std::tie(numEntries, byteSize) = calculateStaticMetadataSize();
> > >> -  staticMetadata_ = std::make_unique<CameraMetadata>(numEntries,
> byteSize);
> > >> +  staticMetadata_ = std::make_unique<CameraMetadata>(32, 512);
>

Shall we add a comment where 32 and 512 come from.
I guess this is the result of calculateStaticMetadataSize() today.


> > >
> > > Presumably the initial count of entries is cheap to allocate, but the
> > > storage size is more expensive. .. is there a distinction on the two?
>
> Compared to memory requirements related to image storage, neither should
> be very expensive.
>
> > > I presume I'll see later ...
> > >
> > >>    if (!staticMetadata_->isValid()) {
> > >>            LOG(HAL, Error) << "Failed to allocate static metadata";
> > >>            staticMetadata_.reset();
> > >> @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata>
> CameraDevice::requestTemplateVideo()
> > >>                              &entry);
> > >>
> > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;
> > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >>
> > >>    /*
> > >>     * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata
> > >>     * has been assembled as {{min, max} {max, max}}.
> > >>     */
> > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > >>                                 entry.data.i32 + 2, 2);
> > >>
> > >>    return previewTemplate;
> > >> @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata>
> CameraDevice::requestTemplateStill()
> > >>     * HIGH_QUALITY.
> > >>     */
> > >>    uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;
> > >> -  previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,
> > >> +  previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> > >>                                 &noiseReduction, 1);
> > >>
> > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;
> > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >>
> > >>    uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;
> > >> -  previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode,
> 1);
> > >> +  previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> > >>
> > >>    uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;
> > >> -  previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode,
> 1);
> > >> +  previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> > >>
> > >>    return previewTemplate;
> > >>  }
> > >> @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata>
> CameraDevice::requestTemplateManual()
> > >>            return nullptr;
> > >>
> > >>    uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;
> > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode,
> 1);
> > >> +  previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> > >>
> > >>    uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> > >>
> > >>    uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;
> > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode,
> 1);
> > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> > >>
> > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;
> > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >>
> > >>    /* \todo get this from available filter densities */
> > >>    float filterDensity = 0.0f;
> > >> @@ -1867,7 +1812,7 @@ const camera_metadata_t
> *CameraDevice::constructDefaultRequestSettings(int type)
> > >>            return nullptr;
> > >>    }
> > >>
> > >> -  requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > >> +  requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > >>                                 &captureIntent, 1);
> > >>
> > >>    requestTemplates_[type] = std::move(requestTemplate);
> > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > >> index 8edbcdfd..88aab012 100644
> > >> --- a/src/android/camera_device.h
> > >> +++ b/src/android/camera_device.h
> > >> @@ -98,7 +98,6 @@ private:
> > >>    std::vector<libcamera::Size>
> > >>    getRawResolutions(const libcamera::PixelFormat &pixelFormat);
> > >>
> > >> -  std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> > >>    libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t
> camera3buffer);
> > >>    void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > >>    void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > >> diff --git a/src/android/camera_metadata.cpp
> b/src/android/camera_metadata.cpp
> > >> index 6f1bcdbe..e0b314ee 100644
> > >> --- a/src/android/camera_metadata.cpp
> > >> +++ b/src/android/camera_metadata.cpp
> > >> @@ -63,49 +63,32 @@ bool CameraMetadata::getEntry(uint32_t tag,
> camera_metadata_ro_entry_t *entry) c
> > >>    return true;
> > >>  }
> > >>
> > >> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t
> count)
> > >> +bool CameraMetadata::resize(size_t count, size_t size)
> > >>  {
> > >>    if (!valid_)
> > >>            return false;
> > >>
> > >> -  if (!add_camera_metadata_entry(metadata_, tag, data, count))
> > >> -          return true;
> > >> -
> > >> -  const char *name = get_camera_metadata_tag_name(tag);
> > >> -  if (name)
> > >> -          LOG(CameraMetadata, Error)
> > >> -                  << "Failed to add tag " << name;
> > >> -  else
> > >> -          LOG(CameraMetadata, Error)
> > >> -                  << "Failed to add unknown tag " << tag;
> > >> -
> > >> -  valid_ = false;
> > >> -
> > >> -  return false;
> > >> -}
> > >> -
> > >> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data,
> size_t count)
> > >> -{
> > >> -  if (!valid_)
> > >> -          return false;
> > >> -
> > >> -  camera_metadata_entry_t entry;
> > >> -  int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> > >> -  if (ret) {
> > >> -          const char *name = get_camera_metadata_tag_name(tag);
> > >> -          LOG(CameraMetadata, Error)
> > >> -                  << "Failed to update tag "
> > >> -                  << (name ? name : "<unknown>") << ": not present";
> > >> -          return false;
> > >> -  }
> > >> -
> > >> -  ret = update_camera_metadata_entry(metadata_, entry.index, data,
> > >> -                                     count, nullptr);
> > >> -  if (ret) {
> > >> -          const char *name = get_camera_metadata_tag_name(tag);
> > >> -          LOG(CameraMetadata, Error)
> > >> -                  << "Failed to update tag " << (name ? name :
> "<unknown>");
> > >> -          return false;
> > >> +  size_t currentEntryCount =
> get_camera_metadata_entry_count(metadata_);
> > >> +  size_t currentEntryCapacity =
> get_camera_metadata_entry_capacity(metadata_);
> > >> +  size_t newEntryCapacity = currentEntryCapacity < currentEntryCount
> + count ?
> > >> +                            currentEntryCapacity * 2 :
> currentEntryCapacity;
> > >> +
> > >> +  size_t currentDataCount =
> get_camera_metadata_data_count(metadata_);
> > >> +  size_t currentDataCapacity =
> get_camera_metadata_data_capacity(metadata_);
> > >> +  size_t newDataCapacity = currentDataCapacity < currentDataCount +
> size ?
> > >> +                           currentDataCapacity * 2 :
> currentDataCapacity;
> > >> +
> > >> +  if (newEntryCapacity > currentEntryCapacity ||
> > >> +      newDataCapacity > currentDataCapacity) {
> > >> +          camera_metadata_t *oldMetadata = metadata_;
> > >> +          metadata_ = allocate_camera_metadata(newEntryCapacity,
> newDataCapacity);
> > >> +          if (!metadata_) {
> > >> +                  metadata_ = oldMetadata;
> > >> +                  return false;
> > >> +          }
> > >> +
> > >> +          append_camera_metadata(metadata_, oldMetadata);
> > >> +          free_camera_metadata(oldMetadata);
> > >
> > > Ok - all that looked simpler than I expected.
> > > Maybe there are optimisations on how or when to resize, but I think
> this
> > > sounds quite reasonable for now?
> > >
> > > Given that we can already pre-reserve if we believe we know the initial
> > > sizes...
> > >
> > >>    }
> > >>
> > >>    return true;
> > >> diff --git a/src/android/camera_metadata.h
> b/src/android/camera_metadata.h
> > >> index d653e2f0..c35ea1b7 100644
> > >> --- a/src/android/camera_metadata.h
> > >> +++ b/src/android/camera_metadata.h
> > >> @@ -7,6 +7,7 @@
> > >>  #ifndef __ANDROID_CAMERA_METADATA_H__
> > >>  #define __ANDROID_CAMERA_METADATA_H__
> > >>
> > >> +#include <iostream>
> > >>  #include <stdint.h>
> > >>
> > >>  #include <system/camera_metadata.h>
> > >> @@ -23,9 +24,43 @@ public:
> > >>    CameraMetadata &operator=(const CameraMetadata &other);
> > >>
> > >>    bool isValid() const { return valid_; }
> > >> +  bool resize(size_t count, size_t size);
> > >>    bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry)
> const;
> > >> -  bool addEntry(uint32_t tag, const void *data, size_t data_count);
> > >> -  bool updateEntry(uint32_t tag, const void *data, size_t
> data_count);
> > >> +
> > >> +  template<typename T>
> > >> +  bool addEntry(uint32_t tag, const T *data, size_t count)
>
> Think of a template as a function that gets duplicated for every type T
> that is used. This will cause lots of duplication. You should gather the
> type-depend code in the beginning of the function, and then move
> everything else to a private function that isn't type-dependent.
>
> I would also consider using a Span<const T> instead of passing data and
> count separately, as well as adding a wrapper for the common case of
> single-element entries
>
>         template<typename T>
>         bool addEntry(uint32_t tag, const T &data)
>         {
>                 return addEntry(Span<const T>{&data, 1});
>         }
>
> You may also change the return type to void, if nobody checks the return
> value.
>
> > >> +  {
> > >> +          if (!valid_)
> > >> +                  return false;
> > >> +
> > >> +          camera_metadata_entry_t entry;
> > >> +          int ret = find_camera_metadata_entry(metadata_, tag,
> &entry);
> > >> +          if (ret) {
> > >> +                  if (!resize(1, count * sizeof(T))) {
> > >
> > > Always calling resize feels a bit odd. Especially as resize can return
> > > true without actually resizing...
> > >
> > >> +                          std::cerr << "Failed to resize";
> > >
> > > if you're using std::cerr, you need std::endl on the end :-)
> > > But I get that this will change.
> > >
> > > In the code you've removed, there was:
> > >     LOG(CameraMetadata, Error)
> > >                     << "Failed to add tag " << name;
> > >
> > > Can't you use that existing LOG infrastructure?
> > >
> > >> +                          return false;
> > >> +                  }
> > >> +
> > >> +                  if (!add_camera_metadata_entry(metadata_, tag,
> data, count))
> > >> +                          return true;
> > >> +
> > >> +                  const char *name =
> get_camera_metadata_tag_name(tag);
> > >> +                  std::cerr << "Failed to add tag " << (name ? name
> : "<unknown>");
> > >> +
> > >> +                  valid_ = false;
> > >> +
> > >> +                  return false;
> > >> +          }
> > >> +
> > >> +          if (!update_camera_metadata_entry(metadata_, entry.index,
> data,
> > >> +                                            count, nullptr))
> > >> +                  return true;
> > >> +
> > >> +          const char *name = get_camera_metadata_tag_name(tag);
> > >> +          std::cerr << "Failed to update tag " << (name ? name :
> "<unknown>");
> > >> +
> > >> +          return false;
> > >> +  }
> > >>
> > >>    camera_metadata_t *get();
> > >>    const camera_metadata_t *get() const;
>
>
Overall, I am fine with this change while I am a bit concerned about the
performance of allocate_camera_metadata and append_camera_metadata.
I expect it causes O(N), where N is the data size, and in the worst case
adding entries will be O(N*T), where T is the number of added entires.
The data size is not considerably small (about 500 bytes), isn't it?
Perhaps is it possible to cache the added entries in the CameraMetadata
class until get() and form as camera_metadata_t upon get(), so adding
entries will be O(N), where N is the data size.

-Hiro

Hi Kieran,

On Thu, May 06, 2021 at 12:05:54PM +0100, Kieran Bingham wrote:
> Hi Paul,
> 
> On 06/05/2021 11:41, Paul Elder wrote:
> > This patch depends on the series "FULL hardware level fixes".
> > 
> > Previously we had to manually declare the size of CameraMetadata on
> > allocation, and its count not be changed after construction. Change
> 
> count could not
> 
> > CameraMetadata's behavior so that the user can simply add entries, and
> > the CameraMetadata will auto-resize (double the size) as necessary. At
> > the same time, make addEntry() also serve the purpose of updateEntry(),
> > and remove updateEntry(). Also remove everything involved with
> > calculating the initial size for any CameraMetadata instances.
> 
> Oh that sweet music to my ears ...

\o/

> 
> 
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > 1 - What do you (plural) think about merging updateEntry() into
> >     addEntry()? I thought that with the dynamic allocation it would be
> >     convenient to use one function. Is there any reason that we should
> >     keep them separate, or is it fine to merge them?
> 
> What's the current distinction?
> Presumably addEntry requires that the entry doesn't yet exist, whereas
> updateEntry would modify an existing entry?

Yeah, that's the distinction.

> 
> For naming, if we were merging both, wouldn't it be better to call it
> '->set(id, value)'?

Oh maybe. A bit short imo, so setEntry? (we can bikeshed this more
later, I've sent a v2 on other stuff)

> 
> 
> > 2 - How can I get logging working in the CameraMetadata header file? The
> >     last time I did that was in ipa_data_serializer.h and that wasn't very
> >     pretty either...
> > 
> > (I haven't tested it on CTS yet; this is just RFC for the API and
> > implementation)
> > ---
> >  src/android/camera_device.cpp   | 79 +++++----------------------------
> >  src/android/camera_device.h     |  1 -
> >  src/android/camera_metadata.cpp | 61 +++++++++----------------
> >  src/android/camera_metadata.h   | 39 +++++++++++++++-
> >  4 files changed, 71 insertions(+), 109 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 114348a6..426e3fcd 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -772,53 +772,6 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
> >  	callbacks_ = callbacks;
> >  }
> >  
> > -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
> > -{
> > -	/*
> > -	 * \todo Keep this in sync with the actual number of entries.
> > -	 * Currently: 63 entries, 1014 bytes of static metadata
> > -	 */
> > -	uint32_t numEntries = 63;
> > -	uint32_t byteSize = 1014;
> > -
> > -	// do i need to add for entries in the available keys?
> > -	// +1, +4 for EDGE_AVAILABLE_EDGE_MODES
> > -	// +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES
> > -	// +1, +4 for BLACK_LEVEL_PATTERN
> > -	// +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES
> > -	// +1, +4 for TONEMAP_MAX_CURVE_POINTS
> > -	// +4x9 = 36 for the new result tags
> > -
> > -	// +36 for new request keys
> > -
> > -	/*
> > -	 * Calculate space occupation in bytes for dynamically built metadata
> > -	 * entries.
> > -	 *
> > -	 * Each stream configuration entry requires 48 bytes:
> > -	 * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
> > -	 * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
> > -	 */
> > -	byteSize += streamConfigurations_.size() * 48;
> > -
> > -	/*
> > -	 * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes
> > -	 * 2 32bits integers for the (0, 0) thumbnail size
> > -	 *
> > -	 * This is a worst case estimates as different configurations with the
> > -	 * same aspect ratio will generate the same size.
> > -	 */
> > -	for (const auto &entry : streamConfigurations_) {
> > -		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> > -			continue;
> > -
> > -		byteSize += 8;
> > -	}
> > -	byteSize += 8;
> > -
> > -	return std::make_tuple(numEntries, byteSize);
> > -}
> > -
> >  /*
> >   * Return static information for the camera.
> >   */
> > @@ -827,15 +780,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  	if (staticMetadata_)
> >  		return staticMetadata_->get();
> >  
> > -	/*
> > -	 * The here reported metadata are enough to implement a basic capture
> > -	 * example application, but a real camera implementation will require
> > -	 * more.
> > -	 */
> > -	uint32_t numEntries;
> > -	uint32_t byteSize;
> > -	std::tie(numEntries, byteSize) = calculateStaticMetadataSize();
> > -	staticMetadata_ = std::make_unique<CameraMetadata>(numEntries, byteSize);
> > +	staticMetadata_ = std::make_unique<CameraMetadata>(32, 512);
> 
> Presumably the initial count of entries is cheap to allocate, but the
> storage size is more expensive. .. is there a distinction on the two?
> 
> I presume I'll see later ...
> 
> 
> >  	if (!staticMetadata_->isValid()) {
> >  		LOG(HAL, Error) << "Failed to allocate static metadata";
> >  		staticMetadata_.reset();
> > @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()
> >  				  &entry);
> >  
> >  	uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;
> > -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >  
> >  	/*
> >  	 * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata
> >  	 * has been assembled as {{min, max} {max, max}}.
> >  	 */
> > -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > +	previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> >  				     entry.data.i32 + 2, 2);
> >  
> >  	return previewTemplate;
> > @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateStill()
> >  	 * HIGH_QUALITY.
> >  	 */
> >  	uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;
> > -	previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,
> > +	previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> >  				     &noiseReduction, 1);
> >  
> >  	uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;
> > -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >  
> >  	uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;
> > -	previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> > +	previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> >  
> >  	uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;
> > -	previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> > +	previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> >  
> >  	return previewTemplate;
> >  }
> > @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateManual()
> >  		return nullptr;
> >  
> >  	uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;
> > -	previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> > +	previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> >  
> >  	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> > -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> > +	previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> >  
> >  	uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;
> > -	previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> > +	previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> >  
> >  	uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;
> > -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> >  
> >  	/* \todo get this from available filter densities */
> >  	float filterDensity = 0.0f;
> > @@ -1867,7 +1812,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> >  		return nullptr;
> >  	}
> >  
> > -	requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > +	requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> >  				     &captureIntent, 1);
> >  
> >  	requestTemplates_[type] = std::move(requestTemplate);
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 8edbcdfd..88aab012 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -98,7 +98,6 @@ private:
> >  	std::vector<libcamera::Size>
> >  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
> >  
> > -	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> >  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> >  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> > index 6f1bcdbe..e0b314ee 100644
> > --- a/src/android/camera_metadata.cpp
> > +++ b/src/android/camera_metadata.cpp
> > @@ -63,49 +63,32 @@ bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) c
> >  	return true;
> >  }
> >  
> > -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
> > +bool CameraMetadata::resize(size_t count, size_t size)
> >  {
> >  	if (!valid_)
> >  		return false;
> >  
> > -	if (!add_camera_metadata_entry(metadata_, tag, data, count))
> > -		return true;
> > -
> > -	const char *name = get_camera_metadata_tag_name(tag);
> > -	if (name)
> > -		LOG(CameraMetadata, Error)
> > -			<< "Failed to add tag " << name;
> > -	else
> > -		LOG(CameraMetadata, Error)
> > -			<< "Failed to add unknown tag " << tag;
> > -
> > -	valid_ = false;
> > -
> > -	return false;
> > -}
> > -
> > -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
> > -{
> > -	if (!valid_)
> > -		return false;
> > -
> > -	camera_metadata_entry_t entry;
> > -	int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> > -	if (ret) {
> > -		const char *name = get_camera_metadata_tag_name(tag);
> > -		LOG(CameraMetadata, Error)
> > -			<< "Failed to update tag "
> > -			<< (name ? name : "<unknown>") << ": not present";
> > -		return false;
> > -	}
> > -
> > -	ret = update_camera_metadata_entry(metadata_, entry.index, data,
> > -					   count, nullptr);
> > -	if (ret) {
> > -		const char *name = get_camera_metadata_tag_name(tag);
> > -		LOG(CameraMetadata, Error)
> > -			<< "Failed to update tag " << (name ? name : "<unknown>");
> > -		return false;
> > +	size_t currentEntryCount = get_camera_metadata_entry_count(metadata_);
> > +	size_t currentEntryCapacity = get_camera_metadata_entry_capacity(metadata_);
> > +	size_t newEntryCapacity = currentEntryCapacity < currentEntryCount + count ?
> > +				  currentEntryCapacity * 2 : currentEntryCapacity;
> > +
> > +	size_t currentDataCount = get_camera_metadata_data_count(metadata_);
> > +	size_t currentDataCapacity = get_camera_metadata_data_capacity(metadata_);
> > +	size_t newDataCapacity = currentDataCapacity < currentDataCount + size ?
> > +				 currentDataCapacity * 2 : currentDataCapacity;
> > +
> > +	if (newEntryCapacity > currentEntryCapacity ||
> > +	    newDataCapacity > currentDataCapacity) {
> > +		camera_metadata_t *oldMetadata = metadata_;
> > +		metadata_ = allocate_camera_metadata(newEntryCapacity, newDataCapacity);
> > +		if (!metadata_) {
> > +			metadata_ = oldMetadata;
> > +			return false;
> > +		}
> > +
> > +		append_camera_metadata(metadata_, oldMetadata);
> > +		free_camera_metadata(oldMetadata);
> 
> Ok - all that looked simpler than I expected.
> Maybe there are optimisations on how or when to resize, but I think this
> sounds quite reasonable for now?
> 
> Given that we can already pre-reserve if we believe we know the initial
> sizes...
> 
> 
> >  	}
> >  
> >  	return true;
> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> > index d653e2f0..c35ea1b7 100644
> > --- a/src/android/camera_metadata.h
> > +++ b/src/android/camera_metadata.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __ANDROID_CAMERA_METADATA_H__
> >  #define __ANDROID_CAMERA_METADATA_H__
> >  
> > +#include <iostream>
> >  #include <stdint.h>
> >  
> >  #include <system/camera_metadata.h>
> > @@ -23,9 +24,43 @@ public:
> >  	CameraMetadata &operator=(const CameraMetadata &other);
> >  
> >  	bool isValid() const { return valid_; }
> > +	bool resize(size_t count, size_t size);
> >  	bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
> > -	bool addEntry(uint32_t tag, const void *data, size_t data_count);
> > -	bool updateEntry(uint32_t tag, const void *data, size_t data_count);
> > +
> > +	template<typename T>
> > +	bool addEntry(uint32_t tag, const T *data, size_t count)
> > +	{
> > +		if (!valid_)
> > +			return false;
> > +
> > +		camera_metadata_entry_t entry;
> > +		int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> > +		if (ret) {
> > +			if (!resize(1, count * sizeof(T))) {
> 
> Always calling resize feels a bit odd. Especially as resize can return
> true without actually resizing...

Well I meant it more like a "resize if necessary".

> 
> > +				std::cerr << "Failed to resize";
> 
> if you're using std::cerr, you need std::endl on the end :-)
> But I get that this will change.
> 
> In the code you've removed, there was:
> 	LOG(CameraMetadata, Error)
> 			<< "Failed to add tag " << name;
> 
> Can't you use that existing LOG infrastructure?

For v2 I've moved the meaty part over to cpp and now LOGging works.


Thanks,

Paul

> 
> 
> > +				return false;
> > +			}
> > +
> > +			if (!add_camera_metadata_entry(metadata_, tag, data, count))
> > +				return true;
> > +
> > +			const char *name = get_camera_metadata_tag_name(tag);
> > +			std::cerr << "Failed to add tag " << (name ? name : "<unknown>");
> > +
> > +			valid_ = false;
> > +
> > +			return false;
> > +		}
> > +
> > +		if (!update_camera_metadata_entry(metadata_, entry.index, data,
> > +						  count, nullptr))
> > +			return true;
> > +
> > +		const char *name = get_camera_metadata_tag_name(tag);
> > +		std::cerr << "Failed to update tag " << (name ? name : "<unknown>");
> > +
> > +		return false;
> > +	}
> >  
> >  	camera_metadata_t *get();
> >  	const camera_metadata_t *get() const;
> >

Hi Laurent,

On Thu, May 06, 2021 at 03:19:58PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> On Thu, May 06, 2021 at 12:14:27PM +0100, Kieran Bingham wrote:
> > On 06/05/2021 12:05, Kieran Bingham wrote:
> > > On 06/05/2021 11:41, Paul Elder wrote:
> > >> This patch depends on the series "FULL hardware level fixes".
> > >>
> > >> Previously we had to manually declare the size of CameraMetadata on
> > >> allocation, and its count not be changed after construction. Change
> > > 
> > > count could not
> > > 
> > >> CameraMetadata's behavior so that the user can simply add entries, and
> > >> the CameraMetadata will auto-resize (double the size) as necessary. At
> > >> the same time, make addEntry() also serve the purpose of updateEntry(),
> > >> and remove updateEntry(). Also remove everything involved with
> > >> calculating the initial size for any CameraMetadata instances.
> > > 
> > > Oh that sweet music to my ears ...
> > > 
> > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >>
> > >> ---
> > >> 1 - What do you (plural) think about merging updateEntry() into
> > >>     addEntry()? I thought that with the dynamic allocation it would be
> > >>     convenient to use one function. Is there any reason that we should
> > >>     keep them separate, or is it fine to merge them?
> > > 
> > > What's the current distinction?
> > > Presumably addEntry requires that the entry doesn't yet exist, whereas
> > > updateEntry would modify an existing entry?
> 
> There may be a performance difference on the backend if we keep them
> separate, so please have a look at that. If we merge them, I'd call the

Ah right. Looks like it's a binary search. So it's comparing convenience
vs performance... maybe one merged, and then two separated ones...?

> resulting function setEntry(). We could even experiment with some sort
> of operator[] syntax.

Ooh [] would be nice :D

> 
> > > For naming, if we were merging both, wouldn't it be better to call it
> > > '->set(id, value)'?
> > > 
> > >> 2 - How can I get logging working in the CameraMetadata header file? The
> > >>     last time I did that was in ipa_data_serializer.h and that wasn't very
> > >>     pretty either...
> > 
> > Looked like there was already a LOG usage here, so you shoudl be able to
> > use that I think.
> > 
> > One last set of thoughts.
> > 
> > It might be interesting to keep a flag 'resized' so you know if a resize
> > did occur, and a mechanism to print out what the (final) storages were,
> > so that even if it were a manual process, someone could hardcode in a
> > better / optimal reservation at the start which would prevent resizes
> > occurring in the first place.
> 
> Make sense, instrumentation is important. We don't have to implement it
> all yet, but designing the API accordingly is important.
> 
> > >> (I haven't tested it on CTS yet; this is just RFC for the API and
> > >> implementation)
> > >> ---
> > >>  src/android/camera_device.cpp   | 79 +++++----------------------------
> > >>  src/android/camera_device.h     |  1 -
> > >>  src/android/camera_metadata.cpp | 61 +++++++++----------------
> > >>  src/android/camera_metadata.h   | 39 +++++++++++++++-
> > >>  4 files changed, 71 insertions(+), 109 deletions(-)
> > >>
> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > >> index 114348a6..426e3fcd 100644
> > >> --- a/src/android/camera_device.cpp
> > >> +++ b/src/android/camera_device.cpp
> > >> @@ -772,53 +772,6 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
> > >>  	callbacks_ = callbacks;
> > >>  }
> > >>  
> > >> -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
> > >> -{
> > >> -	/*
> > >> -	 * \todo Keep this in sync with the actual number of entries.
> > >> -	 * Currently: 63 entries, 1014 bytes of static metadata
> > >> -	 */
> > >> -	uint32_t numEntries = 63;
> > >> -	uint32_t byteSize = 1014;
> > >> -
> > >> -	// do i need to add for entries in the available keys?
> > >> -	// +1, +4 for EDGE_AVAILABLE_EDGE_MODES
> > >> -	// +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES
> > >> -	// +1, +4 for BLACK_LEVEL_PATTERN
> > >> -	// +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES
> > >> -	// +1, +4 for TONEMAP_MAX_CURVE_POINTS
> > >> -	// +4x9 = 36 for the new result tags
> > >> -
> > >> -	// +36 for new request keys
> > >> -
> > >> -	/*
> > >> -	 * Calculate space occupation in bytes for dynamically built metadata
> > >> -	 * entries.
> > >> -	 *
> > >> -	 * Each stream configuration entry requires 48 bytes:
> > >> -	 * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
> > >> -	 * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
> > >> -	 */
> > >> -	byteSize += streamConfigurations_.size() * 48;
> > >> -
> > >> -	/*
> > >> -	 * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes
> > >> -	 * 2 32bits integers for the (0, 0) thumbnail size
> > >> -	 *
> > >> -	 * This is a worst case estimates as different configurations with the
> > >> -	 * same aspect ratio will generate the same size.
> > >> -	 */
> > >> -	for (const auto &entry : streamConfigurations_) {
> > >> -		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> > >> -			continue;
> > >> -
> > >> -		byteSize += 8;
> > >> -	}
> > >> -	byteSize += 8;
> > >> -
> > >> -	return std::make_tuple(numEntries, byteSize);
> > >> -}
> > >> -
> > >>  /*
> > >>   * Return static information for the camera.
> > >>   */
> > >> @@ -827,15 +780,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > >>  	if (staticMetadata_)
> > >>  		return staticMetadata_->get();
> > >>  
> > >> -	/*
> > >> -	 * The here reported metadata are enough to implement a basic capture
> > >> -	 * example application, but a real camera implementation will require
> > >> -	 * more.
> > >> -	 */
> > >> -	uint32_t numEntries;
> > >> -	uint32_t byteSize;
> > >> -	std::tie(numEntries, byteSize) = calculateStaticMetadataSize();
> > >> -	staticMetadata_ = std::make_unique<CameraMetadata>(numEntries, byteSize);
> > >> +	staticMetadata_ = std::make_unique<CameraMetadata>(32, 512);
> > > 
> > > Presumably the initial count of entries is cheap to allocate, but the
> > > storage size is more expensive. .. is there a distinction on the two?
> 
> Compared to memory requirements related to image storage, neither should
> be very expensive.
> 
> > > I presume I'll see later ...
> > > 
> > >>  	if (!staticMetadata_->isValid()) {
> > >>  		LOG(HAL, Error) << "Failed to allocate static metadata";
> > >>  		staticMetadata_.reset();
> > >> @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()
> > >>  				  &entry);
> > >>  
> > >>  	uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;
> > >> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >>  
> > >>  	/*
> > >>  	 * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata
> > >>  	 * has been assembled as {{min, max} {max, max}}.
> > >>  	 */
> > >> -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > >> +	previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > >>  				     entry.data.i32 + 2, 2);
> > >>  
> > >>  	return previewTemplate;
> > >> @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateStill()
> > >>  	 * HIGH_QUALITY.
> > >>  	 */
> > >>  	uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;
> > >> -	previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,
> > >> +	previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> > >>  				     &noiseReduction, 1);
> > >>  
> > >>  	uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;
> > >> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >>  
> > >>  	uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;
> > >> -	previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> > >> +	previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> > >>  
> > >>  	uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;
> > >> -	previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> > >> +	previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> > >>  
> > >>  	return previewTemplate;
> > >>  }
> > >> @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateManual()
> > >>  		return nullptr;
> > >>  
> > >>  	uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;
> > >> -	previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> > >> +	previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> > >>  
> > >>  	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> > >> -	previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> > >> +	previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> > >>  
> > >>  	uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;
> > >> -	previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> > >> +	previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> > >>  
> > >>  	uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;
> > >> -	previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >> +	previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > >>  
> > >>  	/* \todo get this from available filter densities */
> > >>  	float filterDensity = 0.0f;
> > >> @@ -1867,7 +1812,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> > >>  		return nullptr;
> > >>  	}
> > >>  
> > >> -	requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > >> +	requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > >>  				     &captureIntent, 1);
> > >>  
> > >>  	requestTemplates_[type] = std::move(requestTemplate);
> > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > >> index 8edbcdfd..88aab012 100644
> > >> --- a/src/android/camera_device.h
> > >> +++ b/src/android/camera_device.h
> > >> @@ -98,7 +98,6 @@ private:
> > >>  	std::vector<libcamera::Size>
> > >>  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
> > >>  
> > >> -	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> > >>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> > >>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > >>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > >> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> > >> index 6f1bcdbe..e0b314ee 100644
> > >> --- a/src/android/camera_metadata.cpp
> > >> +++ b/src/android/camera_metadata.cpp
> > >> @@ -63,49 +63,32 @@ bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) c
> > >>  	return true;
> > >>  }
> > >>  
> > >> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
> > >> +bool CameraMetadata::resize(size_t count, size_t size)
> > >>  {
> > >>  	if (!valid_)
> > >>  		return false;
> > >>  
> > >> -	if (!add_camera_metadata_entry(metadata_, tag, data, count))
> > >> -		return true;
> > >> -
> > >> -	const char *name = get_camera_metadata_tag_name(tag);
> > >> -	if (name)
> > >> -		LOG(CameraMetadata, Error)
> > >> -			<< "Failed to add tag " << name;
> > >> -	else
> > >> -		LOG(CameraMetadata, Error)
> > >> -			<< "Failed to add unknown tag " << tag;
> > >> -
> > >> -	valid_ = false;
> > >> -
> > >> -	return false;
> > >> -}
> > >> -
> > >> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
> > >> -{
> > >> -	if (!valid_)
> > >> -		return false;
> > >> -
> > >> -	camera_metadata_entry_t entry;
> > >> -	int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> > >> -	if (ret) {
> > >> -		const char *name = get_camera_metadata_tag_name(tag);
> > >> -		LOG(CameraMetadata, Error)
> > >> -			<< "Failed to update tag "
> > >> -			<< (name ? name : "<unknown>") << ": not present";
> > >> -		return false;
> > >> -	}
> > >> -
> > >> -	ret = update_camera_metadata_entry(metadata_, entry.index, data,
> > >> -					   count, nullptr);
> > >> -	if (ret) {
> > >> -		const char *name = get_camera_metadata_tag_name(tag);
> > >> -		LOG(CameraMetadata, Error)
> > >> -			<< "Failed to update tag " << (name ? name : "<unknown>");
> > >> -		return false;
> > >> +	size_t currentEntryCount = get_camera_metadata_entry_count(metadata_);
> > >> +	size_t currentEntryCapacity = get_camera_metadata_entry_capacity(metadata_);
> > >> +	size_t newEntryCapacity = currentEntryCapacity < currentEntryCount + count ?
> > >> +				  currentEntryCapacity * 2 : currentEntryCapacity;
> > >> +
> > >> +	size_t currentDataCount = get_camera_metadata_data_count(metadata_);
> > >> +	size_t currentDataCapacity = get_camera_metadata_data_capacity(metadata_);
> > >> +	size_t newDataCapacity = currentDataCapacity < currentDataCount + size ?
> > >> +				 currentDataCapacity * 2 : currentDataCapacity;
> > >> +
> > >> +	if (newEntryCapacity > currentEntryCapacity ||
> > >> +	    newDataCapacity > currentDataCapacity) {
> > >> +		camera_metadata_t *oldMetadata = metadata_;
> > >> +		metadata_ = allocate_camera_metadata(newEntryCapacity, newDataCapacity);
> > >> +		if (!metadata_) {
> > >> +			metadata_ = oldMetadata;
> > >> +			return false;
> > >> +		}
> > >> +
> > >> +		append_camera_metadata(metadata_, oldMetadata);
> > >> +		free_camera_metadata(oldMetadata);
> > > 
> > > Ok - all that looked simpler than I expected.
> > > Maybe there are optimisations on how or when to resize, but I think this
> > > sounds quite reasonable for now?
> > > 
> > > Given that we can already pre-reserve if we believe we know the initial
> > > sizes...
> > > 
> > >>  	}
> > >>  
> > >>  	return true;
> > >> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> > >> index d653e2f0..c35ea1b7 100644
> > >> --- a/src/android/camera_metadata.h
> > >> +++ b/src/android/camera_metadata.h
> > >> @@ -7,6 +7,7 @@
> > >>  #ifndef __ANDROID_CAMERA_METADATA_H__
> > >>  #define __ANDROID_CAMERA_METADATA_H__
> > >>  
> > >> +#include <iostream>
> > >>  #include <stdint.h>
> > >>  
> > >>  #include <system/camera_metadata.h>
> > >> @@ -23,9 +24,43 @@ public:
> > >>  	CameraMetadata &operator=(const CameraMetadata &other);
> > >>  
> > >>  	bool isValid() const { return valid_; }
> > >> +	bool resize(size_t count, size_t size);
> > >>  	bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
> > >> -	bool addEntry(uint32_t tag, const void *data, size_t data_count);
> > >> -	bool updateEntry(uint32_t tag, const void *data, size_t data_count);
> > >> +
> > >> +	template<typename T>
> > >> +	bool addEntry(uint32_t tag, const T *data, size_t count)
> 
> Think of a template as a function that gets duplicated for every type T
> that is used. This will cause lots of duplication. You should gather the
> type-depend code in the beginning of the function, and then move
> everything else to a private function that isn't type-dependent.
> 
> I would also consider using a Span<const T> instead of passing data and
> count separately, as well as adding a wrapper for the common case of
> single-element entries
> 
> 	template<typename T>
> 	bool addEntry(uint32_t tag, const T &data)
> 	{
> 		return addEntry(Span<const T>{&data, 1});
> 	}
> 
> You may also change the return type to void, if nobody checks the return
> value.

Ooh good idea. I've used this for v2.


Thanks,

Paul

> 
> > >> +	{
> > >> +		if (!valid_)
> > >> +			return false;
> > >> +
> > >> +		camera_metadata_entry_t entry;
> > >> +		int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> > >> +		if (ret) {
> > >> +			if (!resize(1, count * sizeof(T))) {
> > > 
> > > Always calling resize feels a bit odd. Especially as resize can return
> > > true without actually resizing...
> > > 
> > >> +				std::cerr << "Failed to resize";
> > > 
> > > if you're using std::cerr, you need std::endl on the end :-)
> > > But I get that this will change.
> > > 
> > > In the code you've removed, there was:
> > > 	LOG(CameraMetadata, Error)
> > > 			<< "Failed to add tag " << name;
> > > 
> > > Can't you use that existing LOG infrastructure?
> > > 
> > >> +				return false;
> > >> +			}
> > >> +
> > >> +			if (!add_camera_metadata_entry(metadata_, tag, data, count))
> > >> +				return true;
> > >> +
> > >> +			const char *name = get_camera_metadata_tag_name(tag);
> > >> +			std::cerr << "Failed to add tag " << (name ? name : "<unknown>");
> > >> +
> > >> +			valid_ = false;
> > >> +
> > >> +			return false;
> > >> +		}
> > >> +
> > >> +		if (!update_camera_metadata_entry(metadata_, entry.index, data,
> > >> +						  count, nullptr))
> > >> +			return true;
> > >> +
> > >> +		const char *name = get_camera_metadata_tag_name(tag);
> > >> +		std::cerr << "Failed to update tag " << (name ? name : "<unknown>");
> > >> +
> > >> +		return false;
> > >> +	}
> > >>  
> > >>  	camera_metadata_t *get();
> > >>  	const camera_metadata_t *get() const;

Hi Hiro,

On Fri, May 07, 2021 at 11:55:47AM +0900, Hirokazu Honda wrote:
> Hi Paul,
> 
> On Thu, May 6, 2021 at 9:20 PM Laurent Pinchart <
> laurent.pinchart@ideasonboard.com> wrote:
> 
>     Hello,
> 
>     On Thu, May 06, 2021 at 12:14:27PM +0100, Kieran Bingham wrote:
>     > On 06/05/2021 12:05, Kieran Bingham wrote:
>     > > On 06/05/2021 11:41, Paul Elder wrote:
>     > >> This patch depends on the series "FULL hardware level fixes".
>     > >>
>     > >> Previously we had to manually declare the size of CameraMetadata on
>     > >> allocation, and its count not be changed after construction. Change
>     > >
>     > > count could not
>     > >
>     > >> CameraMetadata's behavior so that the user can simply add entries, and
>     > >> the CameraMetadata will auto-resize (double the size) as necessary. At
>     > >> the same time, make addEntry() also serve the purpose of updateEntry
>     (),
>     > >> and remove updateEntry(). Also remove everything involved with
>     > >> calculating the initial size for any CameraMetadata instances.
>     > >
>     > > Oh that sweet music to my ears ...
>     > >
>     > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>     > >>
>     > >> ---
>     > >> 1 - What do you (plural) think about merging updateEntry() into
>     > >>     addEntry()? I thought that with the dynamic allocation it would be
>     > >>     convenient to use one function. Is there any reason that we should
>     > >>     keep them separate, or is it fine to merge them?
>     > >
>     > > What's the current distinction?
>     > > Presumably addEntry requires that the entry doesn't yet exist, whereas
>     > > updateEntry would modify an existing entry?
> 
>     There may be a performance difference on the backend if we keep them
>     separate, so please have a look at that. If we merge them, I'd call the
>     resulting function setEntry(). We could even experiment with some sort
>     of operator[] syntax.
> 
>     > > For naming, if we were merging both, wouldn't it be better to call it
>     > > '->set(id, value)'?
>     > >
>     > >> 2 - How can I get logging working in the CameraMetadata header file?
>     The
>     > >>     last time I did that was in ipa_data_serializer.h and that wasn't
>     very
>     > >>     pretty either...
>     >
>     > Looked like there was already a LOG usage here, so you shoudl be able to
>     > use that I think.
>     >
>     > One last set of thoughts.
>     >
>     > It might be interesting to keep a flag 'resized' so you know if a resize
>     > did occur, and a mechanism to print out what the (final) storages were,
>     > so that even if it were a manual process, someone could hardcode in a
>     > better / optimal reservation at the start which would prevent resizes
>     > occurring in the first place.
> 
>     Make sense, instrumentation is important. We don't have to implement it
>     all yet, but designing the API accordingly is important.
> 
>     > >> (I haven't tested it on CTS yet; this is just RFC for the API and
>     > >> implementation)
>     > >> ---
>     > >>  src/android/camera_device.cpp   | 79
>     +++++----------------------------
>     > >>  src/android/camera_device.h     |  1 -
>     > >>  src/android/camera_metadata.cpp | 61 +++++++++----------------
>     > >>  src/android/camera_metadata.h   | 39 +++++++++++++++-
>     > >>  4 files changed, 71 insertions(+), 109 deletions(-)
>     > >>
>     > >> diff --git a/src/android/camera_device.cpp b/src/android/
>     camera_device.cpp
>     > >> index 114348a6..426e3fcd 100644
>     > >> --- a/src/android/camera_device.cpp
>     > >> +++ b/src/android/camera_device.cpp
>     > >> @@ -772,53 +772,6 @@ void CameraDevice::setCallbacks(const
>     camera3_callback_ops_t *callbacks)
>     > >>    callbacks_ = callbacks;
>     > >>  }
>     > >> 
>     > >> -std::tuple<uint32_t, uint32_t>
>     CameraDevice::calculateStaticMetadataSize()
>     > >> -{
>     > >> -  /*
>     > >> -   * \todo Keep this in sync with the actual number of entries.
>     > >> -   * Currently: 63 entries, 1014 bytes of static metadata
>     > >> -   */
>     > >> -  uint32_t numEntries = 63;
>     > >> -  uint32_t byteSize = 1014;
>     > >> -
>     > >> -  // do i need to add for entries in the available keys?
>     > >> -  // +1, +4 for EDGE_AVAILABLE_EDGE_MODES
>     > >> -  // +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES
>     > >> -  // +1, +4 for BLACK_LEVEL_PATTERN
>     > >> -  // +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES
>     > >> -  // +1, +4 for TONEMAP_MAX_CURVE_POINTS
>     > >> -  // +4x9 = 36 for the new result tags
>     > >> -
>     > >> -  // +36 for new request keys
>     > >> -
>     > >> -  /*
>     > >> -   * Calculate space occupation in bytes for dynamically built
>     metadata
>     > >> -   * entries.
>     > >> -   *
>     > >> -   * Each stream configuration entry requires 48 bytes:
>     > >> -   * 4 32bits integers for
>     ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
>     > >> -   * 4 64bits integers for
>     ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
>     > >> -   */
>     > >> -  byteSize += streamConfigurations_.size() * 48;
>     > >> -
>     > >> -  /*
>     > >> -   * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail
>     sizes
>     > >> -   * 2 32bits integers for the (0, 0) thumbnail size
>     > >> -   *
>     > >> -   * This is a worst case estimates as different configurations with
>     the
>     > >> -   * same aspect ratio will generate the same size.
>     > >> -   */
>     > >> -  for (const auto &entry : streamConfigurations_) {
>     > >> -          if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
>     > >> -                  continue;
>     > >> -
>     > >> -          byteSize += 8;
>     > >> -  }
>     > >> -  byteSize += 8;
>     > >> -
>     > >> -  return std::make_tuple(numEntries, byteSize);
>     > >> -}
>     > >> -
>     > >>  /*
>     > >>   * Return static information for the camera.
>     > >>   */
>     > >> @@ -827,15 +780,7 @@ const camera_metadata_t
>     *CameraDevice::getStaticMetadata()
>     > >>    if (staticMetadata_)
>     > >>            return staticMetadata_->get();
>     > >> 
>     > >> -  /*
>     > >> -   * The here reported metadata are enough to implement a basic
>     capture
>     > >> -   * example application, but a real camera implementation will
>     require
>     > >> -   * more.
>     > >> -   */
>     > >> -  uint32_t numEntries;
>     > >> -  uint32_t byteSize;
>     > >> -  std::tie(numEntries, byteSize) = calculateStaticMetadataSize();
>     > >> -  staticMetadata_ = std::make_unique<CameraMetadata>(numEntries,
>     byteSize);
>     > >> +  staticMetadata_ = std::make_unique<CameraMetadata>(32, 512);
> 
> 
> Shall we add a comment where 32 and 512 come from.
> I guess this is the result of calculateStaticMetadataSize() today.

It was actually arbitrary. Later I'll add some instrumentation and we
can set it to the numbers that come out of that.

>  
> 
>     > >
>     > > Presumably the initial count of entries is cheap to allocate, but the
>     > > storage size is more expensive. .. is there a distinction on the two?
> 
>     Compared to memory requirements related to image storage, neither should
>     be very expensive.
> 
>     > > I presume I'll see later ...
>     > >
>     > >>    if (!staticMetadata_->isValid()) {
>     > >>            LOG(HAL, Error) << "Failed to allocate static metadata";
>     > >>            staticMetadata_.reset();
>     > >> @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata>
>     CameraDevice::requestTemplateVideo()
>     > >>                              &entry);
>     > >> 
>     > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;
>     > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>     > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>     > >> 
>     > >>    /*
>     > >>     * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata
>     > >>     * has been assembled as {{min, max} {max, max}}.
>     > >>     */
>     > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
>     > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
>     > >>                                 entry.data.i32 + 2, 2);
>     > >> 
>     > >>    return previewTemplate;
>     > >> @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata>
>     CameraDevice::requestTemplateStill()
>     > >>     * HIGH_QUALITY.
>     > >>     */
>     > >>    uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;
>     > >> -  previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,
>     > >> +  previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,
>     > >>                                 &noiseReduction, 1);
>     > >> 
>     > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;
>     > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>     > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>     > >> 
>     > >>    uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;
>     > >> -  previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode,
>     1);
>     > >> +  previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
>     > >> 
>     > >>    uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;
>     > >> -  previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode,
>     1);
>     > >> +  previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
>     > >> 
>     > >>    return previewTemplate;
>     > >>  }
>     > >> @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata>
>     CameraDevice::requestTemplateManual()
>     > >>            return nullptr;
>     > >> 
>     > >>    uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;
>     > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode,
>     1);
>     > >> +  previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
>     > >> 
>     > >>    uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
>     > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
>     > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
>     > >> 
>     > >>    uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;
>     > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode,
>     1);
>     > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
>     > >> 
>     > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;
>     > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>     > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
>     > >> 
>     > >>    /* \todo get this from available filter densities */
>     > >>    float filterDensity = 0.0f;
>     > >> @@ -1867,7 +1812,7 @@ const camera_metadata_t
>     *CameraDevice::constructDefaultRequestSettings(int type)
>     > >>            return nullptr;
>     > >>    }
>     > >> 
>     > >> -  requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,
>     > >> +  requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
>     > >>                                 &captureIntent, 1);
>     > >> 
>     > >>    requestTemplates_[type] = std::move(requestTemplate);
>     > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>     > >> index 8edbcdfd..88aab012 100644
>     > >> --- a/src/android/camera_device.h
>     > >> +++ b/src/android/camera_device.h
>     > >> @@ -98,7 +98,6 @@ private:
>     > >>    std::vector<libcamera::Size>
>     > >>    getRawResolutions(const libcamera::PixelFormat &pixelFormat);
>     > >> 
>     > >> -  std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
>     > >>    libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t
>     camera3buffer);
>     > >>    void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>     > >>    void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
>     > >> diff --git a/src/android/camera_metadata.cpp b/src/android/
>     camera_metadata.cpp
>     > >> index 6f1bcdbe..e0b314ee 100644
>     > >> --- a/src/android/camera_metadata.cpp
>     > >> +++ b/src/android/camera_metadata.cpp
>     > >> @@ -63,49 +63,32 @@ bool CameraMetadata::getEntry(uint32_t tag,
>     camera_metadata_ro_entry_t *entry) c
>     > >>    return true;
>     > >>  }
>     > >> 
>     > >> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t
>     count)
>     > >> +bool CameraMetadata::resize(size_t count, size_t size)
>     > >>  {
>     > >>    if (!valid_)
>     > >>            return false;
>     > >> 
>     > >> -  if (!add_camera_metadata_entry(metadata_, tag, data, count))
>     > >> -          return true;
>     > >> -
>     > >> -  const char *name = get_camera_metadata_tag_name(tag);
>     > >> -  if (name)
>     > >> -          LOG(CameraMetadata, Error)
>     > >> -                  << "Failed to add tag " << name;
>     > >> -  else
>     > >> -          LOG(CameraMetadata, Error)
>     > >> -                  << "Failed to add unknown tag " << tag;
>     > >> -
>     > >> -  valid_ = false;
>     > >> -
>     > >> -  return false;
>     > >> -}
>     > >> -
>     > >> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data,
>     size_t count)
>     > >> -{
>     > >> -  if (!valid_)
>     > >> -          return false;
>     > >> -
>     > >> -  camera_metadata_entry_t entry;
>     > >> -  int ret = find_camera_metadata_entry(metadata_, tag, &entry);
>     > >> -  if (ret) {
>     > >> -          const char *name = get_camera_metadata_tag_name(tag);
>     > >> -          LOG(CameraMetadata, Error)
>     > >> -                  << "Failed to update tag "
>     > >> -                  << (name ? name : "<unknown>") << ": not present";
>     > >> -          return false;
>     > >> -  }
>     > >> -
>     > >> -  ret = update_camera_metadata_entry(metadata_, entry.index, data,
>     > >> -                                     count, nullptr);
>     > >> -  if (ret) {
>     > >> -          const char *name = get_camera_metadata_tag_name(tag);
>     > >> -          LOG(CameraMetadata, Error)
>     > >> -                  << "Failed to update tag " << (name ? name : "
>     <unknown>");
>     > >> -          return false;
>     > >> +  size_t currentEntryCount = get_camera_metadata_entry_count
>     (metadata_);
>     > >> +  size_t currentEntryCapacity = get_camera_metadata_entry_capacity
>     (metadata_);
>     > >> +  size_t newEntryCapacity = currentEntryCapacity < currentEntryCount
>     + count ?
>     > >> +                            currentEntryCapacity * 2 :
>     currentEntryCapacity;
>     > >> +
>     > >> +  size_t currentDataCount = get_camera_metadata_data_count
>     (metadata_);
>     > >> +  size_t currentDataCapacity = get_camera_metadata_data_capacity
>     (metadata_);
>     > >> +  size_t newDataCapacity = currentDataCapacity < currentDataCount +
>     size ?
>     > >> +                           currentDataCapacity * 2 :
>     currentDataCapacity;
>     > >> +
>     > >> +  if (newEntryCapacity > currentEntryCapacity ||
>     > >> +      newDataCapacity > currentDataCapacity) {
>     > >> +          camera_metadata_t *oldMetadata = metadata_;
>     > >> +          metadata_ = allocate_camera_metadata(newEntryCapacity,
>     newDataCapacity);
>     > >> +          if (!metadata_) {
>     > >> +                  metadata_ = oldMetadata;
>     > >> +                  return false;
>     > >> +          }
>     > >> +
>     > >> +          append_camera_metadata(metadata_, oldMetadata);
>     > >> +          free_camera_metadata(oldMetadata);
>     > >
>     > > Ok - all that looked simpler than I expected.
>     > > Maybe there are optimisations on how or when to resize, but I think
>     this
>     > > sounds quite reasonable for now?
>     > >
>     > > Given that we can already pre-reserve if we believe we know the initial
>     > > sizes...
>     > >
>     > >>    }
>     > >> 
>     > >>    return true;
>     > >> diff --git a/src/android/camera_metadata.h b/src/android/
>     camera_metadata.h
>     > >> index d653e2f0..c35ea1b7 100644
>     > >> --- a/src/android/camera_metadata.h
>     > >> +++ b/src/android/camera_metadata.h
>     > >> @@ -7,6 +7,7 @@
>     > >>  #ifndef __ANDROID_CAMERA_METADATA_H__
>     > >>  #define __ANDROID_CAMERA_METADATA_H__
>     > >> 
>     > >> +#include <iostream>
>     > >>  #include <stdint.h>
>     > >> 
>     > >>  #include <system/camera_metadata.h>
>     > >> @@ -23,9 +24,43 @@ public:
>     > >>    CameraMetadata &operator=(const CameraMetadata &other);
>     > >> 
>     > >>    bool isValid() const { return valid_; }
>     > >> +  bool resize(size_t count, size_t size);
>     > >>    bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry)
>     const;
>     > >> -  bool addEntry(uint32_t tag, const void *data, size_t data_count);
>     > >> -  bool updateEntry(uint32_t tag, const void *data, size_t
>     data_count);
>     > >> +
>     > >> +  template<typename T>
>     > >> +  bool addEntry(uint32_t tag, const T *data, size_t count)
> 
>     Think of a template as a function that gets duplicated for every type T
>     that is used. This will cause lots of duplication. You should gather the
>     type-depend code in the beginning of the function, and then move
>     everything else to a private function that isn't type-dependent.
> 
>     I would also consider using a Span<const T> instead of passing data and
>     count separately, as well as adding a wrapper for the common case of
>     single-element entries
> 
>             template<typename T>
>             bool addEntry(uint32_t tag, const T &data)
>             {
>                     return addEntry(Span<const T>{&data, 1});
>             }
> 
>     You may also change the return type to void, if nobody checks the return
>     value.
> 
>     > >> +  {
>     > >> +          if (!valid_)
>     > >> +                  return false;
>     > >> +
>     > >> +          camera_metadata_entry_t entry;
>     > >> +          int ret = find_camera_metadata_entry(metadata_, tag, &
>     entry);
>     > >> +          if (ret) {
>     > >> +                  if (!resize(1, count * sizeof(T))) {
>     > >
>     > > Always calling resize feels a bit odd. Especially as resize can return
>     > > true without actually resizing...
>     > >
>     > >> +                          std::cerr << "Failed to resize";
>     > >
>     > > if you're using std::cerr, you need std::endl on the end :-)
>     > > But I get that this will change.
>     > >
>     > > In the code you've removed, there was:
>     > >     LOG(CameraMetadata, Error)
>     > >                     << "Failed to add tag " << name;
>     > >
>     > > Can't you use that existing LOG infrastructure?
>     > >
>     > >> +                          return false;
>     > >> +                  }
>     > >> +
>     > >> +                  if (!add_camera_metadata_entry(metadata_, tag,
>     data, count))
>     > >> +                          return true;
>     > >> +
>     > >> +                  const char *name = get_camera_metadata_tag_name
>     (tag);
>     > >> +                  std::cerr << "Failed to add tag " << (name ? name :
>     "<unknown>");
>     > >> +
>     > >> +                  valid_ = false;
>     > >> +
>     > >> +                  return false;
>     > >> +          }
>     > >> +
>     > >> +          if (!update_camera_metadata_entry(metadata_, entry.index,
>     data,
>     > >> +                                            count, nullptr))
>     > >> +                  return true;
>     > >> +
>     > >> +          const char *name = get_camera_metadata_tag_name(tag);
>     > >> +          std::cerr << "Failed to update tag " << (name ? name : "
>     <unknown>");
>     > >> +
>     > >> +          return false;
>     > >> +  }
>     > >> 
>     > >>    camera_metadata_t *get();
>     > >>    const camera_metadata_t *get() const;
> 
> 
> 
> Overall, I am fine with this change while I am a bit concerned about the
> performance of allocate_camera_metadata and append_camera_metadata.
> I expect it causes O(N), where N is the data size, and in the worst case adding
> entries will be O(N*T), where T is the number of added entires.

I think it would be O(N+T), since it's just allocing another metadata of
bigger size, and then copying the small metadata into the new bigger
metadata.

But also we double the size of the metadata on every required resize, so
the resize won't happen too frequently to cause an issue I think.

> The data size is not considerably small (about 500 bytes), isn't it?
> Perhaps is it possible to cache the added entries in the CameraMetadata class
> until get() and form as camera_metadata_t upon get(), so adding entries will be
> O(N), where N is the data size.

I considered that too. I forgot what the drawback was though. get()
would take longer.


Thanks,

Paul

Hi Hiro,

On Fri, May 07, 2021 at 11:55:47AM +0900, Hirokazu Honda wrote:
> On Thu, May 6, 2021 at 9:20 PM Laurent Pinchart wrote:
> > On Thu, May 06, 2021 at 12:14:27PM +0100, Kieran Bingham wrote:
> > > On 06/05/2021 12:05, Kieran Bingham wrote:
> > > > On 06/05/2021 11:41, Paul Elder wrote:
> > > >> This patch depends on the series "FULL hardware level fixes".
> > > >>
> > > >> Previously we had to manually declare the size of CameraMetadata on
> > > >> allocation, and its count not be changed after construction. Change
> > > >
> > > > count could not
> > > >
> > > >> CameraMetadata's behavior so that the user can simply add entries, and
> > > >> the CameraMetadata will auto-resize (double the size) as necessary. At
> > > >> the same time, make addEntry() also serve the purpose of updateEntry(),
> > > >> and remove updateEntry(). Also remove everything involved with
> > > >> calculating the initial size for any CameraMetadata instances.
> > > >
> > > > Oh that sweet music to my ears ...
> > > >
> > > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > >>
> > > >> ---
> > > >> 1 - What do you (plural) think about merging updateEntry() into
> > > >>     addEntry()? I thought that with the dynamic allocation it would be
> > > >>     convenient to use one function. Is there any reason that we should
> > > >>     keep them separate, or is it fine to merge them?
> > > >
> > > > What's the current distinction?
> > > > Presumably addEntry requires that the entry doesn't yet exist, whereas
> > > > updateEntry would modify an existing entry?
> >
> > There may be a performance difference on the backend if we keep them
> > separate, so please have a look at that. If we merge them, I'd call the
> > resulting function setEntry(). We could even experiment with some sort
> > of operator[] syntax.
> >
> > > > For naming, if we were merging both, wouldn't it be better to call it
> > > > '->set(id, value)'?
> > > >
> > > >> 2 - How can I get logging working in the CameraMetadata header file? The
> > > >>     last time I did that was in ipa_data_serializer.h and that wasn't very
> > > >>     pretty either...
> > >
> > > Looked like there was already a LOG usage here, so you shoudl be able to
> > > use that I think.
> > >
> > > One last set of thoughts.
> > >
> > > It might be interesting to keep a flag 'resized' so you know if a resize
> > > did occur, and a mechanism to print out what the (final) storages were,
> > > so that even if it were a manual process, someone could hardcode in a
> > > better / optimal reservation at the start which would prevent resizes
> > > occurring in the first place.
> >
> > Make sense, instrumentation is important. We don't have to implement it
> > all yet, but designing the API accordingly is important.
> >
> > > >> (I haven't tested it on CTS yet; this is just RFC for the API and
> > > >> implementation)
> > > >> ---
> > > >>  src/android/camera_device.cpp   | 79 +++++----------------------------
> > > >>  src/android/camera_device.h     |  1 -
> > > >>  src/android/camera_metadata.cpp | 61 +++++++++----------------
> > > >>  src/android/camera_metadata.h   | 39 +++++++++++++++-
> > > >>  4 files changed, 71 insertions(+), 109 deletions(-)
> > > >>
> > > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > >> index 114348a6..426e3fcd 100644
> > > >> --- a/src/android/camera_device.cpp
> > > >> +++ b/src/android/camera_device.cpp
> > > >> @@ -772,53 +772,6 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
> > > >>    callbacks_ = callbacks;
> > > >>  }
> > > >>
> > > >> -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
> > > >> -{
> > > >> -  /*
> > > >> -   * \todo Keep this in sync with the actual number of entries.
> > > >> -   * Currently: 63 entries, 1014 bytes of static metadata
> > > >> -   */
> > > >> -  uint32_t numEntries = 63;
> > > >> -  uint32_t byteSize = 1014;
> > > >> -
> > > >> -  // do i need to add for entries in the available keys?
> > > >> -  // +1, +4 for EDGE_AVAILABLE_EDGE_MODES
> > > >> -  // +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES
> > > >> -  // +1, +4 for BLACK_LEVEL_PATTERN
> > > >> -  // +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES
> > > >> -  // +1, +4 for TONEMAP_MAX_CURVE_POINTS
> > > >> -  // +4x9 = 36 for the new result tags
> > > >> -
> > > >> -  // +36 for new request keys
> > > >> -
> > > >> -  /*
> > > >> -   * Calculate space occupation in bytes for dynamically built metadata
> > > >> -   * entries.
> > > >> -   *
> > > >> -   * Each stream configuration entry requires 48 bytes:
> > > >> -   * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
> > > >> -   * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
> > > >> -   */
> > > >> -  byteSize += streamConfigurations_.size() * 48;
> > > >> -
> > > >> -  /*
> > > >> -   * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes
> > > >> -   * 2 32bits integers for the (0, 0) thumbnail size
> > > >> -   *
> > > >> -   * This is a worst case estimates as different configurations with the
> > > >> -   * same aspect ratio will generate the same size.
> > > >> -   */
> > > >> -  for (const auto &entry : streamConfigurations_) {
> > > >> -          if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> > > >> -                  continue;
> > > >> -
> > > >> -          byteSize += 8;
> > > >> -  }
> > > >> -  byteSize += 8;
> > > >> -
> > > >> -  return std::make_tuple(numEntries, byteSize);
> > > >> -}
> > > >> -
> > > >>  /*
> > > >>   * Return static information for the camera.
> > > >>   */
> > > >> @@ -827,15 +780,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > > >>    if (staticMetadata_)
> > > >>            return staticMetadata_->get();
> > > >>
> > > >> -  /*
> > > >> -   * The here reported metadata are enough to implement a basic capture
> > > >> -   * example application, but a real camera implementation will require
> > > >> -   * more.
> > > >> -   */
> > > >> -  uint32_t numEntries;
> > > >> -  uint32_t byteSize;
> > > >> -  std::tie(numEntries, byteSize) = calculateStaticMetadataSize();
> > > >> -  staticMetadata_ = std::make_unique<CameraMetadata>(numEntries, byteSize);
> > > >> +  staticMetadata_ = std::make_unique<CameraMetadata>(32, 512);
> 
> Shall we add a comment where 32 and 512 come from.
> I guess this is the result of calculateStaticMetadataSize() today.
> 
> > > > Presumably the initial count of entries is cheap to allocate, but the
> > > > storage size is more expensive. .. is there a distinction on the two?
> >
> > Compared to memory requirements related to image storage, neither should
> > be very expensive.
> >
> > > > I presume I'll see later ...
> > > >
> > > >>    if (!staticMetadata_->isValid()) {
> > > >>            LOG(HAL, Error) << "Failed to allocate static metadata";
> > > >>            staticMetadata_.reset();
> > > >> @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()
> > > >>                              &entry);
> > > >>
> > > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;
> > > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > > >>
> > > >>    /*
> > > >>     * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata
> > > >>     * has been assembled as {{min, max} {max, max}}.
> > > >>     */
> > > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
> > > >>                                 entry.data.i32 + 2, 2);
> > > >>
> > > >>    return previewTemplate;
> > > >> @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateStill()
> > > >>     * HIGH_QUALITY.
> > > >>     */
> > > >>    uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;
> > > >> -  previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,
> > > >> +  previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> > > >>                                 &noiseReduction, 1);
> > > >>
> > > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;
> > > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > > >>
> > > >>    uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;
> > > >> -  previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> > > >> +  previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);
> > > >>
> > > >>    uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;
> > > >> -  previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> > > >> +  previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);
> > > >>
> > > >>    return previewTemplate;
> > > >>  }
> > > >> @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateManual()
> > > >>            return nullptr;
> > > >>
> > > >>    uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;
> > > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> > > >> +  previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);
> > > >>
> > > >>    uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;
> > > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> > > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);
> > > >>
> > > >>    uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;
> > > >> -  previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> > > >> +  previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);
> > > >>
> > > >>    uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;
> > > >> -  previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > > >> +  previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);
> > > >>
> > > >>    /* \todo get this from available filter densities */
> > > >>    float filterDensity = 0.0f;
> > > >> @@ -1867,7 +1812,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> > > >>            return nullptr;
> > > >>    }
> > > >>
> > > >> -  requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > > >> +  requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > > >>                                 &captureIntent, 1);
> > > >>
> > > >>    requestTemplates_[type] = std::move(requestTemplate);
> > > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > >> index 8edbcdfd..88aab012 100644
> > > >> --- a/src/android/camera_device.h
> > > >> +++ b/src/android/camera_device.h
> > > >> @@ -98,7 +98,6 @@ private:
> > > >>    std::vector<libcamera::Size>
> > > >>    getRawResolutions(const libcamera::PixelFormat &pixelFormat);
> > > >>
> > > >> -  std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
> > > >>    libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> > > >>    void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > > >>    void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > > >> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> > > >> index 6f1bcdbe..e0b314ee 100644
> > > >> --- a/src/android/camera_metadata.cpp
> > > >> +++ b/src/android/camera_metadata.cpp
> > > >> @@ -63,49 +63,32 @@ bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) c
> > > >>    return true;
> > > >>  }
> > > >>
> > > >> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
> > > >> +bool CameraMetadata::resize(size_t count, size_t size)
> > > >>  {
> > > >>    if (!valid_)
> > > >>            return false;
> > > >>
> > > >> -  if (!add_camera_metadata_entry(metadata_, tag, data, count))
> > > >> -          return true;
> > > >> -
> > > >> -  const char *name = get_camera_metadata_tag_name(tag);
> > > >> -  if (name)
> > > >> -          LOG(CameraMetadata, Error)
> > > >> -                  << "Failed to add tag " << name;
> > > >> -  else
> > > >> -          LOG(CameraMetadata, Error)
> > > >> -                  << "Failed to add unknown tag " << tag;
> > > >> -
> > > >> -  valid_ = false;
> > > >> -
> > > >> -  return false;
> > > >> -}
> > > >> -
> > > >> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
> > > >> -{
> > > >> -  if (!valid_)
> > > >> -          return false;
> > > >> -
> > > >> -  camera_metadata_entry_t entry;
> > > >> -  int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> > > >> -  if (ret) {
> > > >> -          const char *name = get_camera_metadata_tag_name(tag);
> > > >> -          LOG(CameraMetadata, Error)
> > > >> -                  << "Failed to update tag "
> > > >> -                  << (name ? name : "<unknown>") << ": not present";
> > > >> -          return false;
> > > >> -  }
> > > >> -
> > > >> -  ret = update_camera_metadata_entry(metadata_, entry.index, data,
> > > >> -                                     count, nullptr);
> > > >> -  if (ret) {
> > > >> -          const char *name = get_camera_metadata_tag_name(tag);
> > > >> -          LOG(CameraMetadata, Error)
> > > >> -                  << "Failed to update tag " << (name ? name : "<unknown>");
> > > >> -          return false;
> > > >> +  size_t currentEntryCount = get_camera_metadata_entry_count(metadata_);
> > > >> +  size_t currentEntryCapacity = get_camera_metadata_entry_capacity(metadata_);
> > > >> +  size_t newEntryCapacity = currentEntryCapacity < currentEntryCount + count ?
> > > >> +                            currentEntryCapacity * 2 : currentEntryCapacity;
> > > >> +
> > > >> +  size_t currentDataCount = get_camera_metadata_data_count(metadata_);
> > > >> +  size_t currentDataCapacity = get_camera_metadata_data_capacity(metadata_);
> > > >> +  size_t newDataCapacity = currentDataCapacity < currentDataCount + size ?
> > > >> +                           currentDataCapacity * 2 : currentDataCapacity;
> > > >> +
> > > >> +  if (newEntryCapacity > currentEntryCapacity ||
> > > >> +      newDataCapacity > currentDataCapacity) {
> > > >> +          camera_metadata_t *oldMetadata = metadata_;
> > > >> +          metadata_ = allocate_camera_metadata(newEntryCapacity, newDataCapacity);
> > > >> +          if (!metadata_) {
> > > >> +                  metadata_ = oldMetadata;
> > > >> +                  return false;
> > > >> +          }
> > > >> +
> > > >> +          append_camera_metadata(metadata_, oldMetadata);
> > > >> +          free_camera_metadata(oldMetadata);
> > > >
> > > > Ok - all that looked simpler than I expected.
> > > > Maybe there are optimisations on how or when to resize, but I think this
> > > > sounds quite reasonable for now?
> > > >
> > > > Given that we can already pre-reserve if we believe we know the initial
> > > > sizes...
> > > >
> > > >>    }
> > > >>
> > > >>    return true;
> > > >> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> > > >> index d653e2f0..c35ea1b7 100644
> > > >> --- a/src/android/camera_metadata.h
> > > >> +++ b/src/android/camera_metadata.h
> > > >> @@ -7,6 +7,7 @@
> > > >>  #ifndef __ANDROID_CAMERA_METADATA_H__
> > > >>  #define __ANDROID_CAMERA_METADATA_H__
> > > >>
> > > >> +#include <iostream>
> > > >>  #include <stdint.h>
> > > >>
> > > >>  #include <system/camera_metadata.h>
> > > >> @@ -23,9 +24,43 @@ public:
> > > >>    CameraMetadata &operator=(const CameraMetadata &other);
> > > >>
> > > >>    bool isValid() const { return valid_; }
> > > >> +  bool resize(size_t count, size_t size);
> > > >>    bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
> > > >> -  bool addEntry(uint32_t tag, const void *data, size_t data_count);
> > > >> -  bool updateEntry(uint32_t tag, const void *data, size_t data_count);
> > > >> +
> > > >> +  template<typename T>
> > > >> +  bool addEntry(uint32_t tag, const T *data, size_t count)
> >
> > Think of a template as a function that gets duplicated for every type T
> > that is used. This will cause lots of duplication. You should gather the
> > type-depend code in the beginning of the function, and then move
> > everything else to a private function that isn't type-dependent.
> >
> > I would also consider using a Span<const T> instead of passing data and
> > count separately, as well as adding a wrapper for the common case of
> > single-element entries
> >
> >         template<typename T>
> >         bool addEntry(uint32_t tag, const T &data)
> >         {
> >                 return addEntry(Span<const T>{&data, 1});
> >         }
> >
> > You may also change the return type to void, if nobody checks the return
> > value.
> >
> > > >> +  {
> > > >> +          if (!valid_)
> > > >> +                  return false;
> > > >> +
> > > >> +          camera_metadata_entry_t entry;
> > > >> +          int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> > > >> +          if (ret) {
> > > >> +                  if (!resize(1, count * sizeof(T))) {
> > > >
> > > > Always calling resize feels a bit odd. Especially as resize can return
> > > > true without actually resizing...
> > > >
> > > >> +                          std::cerr << "Failed to resize";
> > > >
> > > > if you're using std::cerr, you need std::endl on the end :-)
> > > > But I get that this will change.
> > > >
> > > > In the code you've removed, there was:
> > > >     LOG(CameraMetadata, Error)
> > > >                     << "Failed to add tag " << name;
> > > >
> > > > Can't you use that existing LOG infrastructure?
> > > >
> > > >> +                          return false;
> > > >> +                  }
> > > >> +
> > > >> +                  if (!add_camera_metadata_entry(metadata_, tag, data, count))
> > > >> +                          return true;
> > > >> +
> > > >> +                  const char *name = get_camera_metadata_tag_name(tag);
> > > >> +                  std::cerr << "Failed to add tag " << (name ? name : "<unknown>");
> > > >> +
> > > >> +                  valid_ = false;
> > > >> +
> > > >> +                  return false;
> > > >> +          }
> > > >> +
> > > >> +          if (!update_camera_metadata_entry(metadata_, entry.index, data,
> > > >> +                                            count, nullptr))
> > > >> +                  return true;
> > > >> +
> > > >> +          const char *name = get_camera_metadata_tag_name(tag);
> > > >> +          std::cerr << "Failed to update tag " << (name ? name : "<unknown>");
> > > >> +
> > > >> +          return false;
> > > >> +  }
> > > >>
> > > >>    camera_metadata_t *get();
> > > >>    const camera_metadata_t *get() const;
>
> Overall, I am fine with this change while I am a bit concerned about the
> performance of allocate_camera_metadata and append_camera_metadata.
> I expect it causes O(N), where N is the data size, and in the worst case
> adding entries will be O(N*T), where T is the number of added entires.
> The data size is not considerably small (about 500 bytes), isn't it?
> Perhaps is it possible to cache the added entries in the CameraMetadata
> class until get() and form as camera_metadata_t upon get(), so adding
> entries will be O(N), where N is the data size.

I've been thinking about it too, and I think it would make sense. We
could even ditch the Android metadata helper library completly and use a
custom implementation that would integrate better with our usage of the
metadata. I'd consider this as an improvement on top of this patch
though.

Hi Hiro and Laurent,

On Fri, May 07, 2021 at 03:06:36PM +0300, Laurent Pinchart wrote:
> Hi Hiro,
> 
> On Fri, May 07, 2021 at 11:55:47AM +0900, Hirokazu Honda wrote:

<snip>

> > Overall, I am fine with this change while I am a bit concerned about the
> > performance of allocate_camera_metadata and append_camera_metadata.
> > I expect it causes O(N), where N is the data size, and in the worst case
> > adding entries will be O(N*T), where T is the number of added entires.
> > The data size is not considerably small (about 500 bytes), isn't it?
> > Perhaps is it possible to cache the added entries in the CameraMetadata
> > class until get() and form as camera_metadata_t upon get(), so adding
> > entries will be O(N), where N is the data size.
> 
> I've been thinking about it too, and I think it would make sense. We
> could even ditch the Android metadata helper library completly and use a
> custom implementation that would integrate better with our usage of the
> metadata. I'd consider this as an improvement on top of this patch
> though.

I've given this some more thought, and I think the design that I started
out with is probably better.

We need to remember that we'll be producing a metadata on every result,
so it does need to be quick. With what I have right now, it's mostly
constant time insertion, and constant time get(). Some insertions might
be linear (O(N+T)), but I think that can be mitigated by
instrumentation, and then simply allocate slightly more to begin with.

An alternative is to have guaranteed constant time insertion (either via
vector or deque with some custom metadata entry). The issue with this is
that we also have guaranteed linear time get(), as we have to walk the
list to convert the libcamera metadata to android metadata. A mitigation
for this would be to convert during insertion, but then we would be
having to deal with android metadata manually and bypassing the android
metadata library functions that are provided, and I think that's really
not a good idea.

Unless I misunderstood your (Hiro's) proposal. Instead of resizing, we
create a new android metadata container and insert into there? We would
still need to merge it in the end anyway, so I don't think the runtime
would change compared to what I have here, it's just a matter of if the
merging is during insertion or during get().


Thanks,

Paul

Hi Paul,

On Mon, May 10, 2021 at 4:49 PM <paul.elder@ideasonboard.com> wrote:

> Hi Hiro and Laurent,
>
> On Fri, May 07, 2021 at 03:06:36PM +0300, Laurent Pinchart wrote:
> > Hi Hiro,
> >
> > On Fri, May 07, 2021 at 11:55:47AM +0900, Hirokazu Honda wrote:
>
> <snip>
>
> > > Overall, I am fine with this change while I am a bit concerned about
> the
> > > performance of allocate_camera_metadata and append_camera_metadata.
> > > I expect it causes O(N), where N is the data size, and in the worst
> case
> > > adding entries will be O(N*T), where T is the number of added entires.
> > > The data size is not considerably small (about 500 bytes), isn't it?
> > > Perhaps is it possible to cache the added entries in the CameraMetadata
> > > class until get() and form as camera_metadata_t upon get(), so adding
> > > entries will be O(N), where N is the data size.
> >
> > I've been thinking about it too, and I think it would make sense. We
> > could even ditch the Android metadata helper library completly and use a
> > custom implementation that would integrate better with our usage of the
> > metadata. I'd consider this as an improvement on top of this patch
> > though.
>
> I've given this some more thought, and I think the design that I started
> out with is probably better.
>
> We need to remember that we'll be producing a metadata on every result,
> so it does need to be quick. With what I have right now, it's mostly
> constant time insertion, and constant time get(). Some insertions might
> be linear (O(N+T)), but I think that can be mitigated by
> instrumentation, and then simply allocate slightly more to begin with.
>
> An alternative is to have guaranteed constant time insertion (either via
> vector or deque with some custom metadata entry). The issue with this is
> that we also have guaranteed linear time get(), as we have to walk the
> list to convert the libcamera metadata to android metadata. A mitigation
> for this would be to convert during insertion, but then we would be
> having to deal with android metadata manually and bypassing the android
> metadata library functions that are provided, and I think that's really
> not a good idea.
>
> Unless I misunderstood your (Hiro's) proposal. Instead of resizing, we
> create a new android metadata container and insert into there? We would
> still need to merge it in the end anyway, so I don't think the runtime
> would change compared to what I have here, it's just a matter of if the
> merging is during insertion or during get().
>
>
I see your point. You're right.
I thought to simply cache the value with std::map.
But sadly a given value can be any type. We may have to create a heap
storage for them or use std::variant.
It may complicate CameraMeatdata implementation.
I think your solution is better now.

-Hiro

>
> Thanks,
>
> Paul
>