<div dir="ltr"><div dir="ltr">Hi Paul,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 6, 2021 at 9:20 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">Hello,<br>
<br>
On Thu, May 06, 2021 at 12:14:27PM +0100, Kieran Bingham wrote:<br>
> On 06/05/2021 12:05, Kieran Bingham wrote:<br>
> > On 06/05/2021 11:41, Paul Elder wrote:<br>
> >> This patch depends on the series "FULL hardware level fixes".<br>
> >><br>
> >> Previously we had to manually declare the size of CameraMetadata on<br>
> >> allocation, and its count not be changed after construction. Change<br>
> > <br>
> > count could not<br>
> > <br>
> >> CameraMetadata's behavior so that the user can simply add entries, and<br>
> >> the CameraMetadata will auto-resize (double the size) as necessary. At<br>
> >> the same time, make addEntry() also serve the purpose of updateEntry(),<br>
> >> and remove updateEntry(). Also remove everything involved with<br>
> >> calculating the initial size for any CameraMetadata instances.<br>
> > <br>
> > Oh that sweet music to my ears ...<br>
> > <br>
> >> Signed-off-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
> >><br>
> >> ---<br>
> >> 1 - What do you (plural) think about merging updateEntry() into<br>
> >> addEntry()? I thought that with the dynamic allocation it would be<br>
> >> convenient to use one function. Is there any reason that we should<br>
> >> keep them separate, or is it fine to merge them?<br>
> > <br>
> > What's the current distinction?<br>
> > Presumably addEntry requires that the entry doesn't yet exist, whereas<br>
> > updateEntry would modify an existing entry?<br>
<br>
There may be a performance difference on the backend if we keep them<br>
separate, so please have a look at that. If we merge them, I'd call the<br>
resulting function setEntry(). We could even experiment with some sort<br>
of operator[] syntax.<br>
<br>
> > For naming, if we were merging both, wouldn't it be better to call it<br>
> > '->set(id, value)'?<br>
> > <br>
> >> 2 - How can I get logging working in the CameraMetadata header file? The<br>
> >> last time I did that was in ipa_data_serializer.h and that wasn't very<br>
> >> pretty either...<br>
> <br>
> Looked like there was already a LOG usage here, so you shoudl be able to<br>
> use that I think.<br>
> <br>
> One last set of thoughts.<br>
> <br>
> It might be interesting to keep a flag 'resized' so you know if a resize<br>
> did occur, and a mechanism to print out what the (final) storages were,<br>
> so that even if it were a manual process, someone could hardcode in a<br>
> better / optimal reservation at the start which would prevent resizes<br>
> occurring in the first place.<br>
<br>
Make sense, instrumentation is important. We don't have to implement it<br>
all yet, but designing the API accordingly is important.<br>
<br>
> >> (I haven't tested it on CTS yet; this is just RFC for the API and<br>
> >> implementation)<br>
> >> ---<br>
> >> src/android/camera_device.cpp | 79 +++++----------------------------<br>
> >> src/android/camera_device.h | 1 -<br>
> >> src/android/camera_metadata.cpp | 61 +++++++++----------------<br>
> >> src/android/camera_metadata.h | 39 +++++++++++++++-<br>
> >> 4 files changed, 71 insertions(+), 109 deletions(-)<br>
> >><br>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> >> index 114348a6..426e3fcd 100644<br>
> >> --- a/src/android/camera_device.cpp<br>
> >> +++ b/src/android/camera_device.cpp<br>
> >> @@ -772,53 +772,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: 63 entries, 1014 bytes of static metadata<br>
> >> - */<br>
> >> - uint32_t numEntries = 63;<br>
> >> - uint32_t byteSize = 1014;<br>
> >> -<br>
> >> - // do i need to add for entries in the available keys?<br>
> >> - // +1, +4 for EDGE_AVAILABLE_EDGE_MODES<br>
> >> - // +1, +4 for LENS_INFO_AVAILABLE_FILTER_DENSITIES<br>
> >> - // +1, +4 for BLACK_LEVEL_PATTERN<br>
> >> - // +1, +4 for TONEMAP_AVAILABLE_TONE_MAP_MODES<br>
> >> - // +1, +4 for TONEMAP_MAX_CURVE_POINTS<br>
> >> - // +4x9 = 36 for the new result tags<br>
> >> -<br>
> >> - // +36 for new request keys<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>
> >> @@ -827,15 +780,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>(32, 512);<br></blockquote><div><br></div><div>Shall we add a comment where 32 and 512 come from.<br>I guess this is the result of calculateStaticMetadataSize() today.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > <br>
> > Presumably the initial count of entries is cheap to allocate, but the<br>
> > storage size is more expensive. .. is there a distinction on the two?<br>
<br>
Compared to memory requirements related to image storage, neither should<br>
be very expensive.<br>
<br>
> > I presume I'll see later ...<br>
> > <br>
> >> if (!staticMetadata_->isValid()) {<br>
> >> LOG(HAL, Error) << "Failed to allocate static metadata";<br>
> >> staticMetadata_.reset();<br>
> >> @@ -1757,13 +1702,13 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()<br>
> >> &entry);<br>
> >> <br>
> >> uint8_t edgeMode = ANDROID_EDGE_MODE_FAST;<br>
> >> - previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);<br>
> >> + previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);<br>
> >> <br>
> >> /*<br>
> >> * Assume the AE_AVAILABLE_TARGET_FPS_RANGE static metadata<br>
> >> * has been assembled as {{min, max} {max, max}}.<br>
> >> */<br>
> >> - previewTemplate->updateEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,<br>
> >> + previewTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,<br>
> >> entry.data.i32 + 2, 2);<br>
> >> <br>
> >> return previewTemplate;<br>
> >> @@ -1780,17 +1725,17 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateStill()<br>
> >> * HIGH_QUALITY.<br>
> >> */<br>
> >> uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;<br>
> >> - previewTemplate->updateEntry(ANDROID_NOISE_REDUCTION_MODE,<br>
> >> + previewTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,<br>
> >> &noiseReduction, 1);<br>
> >> <br>
> >> uint8_t edgeMode = ANDROID_EDGE_MODE_HIGH_QUALITY;<br>
> >> - previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);<br>
> >> + previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);<br>
> >> <br>
> >> uint8_t shadingMode = ANDROID_SHADING_MODE_HIGH_QUALITY;<br>
> >> - previewTemplate->updateEntry(ANDROID_SHADING_MODE, &shadingMode, 1);<br>
> >> + previewTemplate->addEntry(ANDROID_SHADING_MODE, &shadingMode, 1);<br>
> >> <br>
> >> uint8_t tonemapMode = ANDROID_TONEMAP_MODE_HIGH_QUALITY;<br>
> >> - previewTemplate->updateEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);<br>
> >> + previewTemplate->addEntry(ANDROID_TONEMAP_MODE, &tonemapMode, 1);<br>
> >> <br>
> >> return previewTemplate;<br>
> >> }<br>
> >> @@ -1802,16 +1747,16 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateManual()<br>
> >> return nullptr;<br>
> >> <br>
> >> uint8_t controlMode = ANDROID_CONTROL_MODE_OFF;<br>
> >> - previewTemplate->updateEntry(ANDROID_CONTROL_MODE, &controlMode, 1);<br>
> >> + previewTemplate->addEntry(ANDROID_CONTROL_MODE, &controlMode, 1);<br>
> >> <br>
> >> uint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;<br>
> >> - previewTemplate->updateEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);<br>
> >> + previewTemplate->addEntry(ANDROID_CONTROL_AE_MODE, &aeMode, 1);<br>
> >> <br>
> >> uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_OFF;<br>
> >> - previewTemplate->updateEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);<br>
> >> + previewTemplate->addEntry(ANDROID_CONTROL_AWB_MODE, &awbMode, 1);<br>
> >> <br>
> >> uint8_t edgeMode = ANDROID_EDGE_MODE_OFF;<br>
> >> - previewTemplate->updateEntry(ANDROID_EDGE_MODE, &edgeMode, 1);<br>
> >> + previewTemplate->addEntry(ANDROID_EDGE_MODE, &edgeMode, 1);<br>
> >> <br>
> >> /* \todo get this from available filter densities */<br>
> >> float filterDensity = 0.0f;<br>
> >> @@ -1867,7 +1812,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)<br>
> >> return nullptr;<br>
> >> }<br>
> >> <br>
> >> - requestTemplate->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT,<br>
> >> + requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,<br>
> >> &captureIntent, 1);<br>
> >> <br>
> >> requestTemplates_[type] = std::move(requestTemplate);<br>
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h<br>
> >> index 8edbcdfd..88aab012 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..e0b314ee 100644<br>
> >> --- a/src/android/camera_metadata.cpp<br>
> >> +++ b/src/android/camera_metadata.cpp<br>
> >> @@ -63,49 +63,32 @@ 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>
> >> +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>
> >> - return true;<br>
> >> -<br>
> >> - const char *name = get_camera_metadata_tag_name(tag);<br>
> >> - if (name)<br>
> >> - LOG(CameraMetadata, Error)<br>
> >> - << "Failed to add tag " << name;<br>
> >> - else<br>
> >> - LOG(CameraMetadata, Error)<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>
> >> -{<br>
> >> - if (!valid_)<br>
> >> - return false;<br>
> >> -<br>
> >> - camera_metadata_entry_t entry;<br>
> >> - int ret = find_camera_metadata_entry(metadata_, tag, &entry);<br>
> >> - if (ret) {<br>
> >> - const char *name = get_camera_metadata_tag_name(tag);<br>
> >> - LOG(CameraMetadata, Error)<br>
> >> - << "Failed to update tag "<br>
> >> - << (name ? name : "<unknown>") << ": not present";<br>
> >> - return false;<br>
> >> - }<br>
> >> -<br>
> >> - ret = update_camera_metadata_entry(metadata_, entry.index, data,<br>
> >> - count, nullptr);<br>
> >> - if (ret) {<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>
> >> + 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>
> >> + append_camera_metadata(metadata_, oldMetadata);<br>
> >> + free_camera_metadata(oldMetadata);<br>
> > <br>
> > Ok - all that looked simpler than I expected.<br>
> > Maybe there are optimisations on how or when to resize, but I think this<br>
> > sounds quite reasonable for now?<br>
> > <br>
> > Given that we can already pre-reserve if we believe we know the initial<br>
> > sizes...<br>
> > <br>
> >> }<br>
> >> <br>
> >> return true;<br>
> >> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h<br>
> >> index d653e2f0..c35ea1b7 100644<br>
> >> --- a/src/android/camera_metadata.h<br>
> >> +++ b/src/android/camera_metadata.h<br>
> >> @@ -7,6 +7,7 @@<br>
> >> #ifndef __ANDROID_CAMERA_METADATA_H__<br>
> >> #define __ANDROID_CAMERA_METADATA_H__<br>
> >> <br>
> >> +#include <iostream><br>
> >> #include <stdint.h><br>
> >> <br>
> >> #include <system/camera_metadata.h><br>
> >> @@ -23,9 +24,43 @@ 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>
> >> + bool addEntry(uint32_t tag, const T *data, size_t count)<br>
<br>
Think of a template as a function that gets duplicated for every type T<br>
that is used. This will cause lots of duplication. You should gather the<br>
type-depend code in the beginning of the function, and then move<br>
everything else to a private function that isn't type-dependent.<br>
<br>
I would also consider using a Span<const T> instead of passing data and<br>
count separately, as well as adding a wrapper for the common case of<br>
single-element entries<br>
<br>
template<typename T><br>
bool addEntry(uint32_t tag, const T &data)<br>
{<br>
return addEntry(Span<const T>{&data, 1});<br>
}<br>
<br>
You may also change the return type to void, if nobody checks the return<br>
value.<br>
<br>
> >> + {<br>
> >> + if (!valid_)<br>
> >> + return false;<br>
> >> +<br>
> >> + camera_metadata_entry_t entry;<br>
> >> + int ret = find_camera_metadata_entry(metadata_, tag, &entry);<br>
> >> + if (ret) {<br>
> >> + if (!resize(1, count * sizeof(T))) {<br>
> > <br>
> > Always calling resize feels a bit odd. Especially as resize can return<br>
> > true without actually resizing...<br>
> > <br>
> >> + std::cerr << "Failed to resize";<br>
> > <br>
> > if you're using std::cerr, you need std::endl on the end :-)<br>
> > But I get that this will change.<br>
> > <br>
> > In the code you've removed, there was:<br>
> > LOG(CameraMetadata, Error)<br>
> > << "Failed to add tag " << name;<br>
> > <br>
> > Can't you use that existing LOG infrastructure?<br>
> > <br>
> >> + return false;<br>
> >> + }<br>
> >> +<br>
> >> + if (!add_camera_metadata_entry(metadata_, tag, data, count))<br>
> >> + return true;<br>
> >> +<br>
> >> + const char *name = get_camera_metadata_tag_name(tag);<br>
> >> + std::cerr << "Failed to add tag " << (name ? name : "<unknown>");<br>
> >> +<br>
> >> + valid_ = false;<br>
> >> +<br>
> >> + return false;<br>
> >> + }<br>
> >> +<br>
> >> + if (!update_camera_metadata_entry(metadata_, entry.index, data,<br>
> >> + count, nullptr))<br>
> >> + return true;<br>
> >> +<br>
> >> + const char *name = get_camera_metadata_tag_name(tag);<br>
> >> + std::cerr << "Failed to update tag " << (name ? name : "<unknown>");<br>
> >> +<br>
> >> + return false;<br>
> >> + }<br>
> >> <br>
> >> camera_metadata_t *get();<br>
> >> const camera_metadata_t *get() const;<br>
<br></blockquote><div><br></div><div>Overall, I am fine with this change while I am a bit concerned about the performance of allocate_camera_metadata and append_camera_metadata.</div><div>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.</div><div>The data size is not considerably small (about 500 bytes), isn't it?</div><div>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.</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">
-- <br>
Regards,<br>
<br>
Laurent Pinchart<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>