New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
load DNN from memory buffer #9994
Conversation
@@ -639,6 +639,11 @@ CV__DNN_EXPERIMENTAL_NS_BEGIN | |||
*/ | |||
CV_EXPORTS_W Net readNetFromTensorflow(const String &model, const String &config = String()); | |||
|
|||
/** @brief Reads a network model stored in Tensorflow model in memory | |||
*/ | |||
CV_EXPORTS_W Net readNetFromTensorflowBuffer(const char *bufferModel, size_t lenModel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the same name for it, readNetFromTensorflow
.
I think we can use brief description like at https://docs.opencv.org/master/d3/d63/classcv_1_1Mat.html#a2ec3402f7d165ca34c7fd6e8498a62ca: This is an overloaded member function...
where only parameters are described.
Have you tested python / Java bindings for it?
modules/dnn/src/tensorflow/tf_io.cpp
Outdated
@@ -52,6 +52,18 @@ bool ReadProtoFromBinaryFileTF(const char* filename, Message* proto) { | |||
return success; | |||
} | |||
|
|||
bool ReadProtoFromBinaryBufferTF(const char* data, size_t len, Message* proto) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to take existing two methods, remove std::ifstream
creation from them and replace const char* filename
argument to std::istream& is
one.
In result, we'll have something like this:
bool ReadProtoFromTextFileTF(std::istream& s, Message* proto) {
IstreamInputStream input(&is);
return google::protobuf::TextFormat::Parse(&input, proto);
}
bool ReadProtoFromTextFileTF(const char* buf, size_t len, Message* proto) {
std::istringstream s(std::string(buf, len));
return ReadProtoFromTextFileTF(s, proto);
}
bool ReadProtoFromTextFileTF(const char* filename, Message* proto) {
std::ifstream fs(filename, std::ios::in);
CHECK(fs.is_open()) << "Can't open \"" << filename << "\"";
return ReadProtoFromTextFileTF(fs, proto);
}
The same for binary version or just add boolean flags like text
for each of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with std::istringstream s(std::string(buf, len)) is that, in some cases, it creates a copy of the buffer into a std::string.
However ArrayInputStream input(data, len) guarantees that there is no copy
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.io.zero_copy_stream_impl_lite#ArrayInputStream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r2d3, we just vote to reduce a code duplication. There are two methods ReadProtoFromBinaryFileTF
and ReadProtoFromBinaryBufferTF
are different only at the beginning for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I will reduce code duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the common code in ReadProtoFromBinaryTF
.
I simplified the code in other functions. raw_input
and coded_input
are now created on stack to avoid new/delete.
1fec91e
to
8425aec
Compare
@@ -69,20 +69,61 @@ TEST(Test_TensorFlow, inception_accuracy) | |||
normAssert(ref, out); | |||
} | |||
|
|||
static bool readFileInMemory(const string& filename, char** data, size_t* len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r2d3, do you mind if we return a content by std::string
? In example,
static void readFileInMemory(const string& filename, std::string* content) {
std::ifstream ifs(filename.c_str());
CV_Assert(ifs.is_open());
ifs.seekg(0, std::ios::end);
content->resize(ifs.tellg());
ifs.seekg(0, std::ios::beg);
ifs.read(&content->operator[](0), content->size());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I was thinking about it as it is how I do file loading in personal tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
net = readNetFromTensorflow(dataModel.c_str(), dataModel.size(), | ||
dataConfig.c_str(), dataConfig.size()); | ||
ASSERT_FALSE(net.empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check is useful for both ways of importing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, test is now done for both ways of importing
Hello @dkurt As TensorFlow loading from memory is now working. I began working on Caffe one. I began moving all the ReadProtoFrom* functions to caffe_io.cpp and using them in tf_io.cpp. |
So readNetFromCaffe is now also implemented with a corresponding test (I had to duplicate the I do not know if we could implement the same mechanism for DarkNet and Torch ?! Regards |
* @param bufferModel buffer containing the content of the .caffemodel file | ||
* @param lenModel length of bufferModel | ||
*/ | ||
CV_EXPORTS_W Net readNetFromCaffe(const char *bufferProto, size_t lenProto, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a problem with using it in python because of null characters in serialized protocol buffers. I think we should use just CV_EXPORTS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use only CV_EXPORTS, the function will not be exposed to Python/Java interface.
I should look at how the binding generator works so the function accept Python str instead of string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't be exposed, because bindings don't work properly with native C++ pointers (at least they don't support buffers handling).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In modules/python/src2/hdr_parser.py
, I find a CV_CARRAY
that seems to be able to wrap arrays. But the corresponding #define
has disappeared from OpenCV headers.
121e40b
to
58520b6
Compare
@@ -55,6 +55,43 @@ static std::string _tf(TString filename) | |||
return (getOpenCVExtraDir() + "/dnn/") + filename; | |||
} | |||
|
|||
static bool readFileInMemory(const string& filename, string& content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind if we replace it to test_common.hpp
to reduce duplicated code? (see test_tf_importer.cpp
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. I was looking for this kind of common file and did not find it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return true; | ||
} | ||
|
||
TEST(Test_Caffe, memory_read) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to test an accuracy of one of the models too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the memory_read
test case (as I have done for Test_TensorFlow
) or in a separate test case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In example, replace TEST(Reproducibility_AlexNet, Accuracy)
to
typedef TestWithParam<tuple<bool> > Reproducibility_AlexNet;
TEST_P(Reproducibility_AlexNet, Accuracy) {
bool readFromMemory = get<0>(GetParam());
...
}
INSTANTIATE_TEST_CASE_P(Test_Caffe, Reproducibility_AlexNet, Values(
Bool()
));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
add a corresponding test
a922ca6
to
f723ced
Compare
👍 |
resolves #9765
This pullrequest changes
add an overloaded readNetFromTensorflow/readNetFromCaffe functions to read a TF/Caffe Net from data already in memory