Skip to content
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

Reading Darknet from cv::FileStorage #11104

Merged
merged 3 commits into from
Jul 17, 2018
Merged

Reading Darknet from cv::FileStorage #11104

merged 3 commits into from
Jul 17, 2018

Conversation

tani
Copy link

@tani tani commented Mar 18, 2018

This pullrequest changes

I added new constructor for DarknetImporter to load from std::istream.
Current constructor gets to two pathnames which are raw strings. Without std::filesystem of C++17, some pathname has problem for cross-platform development. This changes makes abstractly to load files.

std::ifstream cfg("path/to/cfg");
std::ifstream model("path/to/model");
Net net = readNetFromDarknet(cfg, model);

And more, by this changes we will be able to include resources into binary, like followings.

std::istringstream cfg("some configuration text");
std::ifstream model("path/to/model");
Net net = readNetFromDarknet(cfg, model);
allow_multiple_commits=1

* @returns Network object that ready to do forward, throw an exception in failure cases.
* @returns Net object.
*/
CV_EXPORTS_W Net readNetFromDarknet(std::istream &cfgStream, std::istream &darknetModelStream);
Copy link
Member

@dkurt dkurt Mar 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due std::istream has no wrapping policy we need to use CV_EXPORTS here and below.

Copy link
Author

@tani tani Mar 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, now I will to replace std::istream to IStream of 3rdparty. Can I add 3rdparty/openexr/IlmImf/ImfIO.h ? Or what do I should use?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got your saying. I will change std::istream to the other buffered structure like #9994.

@@ -508,13 +502,9 @@ namespace cv {
return true;
}


bool ReadDarknetFromWeightsFile(const char *darknetModel, NetParameter *net)
bool ReadDarknetFromWeightsStream(std::istream &darknetModelStream, NetParameter *net)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind just rename darknetModelStream to ifile here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. I renamed.

@@ -179,7 +185,32 @@ class DarknetImporter

