[libcamera-devel] [RFC PATCH] android: camera_metadata: Auto-resize CameraMetadata
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 6 13:14:27 CEST 2021
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 at 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;
>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list