[libcamera-devel,v3,1/3] android: camera_metadata: Auto-resize CameraMetadata

12253 diff mbox series

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

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

---
Changes in v3:
- dependency on "FULL hardware level fixes" removed
- split addEntry and updateEntry
  - i think the lookup on every (merged) addEntry isn't worth it, since
    the user probably knows if they need to add or update
- add auto-resize (and corresponding check) to updateEntry as well
- add the templates and overloads to updateEntry as well
- increase the default size for the static metadata to be above what we
  had before
- valid_ is now used to indicate a failed addEntry/updateEntry in
  addition to metadata_ == nullptr. this way the user doesn't have to
  check the return value of every single addEntry/updateEntry call, and
  can just check valid_ at the end
- print resize when it happens
- updateEntry resizes if the new data size is greater than the old data

Question: I think on failure of addEntry/updateEntry the metadata could
still be "valid", it's just that the content isn't what the user
expects. Should we keep valid_ as I've repurposed it here in v3, or
should we add another flag?

Changes in v2:
- add overloading of addEntry
- better logging with the separation of core code vs template code
- addEntry failure makes the metadata invalid

The main feature of v2 is the overloading of addEntry. I have three
overloads and the main one:
- tag, single piece of data (reference)
- tag, vector (v3: s/span/vector/)
- tag, data pointer, count
- tag, data pointer, count, size of one instance of the data

The last one is the main one, and is not templated. The first two are
nice and convenient, because now we can do things like:

uint32_t orientation = 0;
resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, orientation);

and

std::vector<uint32_t> availableStates = { SOME_STATE_1, SOME_STATE_2 };
metadata->addEntry(ANDROID_AVAILABLE_STATES, availableStates);

The third is for when android metadata is copied directly from another
piece of android metadata. This is very common, and wasn't very nice
having to wrap everything in a Span. All the old calls are
backward-compatible using this overload.
---
 src/android/camera_device.cpp   | 47 +-----------------
 src/android/camera_device.h     |  1 -
 src/android/camera_metadata.cpp | 84 +++++++++++++++++++++++++++++----
 src/android/camera_metadata.h   | 44 ++++++++++++++++-
 4 files changed, 117 insertions(+), 59 deletions(-)

Hi Paul, thank you for the patch.

On Wed, May 12, 2021 at 7:25 PM Paul Elder <paul.elder@ideasonboard.com>
wrote:

