completely new C++ persistence implementation by vpisarev · Pull Request #13011 · opencv/opencv

vpisarev · 2018-11-01T12:54:55Z

replaced the old persistence (CvMemStorage, CvSeq-based) with the new C++ implementation. The new persistence provides the same C++ API. The C API has gone. FileNodeIterator is now just one-directional, but the parsed file storage takes 3-4x less memory than in the previous implementation.

…OpenCV compiles fine! the tests have not been run yet

…amples from build

alalek

It would be nice to resolve interface/public API/public headers issues.

Implementation-specific issues have lower priority and they are mostly come from previous implementation.

alalek · 2018-11-02T02:00:39Z

modules/core/include/opencv2/core/persistence.hpp

        APPEND      = 2, //!< value, open the file for appending
        MEMORY      = 4, //!< flag, read data from source or write data to the internal buffer (which is
-                         //!< returned by FileStorage::release)
+        //!< returned by FileStorage::release)


unnecessary change

alalek · 2018-11-02T02:03:02Z

modules/core/include/opencv2/core/persistence.hpp

-    The full constructor opens the file. Alternatively you can use the default constructor and then
-    call FileStorage::open.
+     The full constructor opens the file. Alternatively you can use the default constructor and then
+     call FileStorage::open.


unnecessary indentation changes here and below

alalek · 2018-11-02T02:05:27Z

modules/core/include/opencv2/core/persistence.hpp

-    int state; //!< the writer state
-};
+    int state;
+    std::string elname;


Private implementation details should be avoided.

Also related enum (line 321) should go away from public headers:

enum { UNDEFINED = 0, VALUE_EXPECTED = 1, NAME_EXPECTED = 2, INSIDE_MAP = 4 };

Especially it is re-implemented here.

alalek · 2018-11-02T02:07:04Z

