[{"id":16924,"web_url":"https://patchwork.libcamera.org/comment/16924/","msgid":"<CAO5uPHM=4NfzFr3mioHz4gBrBZM9=9YTkiugObfRfsxXpfv5-A@mail.gmail.com>","date":"2021-05-13T03:26:29","subject":"Re: [libcamera-devel] [PATCH v3 1/3] android: camera_metadata:\n\tAuto-resize CameraMetadata","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Paul, thank you for the patch.\n\nOn Wed, May 12, 2021 at 7:25 PM Paul Elder <paul.elder@ideasonboard.com>\nwrote:\n\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>\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, vector (v3: s/span/vector/)\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. This is very common, and wasn't very nice\n> having to wrap everything in a Span. All the old calls are\n> backward-compatible 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   | 44 ++++++++++++++++-\n>  4 files changed, 117 insertions(+), 59 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\n> camera3_callback_ops_t *callbacks)\n>         callbacks_ = callbacks;\n>  }\n>\n> -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()\n> -{\n> -       /*\n> -        * \\todo Keep this in sync with the actual number of entries.\n> -        * Currently: 54 entries, 874 bytes of static metadata\n> -        */\n> -       uint32_t numEntries = 54;\n> -       uint32_t byteSize = 874;\n> -\n> -       /*\n> -        * Calculate space occupation in bytes for dynamically built\n> metadata\n> -        * entries.\n> -        *\n> -        * Each stream configuration entry requires 48 bytes:\n> -        * 4 32bits integers for\n> ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS\n> -        * 4 64bits integers for\n> ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS\n> -        */\n> -       byteSize += streamConfigurations_.size() * 48;\n> -\n> -       /*\n> -        * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail\n> sizes\n> -        * 2 32bits integers for the (0, 0) thumbnail size\n> -        *\n> -        * This is a worst case estimates as different configurations with\n> the\n> -        * same aspect ratio will generate the same size.\n> -        */\n> -       for (const auto &entry : streamConfigurations_) {\n> -               if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)\n> -                       continue;\n> -\n> -               byteSize += 8;\n> -       }\n> -       byteSize += 8;\n> -\n> -       return 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\n> *CameraDevice::getStaticMetadata()\n>         if (staticMetadata_)\n>                 return staticMetadata_->get();\n>\n> -       /*\n> -        * The here reported metadata are enough to implement a basic\n> capture\n> -        * example application, but a real camera implementation will\n> require\n> -        * more.\n> -        */\n> -       uint32_t numEntries;\n> -       uint32_t byteSize;\n> -       std::tie(numEntries, byteSize) = calculateStaticMetadataSize();\n> -       staticMetadata_ = std::make_unique<CameraMetadata>(numEntries,\n> byteSize);\n> +       staticMetadata_ = std::make_unique<CameraMetadata>(64, 1024);\n>         if (!staticMetadata_->isValid()) {\n>                 LOG(HAL, Error) << \"Failed to allocate static metadata\";\n>                 staticMetadata_.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>         std::vector<libcamera::Size>\n>         getRawResolutions(const libcamera::PixelFormat &pixelFormat);\n>\n> -       std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();\n>         libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t\n> camera3buffer);\n>         void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>         void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> diff --git a/src/android/camera_metadata.cpp\n> b/src/android/camera_metadata.cpp\n> index 6f1bcdbe..6394fd74 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -63,14 +63,67 @@ bool CameraMetadata::getEntry(uint32_t tag,\n> camera_metadata_ro_entry_t *entry) c\n>         return true;\n>  }\n>\n> -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t\n> 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>         if (!valid_)\n>                 return false;\n>\n> -       if (!add_camera_metadata_entry(metadata_, tag, data, count))\n> +       if (!count && !size)\n>                 return true;\n>\n> +       size_t currentEntryCount =\n> get_camera_metadata_entry_count(metadata_);\n> +       size_t currentEntryCapacity =\n> get_camera_metadata_entry_capacity(metadata_);\n> +       size_t newEntryCapacity = currentEntryCapacity < currentEntryCount\n> + count ?\n> +                                 currentEntryCapacity * 2 :\n> currentEntryCapacity;\n> +\n> +       size_t currentDataCount =\n> get_camera_metadata_data_count(metadata_);\n> +       size_t currentDataCapacity =\n> get_camera_metadata_data_capacity(metadata_);\n> +       size_t newDataCapacity = currentDataCapacity < currentDataCount +\n> size ?\n> +                                currentDataCapacity * 2 :\n> currentDataCapacity;\n> +\n> +       if (newEntryCapacity > currentEntryCapacity ||\n> +           newDataCapacity > currentDataCapacity) {\n> +               camera_metadata_t *oldMetadata = metadata_;\n> +               metadata_ = allocate_camera_metadata(newEntryCapacity,\n> newDataCapacity);\n> +               if (!metadata_) {\n> +                       metadata_ = oldMetadata;\n> +                       return false;\n> +               }\n> +\n> +               LOG(CameraMetadata, Info)\n> +                       << \"Resized: old entry capacity \" <<\n> currentEntryCapacity\n> +                       << \", old data capacity \" << currentDataCapacity\n> +                       << \", new entry capacity \" << newEntryCapacity\n> +                       << \", new data capacity \" << newDataCapacity;\n> +\n> +               append_camera_metadata(metadata_, oldMetadata);\n> +               free_camera_metadata(oldMetadata);\n> +       }\n> +\n> +       return true;\n> +}\n> +\n> +void CameraMetadata::addEntry(uint32_t tag, const void *data, size_t\n> count,\n> +                             size_t sizeofT)\n> +{\n> +       if (!valid_)\n> +               return;\n> +\n> +       if (!resize(1, count * sizeofT)) {\n> +               LOG(CameraMetadata, Error) << \"Failed to resize\";\n> +               valid_ = false;\n> +               return;\n> +       }\n> +\n> +       if (!add_camera_metadata_entry(metadata_, tag, data, count))\n> +               return;\n> +\n>         const char *name = get_camera_metadata_tag_name(tag);\n>         if (name)\n>                 LOG(CameraMetadata, Error)\n> @@ -80,14 +133,13 @@ bool CameraMetadata::addEntry(uint32_t tag, const\n> void *data, size_t count)\n>                         << \"Failed to add unknown tag \" << tag;\n>\n>         valid_ = false;\n> -\n> -       return false;\n>  }\n>\n> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t\n> count)\n> +void CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t\n> count,\n> +                                size_t sizeofT)\n>  {\n>         if (!valid_)\n> -               return false;\n> +               return;\n>\n>         camera_metadata_entry_t entry;\n>         int ret = find_camera_metadata_entry(metadata_, tag, &entry);\n> @@ -96,7 +148,20 @@ bool CameraMetadata::updateEntry(uint32_t tag, const\n> void *data, size_t count)\n>                 LOG(CameraMetadata, Error)\n>                         << \"Failed to update tag \"\n>                         << (name ? name : \"<unknown>\") << \": not present\";\n> -               return false;\n> +               return;\n> +       }\n> +\n> +       size_t oldSize =\n> +               calculate_camera_metadata_entry_data_size(entry.type,\n> +                                                         entry.count);\n> +       size_t newSize =\n> +               calculate_camera_metadata_entry_data_size(entry.type,\n> +                                                         count * sizeofT);\n>\n\nI am confused by this.\ncalculate_camera_metadata_entry_data_size() looks up the size of one\nelement for the type and computes the required size multiplying by the\ncount.\nThat is, newSize seems to be sizeofT * count * sizeofT.\nAm I missing something?\n\n+       size_t sizeIncrement = newSize - oldSize > 0 ? newSize - oldSize :\n> 0;\n> +       if (!resize(0, count * sizeIncrement)) {\n> +               LOG(CameraMetadata, Error) << \"Failed to resize\";\n> +               valid_ = false;\n> +               return;\n>         }\n>\n>         ret = update_camera_metadata_entry(metadata_, entry.index, data,\n> @@ -105,10 +170,9 @@ bool CameraMetadata::updateEntry(uint32_t tag, const\n> void *data, size_t count)\n>                 const char *name = get_camera_metadata_tag_name(tag);\n>                 LOG(CameraMetadata, Error)\n>                         << \"Failed to update tag \" << (name ? name :\n> \"<unknown>\");\n> -               return false;\n> -       }\n>\n> -       return true;\n> +               valid_ = false;\n> +       }\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..aa875fe4 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,48 @@ public:\n>         CameraMetadata &operator=(const CameraMetadata &other);\n>\n>         bool isValid() const { return valid_; }\n> +       bool resize(size_t count, size_t size);\n>         bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry)\n> const;\n> -       bool addEntry(uint32_t tag, const void *data, size_t data_count);\n> -       bool updateEntry(uint32_t tag, const void *data, size_t\n> data_count);\n> +\n> +       template<typename T,\n> +                std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n> +       void addEntry(uint32_t tag, const T &data)\n> +       {\n> +               addEntry(tag, &data, 1, sizeof(T));\n> +       }\n> +\n> +       template<typename T>\n> +       void addEntry(uint32_t tag, std::vector<T> &data)\n> +       {\n> +               addEntry(tag, data.data(), data.size(), sizeof(T));\n> +       }\n> +\n>\n\ns/std::vector<T>/Span<const T>/\nditto to updateEntry().\n\nWe can use a row array, for example, for aeAvailableAntiBandingModes.\n\n-Hiro\n\n+       template<typename T>\n> +       void addEntry(uint32_t tag, const T *data, size_t count)\n> +       {\n> +               addEntry(tag, data, count, sizeof(T));\n> +       }\n> +\n> +       template<typename T>\n> +       void updateEntry(uint32_t tag, const T &data)\n> +       {\n> +               updateEntry(tag, &data, 1, sizeof(T));\n> +       }\n> +\n> +       template<typename T>\n> +       void updateEntry(uint32_t tag, std::vector<T> &data)\n> +       {\n> +               updateEntry(tag, data.data(), data.size(), sizeof(T));\n> +       }\n> +\n> +       template<typename T>\n> +       void updateEntry(uint32_t tag, const T *data, size_t count)\n> +       {\n> +               updateEntry(tag, data, count, sizeof(T));\n> +       }\n> +\n> +       void addEntry(uint32_t tag, const void *data, size_t count, size_t\n> sizeofT);\n> +       void updateEntry(uint32_t tag, const void *data, size_t count,\n> size_t sizeofT);\n>\n>         camera_metadata_t *get();\n>         const camera_metadata_t *get() const;\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 A5118C31EB\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 13 May 2021 03:26:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 120C46891D;\n\tThu, 13 May 2021 05:26:43 +0200 (CEST)","from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com\n\t[IPv6:2a00:1450:4864:20::52c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75B08602B5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 May 2021 05:26:41 +0200 (CEST)","by mail-ed1-x52c.google.com with SMTP id c22so29432439edn.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 12 May 2021 20:26:41 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"YhoXG9+A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ns3z/e+loc1HmIFPpaPVgY0JLad31KRkB02PHX5n73U=;\n\tb=YhoXG9+ApEvnU9/OOkQWEq+bZPl/7l8cPEJTDodEyPzo8zPs3Hxh2ujb9R/AggiWdW\n\tLOC1kl/iFPK9YiBq0ZFKoRxhlI5w7L3eoQ2kYMdqncSnTkTNzTGVeRKzPKmNkt4VOX++\n\tN++EpTwO4krVXUW1XVT+Qr8GPEbwHV+kNzBSE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ns3z/e+loc1HmIFPpaPVgY0JLad31KRkB02PHX5n73U=;\n\tb=AsnnYDOsyJgODE7tcRkND8Y8CmonwcKWTvKL8vXPe765PWSO39xNXOM77ns6jR3xmw\n\t9CVCr30D/4TUsEI0efVuJygZo8qV3jPbu1+nlSTk+CKtdrewG4vw1Mt8irvTT8UAiNMm\n\t3wx8cbDS412h48B1iqiLRFmllpS+ctFhLVn520HdEsF563VIMx9AZKTUS2BuCIplVEz2\n\tWy9Ze94rMQaM6n1AZAOdUW3+muqZOXHAL3CDtGXuSrJSJpej1tmPHQiC55B2hhlbZD/2\n\tw6ok6ZO3el8N4kU/g1neMlo9QhZMwrWqMbROdJI6TW5V/iMxsyzaZM/bGLzEhwuA+XL+\n\t8Ekg==","X-Gm-Message-State":"AOAM533JI6z5vrGNppG3QncUXR7Sk8KRqXiGj5aisuxWcJCwfxvqd6rM\n\tEhDbGtuyy/MTHIuf93Cc/Sf9Z7RZFL1IkMRWwgwvJpKGGJEXgw==","X-Google-Smtp-Source":"ABdhPJyJbGtjouA05lkIq1Y9QMixH5f3ajSDvxsdg7rt7joOnZLjzf20h//KSVSPfexW4HrEYmC4djX3XJ64T4A3SYk=","X-Received":"by 2002:a50:8e44:: with SMTP id 4mr47566270edx.244.1620876400966;\n\tWed, 12 May 2021 20:26:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210512102541.722956-1-paul.elder@ideasonboard.com>\n\t<20210512102541.722956-2-paul.elder@ideasonboard.com>","In-Reply-To":"<20210512102541.722956-2-paul.elder@ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Thu, 13 May 2021 12:26:29 +0900","Message-ID":"<CAO5uPHM=4NfzFr3mioHz4gBrBZM9=9YTkiugObfRfsxXpfv5-A@mail.gmail.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"multipart/mixed;\n\tboundary=\"===============7043124489376895166==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16929,"web_url":"https://patchwork.libcamera.org/comment/16929/","msgid":"<YJzmFLQBYfPArt+7@oden.dyn.berto.se>","date":"2021-05-13T08:40:52","subject":"Re: [libcamera-devel] [PATCH v3 1/3] android: camera_metadata:\n\tAuto-resize CameraMetadata","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul,\n\nThanks for this awesome work.\n\nOn 2021-05-12 19:25:39 +0900, 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\nWith the Span improvement suggested by Hiro addressed,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nAlso a small not below in case you feel it's a good idea.\n\n> \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, vector (v3: s/span/vector/)\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. This is very common, and wasn't very nice\n> having to wrap everything in a Span. All the old calls are\n> backward-compatible 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   | 44 ++++++++++++++++-\n>  4 files changed, 117 insertions(+), 59 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>  \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..6394fd74 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -63,14 +63,67 @@ 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 (!add_camera_metadata_entry(metadata_, tag, data, count))\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> +\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> +void 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;\n> +\n> +\tif (!resize(1, count * sizeofT)) {\n> +\t\tLOG(CameraMetadata, Error) << \"Failed to resize\";\n> +\t\tvalid_ = false;\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (!add_camera_metadata_entry(metadata_, tag, data, count))\n> +\t\treturn;\n> +\n>  \tconst char *name = get_camera_metadata_tag_name(tag);\n>  \tif (name)\n>  \t\tLOG(CameraMetadata, Error)\n> @@ -80,14 +133,13 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)\n>  \t\t\t<< \"Failed to add unknown tag \" << tag;\n>  \n>  \tvalid_ = false;\n> -\n> -\treturn false;\n>  }\n>  \n> -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n> +void CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count,\n> +\t\t\t\t size_t sizeofT)\n\nnit: Would it hurt to keep the bool return value?\n\n>  {\n>  \tif (!valid_)\n> -\t\treturn false;\n> +\t\treturn;\n>  \n>  \tcamera_metadata_entry_t entry;\n>  \tint ret = find_camera_metadata_entry(metadata_, tag, &entry);\n> @@ -96,7 +148,20 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\n>  \t\tLOG(CameraMetadata, Error)\n>  \t\t\t<< \"Failed to update tag \"\n>  \t\t\t<< (name ? name : \"<unknown>\") << \": not present\";\n> -\t\treturn false;\n> +\t\treturn;\n> +\t}\n> +\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 * sizeofT);\n> +\tsize_t sizeIncrement = newSize - oldSize > 0 ? newSize - oldSize : 0;\n> +\tif (!resize(0, count * sizeIncrement)) {\n> +\t\tLOG(CameraMetadata, Error) << \"Failed to resize\";\n> +\t\tvalid_ = false;\n> +\t\treturn;\n>  \t}\n>  \n>  \tret = update_camera_metadata_entry(metadata_, entry.index, data,\n> @@ -105,10 +170,9 @@ bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)\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> -\t\treturn false;\n> -\t}\n>  \n> -\treturn true;\n> +\t\tvalid_ = false;\n> +\t}\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..aa875fe4 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,48 @@ 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> +\tvoid addEntry(uint32_t tag, const T &data)\n> +\t{\n> +\t\taddEntry(tag, &data, 1, sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename T>\n> +\tvoid addEntry(uint32_t tag, std::vector<T> &data)\n> +\t{\n> +\t\taddEntry(tag, data.data(), data.size(), sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename T>\n> +\tvoid addEntry(uint32_t tag, const T *data, size_t count)\n> +\t{\n> +\t\taddEntry(tag, data, count, sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename T>\n> +\tvoid updateEntry(uint32_t tag, const T &data)\n> +\t{\n> +\t\tupdateEntry(tag, &data, 1, sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename T>\n> +\tvoid updateEntry(uint32_t tag, std::vector<T> &data)\n> +\t{\n> +\t\tupdateEntry(tag, data.data(), data.size(), sizeof(T));\n> +\t}\n> +\n> +\ttemplate<typename T>\n> +\tvoid updateEntry(uint32_t tag, const T *data, size_t count)\n> +\t{\n> +\t\tupdateEntry(tag, data, count, sizeof(T));\n> +\t}\n> +\n> +\tvoid addEntry(uint32_t tag, const void *data, size_t count, size_t sizeofT);\n> +\tvoid updateEntry(uint32_t tag, const void *data, size_t count, size_t sizeofT);\n>  \n>  \tcamera_metadata_t *get();\n>  \tconst camera_metadata_t *get() const;\n> -- \n> 2.27.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 39CD7C31EB\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 13 May 2021 08:40:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DB5C6891D;\n\tThu, 13 May 2021 10:40:56 +0200 (CEST)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1CCB668919\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 May 2021 10:40:55 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id t11so37438917lfl.11\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 May 2021 01:40:55 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tj1sm224019lfb.142.2021.05.13.01.40.53\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 13 May 2021 01:40:53 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"aekomtXo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=ghfYAoDuGNpjOo7k8c8faEZ8ZoIMVqy8USQv9svCocA=;\n\tb=aekomtXonppcqjD3uFrMz8AfjDptn3Acpg1yh5ubuJ3gRh/bNr2dqk4/57Jl65K06r\n\tFkGZEeAAdot9w3cEwx1a9kCljBCXNqrN5CDcjowaQdZqHYvK3vV6XC0iVfWd7bUEtOXA\n\t8YU1dBxRNLXUPRhrsbJ8MaNI399ipJIG21Mv0zf0fMLcTqbxY3V20i/AmRsJF8CW9HBU\n\tc2ylRLwWOhEafQ9A++zCQeARN2UskOG2uW5SaL4VFmpTh+0OItV88g8IvI+R868uwr2z\n\tb+HC9Dr23PuIvJ9aUjHy0TUKic2BDX1RyLw8SFYjsLPzFrgG/+mFRhrs/QONSH7DBXmD\n\tWGng==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=ghfYAoDuGNpjOo7k8c8faEZ8ZoIMVqy8USQv9svCocA=;\n\tb=tWudRIWhFRlHUzw/UaU5UG3b76KnLPFdSouIypjL5lGVFz11cRnXIP9GyHynAWMEC3\n\tO63mmGSTewqdOympivwl3uDei+1nTB7SWw0CDhL/W1I/D67qUm8XwyUFVPRUbXNdISFx\n\tLAl5mI3kTYYE6t6JPWi8GhNavKVyGyerrE6WWuYQ7fj6+qFZ2g8WatqoYyVaeVgREQeZ\n\tGQ72Cr/wqOKAnHcGR3Gbhc1DrtUtrz7Fy94tZqf9BC1TYz5aKNJ3dsMpR7oO966Kpmii\n\tXPk7jsfF9irn/7xLOxBciA+DerDP4gkoA1Z1zRGxfYNhGkOpctMBun1WEfbibcAPLnrQ\n\tLGqg==","X-Gm-Message-State":"AOAM533JdKpjNbKoX+4x0Zy+7kX6/E2SWQPF8wDz417c9p2fAQ2Lwto0\n\tt5zE/5xF/L1OLYYhi74YypqdXviHy8OHiA==","X-Google-Smtp-Source":"ABdhPJxOyOaI5tvuc5g5OYnasERM5Z3UQZQQYZvbGQ/khr0wzPzzksVfZnwkV8lJf18Y88+nPhHuqA==","X-Received":"by 2002:a19:c10:: with SMTP id 16mr24213442lfm.332.1620895254220;\n\tThu, 13 May 2021 01:40:54 -0700 (PDT)","Date":"Thu, 13 May 2021 10:40:52 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<YJzmFLQBYfPArt+7@oden.dyn.berto.se>","References":"<20210512102541.722956-1-paul.elder@ideasonboard.com>\n\t<20210512102541.722956-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210512102541.722956-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16936,"web_url":"https://patchwork.libcamera.org/comment/16936/","msgid":"<20210514061638.GW195599@pyrite.rasen.tech>","date":"2021-05-14T06:16:38","subject":"Re: [libcamera-devel] [PATCH v3 1/3] android: camera_metadata:\n\tAuto-resize CameraMetadata","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the review.\n\nOn Thu, May 13, 2021 at 12:26:29PM +0900, Hirokazu Honda wrote:\n> Hi Paul, thank you for the patch.\n> \n> On Wed, May 12, 2021 at 7:25 PM Paul Elder <paul.elder@ideasonboard.com> wrote:\n> \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> \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, vector (v3: s/span/vector/)\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. This is very common, and wasn't very nice\n>     having to wrap everything in a Span. All the old calls are\n>     backward-compatible 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   | 44 ++++++++++++++++-\n>      4 files changed, 117 insertions(+), 59 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\n>     camera3_callback_ops_t *callbacks)\n>             callbacks_ = callbacks;\n>      }\n> \n>     -std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()\n>     -{\n>     -       /*\n>     -        * \\todo Keep this in sync with the actual number of entries.\n>     -        * Currently: 54 entries, 874 bytes of static metadata\n>     -        */\n>     -       uint32_t numEntries = 54;\n>     -       uint32_t byteSize = 874;\n>     -\n>     -       /*\n>     -        * Calculate space occupation in bytes for dynamically built\n>     metadata\n>     -        * entries.\n>     -        *\n>     -        * Each stream configuration entry requires 48 bytes:\n>     -        * 4 32bits integers for\n>     ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS\n>     -        * 4 64bits integers for\n>     ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS\n>     -        */\n>     -       byteSize += streamConfigurations_.size() * 48;\n>     -\n>     -       /*\n>     -        * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail\n>     sizes\n>     -        * 2 32bits integers for the (0, 0) thumbnail size\n>     -        *\n>     -        * This is a worst case estimates as different configurations with\n>     the\n>     -        * same aspect ratio will generate the same size.\n>     -        */\n>     -       for (const auto &entry : streamConfigurations_) {\n>     -               if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)\n>     -                       continue;\n>     -\n>     -               byteSize += 8;\n>     -       }\n>     -       byteSize += 8;\n>     -\n>     -       return 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\n>     *CameraDevice::getStaticMetadata()\n>             if (staticMetadata_)\n>                     return staticMetadata_->get();\n> \n>     -       /*\n>     -        * The here reported metadata are enough to implement a basic\n>     capture\n>     -        * example application, but a real camera implementation will\n>     require\n>     -        * more.\n>     -        */\n>     -       uint32_t numEntries;\n>     -       uint32_t byteSize;\n>     -       std::tie(numEntries, byteSize) = calculateStaticMetadataSize();\n>     -       staticMetadata_ = std::make_unique<CameraMetadata>(numEntries,\n>     byteSize);\n>     +       staticMetadata_ = std::make_unique<CameraMetadata>(64, 1024);\n>             if (!staticMetadata_->isValid()) {\n>                     LOG(HAL, Error) << \"Failed to allocate static metadata\";\n>                     staticMetadata_.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>             std::vector<libcamera::Size>\n>             getRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> \n>     -       std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();\n>             libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t\n>     camera3buffer);\n>             void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>             void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n>     diff --git a/src/android/camera_metadata.cpp b/src/android/\n>     camera_metadata.cpp\n>     index 6f1bcdbe..6394fd74 100644\n>     --- a/src/android/camera_metadata.cpp\n>     +++ b/src/android/camera_metadata.cpp\n>     @@ -63,14 +63,67 @@ bool CameraMetadata::getEntry(uint32_t tag,\n>     camera_metadata_ro_entry_t *entry) c\n>             return true;\n>      }\n> \n>     -bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t\n>     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>             if (!valid_)\n>                     return false;\n> \n>     -       if (!add_camera_metadata_entry(metadata_, tag, data, count))\n>     +       if (!count && !size)\n>                     return true;\n> \n>     +       size_t currentEntryCount = get_camera_metadata_entry_count\n>     (metadata_);\n>     +       size_t currentEntryCapacity = get_camera_metadata_entry_capacity\n>     (metadata_);\n>     +       size_t newEntryCapacity = currentEntryCapacity < currentEntryCount\n>     + count ?\n>     +                                 currentEntryCapacity * 2 :\n>     currentEntryCapacity;\n>     +\n>     +       size_t currentDataCount = get_camera_metadata_data_count\n>     (metadata_);\n>     +       size_t currentDataCapacity = get_camera_metadata_data_capacity\n>     (metadata_);\n>     +       size_t newDataCapacity = currentDataCapacity < currentDataCount +\n>     size ?\n>     +                                currentDataCapacity * 2 :\n>     currentDataCapacity;\n>     +\n>     +       if (newEntryCapacity > currentEntryCapacity ||\n>     +           newDataCapacity > currentDataCapacity) {\n>     +               camera_metadata_t *oldMetadata = metadata_;\n>     +               metadata_ = allocate_camera_metadata(newEntryCapacity,\n>     newDataCapacity);\n>     +               if (!metadata_) {\n>     +                       metadata_ = oldMetadata;\n>     +                       return false;\n>     +               }\n>     +\n>     +               LOG(CameraMetadata, Info)\n>     +                       << \"Resized: old entry capacity \" <<\n>     currentEntryCapacity\n>     +                       << \", old data capacity \" << currentDataCapacity\n>     +                       << \", new entry capacity \" << newEntryCapacity\n>     +                       << \", new data capacity \" << newDataCapacity;\n>     +\n>     +               append_camera_metadata(metadata_, oldMetadata);\n>     +               free_camera_metadata(oldMetadata);\n>     +       }\n>     +\n>     +       return true;\n>     +}\n>     +\n>     +void CameraMetadata::addEntry(uint32_t tag, const void *data, size_t\n>     count,\n>     +                             size_t sizeofT)\n>     +{\n>     +       if (!valid_)\n>     +               return;\n>     +\n>     +       if (!resize(1, count * sizeofT)) {\n>     +               LOG(CameraMetadata, Error) << \"Failed to resize\";\n>     +               valid_ = false;\n>     +               return;\n>     +       }\n>     +\n>     +       if (!add_camera_metadata_entry(metadata_, tag, data, count))\n>     +               return;\n>     +\n>             const char *name = get_camera_metadata_tag_name(tag);\n>             if (name)\n>                     LOG(CameraMetadata, Error)\n>     @@ -80,14 +133,13 @@ bool CameraMetadata::addEntry(uint32_t tag, const void\n>     *data, size_t count)\n>                             << \"Failed to add unknown tag \" << tag;\n> \n>             valid_ = false;\n>     -\n>     -       return false;\n>      }\n> \n>     -bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t\n>     count)\n>     +void CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t\n>     count,\n>     +                                size_t sizeofT)\n>      {\n>             if (!valid_)\n>     -               return false;\n>     +               return;\n> \n>             camera_metadata_entry_t entry;\n>             int ret = find_camera_metadata_entry(metadata_, tag, &entry);\n>     @@ -96,7 +148,20 @@ bool CameraMetadata::updateEntry(uint32_t tag, const\n>     void *data, size_t count)\n>                     LOG(CameraMetadata, Error)\n>                             << \"Failed to update tag \"\n>                             << (name ? name : \"<unknown>\") << \": not present\";\n>     -               return false;\n>     +               return;\n>     +       }\n>     +\n>     +       size_t oldSize =\n>     +               calculate_camera_metadata_entry_data_size(entry.type,\n>     +                                                         entry.count);\n>     +       size_t newSize =\n>     +               calculate_camera_metadata_entry_data_size(entry.type,\n>     +                                                         count * sizeofT);\n> \n> \n> I am confused by this.\n> calculate_camera_metadata_entry_data_size() looks up the size of one element\n> for the type and computes the required size multiplying by the count.\n> That is, newSize seems to be sizeofT * count * sizeofT.\n> Am I missing something?\n\nAh you're right. I've removed the extra multiplication.\n\nIt's a bit confusing whether \"count\" means \"number of bytes\" or \"number\nof entries\" :/\n\n> \n> \n>     +       size_t sizeIncrement = newSize - oldSize > 0 ? newSize - oldSize :\n>     0;\n>     +       if (!resize(0, count * sizeIncrement)) {\n>     +               LOG(CameraMetadata, Error) << \"Failed to resize\";\n>     +               valid_ = false;\n>     +               return;\n>             }\n> \n>             ret = update_camera_metadata_entry(metadata_, entry.index, data,\n>     @@ -105,10 +170,9 @@ bool CameraMetadata::updateEntry(uint32_t tag, const\n>     void *data, size_t count)\n>                     const char *name = get_camera_metadata_tag_name(tag);\n>                     LOG(CameraMetadata, Error)\n>                             << \"Failed to update tag \" << (name ? name : \"\n>     <unknown>\");\n>     -               return false;\n>     -       }\n> \n>     -       return true;\n>     +               valid_ = false;\n>     +       }\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..aa875fe4 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,48 @@ public:\n>             CameraMetadata &operator=(const CameraMetadata &other);\n> \n>             bool isValid() const { return valid_; }\n>     +       bool resize(size_t count, size_t size);\n>             bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry)\n>     const;\n>     -       bool addEntry(uint32_t tag, const void *data, size_t data_count);\n>     -       bool updateEntry(uint32_t tag, const void *data, size_t\n>     data_count);\n>     +\n>     +       template<typename T,\n>     +                std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>\n>     +       void addEntry(uint32_t tag, const T &data)\n>     +       {\n>     +               addEntry(tag, &data, 1, sizeof(T));\n>     +       }\n>     +\n>     +       template<typename T>\n>     +       void addEntry(uint32_t tag, std::vector<T> &data)\n>     +       {\n>     +               addEntry(tag, data.data(), data.size(), sizeof(T));\n>     +       }\n>     +\n> \n> \n> s/std::vector<T>/Span<const T>/\n> ditto to updateEntry().\n\nI got ‘std::vector<int>’ is not derived from ‘libcamera::Span<const T>’\nwhen I try that, and the template engine fails to match it.\n\nI've used:\n\ntemplate<typename S, typename T = typename S::value_type>\nbool addEntry(uint32_t tag, S &data)\n\ninstead, which works for STL containers but not for C arrays, since this\nuses data.data() and data.size().\n\nOr maybe (I've just sent v4...) I could add another overload for C\narrays and then convert to Span inside...?\n\n\nThanks,\n\nPaul\n\n\n> \n> We can use a row array, for example, for aeAvailableAntiBandingModes.\n> \n> \n>     +       template<typename T>\n>     +       void addEntry(uint32_t tag, const T *data, size_t count)\n>     +       {\n>     +               addEntry(tag, data, count, sizeof(T));\n>     +       }\n>     +\n>     +       template<typename T>\n>     +       void updateEntry(uint32_t tag, const T &data)\n>     +       {\n>     +               updateEntry(tag, &data, 1, sizeof(T));\n>     +       }\n>     +\n>     +       template<typename T>\n>     +       void updateEntry(uint32_t tag, std::vector<T> &data)\n>     +       {\n>     +               updateEntry(tag, data.data(), data.size(), sizeof(T));\n>     +       }\n>     +\n>     +       template<typename T>\n>     +       void updateEntry(uint32_t tag, const T *data, size_t count)\n>     +       {\n>     +               updateEntry(tag, data, count, sizeof(T));\n>     +       }\n>     +\n>     +       void addEntry(uint32_t tag, const void *data, size_t count, size_t\n>     sizeofT);\n>     +       void updateEntry(uint32_t tag, const void *data, size_t count,\n>     size_t sizeofT);\n> \n>             camera_metadata_t *get();\n>             const camera_metadata_t *get() const;\n>     --\n>     2.27.0","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 64326C31F3\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 14 May 2021 06:16:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFA106891C;\n\tFri, 14 May 2021 08:16:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCA24602B4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 May 2021 08:16:45 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3DA319F0;\n\tFri, 14 May 2021 08:16:43 +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=\"vk1wcx1t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620973005;\n\tbh=CBrYKq43siXqhqyekgdd6ueMpP66nefR3Tr2BLuNo7s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vk1wcx1tB1VUqo/A6rKay800GP82JzGeoAkEDa2ytxQvgkhF+LWLfGhZlGT2iA0UH\n\tt0CK5HJJqquvhTdIZHQW0q5vJveDima+Gp1Gf/H98oIOEBSoTb4UDMra9aP4PKlyyg\n\t0Smf3L/PwBxooATqSb17KDxGQoOZil6V7FNElc0k=","Date":"Fri, 14 May 2021 15:16:38 +0900","From":"paul.elder@ideasonboard.com","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<20210514061638.GW195599@pyrite.rasen.tech>","References":"<20210512102541.722956-1-paul.elder@ideasonboard.com>\n\t<20210512102541.722956-2-paul.elder@ideasonboard.com>\n\t<CAO5uPHM=4NfzFr3mioHz4gBrBZM9=9YTkiugObfRfsxXpfv5-A@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHM=4NfzFr3mioHz4gBrBZM9=9YTkiugObfRfsxXpfv5-A@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","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>"}}]