> Previously we had to manually declare the size of CameraMetadata on
> allocation, and its count could not be changed after construction.
> Change CameraMetadata's behavior so that the user can simply add or
> update entries, and the CameraMetadata will auto-resize (double the
> size) as necessary. Also remove everything involved with calculating
> the initial size for any CameraMetadata instances.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v3:
> - dependency on "FULL hardware level fixes" removed
> - split addEntry and updateEntry
>   - i think the lookup on every (merged) addEntry isn't worth it, since
>     the user probably knows if they need to add or update
> - add auto-resize (and corresponding check) to updateEntry as well
> - add the templates and overloads to updateEntry as well
> - increase the default size for the static metadata to be above what we
>   had before
> - valid_ is now used to indicate a failed addEntry/updateEntry in
>   addition to metadata_ == nullptr. this way the user doesn't have to
>   check the return value of every single addEntry/updateEntry call, and
>   can just check valid_ at the end
> - print resize when it happens
> - updateEntry resizes if the new data size is greater than the old data
>
> Question: I think on failure of addEntry/updateEntry the metadata could
> still be "valid", it's just that the content isn't what the user
> expects. Should we keep valid_ as I've repurposed it here in v3, or
> should we add another flag?
>
> Changes in v2:
> - add overloading of addEntry
> - better logging with the separation of core code vs template code
> - addEntry failure makes the metadata invalid
>
> The main feature of v2 is the overloading of addEntry. I have three
> overloads and the main one:
> - tag, single piece of data (reference)
> - tag, vector (v3: s/span/vector/)
> - tag, data pointer, count
> - tag, data pointer, count, size of one instance of the data
>
> The last one is the main one, and is not templated. The first two are
> nice and convenient, because now we can do things like:
>
> uint32_t orientation = 0;
> resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, orientation);
>
> and
>
> std::vector<uint32_t> availableStates = { SOME_STATE_1, SOME_STATE_2 };
> metadata->addEntry(ANDROID_AVAILABLE_STATES, availableStates);
>
> The third is for when android metadata is copied directly from another
> piece of android metadata. This is very common, and wasn't very nice
> having to wrap everything in a Span. All the old calls are
> backward-compatible using this overload.
> ---
>  src/android/camera_device.cpp   | 47 +-----------------
>  src/android/camera_device.h     |  1 -
>  src/android/camera_metadata.cpp | 84 +++++++++++++++++++++++++++++----
>  src/android/camera_metadata.h   | 44 ++++++++++++++++-
>  4 files changed, 117 insertions(+), 59 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 7d4d0feb..74f6915c 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -773,43 +773,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: 54 entries, 874 bytes of static metadata
> -        */
> -       uint32_t numEntries = 54;
> -       uint32_t byteSize = 874;
> -
> -       /*
> -        * 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.
>   */
> @@ -818,15 +781,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>(64, 1024);
>         if (!staticMetadata_->isValid()) {
>                 LOG(HAL, Error) << "Failed to allocate static metadata";
>                 staticMetadata_.reset();
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 23457e47..ae162a45 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..6394fd74 100644
> --- a/src/android/camera_metadata.cpp
> +++ b/src/android/camera_metadata.cpp
> @@ -63,14 +63,67 @@ 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)
> +/*
> + * \brief Resize the metadata container, if necessary
> + * \param[in] count Number of entries to add to the container
> + * \param[in] size Total size of entries to add, in bytes
> + * \return True if resize was successful or unnecessary, false otherwise
> + */
> +bool CameraMetadata::resize(size_t count, size_t size)
>  {
>         if (!valid_)
>                 return false;
>
> -       if (!add_camera_metadata_entry(metadata_, tag, data, count))
> +       if (!count && !size)
>                 return true;
>
> +       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;
> +               }
> +
> +               LOG(CameraMetadata, Info)
> +                       << "Resized: old entry capacity " <<
> currentEntryCapacity
> +                       << ", old data capacity " << currentDataCapacity
> +                       << ", new entry capacity " << newEntryCapacity
> +                       << ", new data capacity " << newDataCapacity;
> +
> +               append_camera_metadata(metadata_, oldMetadata);
> +               free_camera_metadata(oldMetadata);
> +       }
> +
> +       return true;
> +}
> +
> +void CameraMetadata::addEntry(uint32_t tag, const void *data, size_t
> count,
> +                             size_t sizeofT)
> +{
> +       if (!valid_)
> +               return;
> +
> +       if (!resize(1, count * sizeofT)) {
> +               LOG(CameraMetadata, Error) << "Failed to resize";
> +               valid_ = false;
> +               return;
> +       }
> +
> +       if (!add_camera_metadata_entry(metadata_, tag, data, count))
> +               return;
> +
>         const char *name = get_camera_metadata_tag_name(tag);
>         if (name)
>                 LOG(CameraMetadata, Error)
> @@ -80,14 +133,13 @@ bool CameraMetadata::addEntry(uint32_t tag, const
> void *data, size_t count)
>                         << "Failed to add unknown tag " << tag;
>
>         valid_ = false;
> -
> -       return false;
>  }
>
> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t
> count)
> +void CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t
> count,
> +                                size_t sizeofT)
>  {
>         if (!valid_)
> -               return false;
> +               return;
>
>         camera_metadata_entry_t entry;
>         int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> @@ -96,7 +148,20 @@ bool CameraMetadata::updateEntry(uint32_t tag, const
> void *data, size_t count)
>                 LOG(CameraMetadata, Error)
>                         << "Failed to update tag "
>                         << (name ? name : "<unknown>") << ": not present";
> -               return false;
> +               return;
> +       }
> +
> +       size_t oldSize =
> +               calculate_camera_metadata_entry_data_size(entry.type,
> +                                                         entry.count);
> +       size_t newSize =
> +               calculate_camera_metadata_entry_data_size(entry.type,
> +                                                         count * sizeofT);
>

I am confused by this.
calculate_camera_metadata_entry_data_size() looks up the size of one
element for the type and computes the required size multiplying by the
count.
That is, newSize seems to be sizeofT * count * sizeofT.
Am I missing something?

