<div dir="ltr"><div dir="ltr">Hi Paul, thank you for the patch.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 12, 2021 at 7:25 PM Paul Elder <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Previously we had to manually declare the size of CameraMetadata on<br>
allocation, and its count could not be changed after construction.<br>
Change CameraMetadata's behavior so that the user can simply add or<br>
update entries, and the CameraMetadata will auto-resize (double the<br>
size) as necessary. Also remove everything involved with calculating<br>
the initial size for any CameraMetadata instances.<br>
<br>
Signed-off-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
<br>
---<br>
Changes in v3:<br>
- dependency on "FULL hardware level fixes" removed<br>
- split addEntry and updateEntry<br>
- i think the lookup on every (merged) addEntry isn't worth it, since<br>
the user probably knows if they need to add or update<br>
- add auto-resize (and corresponding check) to updateEntry as well<br>
- add the templates and overloads to updateEntry as well<br>
- increase the default size for the static metadata to be above what we<br>
had before<br>
- valid_ is now used to indicate a failed addEntry/updateEntry in<br>
addition to metadata_ == nullptr. this way the user doesn't have to<br>
check the return value of every single addEntry/updateEntry call, and<br>
can just check valid_ at the end<br>
- print resize when it happens<br>
- updateEntry resizes if the new data size is greater than the old data<br>
<br>
Question: I think on failure of addEntry/updateEntry the metadata could<br>
still be "valid", it's just that the content isn't what the user<br>
expects. Should we keep valid_ as I've repurposed it here in v3, or<br>
should we add another flag?<br>
<br>
Changes in v2:<br>
- add overloading of addEntry<br>
- better logging with the separation of core code vs template code<br>
- addEntry failure makes the metadata invalid<br>
<br>
The main feature of v2 is the overloading of addEntry. I have three<br>
overloads and the main one:<br>
- tag, single piece of data (reference)<br>
- tag, vector (v3: s/span/vector/)<br>
- tag, data pointer, count<br>
- tag, data pointer, count, size of one instance of the data<br>
<br>
The last one is the main one, and is not templated. The first two are<br>
nice and convenient, because now we can do things like:<br>
<br>
uint32_t orientation = 0;<br>
resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, orientation);<br>
<br>
and<br>
<br>
std::vector<uint32_t> availableStates = { SOME_STATE_1, SOME_STATE_2 };<br>
metadata->addEntry(ANDROID_AVAILABLE_STATES, availableStates);<br>
<br>
The third is for when android metadata is copied directly from another<br>
piece of android metadata. This is very common, and wasn't very nice<br>
having to wrap everything in a Span. All the old calls are<br>
backward-compatible using this overload.<br>
---<br>
src/android/camera_device.cpp | 47 +-----------------<br>
src/android/camera_device.h | 1 -<br>
src/android/camera_metadata.cpp | 84 +++++++++++++++++++++++++++++----<br>
src/android/camera_metadata.h | 44 ++++++++++++++++-<br>
4 files changed, 117 insertions(+), 59 deletions(-)<br>
<br>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
index 7d4d0feb..74f6915c 100644<br>
--- a/src/android/camera_device.cpp<br>
+++ b/src/android/camera_device.cpp<br>
@@ -773,43 +773,6 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)<br>
callbacks_ = callbacks;<br>
}<br>
<br>
-std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()<br>
-{<br>
- /*<br>
- * \todo Keep this in sync with the actual number of entries.<br>
- * Currently: 54 entries, 874 bytes of static metadata<br>
- */<br>
- uint32_t numEntries = 54;<br>
- uint32_t byteSize = 874;<br>
-<br>
- /*<br>
- * Calculate space occupation in bytes for dynamically built metadata<br>
- * entries.<br>
- *<br>
- * Each stream configuration entry requires 48 bytes:<br>
- * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS<br>
- * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS<br>
- */<br>
- byteSize += streamConfigurations_.size() * 48;<br>
-<br>
- /*<br>
- * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes<br>
- * 2 32bits integers for the (0, 0) thumbnail size<br>
- *<br>
- * This is a worst case estimates as different configurations with the<br>
- * same aspect ratio will generate the same size.<br>
- */<br>
- for (const auto &entry : streamConfigurations_) {<br>
- if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)<br>
- continue;<br>
-<br>
- byteSize += 8;<br>
- }<br>
- byteSize += 8;<br>
-<br>
- return std::make_tuple(numEntries, byteSize);<br>
-}<br>
-<br>
/*<br>
* Return static information for the camera.<br>
*/<br>
@@ -818,15 +781,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()<br>
if (staticMetadata_)<br>
return staticMetadata_->get();<br>
<br>
- /*<br>
- * The here reported metadata are enough to implement a basic capture<br>
- * example application, but a real camera implementation will require<br>
- * more.<br>
- */<br>
- uint32_t numEntries;<br>
- uint32_t byteSize;<br>
- std::tie(numEntries, byteSize) = calculateStaticMetadataSize();<br>
- staticMetadata_ = std::make_unique<CameraMetadata>(numEntries, byteSize);<br>
+ staticMetadata_ = std::make_unique<CameraMetadata>(64, 1024);<br>
if (!staticMetadata_->isValid()) {<br>
LOG(HAL, Error) << "Failed to allocate static metadata";<br>
staticMetadata_.reset();<br>
diff --git a/src/android/camera_device.h b/src/android/camera_device.h<br>
index 23457e47..ae162a45 100644<br>
--- a/src/android/camera_device.h<br>
+++ b/src/android/camera_device.h<br>
@@ -98,7 +98,6 @@ private:<br>
std::vector<libcamera::Size><br>
getRawResolutions(const libcamera::PixelFormat &pixelFormat);<br>
<br>
- std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();<br>
libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);<br>
void notifyShutter(uint32_t frameNumber, uint64_t timestamp);<br>
void notifyError(uint32_t frameNumber, camera3_stream_t *stream);<br>
diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp<br>
index 6f1bcdbe..6394fd74 100644<br>
--- a/src/android/camera_metadata.cpp<br>
+++ b/src/android/camera_metadata.cpp<br>
@@ -63,14 +63,67 @@ bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) c<br>
return true;<br>
}<br>
<br>
-bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)<br>
+/*<br>
+ * \brief Resize the metadata container, if necessary<br>
+ * \param[in] count Number of entries to add to the container<br>
+ * \param[in] size Total size of entries to add, in bytes<br>
+ * \return True if resize was successful or unnecessary, false otherwise<br>
+ */<br>
+bool CameraMetadata::resize(size_t count, size_t size)<br>
{<br>
if (!valid_)<br>
return false;<br>
<br>
- if (!add_camera_metadata_entry(metadata_, tag, data, count))<br>
+ if (!count && !size)<br>
return true;<br>
<br>
+ size_t currentEntryCount = get_camera_metadata_entry_count(metadata_);<br>
+ size_t currentEntryCapacity = get_camera_metadata_entry_capacity(metadata_);<br>
+ size_t newEntryCapacity = currentEntryCapacity < currentEntryCount + count ?<br>
+ currentEntryCapacity * 2 : currentEntryCapacity;<br>
+<br>
+ size_t currentDataCount = get_camera_metadata_data_count(metadata_);<br>
+ size_t currentDataCapacity = get_camera_metadata_data_capacity(metadata_);<br>
+ size_t newDataCapacity = currentDataCapacity < currentDataCount + size ?<br>
+ currentDataCapacity * 2 : currentDataCapacity;<br>
+<br>
+ if (newEntryCapacity > currentEntryCapacity ||<br>
+ newDataCapacity > currentDataCapacity) {<br>
+ camera_metadata_t *oldMetadata = metadata_;<br>
+ metadata_ = allocate_camera_metadata(newEntryCapacity, newDataCapacity);<br>
+ if (!metadata_) {<br>
+ metadata_ = oldMetadata;<br>
+ return false;<br>
+ }<br>
+<br>
+ LOG(CameraMetadata, Info)<br>
+ << "Resized: old entry capacity " << currentEntryCapacity<br>
+ << ", old data capacity " << currentDataCapacity<br>
+ << ", new entry capacity " << newEntryCapacity<br>
+ << ", new data capacity " << newDataCapacity;<br>
+<br>
+ append_camera_metadata(metadata_, oldMetadata);<br>
+ free_camera_metadata(oldMetadata);<br>
+ }<br>
+<br>
+ return true;<br>
+}<br>
+<br>
+void CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,<br>
+ size_t sizeofT)<br>
+{<br>
+ if (!valid_)<br>
+ return;<br>
+<br>
+ if (!resize(1, count * sizeofT)) {<br>
+ LOG(CameraMetadata, Error) << "Failed to resize";<br>
+ valid_ = false;<br>
+ return;<br>
+ }<br>
+<br>
+ if (!add_camera_metadata_entry(metadata_, tag, data, count))<br>
+ return;<br>
+<br>
const char *name = get_camera_metadata_tag_name(tag);<br>
if (name)<br>
LOG(CameraMetadata, Error)<br>
@@ -80,14 +133,13 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)<br>
<< "Failed to add unknown tag " << tag;<br>
<br>
valid_ = false;<br>
-<br>
- return false;<br>
}<br>
<br>
-bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)<br>
+void CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count,<br>
+ size_t sizeofT)<br>
{<br>
if (!valid_)<br>
- return false;<br>
+ return;<br>
<br>
camera_metadata_entry_t entry;<br>
int ret = find_camera_metadata_entry(metadata_, tag, &entry);<br>
@@ -96,7 +148,20 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)<br>
LOG(CameraMetadata, Error)<br>
<< "Failed to update tag "<br>
<< (name ? name : "<unknown>") << ": not present";<br>
- return false;<br>
+ return;<br>
+ }<br>
+<br>
+ size_t oldSize =<br>
+ calculate_camera_metadata_entry_data_size(entry.type,<br>
+ entry.count);<br>
+ size_t newSize =<br>
+ calculate_camera_metadata_entry_data_size(entry.type,<br>
+ count * sizeofT);<br></blockquote><div><br></div><div>I am confused by this.</div><div>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.</div><div>That is, newSize seems to be sizeofT * count * sizeofT.</div><div>Am I missing something?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ size_t sizeIncrement = newSize - oldSize > 0 ? newSize - oldSize : 0;<br>
+ if (!resize(0, count * sizeIncrement)) {<br>
+ LOG(CameraMetadata, Error) << "Failed to resize";<br>
+ valid_ = false;<br>
+ return;<br>
}<br>
<br>
ret = update_camera_metadata_entry(metadata_, entry.index, data,<br>
@@ -105,10 +170,9 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)<br>
const char *name = get_camera_metadata_tag_name(tag);<br>
LOG(CameraMetadata, Error)<br>
<< "Failed to update tag " << (name ? name : "<unknown>");<br>
- return false;<br>
- }<br>
<br>
- return true;<br>
+ valid_ = false;<br>
+ }<br>
}<br>
<br>
camera_metadata_t *CameraMetadata::get()<br>
diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h<br>
index d653e2f0..aa875fe4 100644<br>
--- a/src/android/camera_metadata.h<br>
+++ b/src/android/camera_metadata.h<br>
@@ -8,6 +8,7 @@<br>
#define __ANDROID_CAMERA_METADATA_H__<br>
<br>
#include <stdint.h><br>
+#include <vector><br>
<br>
#include <system/camera_metadata.h><br>
<br>
@@ -23,9 +24,48 @@ public:<br>
CameraMetadata &operator=(const CameraMetadata &other);<br>
<br>
bool isValid() const { return valid_; }<br>
+ bool resize(size_t count, size_t size);<br>
bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;<br>
- bool addEntry(uint32_t tag, const void *data, size_t data_count);<br>
- bool updateEntry(uint32_t tag, const void *data, size_t data_count);<br>
+<br>
+ template<typename T,<br>
+ std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr><br>
+ void addEntry(uint32_t tag, const T &data)<br>
+ {<br>
+ addEntry(tag, &data, 1, sizeof(T));<br>
+ }<br>
+<br>
+ template<typename T><br>
+ void addEntry(uint32_t tag, std::vector<T> &data)<br>
+ {<br>
+ addEntry(tag, data.data(), data.size(), sizeof(T));<br>
+ }<br>
+<br></blockquote><div><br></div><div>s/std::vector<T>/Span<const T>/</div><div>ditto to updateEntry().</div><div><br></div><div>We can use a row array, for example, for aeAvailableAntiBandingModes.</div><div><br></div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ template<typename T><br>
+ void addEntry(uint32_t tag, const T *data, size_t count)<br>
+ {<br>
+ addEntry(tag, data, count, sizeof(T));<br>
+ }<br>
+<br>
+ template<typename T><br>
+ void updateEntry(uint32_t tag, const T &data)<br>
+ {<br>
+ updateEntry(tag, &data, 1, sizeof(T));<br>
+ }<br>
+<br>
+ template<typename T><br>
+ void updateEntry(uint32_t tag, std::vector<T> &data)<br>
+ {<br>
+ updateEntry(tag, data.data(), data.size(), sizeof(T));<br>
+ }<br>
+<br>
+ template<typename T><br>
+ void updateEntry(uint32_t tag, const T *data, size_t count)<br>
+ {<br>
+ updateEntry(tag, data, count, sizeof(T));<br>
+ }<br>
+<br>
+ void addEntry(uint32_t tag, const void *data, size_t count, size_t sizeofT);<br>
+ void updateEntry(uint32_t tag, const void *data, size_t count, size_t sizeofT);<br>
<br>
camera_metadata_t *get();<br>
const camera_metadata_t *get() const;<br>
-- <br>
2.27.0<br>
<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>