From patchwork Tue Jan 14 16:32:56 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 2646 X-Patchwork-Delegate: jacopo@jmondi.org Return-Path: Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 02A866074F for ; Tue, 14 Jan 2020 17:30:35 +0100 (CET) X-Originating-IP: 2.224.242.101 Received: from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101]) (Authenticated sender: jacopo@jmondi.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 984CE2001B; Tue, 14 Jan 2020 16:30:34 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Tue, 14 Jan 2020 17:32:56 +0100 Message-Id: <20200114163256.39236-1-jacopo@jmondi.org> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200113164245.52535-18-jacopo@jmondi.org> References: <20200113164245.52535-18-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v1.1 17/23] libcamera: control: Deep-copy control values X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Jan 2020 16:30:35 -0000 Implement operator=() and copy constructor for the ControlValue class, deep-copying the content of the 'other' ControlValue by using ControlValue::set() operation which, in case of compound ControlValue, guarantees proper memory allocation and data copy. Signed-off-by: Jacopo Mondi --- Valgrind reports the usage of unitialized value in ControlValue::relase() This was caused by the copy constructor calling set<>() which calls release() before the type of the control values was initialized (we're at construction time) Fix this by initializing type before calling operator=() -- 2.24.1 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -201,6 +201,11 @@ void ControlValue::copyValue(const ControlValue &other) */ ControlValue::ControlValue(const ControlValue &other) { + /* + * Initialze the type: operator=() calls set() which calls release() + * which inspects the control value type. Be safe and set it to none. + */ + type_ = ControlTypeNone; *this = other; } --- include/libcamera/controls.h | 5 +++ src/libcamera/controls.cpp | 67 ++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index bdbdb213528d..d13144b69198 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -43,6 +43,9 @@ public: ControlValue(Span &values); ~ControlValue(); + ControlValue(const ControlValue &other); + ControlValue &operator=(const ControlValue &other); + ControlType type() const { return type_; } bool isNone() const { return type_ == ControlTypeNone; } std::size_t numElements() const { return numElements_; } @@ -81,6 +84,8 @@ private: std::size_t numElements_; void release(); + template + void copyValue(const ControlValue &source); bool compareElement(const ControlValue &other) const; bool compareElement(const ControlValue &other, unsigned int i) const; std::string elemToString(unsigned int i) const; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index c311ab1d6af1..ddf666040ae7 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -189,6 +189,73 @@ ControlValue::~ControlValue() release(); } +template +void ControlValue::copyValue(const ControlValue &other) +{ + set(other.get()); +} + +/** + * \brief Contruct a ControlValue with the content of \a other + * \param[in] other The ControlValue to copy content from + */ +ControlValue::ControlValue(const ControlValue &other) +{ + /* + * Initialze the type: operator=() calls set() which calls release() + * which inspects the control value type. Be safe and set it to none. + */ + type_ = ControlTypeNone; + *this = other; +} + +/** + * \brief Replace the content of the ControlValue with the one of \a other + * \param[in] other The ControlValue to copy content from + * + * Deep-copy the content of \a other into the ControlValue by reserving memory + * and copy data there in case \a other transports arrays of values in one of + * its pointer data members. + * + * \return The ControlValue with its content replaced with the one of \a other + */ +ControlValue &ControlValue::operator=(const ControlValue &other) +{ + switch (other.type_) { + case ControlTypeBool: + copyValue(other); + break; + case ControlTypeInteger32: + copyValue(other); + break; + case ControlTypeInteger64: + copyValue(other); + break; + case ControlTypeFloat: + copyValue(other); + break; + case ControlTypeCompoundBool: + copyValue>(other); + break; + case ControlTypeCompoundInt32: + copyValue>(other); + break; + case ControlTypeCompoundInt64: + copyValue>(other); + break; + case ControlTypeCompoundFloat: + copyValue>(other); + break; + default: + break; + } + + type_ = other.type_; + numElements_ = other.numElements_; + + return *this; +} + /** * \fn ControlValue::type() * \brief Retrieve the data type of the value From patchwork Tue Jan 14 16:31:53 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 2645 X-Patchwork-Delegate: jacopo@jmondi.org Return-Path: Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3CF106017C for ; Tue, 14 Jan 2020 17:29:50 +0100 (CET) X-Originating-IP: 2.224.242.101 Received: from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101]) (Authenticated sender: jacopo@jmondi.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id A4EB1E0006; Tue, 14 Jan 2020 16:29:49 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Tue, 14 Jan 2020 17:31:53 +0100 Message-Id: <20200114163153.39167-1-jacopo@jmondi.org> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200113164245.52535-23-jacopo@jmondi.org> References: <20200113164245.52535-23-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v1.1 22/23] libcamera: control_serializer: Add support for compound controls X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Jan 2020 16:29:50 -0000 Add support for serializing and deserializing control values which transport compound values. Signed-off-by: Jacopo Mondi --- sa rightfully noticed by Laurent in offline discussion, there was quite a big leak in the code as the Span<> class does not enforce memory ownership, so the memory location that is provided to it during data loading should be manually released. Fix this by wrapping it in an unique_ptr<> void ControlSerializer::loadData(ByteStreamBuffer &buffer, unsigned int count, ControlValue *value) { - Span data(new T[count], count); + std::unique_ptr mem(new T[count]); + Span data(mem.get(), count); buffer.read(&data); /* Valgrind does not report leaks anymore by complains about unique_ptr<> destructor getting confused. ==38940== Mismatched free() / delete / delete [] ==38940== at 0x4839EAB: operator delete(void*) (vg_replace_malloc.c:586) ==38940== by 0x4A02E6D: std::default_delete::operator()(int*) const (unique_ptr.h:81) ==38940== by 0x4A02BBC: std::unique_ptr >::~unique_ptr() (unique_ptr.h:284) ==38940== by 0x49FEF84: void libcamera::ControlSerializer::loadData(libcamera::ByteStreamBuffer&, unsigned int, libcamera::ControlValue*) (control_serializer.cpp:377) Might be a false positive as the heap is clean ==38940== HEAP SUMMARY: ==38940== in use at exit: 0 bytes in 0 blocks ==38940== total heap usage: 4,497 allocs, 4,497 frees, 1,350,869 bytes allocated --- src/libcamera/control_serializer.cpp | 124 ++++++++++++++++++--- src/libcamera/include/control_serializer.h | 10 +- 2 files changed, 118 insertions(+), 16 deletions(-) -- 2.24.1 diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index fd91baf15156..c24cb7a57195 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include "byte_stream_buffer.h" #include "log.h" @@ -30,11 +31,15 @@ LOG_DEFINE_CATEGORY(Serializer) namespace { static constexpr size_t ControlValueSize[] = { - [ControlTypeNone] = 1, - [ControlTypeBool] = sizeof(bool), - [ControlTypeInteger32] = sizeof(int32_t), - [ControlTypeInteger64] = sizeof(int64_t), - [ControlTypeFloat] = sizeof(float), + [ControlTypeNone] = 1, + [ControlTypeBool] = sizeof(bool), + [ControlTypeInteger32] = sizeof(int32_t), + [ControlTypeInteger64] = sizeof(int64_t), + [ControlTypeFloat] = sizeof(float), + [ControlTypeCompoundBool] = sizeof(bool), + [ControlTypeCompoundInt32] = sizeof(int32_t), + [ControlTypeCompoundInt64] = sizeof(int64_t), + [ControlTypeCompoundFloat] = sizeof(float), }; } /* namespace */ @@ -107,7 +112,7 @@ void ControlSerializer::reset() size_t ControlSerializer::binarySize(const ControlValue &value) { - return ControlValueSize[value.type()]; + return ControlValueSize[value.type()] * value.numElements(); } size_t ControlSerializer::binarySize(const ControlRange &range) @@ -183,6 +188,30 @@ void ControlSerializer::store(const ControlValue &value, break; } + case ControlTypeCompoundBool: { + Span data = value.get>(); + buffer.write(&data); + break; + } + + case ControlTypeCompoundInt32: { + Span data = value.get>(); + buffer.write(data); + break; + } + + case ControlTypeCompoundInt64: { + Span data = value.get>(); + buffer.write(data); + break; + } + + case ControlTypeCompoundFloat: { + Span data = value.get>(); + buffer.write(data); + break; + } + default: break; } @@ -318,7 +347,7 @@ int ControlSerializer::serialize(const ControlList &list, struct ipa_control_value_entry entry; entry.id = id; - entry.count = 1; + entry.count = value.numElements(); entry.type = value.type(); entry.offset = values.offset(); entries.write(&entry); @@ -332,35 +361,75 @@ int ControlSerializer::serialize(const ControlList &list, return 0; } +template +void ControlSerializer::loadData(ByteStreamBuffer &buffer, unsigned int count, + ControlValue *value) +{ + std::unique_ptr mem(new T[count]); + Span data(mem.get(), count); + buffer.read(&data); + + /* + * Use of ControlValue::set() guarantees the memory content + * is copied into the ControlValue. + */ + value->set(data); +} + template<> ControlValue ControlSerializer::load(ControlType type, - ByteStreamBuffer &b) + ByteStreamBuffer &buffer, + unsigned int count) { switch (type) { case ControlTypeBool: { bool value; - b.read(&value); + buffer.read(&value); return ControlValue(value); } case ControlTypeInteger32: { int32_t value; - b.read(&value); + buffer.read(&value); return ControlValue(value); } case ControlTypeInteger64: { int64_t value; - b.read(&value); + buffer.read(&value); return ControlValue(value); } case ControlTypeFloat: { float value; - b.read(&value); + buffer.read(&value); return ControlValue(value); } + case ControlTypeCompoundBool: { + ControlValue value; + loadData(buffer, count, &value); + return value; + } + + case ControlTypeCompoundInt32: { + ControlValue value; + loadData(buffer, count, &value); + return value; + } + + case ControlTypeCompoundInt64: { + ControlValue value; + loadData(buffer, count, &value); + return value; + } + + case ControlTypeCompoundFloat: { + ControlValue value; + loadData(buffer, count, &value); + return value; + } + default: return ControlValue(); } @@ -370,8 +439,32 @@ template<> ControlRange ControlSerializer::load(ControlType type, ByteStreamBuffer &b) { - ControlValue min = load(type, b); - ControlValue max = load(type, b); + /* + * The 'type' parameter represents the type of the Control + * the ControlRange refers to. Even if the Control is a compound, + * its range elements are not: adjust the type opportunely. + */ + ControlType rangeType; + switch (type) { + case ControlTypeCompoundBool: + rangeType = ControlTypeBool; + break; + case ControlTypeCompoundInt32: + rangeType = ControlTypeInteger32; + break; + case ControlTypeCompoundInt64: + rangeType = ControlTypeInteger64; + break; + case ControlTypeCompoundFloat: + rangeType = ControlTypeFloat; + break; + default: + rangeType = type; + break; + } + + ControlValue min = load(rangeType, b); + ControlValue max = load(rangeType, b); return ControlRange(min, max); } @@ -519,7 +612,8 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer } ControlType type = static_cast(entry.type); - ctrls.set(entry.id, load(type, values)); + ctrls.set(entry.id, + load(type, values, entry.count)); } return ctrls; diff --git a/src/libcamera/include/control_serializer.h b/src/libcamera/include/control_serializer.h index 55259913a2ca..2d76ffe7ed84 100644 --- a/src/libcamera/include/control_serializer.h +++ b/src/libcamera/include/control_serializer.h @@ -10,6 +10,7 @@ #include #include #include +#include #include @@ -40,8 +41,15 @@ private: static void store(const ControlValue &value, ByteStreamBuffer &buffer); static void store(const ControlRange &range, ByteStreamBuffer &buffer); + template::value>::type * = nullptr> + T load(ControlType type, ByteStreamBuffer &buffer, unsigned int count = 1); + template::value>::type * = nullptr> + T load(ControlType type, ByteStreamBuffer &buffer); template - T load(ControlType type, ByteStreamBuffer &b); + void loadData(ByteStreamBuffer &buffer, unsigned int count, + ControlValue *value); unsigned int serial_; std::vector> controlIds_;