+       size_t sizeIncrement = newSize - oldSize > 0 ? newSize - oldSize :
> 0;
> +       if (!resize(0, count * sizeIncrement)) {
> +               LOG(CameraMetadata, Error) << "Failed to resize";
> +               valid_ = false;
> +               return;
>         }
>
>         ret = update_camera_metadata_entry(metadata_, entry.index, data,
> @@ -105,10 +170,9 @@ bool CameraMetadata::updateEntry(uint32_t tag, const
> void *data, size_t count)
>                 const char *name = get_camera_metadata_tag_name(tag);
>                 LOG(CameraMetadata, Error)
>                         << "Failed to update tag " << (name ? name :
> "<unknown>");
> -               return false;
> -       }
>
> -       return true;
> +               valid_ = false;
> +       }
>  }
>
>  camera_metadata_t *CameraMetadata::get()
> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> index d653e2f0..aa875fe4 100644
> --- a/src/android/camera_metadata.h
> +++ b/src/android/camera_metadata.h
> @@ -8,6 +8,7 @@
>  #define __ANDROID_CAMERA_METADATA_H__
>
>  #include <stdint.h>
> +#include <vector>
>
>  #include <system/camera_metadata.h>
>
> @@ -23,9 +24,48 @@ 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,
> +                std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +       void addEntry(uint32_t tag, const T &data)
> +       {
> +               addEntry(tag, &data, 1, sizeof(T));
> +       }
> +
> +       template<typename T>
> +       void addEntry(uint32_t tag, std::vector<T> &data)
> +       {
> +               addEntry(tag, data.data(), data.size(), sizeof(T));
> +       }
> +
>

s/std::vector<T>/Span<const T>/
ditto to updateEntry().

We can use a row array, for example, for aeAvailableAntiBandingModes.

-Hiro

+       template<typename T>
> +       void addEntry(uint32_t tag, const T *data, size_t count)
> +       {
> +               addEntry(tag, data, count, sizeof(T));
> +       }
> +
> +       template<typename T>
> +       void updateEntry(uint32_t tag, const T &data)
> +       {
> +               updateEntry(tag, &data, 1, sizeof(T));
> +       }
> +
> +       template<typename T>
> +       void updateEntry(uint32_t tag, std::vector<T> &data)
> +       {
> +               updateEntry(tag, data.data(), data.size(), sizeof(T));
> +       }
> +
> +       template<typename T>
> +       void updateEntry(uint32_t tag, const T *data, size_t count)
> +       {
> +               updateEntry(tag, data, count, sizeof(T));
> +       }
> +
> +       void addEntry(uint32_t tag, const void *data, size_t count, size_t
> sizeofT);
> +       void updateEntry(uint32_t tag, const void *data, size_t count,
> size_t sizeofT);
>
>         camera_metadata_t *get();
>         const camera_metadata_t *get() const;
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Hi Paul,

Thanks for this awesome work.

On 2021-05-12 19:25:39 +0900, Paul Elder wrote:
> Previously we had to manually declare the size of CameraMetadata on
> allocation, and its count could not be changed after construction.
> Change CameraMetadata's behavior so that the user can simply add or
> update entries, and the CameraMetadata will auto-resize (double the
> size) as necessary. Also remove everything involved with calculating
> the initial size for any CameraMetadata instances.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

With the Span improvement suggested by Hiro addressed,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Also a small not below in case you feel it's a good idea.