Net readNetFromDarknet(const String &cfgFile, const String &darknetModel /*= String()*/)
{
DarknetImporter darknetImporter(cfgFile.c_str(), darknetModel.c_str());
std::ifstream cfgStream(cfgFile.c_str());
CV_Assert(cfgStream.is_open());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were assertions include paths to files,

CV_Error(cv::Error::StsParseError, "Failed to parse NetParameter file: " + std::string(cfgFile));

I think it's useful to keep some kind of error message that prints actual paths.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so too. I added it.

@tani tani changed the title Reading Darknet from std::istream Reading Darknet from std::FileStorage Mar 19, 2018
@tani tani changed the title Reading Darknet from std::FileStorage Reading Darknet from cv::FileStorage Mar 19, 2018
@tani
Copy link
Author

tani commented Mar 19, 2018

@dkurt I added readNetFromDarknet(FileNode &cfgFile, FileNode &darknetModel) instread of the first request. This changes solves the topic of this request.

  1. The parameters in the file which has specific pathname, can pass into FileStorage with std::string which is read via std::ifstream.
  2. FileStorage can have the data in memory. So we will be able to include resources into binary, as const char*.

FileStorage ofs(".xml", FileStorage::WRITE | FileStorage::MEMORY);
ofs.write("cfgFile", buffer.str());
FileStorage ifs(ofs.releaseAndGetString(), FileStorage::READ | FileStorage::MEMORY | FileStorage::FORMAT_XML);
Net net = readNetFromDarknet(ifs["cfgFile"]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Asciian, please remind me why you decided to replace std::istream to cv::FileNode? The last one seems to be more complicated here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkurt Because std::istream couldn't become the arguments of exporting functions and knew no way to implements the topic of this issue without cv::FileNode. If you know that, Please tell me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkurt Ah, I know another way. With raw string, we can implements that. But the way requires adding the function two arguments which are used for telling the length of content.

ofs.write("cfgFile", buffer.str());
FileStorage ifs(ofs.releaseAndGetString(), FileStorage::READ | FileStorage::MEMORY | FileStorage::FORMAT_XML);
Net net = readNetFromDarknet(ifs["cfgFile"]);
ASSERT_FALSE(net.empty());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to check an accuracy too. Could you make some of existing accuracy tests to be parametric similar to one of our tests for loading models from a buffer?

TEST_P(Reproducibility_AlexNet, Accuracy)
{
bool readFromMemory = get<0>(GetParam());
Net net;
{
const string proto = findDataFile("dnn/bvlc_alexnet.prototxt", false);
const string model = findDataFile("dnn/bvlc_alexnet.caffemodel", false);
if (readFromMemory)
{
string dataProto;
ASSERT_TRUE(readFileInMemory(proto, dataProto));
string dataModel;
ASSERT_TRUE(readFileInMemory(model, dataModel));
net = readNetFromCaffe(dataProto.c_str(), dataProto.size(),
dataModel.c_str(), dataModel.size());
}
else
net = readNetFromCaffe(proto, model);
ASSERT_FALSE(net.empty());
}
net.setPreferableTarget(get<1>(GetParam()));
Mat sample = imread(_tf("grace_hopper_227.png"));
ASSERT_TRUE(!sample.empty());
net.setInput(blobFromImage(sample, 1.0f, Size(227, 227), Scalar(), false), "data");
Mat out = net.forward("prob");
Mat ref = blobFromNPY(_tf("caffe_alexnet_prob.npy"));
normAssert(ref, out);
}

@dkurt
Copy link
Member

dkurt commented Jul 4, 2018

@Asciian, I've reverted this changes back to std::istream because we cannot write a raw binary data (weights) into a text .xml file because it breaks a markup.

@tani
Copy link
Author

tani commented Jul 4, 2018

I got it. BTW, I guess std::istream isn’t exported for Python and Java. Do you have any idea? We have the only way via FileNode, don’t you?

@dkurt
Copy link
Member

dkurt commented Jul 5, 2018

@Asciian, I see your point. Please let me experiment a bit with a FileStorage and a binary data.

@vpisarev
Copy link
Contributor

vpisarev commented Jul 6, 2018

@dkurt, filestorage supports base64-encoded data; I suggest to use that. Maybe embedded a Darknet model (or any deep learning model) into a FileStorage is not very good idea in general and we or users can implement so alternative options. E.g. extend our Darknet parser to read the net from memory, if it was the original intention.

Asciian and others added 2 commits July 9, 2018 10:02
Remove some assertions

Replace std::ifstream to std::istream

Add test for new importer

Remove constructor to load file

Rename cfgStream and darknetModelStream to ifile

Add error notification to inform pathname to user

Use FileStorage instead of std::istream

Use FileNode instead of FileStorage

Fix typo
@dkurt
Copy link
Member

dkurt commented Jul 10, 2018

@Asciian, I've added methods which receive bytes buffers via std::vector<char>. In python you may use it in the following way:

import cv2 as cv
import numpy as np

with open('yolov3.cfg', 'rt') as f:
    cfgFile = bytearray(f.read())
with open('yolov3.weights', 'rb') as f:
    weightsFile = bytearray(f.read())

inp = np.random.standard_normal([1, 3, 416, 416]).astype(np.float32)

net = cv.dnn.readNetFromDarknet(cfgFile, weightsFile)
net.setInput(inp)
out = net.forward()

@tani
Copy link
Author

tani commented Jul 10, 2018

It looks good. However, as my comments,

  • cloud you tell me which I should use in JavaAPI? I'd like to try it in JavaAPI.
  • Now we have three way to read these networks, with string as filename, with bytearrays as content which requires 2 arguments, with string as buffers which requires 4 arguments. It very confuses users because there are almost string but they have different semantics. Would you like to redefine the interface for the development? If you don't mind, I'll open the new issue.

@dkurt
Copy link
Member

dkurt commented Jul 11, 2018

@Asciian, Thank you. I've fixed that. Please check dnn/misc/java/test/DnnTensorFlowTest.java file for details:

File modelFile = new File(modelFileName);
byte[] modelBuffer = new byte[ (int)modelFile.length() ];

try {
    FileInputStream fis = new FileInputStream(modelFile);
    fis.read(modelBuffer);
    fis.close();
} catch (IOException e) {
    System.out.println(e.getMessage());
}
net = Dnn.readNetFromTensorflow(new MatOfByte(modelBuffer));

@tani
Copy link
Author

tani commented Jul 11, 2018

That's a bytearray. Thank you. I said that std::vector<char> looks like string. But std::vector<uchar> a completely different from it. Please don't mind the second comments.

}
catch (Exception e) {
System.out.println(e.getMessage());
throw e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tests we can use this:

fail("DNN forward() failed: " + e.getMessage());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Fixed that.

@opencv-pushbot opencv-pushbot merged commit 8b5f061 into opencv:3.4 Jul 17, 2018
@alalek alalek mentioned this pull request Jul 17, 2018
@tani
Copy link
Author

tani commented Jul 18, 2018

@alalek and @dkurt I'm glad to see that this request is merged. Thank you so much.

@dkurt dkurt mentioned this pull request Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants