From: Inki Dae Date: Fri, 31 May 2024 08:56:42 +0000 (+0900) Subject: introduce SharedBuffer class X-Git-Tag: accepted/tizen/unified/20240903.110722~34 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=9aa3406dcb613558b566b1e6eb298a9ab8609b74;p=platform%2Fcore%2Fapi%2Fsingleo.git introduce SharedBuffer class Introduce SharedBuffer class which can be commonly used by singleo sub modules. The purpose of SharedBuffer is to share buffer objects - such as ImageDataType and RawDataType - between sub modules to avoid from memory copy. In this patch, SharedBuffer is used to share the input data objects between nodes. The shared buffer objects will be released if no nodes who use the objects. I.e., with the test case, GraphD -----> face_landmark_detection -------- ... ---> bridge --| |----> ... -----> face_detection ----------------- Bridge node creates a SharedBuffer object with a new ImageDataType buffer, and shares the SharedBuffer object with two nodes - one for face_landmark_detection and other for face_detection without memory copy. And then the ImageDataType buffer will be released by release() function of ImageDataType structure after the face_landmark_detection and face_detection nodes are completed. In addition, this patch fixes a critial behavior issue that it does deep-copy even type-casting, which leads unexpected memory leak. Therefore, this patch moves deep-copy code into clone() of ImageDataType structure because keeping the deep-copy code in ctor of ImageDataType structure has two behaviors - one is deep-copy and other is just type-casting. In general, type-casting never intend deep-copy so it's a wrong behavior. Change-Id: I29e9ffb2ab8c2bdc9f4fbb67ec5b1591596bb291 Signed-off-by: Inki Dae --- diff --git a/common/include/SharedBuffer.h b/common/include/SharedBuffer.h new file mode 100644 index 0000000..aa6e9ac --- /dev/null +++ b/common/include/SharedBuffer.h @@ -0,0 +1,63 @@ +/** + * Copyright (c) 2024 Samsung Electronics Co., Ltd All Rights Reserved + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef __SHARED_BUFFER_H__ +#define __SHARED_BUFFER_H__ + +#include +#include +#include + +#include "SingleoCommonTypes.h" + +namespace singleo +{ +class SharedBuffer +{ +private: + std::vector > _data; + std::atomic ref_count; + +public: + SharedBuffer() = default; + ~SharedBuffer() = default; + + void addInput(std::shared_ptr data) + { + _data.push_back(data); + } + + std::vector > &getInputs() + { + return _data; + } + + void addRef() + { + ref_count.fetch_add(1, std::memory_order_relaxed); + } + + void release() + { + if (ref_count.fetch_sub(1, std::memory_order_acq_rel) == 1) + for (auto &data : _data) + data->release(); + } +}; + +} + +#endif \ No newline at end of file diff --git a/common/include/SingleoCommonTypes.h b/common/include/SingleoCommonTypes.h index 1e842ba..fd9a0fb 100644 --- a/common/include/SingleoCommonTypes.h +++ b/common/include/SingleoCommonTypes.h @@ -45,12 +45,15 @@ enum class ImagePixelFormat { NONE, YUV420, RGB888 }; struct BaseDataType { DataType _data_type { DataType::NONE }; + bool is_owned { false }; + BaseDataType(DataType data_type) : _data_type(data_type) {} virtual ~BaseDataType() {} virtual std::shared_ptr clone() = 0; + virtual void release() = 0; }; struct FileDataType : public BaseDataType { @@ -62,6 +65,9 @@ struct FileDataType : public BaseDataType { { return std::make_shared(*this); } + + void release() override + {} }; struct ImageDataType : public BaseDataType { @@ -70,33 +76,38 @@ struct ImageDataType : public BaseDataType { unsigned int height {}; unsigned int byte_per_pixel {}; ImagePixelFormat pixel_format { ImagePixelFormat::NONE }; - bool custom { false }; ImageDataType() : BaseDataType(DataType::IMAGE) {} - ImageDataType(const ImageDataType &other) : BaseDataType(DataType::IMAGE) + std::shared_ptr clone() override { - unsigned int size = other.width * other.height * other.byte_per_pixel; + auto newImage = std::make_shared(); + + unsigned int size = width * height * byte_per_pixel; if (size == 0) { SINGLEO_LOGE("ImageDataType::operator=() : Invalid size"); throw singleo::exception::InvalidOperation("ImageDataType::operator=() : Invalid size"); } - ptr = new unsigned char[size]; - memcpy(ptr, other.ptr, size); + newImage->ptr = new unsigned char[size]; + memcpy(newImage->ptr, this->ptr, size); - width = other.width; - height = other.height; - byte_per_pixel = other.byte_per_pixel; - pixel_format = other.pixel_format; - } + newImage->width = width; + newImage->height = height; + newImage->byte_per_pixel = byte_per_pixel; + newImage->pixel_format = pixel_format; + newImage->is_owned = true; - ImageDataType &operator=(const ImageDataType &other) = delete; + return newImage; + } - std::shared_ptr clone() override + void release() override { - return std::make_shared(*this); + if (is_owned) { + delete[] ptr; + ptr = nullptr; + } } }; @@ -110,6 +121,14 @@ struct RawDataType : public BaseDataType { { return std::make_shared(*this); } + + void release() override + { + if (is_owned) { + delete[] ptr; + ptr = nullptr; + } + } }; enum class ResultType { NONE, OBJECT_DETECTION, FACE_DETECTION, FACE_LANDMARK }; diff --git a/services/auto_zoom/src/AutoZoom.cpp b/services/auto_zoom/src/AutoZoom.cpp index 4657ee3..b4a4ddc 100644 --- a/services/auto_zoom/src/AutoZoom.cpp +++ b/services/auto_zoom/src/AutoZoom.cpp @@ -91,23 +91,29 @@ void AutoZoom::inputFeedCb(BaseDataType &data) { ImagePreprocessor preprocessor(data); ImageDataType preprocessed = dynamic_cast(preprocessor.getData()); - ImageDataType copied(preprocessed); + auto copied = dynamic_pointer_cast(preprocessed.clone()); + + // Set is_owned to false to prevent from releasing copied image data + // because the owner of copied is AutoZoom so the copied will be released + // in performAsync function. + // Ps. if is_owned is true, copied image data can be released while task manager is being performed. + copied->is_owned = false; if (_user_cb) { // if postprocessor isn't in progress, postprocess current camera preview image. if (!_postprocessor->isWorking()) { - ImageDataType zoom_data = dynamic_cast(_postprocessor->getOutput(copied)); + ImageDataType zoom_data = dynamic_cast(_postprocessor->getOutput(*copied)); _user_cb(zoom_data.ptr, zoom_data.width, zoom_data.height, zoom_data.byte_per_pixel, _user_data); } else { - _user_cb(copied.ptr, copied.width, copied.height, copied.byte_per_pixel, _user_data); + _user_cb(copied->ptr, copied->width, copied->height, copied->byte_per_pixel, _user_data); } } // Make sure to release copied buffer if incoming queue isn't empty so skipped pushing the buffer. // If empty then push the buffer to the incoming queue. This buffer will be released at the end of runTaskManager function. - if (_async_manager->pushInput(copied) != SINGLEO_ERROR_NONE) - delete copied.ptr; + if (_async_manager->pushInput(*copied) != SINGLEO_ERROR_NONE) + delete copied->ptr; } bool AutoZoom::isKeyValid(std::string key) @@ -183,8 +189,9 @@ void AutoZoom::performAsync() auto imageData = dynamic_cast(data); - // A given imageData has been used so release it here. - delete imageData.ptr; + // A given imageData - allocated by inputFeedCb function - has been used so release it here. + if (!imageData.is_owned) + delete imageData.ptr; }); } diff --git a/services/task_manager/include/BridgeNode.h b/services/task_manager/include/BridgeNode.h index fe5199e..b8f497d 100644 --- a/services/task_manager/include/BridgeNode.h +++ b/services/task_manager/include/BridgeNode.h @@ -50,6 +50,8 @@ public: std::copy(d->results().begin(), d->results().end(), std::back_inserter(_results)); _cb(this); + + _inputBuffer->release(); } }; diff --git a/services/task_manager/include/CallbackNode.h b/services/task_manager/include/CallbackNode.h index 20b7cae..c343255 100644 --- a/services/task_manager/include/CallbackNode.h +++ b/services/task_manager/include/CallbackNode.h @@ -22,6 +22,7 @@ #include "INode.h" #include "SingleoException.h" +#include "SharedBuffer.h" namespace singleo { @@ -34,8 +35,8 @@ protected: std::string _name; std::vector > _dependencies; NodeCb _cb; - std::vector > _inputs; - std::shared_ptr _output; + std::shared_ptr _inputBuffer; + std::shared_ptr _outputBuffer; std::vector > _results; bool _completed { false }; std::condition_variable _event; @@ -50,12 +51,12 @@ public: { return _name; } - void addInput(std::shared_ptr input) override; - std::vector > &getInputs() override; + void setInputBuffer(std::shared_ptr inputBuffer) override; + std::shared_ptr &getInputBuffer() override; void addDependency(std::shared_ptr node) override; std::vector > &getDependencies() override; - void setOutput(std::shared_ptr output) override; - std::shared_ptr &getOutput() override; + void setOutputBuffer(std::shared_ptr outputBuffer) override; + std::shared_ptr &getOutputBuffer() override; void wait() override; void wakeup() override; virtual void configure() = 0; diff --git a/services/task_manager/include/EndpointNode.h b/services/task_manager/include/EndpointNode.h index cf9d073..3ef78cb 100644 --- a/services/task_manager/include/EndpointNode.h +++ b/services/task_manager/include/EndpointNode.h @@ -47,6 +47,8 @@ public: if (_cb) _cb(this); + + _inputBuffer->release(); } }; diff --git a/services/task_manager/include/INode.h b/services/task_manager/include/INode.h index 80ce83f..0e0b037 100644 --- a/services/task_manager/include/INode.h +++ b/services/task_manager/include/INode.h @@ -23,6 +23,7 @@ #include "SingleoCommonTypes.h" #include "ServiceDataType.h" +#include "SharedBuffer.h" namespace singleo { @@ -37,12 +38,12 @@ public: virtual NodeType getType() = 0; virtual std::string &getName() = 0; - virtual void addInput(std::shared_ptr input) = 0; - virtual std::vector > &getInputs() = 0; + virtual void setInputBuffer(std::shared_ptr inputBuffer) = 0; + virtual std::shared_ptr &getInputBuffer() = 0; virtual void addDependency(std::shared_ptr node) = 0; virtual std::vector > &getDependencies() = 0; - virtual void setOutput(std::shared_ptr output) = 0; - virtual std::shared_ptr &getOutput() = 0; + virtual void setOutputBuffer(std::shared_ptr outputBuffer) = 0; + virtual std::shared_ptr &getOutputBuffer() = 0; virtual void configure() = 0; virtual void invoke() = 0; virtual std::vector > &results() = 0; diff --git a/services/task_manager/include/TaskNode.h b/services/task_manager/include/TaskNode.h index 38807c9..a63b389 100644 --- a/services/task_manager/include/TaskNode.h +++ b/services/task_manager/include/TaskNode.h @@ -22,6 +22,7 @@ #include "INode.h" #include "SingleoException.h" +#include "SharedBuffer.h" namespace singleo { @@ -33,8 +34,8 @@ protected: NodeType _type { NodeType::NONE }; std::string _name; std::vector > _dependencies; - std::vector > _inputs; - std::shared_ptr _output; + std::shared_ptr _inputBuffer; + std::shared_ptr _outputBuffer; bool _completed { false }; std::condition_variable _event; std::mutex _mutex; @@ -48,12 +49,12 @@ public: { return _name; } - void addInput(std::shared_ptr input) override; - std::vector > &getInputs() override; + void setInputBuffer(std::shared_ptr inputBuffer) override; + std::shared_ptr &getInputBuffer() override; void addDependency(std::shared_ptr node) override; std::vector > &getDependencies() override; - void setOutput(std::shared_ptr output) override; - std::shared_ptr &getOutput() override; + void setOutputBuffer(std::shared_ptr outputBuffer) override; + std::shared_ptr &getOutputBuffer() override; void wait() override; void wakeup() override; virtual void configure() = 0; diff --git a/services/task_manager/src/CallbackNode.cpp b/services/task_manager/src/CallbackNode.cpp index 46d5e0a..ab9664f 100644 --- a/services/task_manager/src/CallbackNode.cpp +++ b/services/task_manager/src/CallbackNode.cpp @@ -31,14 +31,15 @@ NodeType CallbackNode::getType() return _type; } -void CallbackNode::addInput(shared_ptr input) +void CallbackNode::setInputBuffer(shared_ptr inputBuffer) { - _inputs.push_back(input); + inputBuffer->addRef(); + _inputBuffer = inputBuffer; } -vector > &CallbackNode::getInputs() +shared_ptr &CallbackNode::getInputBuffer() { - return _inputs; + return _inputBuffer; } void CallbackNode::setCb(const NodeCb &cb) @@ -56,14 +57,15 @@ std::vector > &CallbackNode::getDependencies() return _dependencies; } -void CallbackNode::setOutput(std::shared_ptr output) +void CallbackNode::setOutputBuffer(std::shared_ptr outputBuffer) { - _output = output; + outputBuffer->addRef(); + _outputBuffer = outputBuffer; } -std::shared_ptr &CallbackNode::getOutput() +std::shared_ptr &CallbackNode::getOutputBuffer() { - return _output; + return _outputBuffer; } std::vector > &CallbackNode::results() diff --git a/services/task_manager/src/InferenceNode.cpp b/services/task_manager/src/InferenceNode.cpp index c62a4a3..a04176b 100644 --- a/services/task_manager/src/InferenceNode.cpp +++ b/services/task_manager/src/InferenceNode.cpp @@ -37,21 +37,19 @@ void InferenceNode::configure() void InferenceNode::invoke() { - // TODO. consider for multiple inputs later. - _task->invoke(*_inputs[0]); + auto &inputs = _inputBuffer->getInputs(); - auto input = dynamic_pointer_cast(_inputs[0]); + // TODO. consider for multiple inputs later. + _task->invoke(*inputs[0]); - // Inference request has been completed so release input data if the data was internally allocated - // by callback node. - if (input->custom) - delete input->ptr; + // Inference request has been completed so release input data. + _inputBuffer->release(); - _inputs.clear(); _results.clear(); _resultMutex.lock(); _results.push_back(_task->result().clone()); + _resultMutex.unlock(); } diff --git a/services/task_manager/src/TaskManager.cpp b/services/task_manager/src/TaskManager.cpp index c84614f..2a46402 100644 --- a/services/task_manager/src/TaskManager.cpp +++ b/services/task_manager/src/TaskManager.cpp @@ -56,12 +56,20 @@ void TaskManager::threadCb(shared_ptr &node) if (!node->getDependencies().empty()) { // TODO. consider for multiple dependencies later. auto &callbackNode = node->getDependencies()[0]; + auto &outputBuffer = callbackNode->getOutputBuffer(); - node->addInput(callbackNode->getOutput()); + node->setInputBuffer(outputBuffer); + + // output buffer has been shared to node so release it here. + outputBuffer->release(); } } else { // TODO. consider for mulitple inputs later. - node->addInput(_inputs[0]); + auto inputBuffer = make_shared(); + for (auto &i : _inputs) + inputBuffer->addInput(i); + + node->setInputBuffer(inputBuffer); } node->invoke(); @@ -70,9 +78,11 @@ void TaskManager::threadCb(shared_ptr &node) void TaskManager::addInput(BaseDataType &input) { + auto new_input = make_shared(); auto imageData = dynamic_cast(input); - _inputs.push_back(make_shared(imageData)); + *new_input = imageData; + _inputs.push_back(new_input); } vector > &TaskManager::getInputs() @@ -166,6 +176,11 @@ void TaskManager::run() std::vector > threads; + auto inputBuffer = make_shared(); + + for (auto &i : _inputs) + inputBuffer->addInput(i); + for (auto &n : _nodes) { // Set input as source of current node. // If no dependency then this node has to receive input source @@ -178,7 +193,7 @@ void TaskManager::run() } // TODO. consider for multiple sources later. - n->addInput(_inputs[0]); + n->setInputBuffer(inputBuffer); } // Wait until all nodes added to this node as dependency are completed diff --git a/services/task_manager/src/TaskNode.cpp b/services/task_manager/src/TaskNode.cpp index f32936f..d11dfc9 100644 --- a/services/task_manager/src/TaskNode.cpp +++ b/services/task_manager/src/TaskNode.cpp @@ -27,14 +27,15 @@ NodeType TaskNode::getType() return _type; } -void TaskNode::addInput(shared_ptr input) +void TaskNode::setInputBuffer(shared_ptr inputBuffer) { - _inputs.push_back(input); + inputBuffer->addRef(); + _inputBuffer = inputBuffer; } -vector > &TaskNode::getInputs() +shared_ptr &TaskNode::getInputBuffer() { - return _inputs; + return _inputBuffer; } void TaskNode::addDependency(std::shared_ptr node) @@ -47,14 +48,15 @@ std::vector > &TaskNode::getDependencies() return _dependencies; } -void TaskNode::setOutput(std::shared_ptr output) +void TaskNode::setOutputBuffer(std::shared_ptr outputBuffer) { - _output = output; + outputBuffer->addRef(); + _outputBuffer = outputBuffer; } -std::shared_ptr &TaskNode::getOutput() +std::shared_ptr &TaskNode::getOutputBuffer() { - return _output; + return _outputBuffer; } void TaskNode::wait() diff --git a/test/services/test_task_manager.cpp b/test/services/test_task_manager.cpp index a1dde3d..48db6bb 100644 --- a/test/services/test_task_manager.cpp +++ b/test/services/test_task_manager.cpp @@ -38,13 +38,12 @@ using namespace singleo::services; void BridgeNodeCallback(INode *node) { auto callbackNode = dynamic_cast(node); - shared_ptr imageData = dynamic_pointer_cast(callbackNode->getInputs()[0]); - ImageDataType newImage(*imageData); + auto &inputBuffer = callbackNode->getInputBuffer(); + auto newBaseData = inputBuffer->getInputs()[0]->clone(); + auto newImage = dynamic_pointer_cast(newBaseData); const int answer[][4] = { { 553, 87, 583, 129 }, { 397, 110, 427, 149 } }; - newImage.custom = true; - - cv::Mat cv_image(cv::Size(newImage.width, newImage.height), CV_MAKETYPE(CV_8U, 3), newImage.ptr); + cv::Mat cv_image(cv::Size(newImage->width, newImage->height), CV_MAKETYPE(CV_8U, 3), newImage->ptr); auto &results = callbackNode->results(); for (auto r : results) { @@ -61,7 +60,9 @@ void BridgeNodeCallback(INode *node) } } - callbackNode->setOutput(make_shared(newImage)); + auto outputBuffer = make_shared(); + outputBuffer->addInput(newBaseData); + callbackNode->setOutputBuffer(outputBuffer); } // GraphA : input ----> face_detection ----> bridge ----> face_landmark_detection ----> endpoint ----> output @@ -207,7 +208,7 @@ void LastNodeCallback(INode *node) // -----> bridge -----> face_landmark_detection -------- // input -----> face_detection --| |----> endpoint ----> output // ----------------------------------------------------- -TEST(SingloTaskManager, MultipleNodesBasedGraphDShouldWork) +TEST(SingloTaskManager, MultipleNodesBasedGraphCShouldWork) { cv::Mat cv_image = cv::imread(IMG_FACE, cv::IMREAD_COLOR); cv::cvtColor(cv_image, cv_image, cv::COLOR_BGR2RGB); @@ -279,3 +280,85 @@ TEST(SingloTaskManager, MultipleNodesBasedGraphDShouldWork) taskManager->clear(); } } + +// GraphD: +// -----> face_landmark_detection -------- +// input -----> face_detection ------> bridge --| |----> endpoint ----> output +// -----> face_detection ----------------- +TEST(SingloTaskManager, MultipleNodesBasedGraphDShouldWork) +{ + cv::Mat cv_image = cv::imread(IMG_FACE, cv::IMREAD_COLOR); + cv::cvtColor(cv_image, cv_image, cv::COLOR_BGR2RGB); + const int fd_answer[][4] = { { 553, 87, 583, 129 }, { 397, 110, 427, 149 } }; + const int fld_answer[][2] = { { 243, 133 }, { 404, 135 }, { 347, 197 }, { 250, 266 }, { 385, 266 } }; + + ASSERT_FALSE(cv_image.empty()); + + ImageDataType image_data; + + image_data.width = cv_image.cols; + image_data.height = cv_image.rows; + image_data.byte_per_pixel = cv_image.channels(); + image_data.ptr = cv_image.data; + + auto factory = InferenceTaskFactory::instance().create("MvInferenceTaskFactory"); + + auto taskManager = make_unique(); + const unsigned int maxIteration = 10; + + for (unsigned int cnt = 0; cnt < maxIteration; ++cnt) { + taskManager->addInput(image_data); + + auto face_detection_node_a = make_shared("face_detection_a"); + face_detection_node_a->setInferenceTask(factory->createFaceDetection()); + taskManager->addNode(face_detection_node_a); + + auto bridge_node = make_shared(); + bridge_node->setCb(BridgeNodeCallback); + bridge_node->addDependency(face_detection_node_a); + taskManager->addNode(bridge_node); + + auto face_landmark_node = make_shared("face_landmark"); + face_landmark_node->setInferenceTask(factory->createFaceLandmarkDetection()); + face_landmark_node->addDependency(bridge_node); + taskManager->addNode(face_landmark_node); + + auto face_detection_node_b = make_shared("face_detection_b"); + face_detection_node_b->setInferenceTask(factory->createFaceDetection()); + face_detection_node_b->addDependency(bridge_node); + taskManager->addNode(face_detection_node_b); + + auto endpoint_node = make_shared(); + endpoint_node->setCb(LastNodeCallback); + endpoint_node->addDependency(face_detection_node_b); + endpoint_node->addDependency(face_landmark_node); + taskManager->addNode(endpoint_node); + + taskManager->run(); + + auto results = taskManager->output(); + for (auto &result : results) { + if (result->_type == ResultType::FACE_DETECTION) { + auto fd_result = dynamic_pointer_cast(result); + unsigned int idx = 0; + + for (auto rect : fd_result->_rects) { + ASSERT_EQ(rect.left, fd_answer[idx][0]); + ASSERT_EQ(rect.top, fd_answer[idx][1]); + ASSERT_EQ(rect.right, fd_answer[idx][2]); + ASSERT_EQ(rect.bottom, fd_answer[idx++][3]); + } + } else { + auto fld_result = dynamic_pointer_cast(result); + unsigned int idx = 0; + + for (auto &point : fld_result->_points) { + ASSERT_EQ(point.x, fld_answer[idx][0]); + ASSERT_EQ(point.y, fld_answer[idx++][1]); + } + } + } + + taskManager->clear(); + } +}