> 
> ---
> Changes in v3:
> - dependency on "FULL hardware level fixes" removed
> - split addEntry and updateEntry
>   - i think the lookup on every (merged) addEntry isn't worth it, since
>     the user probably knows if they need to add or update
> - add auto-resize (and corresponding check) to updateEntry as well
> - add the templates and overloads to updateEntry as well
> - increase the default size for the static metadata to be above what we
>   had before
> - valid_ is now used to indicate a failed addEntry/updateEntry in
>   addition to metadata_ == nullptr. this way the user doesn't have to
>   check the return value of every single addEntry/updateEntry call, and
>   can just check valid_ at the end
> - print resize when it happens
> - updateEntry resizes if the new data size is greater than the old data
> 
> Question: I think on failure of addEntry/updateEntry the metadata could
> still be "valid", it's just that the content isn't what the user
> expects. Should we keep valid_ as I've repurposed it here in v3, or
> should we add another flag?
> 
> Changes in v2:
> - add overloading of addEntry
> - better logging with the separation of core code vs template code
> - addEntry failure makes the metadata invalid
> 
> The main feature of v2 is the overloading of addEntry. I have three
> overloads and the main one:
> - tag, single piece of data (reference)
> - tag, vector (v3: s/span/vector/)
> - tag, data pointer, count
> - tag, data pointer, count, size of one instance of the data
> 
> The last one is the main one, and is not templated. The first two are
> nice and convenient, because now we can do things like:
> 
> uint32_t orientation = 0;
> resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, orientation);
> 
> and
> 
> std::vector<uint32_t> availableStates = { SOME_STATE_1, SOME_STATE_2 };
> metadata->addEntry(ANDROID_AVAILABLE_STATES, availableStates);
> 
> The third is for when android metadata is copied directly from another
> piece of android metadata. This is very common, and wasn't very nice
> having to wrap everything in a Span. All the old calls are
> backward-compatible using this overload.
> ---
>  src/android/camera_device.cpp   | 47 +-----------------
>  src/android/camera_device.h     |  1 -
>  src/android/camera_metadata.cpp | 84 +++++++++++++++++++++++++++++----
>  src/android/camera_metadata.h   | 44 ++++++++++++++++-
>  4 files changed, 117 insertions(+), 59 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 7d4d0feb..74f6915c 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -773,43 +773,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: 54 entries, 874 bytes of static metadata
> -	 */
> -	uint32_t numEntries = 54;
> -	uint32_t byteSize = 874;
> -
> -	/*
> -	 * 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.
>   */
> @@ -818,15 +781,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>(64, 1024);
>  	if (!staticMetadata_->isValid()) {
>  		LOG(HAL, Error) << "Failed to allocate static metadata";
>  		staticMetadata_.reset();
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 23457e47..ae162a45 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..6394fd74 100644
> --- a/src/android/camera_metadata.cpp
> +++ b/src/android/camera_metadata.cpp
> @@ -63,14 +63,67 @@ 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)
> +/*
> + * \brief Resize the metadata container, if necessary
> + * \param[in] count Number of entries to add to the container
> + * \param[in] size Total size of entries to add, in bytes
> + * \return True if resize was successful or unnecessary, false otherwise
> + */
> +bool CameraMetadata::resize(size_t count, size_t size)
>  {
>  	if (!valid_)
>  		return false;
>  
> -	if (!add_camera_metadata_entry(metadata_, tag, data, count))
> +	if (!count && !size)
>  		return true;
>  
> +	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;
> +		}
> +
> +		LOG(CameraMetadata, Info)
> +			<< "Resized: old entry capacity " << currentEntryCapacity
> +			<< ", old data capacity " << currentDataCapacity
> +			<< ", new entry capacity " << newEntryCapacity
> +			<< ", new data capacity " << newDataCapacity;
> +
> +		append_camera_metadata(metadata_, oldMetadata);
> +		free_camera_metadata(oldMetadata);
> +	}
> +
> +	return true;
> +}
> +
> +void CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,
> +			      size_t sizeofT)
> +{
> +	if (!valid_)
> +		return;
> +
> +	if (!resize(1, count * sizeofT)) {
> +		LOG(CameraMetadata, Error) << "Failed to resize";
> +		valid_ = false;
> +		return;
> +	}
> +
> +	if (!add_camera_metadata_entry(metadata_, tag, data, count))
> +		return;
> +
>  	const char *name = get_camera_metadata_tag_name(tag);
>  	if (name)
>  		LOG(CameraMetadata, Error)
> @@ -80,14 +133,13 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
>  			<< "Failed to add unknown tag " << tag;
>  
>  	valid_ = false;
> -
> -	return false;
>  }
>  
> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
> +void CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count,
> +				 size_t sizeofT)

nit: Would it hurt to keep the bool return value?

