[libcamera-devel] [RFC PATCH] android: camera_metadata: Auto-resize CameraMetadata
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri May 7 14:06:36 CEST 2021
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 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?
> >
> > 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.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list