[libcamera-devel] [PATCH v3 1/3] android: camera_metadata: Auto-resize CameraMetadata
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu May 13 10:40:52 CEST 2021
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 at ideasonboard.com>
With the Span improvement suggested by Hiro addressed,
Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list