modules/core/include/opencv2/core/persistence.hpp


    /** @brief Returns type of the node.
-    @returns Type of the node. See FileNode::Type
+     @returns Type of the node. See FileNode::Type


FileNode::Type

Reference is broken because name of enum is dropped.
Why?

alalek · 2018-11-02T02:29:28Z

modules/core/src/persistence.cpp

+#endif
+}
+
+static inline void writeReal(uchar* p, double fval)


API of these functions doesn't look safe.
Who controls and checks size of available buffer?

alalek · 2018-11-02T04:41:41Z

modules/core/src/persistence.cpp

+{
+    const uchar* p = ptr();
+    if( !p || (*p & TYPE_MASK) != STRING )
+        return std::string();


Why not CV_Error() ?

alalek · 2018-11-02T04:42:11Z

modules/core/src/persistence.cpp

+    const uchar* p = ptr();
+    if( !p || (*p & TYPE_MASK) != STRING )
+        return std::string();
+    p += (*p & NAMED) ? 5 : 1;


5
1

Avoid magic numbers.

alalek · 2018-11-02T04:47:11Z

modules/core/include/opencv2/core/persistence.hpp

+     Usually it is more convenient to use operator `>>` instead of this method.
+     @param fmt Specification of each array element. See @ref format_spec "format specification"
+     @param vec Pointer to the destination array.
+     @param len Number of elements to read. If it is greater than number of remaining elements then all


Here it is used as buffer size.

yes, the description is wrong; it confused me too. it's always used as the buffer size. We need to fix it.

alalek · 2018-11-02T04:53:57Z

modules/features2d/test/test_matchers_algorithmic.cpp

+    "   -\n"
+    "      name: algorithm\n"
+    "      type: 9\n"  // FLANN_INDEX_TYPE_ALGORITHM
+    "      value: 6\n"// this line is changed!


this line is changed!

Do we still need this?
Where is it from? It is not a part of my drop CV_USRTYPE1 patch.

I basically copied 1:1 one of java tests that, since I did not want to debug java tests. If we remove FLANN sometimes (not in OpenCV 4.0, I guess), this test can be removed as well.

alalek · 2018-11-02T04:57:25Z

modules/objdetect/src/cascadedetect_convert.cpp

+        return false;
+    FileNode oldroot = oldfs.getFirstTopLevelNode();
+
+    bool ok = haar_cvt::convert(oldroot, newfs);


IMHO, This conversion code should be placed near traincascade app.

disagree. Support of the old-style XML haar detector formats does not take much code, it's one little function. And it let me not to convert the old XML cascades in the test data to the new format, but rather read and use them as-is. Let's retain it in objdetect.

But it definitely makes sense to remove completely the old C-API Haar/LBP detector code from objdetect before 4.0. I'll prepare the corresponding PR shortly.

vpisarev · 2018-11-02T08:09:45Z

@alalek, thank you for the thorough review! at least some of the points really make sense to address. Let's merge your 3.4=>master patch and then do some other big coarse-scale changes (move modules between opencv and opencv_contrib, remove some old stuff) and then I make extra PR or a few that fix some of the found issues

diepdtnse03145 · 2018-11-30T04:25:16Z

apps/CMakeLists.txt

  endif()
 endmacro()

-ocv_add_app(traincascade)


Why are createsamples and traincascade not included in this build?

* integrated the new C++ persistence; removed old persistence; most of OpenCV compiles fine! the tests have not been run yet * fixed multiple bugs in the new C++ persistence * fixed raw size of the parsed empty sequences * [temporarily] excluded obsolete applications traincascade and createsamples from build * fixed several compiler warnings and multiple test failures * undo changes in cocoa window rendering (that was fixed in another PR) * fixed more compile warnings and the remaining test failures (hopefully) * trying to fix the last little warning

vpisarev added 4 commits October 31, 2018 20:14

integrated the new C++ persistence; removed old persistence; most of …

253dba3

…OpenCV compiles fine! the tests have not been run yet

fixed multiple bugs in the new C++ persistence

feaf84d

fixed raw size of the parsed empty sequences

5a5298b

[temporarily] excluded obsolete applications traincascade and creates…

86290dc

…amples from build

vpisarev mentioned this pull request Nov 1, 2018

new C++ persistence opencv/opencv_contrib#1864

Merged

vpisarev added 4 commits November 1, 2018 19:18

fixed several compiler warnings and multiple test failures

ae3d0f0

undo changes in cocoa window rendering (that was fixed in another PR)

b4d2763

fixed more compile warnings and the remaining test failures (hopefully)

13f421c

trying to fix the last little warning

9a55eb0

vpisarev self-assigned this Nov 1, 2018

vpisarev merged commit 0f62220 into opencv:master Nov 1, 2018

alalek reviewed Nov 2, 2018

View reviewed changes

alalek mentioned this pull request Nov 2, 2018

Persistence refactoring: CUDA code is broken #13021

Open

vpisarev deleted the new_persistence2 branch November 3, 2018 23:35

diepdtnse03145 reviewed Nov 30, 2018

View reviewed changes

alalek mentioned this pull request Dec 3, 2018

core(persistence): fix KW issues #13346

Merged

alalek mentioned this pull request Jul 4, 2019

core: Persistence base64 encoding seems broken #14977

Closed

alalek mentioned this pull request Jul 25, 2019

core(persistence): added null ptr checks #15145

Merged

alalek mentioned this pull request Nov 7, 2019

core(persistence): add more checks for implementation limitations #15868

Merged

mshabunin mentioned this pull request Apr 2, 2020

null-pointer-dereference and buffer overflow: OpenCV 3.4 #16951

Open

alalek mentioned this pull request May 30, 2020

FileStorage access segfaults after assignment (assignment operator invalidates object) #17412

Closed

4 tasks

alalek mentioned this pull request Jan 29, 2021

test_io fails on master -> very basic coding error in persistence.cpp found via debugging and code review: no error checking fopen(). WTF? #19418

Open

4 tasks

alalek mentioned this pull request Jun 29, 2021

core(persistence): fix types format handling, fix 16F support #20321

Merged

alalek mentioned this pull request Jan 18, 2022

core(persistence): avoid NULL pointer dereference #21467

Merged

Uh oh!

Conversation

vpisarev commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpisarev Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpisarev commented Nov 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vpisarev commented Nov 1, 2018 •

edited

Loading

vpisarev Nov 2, 2018 •

edited

Loading