>  {
>  	if (!valid_)
> -		return false;
> +		return;
>  
>  	camera_metadata_entry_t entry;
>  	int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> @@ -96,7 +148,20 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
>  		LOG(CameraMetadata, Error)
>  			<< "Failed to update tag "
>  			<< (name ? name : "<unknown>") << ": not present";
> -		return false;
> +		return;
> +	}
> +
> +	size_t oldSize =
> +		calculate_camera_metadata_entry_data_size(entry.type,
> +							  entry.count);
> +	size_t newSize =
> +		calculate_camera_metadata_entry_data_size(entry.type,
> +							  count * sizeofT);
> +	size_t sizeIncrement = newSize - oldSize > 0 ? newSize - oldSize : 0;
> +	if (!resize(0, count * sizeIncrement)) {
> +		LOG(CameraMetadata, Error) << "Failed to resize";
> +		valid_ = false;
> +		return;
>  	}
>  
>  	ret = update_camera_metadata_entry(metadata_, entry.index, data,
> @@ -105,10 +170,9 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
>  		const char *name = get_camera_metadata_tag_name(tag);
>  		LOG(CameraMetadata, Error)
>  			<< "Failed to update tag " << (name ? name : "<unknown>");
> -		return false;
> -	}
>  
> -	return true;
> +		valid_ = false;
> +	}
>  }
>  
>  camera_metadata_t *CameraMetadata::get()
> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> index d653e2f0..aa875fe4 100644
> --- a/src/android/camera_metadata.h
> +++ b/src/android/camera_metadata.h
> @@ -8,6 +8,7 @@
>  #define __ANDROID_CAMERA_METADATA_H__
>  
>  #include <stdint.h>
> +#include <vector>
>  
>  #include <system/camera_metadata.h>
>  
> @@ -23,9 +24,48 @@ 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,
> +		 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +	void addEntry(uint32_t tag, const T &data)
> +	{
> +		addEntry(tag, &data, 1, sizeof(T));
> +	}
> +
> +	template<typename T>
> +	void addEntry(uint32_t tag, std::vector<T> &data)
> +	{
> +		addEntry(tag, data.data(), data.size(), sizeof(T));
> +	}
> +
> +	template<typename T>
> +	void addEntry(uint32_t tag, const T *data, size_t count)
> +	{
> +		addEntry(tag, data, count, sizeof(T));
> +	}
> +
> +	template<typename T>
> +	void updateEntry(uint32_t tag, const T &data)
> +	{
> +		updateEntry(tag, &data, 1, sizeof(T));
> +	}
> +
> +	template<typename T>
> +	void updateEntry(uint32_t tag, std::vector<T> &data)
> +	{
> +		updateEntry(tag, data.data(), data.size(), sizeof(T));
> +	}
> +
> +	template<typename T>
> +	void updateEntry(uint32_t tag, const T *data, size_t count)
> +	{
> +		updateEntry(tag, data, count, sizeof(T));
> +	}
> +
> +	void addEntry(uint32_t tag, const void *data, size_t count, size_t sizeofT);
> +	void updateEntry(uint32_t tag, const void *data, size_t count, size_t sizeofT);
>  
>  	camera_metadata_t *get();
>  	const camera_metadata_t *get() const;
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Hiro,

Thank you for the review.

