[{"id":16938,"web_url":"https://patchwork.libcamera.org/comment/16938/","msgid":"<98c2e935-1d71-350d-27ae-9d8252002d33@ideasonboard.com>","date":"2021-05-14T08:35:13","subject":"Re: [libcamera-devel] [PATCH v5 1/3] android: camera_metadata:\n\tAuto-resize CameraMetadata","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Paul,\n\nOn 14/05/2021 07:43, Paul Elder wrote:\n> Previously we had to manually declare the size of CameraMetadata on\n> allocation, and its count could not be changed after construction.\n> Change CameraMetadata's behavior so that the user can simply add or\n> update entries, and the CameraMetadata will auto-resize (double the\n> size) as necessary. Also remove everything involved with calculating\n> the initial size for any CameraMetadata instances.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> \n> ---\n> Changes in v5:\n> - add overload for C arrays\n> \n> Changes in v4:\n> - upgrade the second overload to allow STL containers instead of just\n>   vector\n>   - the main use case I'm imagining is vector and array. C arrays won't\n>     work because they don't have .data() and .size() (the latter is the\n>     more important one)\n>   - libcamera::Span doesn't work because the template engine can't match\n>     std::vector/std::array to libcamera::Span\n>     - ‘std::vector<int>’ is not derived from ‘libcamera::Span<const T>’\n> - return bool from addEntry and updateEntry\n> \n> Changes in v3:\n> - dependency on \"FULL hardware level fixes\" removed\n> - split addEntry and updateEntry\n>   - i think the lookup on every (merged) addEntry isn't worth it, since\n>     the user probably knows if they need to add or update\n> - add auto-resize (and corresponding check) to updateEntry as well\n> - add the templates and overloads to updateEntry as well\n> - increase the default size for the static metadata to be above what we\n>   had before\n> - valid_ is now used to indicate a failed addEntry/updateEntry in\n>   addition to metadata_ == nullptr. this way the user doesn't have to\n>   check the return value of every single addEntry/updateEntry call, and\n>   can just check valid_ at the end\n> - print resize when it happens\n> - updateEntry resizes if the new data size is greater than the old data\n> \n> Question: I think on failure of addEntry/updateEntry the metadata could\n> still be \"valid\", it's just that the content isn't what the user\n> expects. Should we keep valid_ as I've repurposed it here in v3, or\n> should we add another flag?\n> \n> Changes in v2:\n> - add overloading of addEntry\n> - better logging with the separation of core code vs template code\n> - addEntry failure makes the metadata invalid\n> \n> The main feature of v2 is the overloading of addEntry. I have three\n> overloads and the main one:\n> - tag, single piece of data (reference)\n> - tag, C array (v5)\n> - tag, STL container (v3: s/span/vector/) (v4: s/vector/STL container/)\n> - tag, data pointer, count\n> - tag, data pointer, count, size of one instance of the data\n> \n> The last one is the main one, and is not templated. The first two are\n> nice and convenient, because now we can do things like:\n> \n> uint32_t orientation = 0;\n> resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, orientation);\n> \n> and\n> \n> std::vector<uint32_t> availableStates = { SOME_STATE_1, SOME_STATE_2 };\n> metadata->addEntry(ANDROID_AVAILABLE_STATES, availableStates);\n> \n> The third is for when android metadata is copied directly from another\n> piece of android metadata. All the old calls are backward-compatible\n> using this overload.\n> ---\n>  src/android/camera_device.cpp   | 47 +-----------------\n>  src/android/camera_device.h     |  1 -\n>  src/android/camera_metadata.cpp | 84 +++++++++++++++++++++++++++++----\n>  src/android/camera_metadata.h   | 52 +++++++++++++++++++-\n>  4 files changed, 127 insertions(+), 57 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 7d4d0feb..74f6915c 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -773,43 +773,6 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)\n>  \tcallbacks_ = callbacks;\n>  }\n>  \n> -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()\n> -{\n> -\t/*\n> -\t * \\todo Keep this in sync with the actual number of entries.\n> -\t * Currently: 54 entries, 874 bytes of static metadata\n> -\t */\n> -\tuint32_t numEntries = 54;\n> -\tuint32_t byteSize = 874;\n> -\n> -\t/*\n> -\t * Calculate space occupation in bytes for dynamically built metadata\n> -\t * entries.\n> -\t *\n> -\t * Each stream configuration entry requires 48 bytes:\n> -\t * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS\n> -\t * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS\n> -\t */\n> -\tbyteSize += streamConfigurations_.size() * 48;\n> -\n> -\t/*\n> -\t * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes\n> -\t * 2 32bits integers for the (0, 0) thumbnail size\n> -\t *\n> -\t * This is a worst case estimates as different configurations with the\n> -\t * same aspect ratio will generate the same size.\n> -\t */\n> -\tfor (const auto &entry : streamConfigurations_) {\n> -\t\tif (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)\n> -\t\t\tcontinue;\n> -\n> -\t\tbyteSize += 8;\n> -\t}\n> -\tbyteSize += 8;\n> -\n> -\treturn std::make_tuple(numEntries, byteSize);\n> -}\n> -\n>  /*\n>   * Return static information for the camera.\n>   */\n> @@ -818,15 +781,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tif (staticMetadata_)\n>  \t\treturn staticMetadata_->get();\n>  \n> -\t/*\n> -\t * The here reported metadata are enough to implement a basic capture\n> -\t * example application, but a real camera implementation will require\n> -\t * more.\n> -\t */\n> -\tuint32_t numEntries;\n> -\tuint32_t byteSize;\n> -\tstd::tie(numEntries, byteSize) = calculateStaticMetadataSize();\n> -\tstaticMetadata_ = std::make_unique<CameraMetadata>(numEntries, byteSize);\n> +\tstaticMetadata_ = std::make_unique<CameraMetadata>(64, 1024);\n\nAs these are the initial values that can be optimised, I'd probably make\nthem defines and put them right at the top of the file so they're easy\nto find to update, with documentation describing what they define.\n\nHowever - that can be done on top, once we have 'real/measured' numbers\nto put in there.\n\n>  \tif (!staticMetadata_->isValid()) {\n>  \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n>  \t\tstaticMetadata_.reset();\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 23457e47..ae162a45 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -98,7 +98,6 @@ private:\n>  \tstd::vector<libcamera::Size>\n>  \tgetRawResolutions(const libcamera::PixelFormat &pixelFormat);\n>  \n> -\tstd::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();\n>  \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n>  \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>  \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> index 6f1bcdbe..bf8d2781 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -63,11 +63,64 @@ bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) c\n>  \treturn true;\n>  }\n>  \n> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n> +/*\n> + * \\brief Resize the metadata container, if necessary\n> + * \\param[in] count Number of entries to add to the container\n> + * \\param[in] size Total size of entries to add, in bytes\n> + * \\return True if resize was successful or unnecessary, false otherwise\n> + */\n> +bool CameraMetadata::resize(size_t count, size_t size)\n> +{\n> +\tif (!valid_)\n> +\t\treturn false;\n> +\n> +\tif (!count && !size)\n> +\t\treturn true;\n> +\n> +\tsize_t currentEntryCount = get_camera_metadata_entry_count(metadata_);\n> +\tsize_t currentEntryCapacity = get_camera_metadata_entry_capacity(metadata_);\n> +\tsize_t newEntryCapacity = currentEntryCapacity < currentEntryCount + count ?\n> +\t\t\t\t  currentEntryCapacity * 2 : currentEntryCapacity;\n> +\n> +\tsize_t currentDataCount = get_camera_metadata_data_count(metadata_);\n> +\tsize_t currentDataCapacity = get_camera_metadata_data_capacity(metadata_);\n> +\tsize_t newDataCapacity = currentDataCapacity < currentDataCount + size ?\n> +\t\t\t\t currentDataCapacity * 2 : currentDataCapacity;\n> +\n> +\tif (newEntryCapacity > currentEntryCapacity ||\n> +\t    newDataCapacity > currentDataCapacity) {\n> +\t\tcamera_metadata_t *oldMetadata = metadata_;\n> +\t\tmetadata_ = allocate_camera_metadata(newEntryCapacity, newDataCapacity);\n> +\t\tif (!metadata_) {\n> +\t\t\tmetadata_ = oldMetadata;\n> +\t\t\treturn false;\n> +\t\t}\n> +\n> +\t\tLOG(CameraMetadata, Info)\n\n\nI wasn't sure this should be info level, because it's not the doubled\nsize that's helpful to reserve initially - we can (for static metadata)\ntake the maximum sizes used when all the data is stored.\n\nHowever, if we ever do resize, that's an event that should be advertised\nin the logs so that it can be optimised out, so I've changed my mind.\n\nInfo here is good.\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n\n> +\t\t\t<< \"Resized: old entry capacity \" << currentEntryCapacity\n> +\t\t\t<< \", old data capacity \" << currentDataCapacity\n> +\t\t\t<< \", new entry capacity \" << newEntryCapacity\n> +\t\t\t<< \", new data capacity \" << newDataCapacity;\n> +\n> +\t\tappend_camera_metadata(metadata_, oldMetadata);\n> +\t\tfree_camera_metadata(oldMetadata);\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> +\t\t\t      size_t sizeofT)\n>  {\n>  \tif (!valid_)\n>  \t\treturn false;\n>  \n> +\tif (!resize(1, count * sizeofT)) {\n> +\t\tLOG(CameraMetadata, Error) << \"Failed to resize\";\n> +\t\tvalid_ = false;\n> +\t\treturn false;\n> +\t}\n> +\n>  \tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n>  \t\treturn true;\n>  \n> @@ -99,16 +152,31 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n>  \t\treturn false;\n>  \t}\n>  \n> -\tret = update_camera_metadata_entry(metadata_, entry.index, data,\n> -\t\t\t\t\t   count, nullptr);\n> -\tif (ret) {\n> -\t\tconst char *name = get_camera_metadata_tag_name(tag);\n> -\t\tLOG(CameraMetadata, Error)\n> -\t\t\t<< \"Failed to update tag \" << (name ? name : \"<unknown>\");\n> +\tsize_t oldSize =\n> +\t\tcalculate_camera_metadata_entry_data_size(entry.type,\n> +\t\t\t\t\t\t\t  entry.count);\n> +\tsize_t newSize =\n> +\t\tcalculate_camera_metadata_entry_data_size(entry.type,\n> +\t\t\t\t\t\t\t  count);\n> +\tsize_t sizeIncrement = newSize - oldSize > 0 ? newSize - oldSize : 0;\n> +\tif (!resize(0, sizeIncrement)) {\n> +\t\tLOG(CameraMetadata, Error) << \"Failed to resize\";\n> +\t\tvalid_ = false;\n>  \t\treturn false;\n>  \t}\n>  \n> -\treturn true;\n> +\tret = update_camera_metadata_entry(metadata_, entry.index, data,\n> +\t\t\t\t\t   count, nullptr);\n> +\tif (!ret)\n> +\t\treturn true;\n> +\n> +\tconst char *name = get_camera_metadata_tag_name(tag);\n> +\tLOG(CameraMetadata, Error)\n> +\t\t<< \"Failed to update tag \" << (name ? name : \"<unknown>\");\n> +\n> +\tvalid_ = false;\n> +\n> +\treturn false;\n>  }\n>  \n>  camera_metadata_t *CameraMetadata::get()\n> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> index d653e2f0..07afd4b2 100644\n> --- a/src/android/camera_metadata.h\n> +++ b/src/android/camera_metadata.h\n> @@ -8,6 +8,7 @@\n>  #define __ANDROID_CAMERA_METADATA_H__\n>  \n>  #include <stdint.h>\n> +#include <vector>\n>  \n>  #include <system/camera_metadata.h>\n>  \n> @@ -23,9 +24,56 @@ public:\n>  \tCameraMetadata &operator=(const CameraMetadata &other);\n>  \n>  \tbool isValid() const { return valid_; }\n> +\tbool resize(size_t count, size_t size);\n>  \tbool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;\n> -\tbool addEntry(uint32_t tag, const void *data, size_t data_count);\n> -\tbool updateEntry(uint32_t tag, const void *data, size_t data_count);\n> +\n> +\ttemplate<typename T,\n> +\t\t std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +\tbool addEntry(uint32_t tag, const T &data)\n> +\t{\n> +\t\treturn addEntry(tag, &data, 1, sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename T, size_t size>\n> +\tbool addEntry(uint32_t tag, const T (&data)[size])\n> +\t{\n> +\t\treturn addEntry(tag, data, size, sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename S,\n> +\t\t typename T = typename S::value_type>\n> +\tbool addEntry(uint32_t tag, S &data)\n> +\t{\n> +\t\treturn addEntry(tag, data.data(), data.size(), sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename T>\n> +\tbool addEntry(uint32_t tag, const T *data, size_t count)\n> +\t{\n> +\t\treturn addEntry(tag, data, count, sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename T>\n> +\tbool updateEntry(uint32_t tag, const T &data)\n> +\t{\n> +\t\treturn updateEntry(tag, &data, 1);\n> +\t}\n> +\n> +\ttemplate<typename T, size_t size>\n> +\tbool updateEntry(uint32_t tag, const T (&data)[size])\n> +\t{\n> +\t\treturn updateEntry(tag, data, size, sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename S,\n> +\t\t typename T = typename S::value_type>\n> +\tbool updateEntry(uint32_t tag, S &data)\n> +\t{\n> +\t\treturn updateEntry(tag, data.data(), data.size());\n> +\t}\n> +\n> +\tbool addEntry(uint32_t tag, const void *data, size_t count, size_t sizeofT);\n> +\tbool updateEntry(uint32_t tag, const void *data, size_t count);\n>  \n>  \tcamera_metadata_t *get();\n>  \tconst camera_metadata_t *get() const;\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 012AEC31F7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 14 May 2021 08:35:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56B2268920;\n\tFri, 14 May 2021 10:35:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39DDA68911\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 May 2021 10:35:17 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 43A969F0;\n\tFri, 14 May 2021 10:35:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AwaytAUr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620981316;\n\tbh=dMrq1OXR8DBrtbUJGLuvt6i59Q5aQ13oAJJ9A4jyOFA=;\n\th=Reply-To:To:References:From:Subject:Date:In-Reply-To:From;\n\tb=AwaytAUrBIzsv57mXQsexjcRQrRaWBCVHv8HF8GSc8s41X5Z2IZMvs//TQ0L3hgSZ\n\tJgrmhlEdqZKyDRmuD1UavP5d+PqKElR3cxyxByByi7tRrqiBxhsHaRN5MLTGaNWr8o\n\tRbk0Bss3PPW6GqsgLFbfeEXsne1k2+SToKEGiMpA=","To":"Paul Elder <paul.elder@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210514064303.798045-1-paul.elder@ideasonboard.com>\n\t<20210514064303.798045-2-paul.elder@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Organization":"Ideas on Board","Message-ID":"<98c2e935-1d71-350d-27ae-9d8252002d33@ideasonboard.com>","Date":"Fri, 14 May 2021 09:35:13 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210514064303.798045-2-paul.elder@ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] android: camera_metadata:\n\tAuto-resize CameraMetadata","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]