On Thu, May 13, 2021 at 12:26:29PM +0900, Hirokazu Honda wrote:
> Hi Paul, thank you for the patch.
> 
> On Wed, May 12, 2021 at 7:25 PM Paul Elder <paul.elder@ideasonboard.com> wrote:
> 
>     Previously we had to manually declare the size of CameraMetadata on
>     allocation, and its count could not be changed after construction.
>     Change CameraMetadata's behavior so that the user can simply add or
>     update entries, and the CameraMetadata will auto-resize (double the
>     size) as necessary. Also remove everything involved with calculating
>     the initial size for any CameraMetadata instances.
> 
>     Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
>     ---
>     Changes in v3:
>     - dependency on "FULL hardware level fixes" removed
>     - split addEntry and updateEntry
>       - i think the lookup on every (merged) addEntry isn't worth it, since
>         the user probably knows if they need to add or update
>     - add auto-resize (and corresponding check) to updateEntry as well
>     - add the templates and overloads to updateEntry as well
>     - increase the default size for the static metadata to be above what we
>       had before
>     - valid_ is now used to indicate a failed addEntry/updateEntry in
>       addition to metadata_ == nullptr. this way the user doesn't have to
>       check the return value of every single addEntry/updateEntry call, and
>       can just check valid_ at the end
>     - print resize when it happens
>     - updateEntry resizes if the new data size is greater than the old data
> 
>     Question: I think on failure of addEntry/updateEntry the metadata could
>     still be "valid", it's just that the content isn't what the user
>     expects. Should we keep valid_ as I've repurposed it here in v3, or
>     should we add another flag?
> 
>     Changes in v2:
>     - add overloading of addEntry
>     - better logging with the separation of core code vs template code
>     - addEntry failure makes the metadata invalid
> 
>     The main feature of v2 is the overloading of addEntry. I have three
>     overloads and the main one:
>     - tag, single piece of data (reference)
>     - tag, vector (v3: s/span/vector/)
>     - tag, data pointer, count
>     - tag, data pointer, count, size of one instance of the data
> 
>     The last one is the main one, and is not templated. The first two are
>     nice and convenient, because now we can do things like:
> 
>     uint32_t orientation = 0;
>     resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, orientation);
> 
>     and
> 
>     std::vector<uint32_t> availableStates = { SOME_STATE_1, SOME_STATE_2 };
>     metadata->addEntry(ANDROID_AVAILABLE_STATES, availableStates);
> 
>     The third is for when android metadata is copied directly from another
>     piece of android metadata. This is very common, and wasn't very nice
>     having to wrap everything in a Span. All the old calls are
>     backward-compatible using this overload.
>     ---
>      src/android/camera_device.cpp   | 47 +-----------------
>      src/android/camera_device.h     |  1 -
>      src/android/camera_metadata.cpp | 84 +++++++++++++++++++++++++++++----
>      src/android/camera_metadata.h   | 44 ++++++++++++++++-
>      4 files changed, 117 insertions(+), 59 deletions(-)
> 
>     diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>     index 7d4d0feb..74f6915c 100644
>     --- a/src/android/camera_device.cpp
>     +++ b/src/android/camera_device.cpp
>     @@ -773,43 +773,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: 54 entries, 874 bytes of static metadata
>     -        */
>     -       uint32_t numEntries = 54;
>     -       uint32_t byteSize = 874;
>     -
>     -       /*
>     -        * 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.
>       */
>     @@ -818,15 +781,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>(64, 1024);
>             if (!staticMetadata_->isValid()) {
>                     LOG(HAL, Error) << "Failed to allocate static metadata";
>                     staticMetadata_.reset();
>     diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>     index 23457e47..ae162a45 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..6394fd74 100644
>     --- a/src/android/camera_metadata.cpp
>     +++ b/src/android/camera_metadata.cpp
>     @@ -63,14 +63,67 @@ 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)
>     +/*
>     + * \brief Resize the metadata container, if necessary
>     + * \param[in] count Number of entries to add to the container
>     + * \param[in] size Total size of entries to add, in bytes
>     + * \return True if resize was successful or unnecessary, false otherwise
>     + */
>     +bool CameraMetadata::resize(size_t count, size_t size)
>      {
>             if (!valid_)
>                     return false;
> 
>     -       if (!add_camera_metadata_entry(metadata_, tag, data, count))
>     +       if (!count && !size)
>                     return true;
> 
>     +       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;
>     +               }
>     +
>     +               LOG(CameraMetadata, Info)
>     +                       << "Resized: old entry capacity " <<
>     currentEntryCapacity
>     +                       << ", old data capacity " << currentDataCapacity
>     +                       << ", new entry capacity " << newEntryCapacity
>     +                       << ", new data capacity " << newDataCapacity;
>     +
>     +               append_camera_metadata(metadata_, oldMetadata);
>     +               free_camera_metadata(oldMetadata);
>     +       }
>     +
>     +       return true;
>     +}
>     +
>     +void CameraMetadata::addEntry(uint32_t tag, const void *data, size_t
>     count,
>     +                             size_t sizeofT)
>     +{
>     +       if (!valid_)
>     +               return;
>     +
>     +       if (!resize(1, count * sizeofT)) {
>     +               LOG(CameraMetadata, Error) << "Failed to resize";
>     +               valid_ = false;
>     +               return;
>     +       }
>     +
>     +       if (!add_camera_metadata_entry(metadata_, tag, data, count))
>     +               return;
>     +
>             const char *name = get_camera_metadata_tag_name(tag);
>             if (name)
>                     LOG(CameraMetadata, Error)
>     @@ -80,14 +133,13 @@ bool CameraMetadata::addEntry(uint32_t tag, const void
>     *data, size_t count)
>                             << "Failed to add unknown tag " << tag;
> 
>             valid_ = false;
>     -
>     -       return false;
>      }
> 
>     -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t
>     count)
>     +void CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t
>     count,
>     +                                size_t sizeofT)
>      {
>             if (!valid_)
>     -               return false;
>     +               return;
> 
>             camera_metadata_entry_t entry;
>             int ret = find_camera_metadata_entry(metadata_, tag, &entry);
>     @@ -96,7 +148,20 @@ bool CameraMetadata::updateEntry(uint32_t tag, const
>     void *data, size_t count)
>                     LOG(CameraMetadata, Error)
>                             << "Failed to update tag "
>                             << (name ? name : "<unknown>") << ": not present";
>     -               return false;
>     +               return;
>     +       }
>     +
>     +       size_t oldSize =
>     +               calculate_camera_metadata_entry_data_size(entry.type,
>     +                                                         entry.count);
>     +       size_t newSize =
>     +               calculate_camera_metadata_entry_data_size(entry.type,
>     +                                                         count * sizeofT);
> 
> 
> I am confused by this.
> calculate_camera_metadata_entry_data_size() looks up the size of one element
> for the type and computes the required size multiplying by the count.
> That is, newSize seems to be sizeofT * count * sizeofT.
> Am I missing something?

Ah you're right. I've removed the extra multiplication.

It's a bit confusing whether "count" means "number of bytes" or "number
of entries" :/

> 
> 
>     +       size_t sizeIncrement = newSize - oldSize > 0 ? newSize - oldSize :
>     0;
>     +       if (!resize(0, count * sizeIncrement)) {
>     +               LOG(CameraMetadata, Error) << "Failed to resize";
>     +               valid_ = false;
>     +               return;
>             }
> 
>             ret = update_camera_metadata_entry(metadata_, entry.index, data,
>     @@ -105,10 +170,9 @@ bool CameraMetadata::updateEntry(uint32_t tag, const
>     void *data, size_t count)
>                     const char *name = get_camera_metadata_tag_name(tag);
>                     LOG(CameraMetadata, Error)
>                             << "Failed to update tag " << (name ? name : "
>     <unknown>");
>     -               return false;
>     -       }
> 
>     -       return true;
>     +               valid_ = false;
>     +       }
>      }
> 
>      camera_metadata_t *CameraMetadata::get()
>     diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
>     index d653e2f0..aa875fe4 100644
>     --- a/src/android/camera_metadata.h
>     +++ b/src/android/camera_metadata.h
>     @@ -8,6 +8,7 @@
>      #define __ANDROID_CAMERA_METADATA_H__
> 
>      #include <stdint.h>
>     +#include <vector>
> 
>      #include <system/camera_metadata.h>
> 
>     @@ -23,9 +24,48 @@ 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,
>     +                std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
>     +       void addEntry(uint32_t tag, const T &data)
>     +       {
>     +               addEntry(tag, &data, 1, sizeof(T));
>     +       }
>     +
>     +       template<typename T>
>     +       void addEntry(uint32_t tag, std::vector<T> &data)
>     +       {
>     +               addEntry(tag, data.data(), data.size(), sizeof(T));
>     +       }
>     +
> 
> 
> s/std::vector<T>/Span<const T>/
> ditto to updateEntry().

I got ‘std::vector<int>’ is not derived from ‘libcamera::Span<const T>’
when I try that, and the template engine fails to match it.

I've used:

template<typename S, typename T = typename S::value_type>
bool addEntry(uint32_t tag, S &data)

instead, which works for STL containers but not for C arrays, since this
uses data.data() and data.size().

Or maybe (I've just sent v4...) I could add another overload for C
arrays and then convert to Span inside...?


Thanks,

Paul


> 
> We can use a row array, for example, for aeAvailableAntiBandingModes.
> 
> 
>     +       template<typename T>
>     +       void addEntry(uint32_t tag, const T *data, size_t count)
>     +       {
>     +               addEntry(tag, data, count, sizeof(T));
>     +       }
>     +
>     +       template<typename T>
>     +       void updateEntry(uint32_t tag, const T &data)
>     +       {
>     +               updateEntry(tag, &data, 1, sizeof(T));
>     +       }
>     +
>     +       template<typename T>
>     +       void updateEntry(uint32_t tag, std::vector<T> &data)
>     +       {
>     +               updateEntry(tag, data.data(), data.size(), sizeof(T));
>     +       }
>     +
>     +       template<typename T>
>     +       void updateEntry(uint32_t tag, const T *data, size_t count)
>     +       {
>     +               updateEntry(tag, data, count, sizeof(T));
>     +       }
>     +
>     +       void addEntry(uint32_t tag, const void *data, size_t count, size_t
>     sizeofT);
>     +       void updateEntry(uint32_t tag, const void *data, size_t count,
>     size_t sizeofT);
> 
>             camera_metadata_t *get();
>             const camera_metadata_t *get() const;
>     --
>     2.27.0