Fix bug with int64 support for FileStorage #26846

MaximSmolskiy · 2025-01-26T14:07:07Z

Pull Request Readiness Checklist

In current implementation of int64 support raw size of recorded integer is variable (4 or 8 bytes depending on value). But then we iterate over nodes we need to know it exact value

opencv/modules/core/src/persistence.cpp

Lines 2596 to 2609 in dfad11a

    
           FileNodeIterator& FileNodeIterator::operator ++ () 
        
           { 
        
               if( idx == nodeNElems || !fs ) 
        
                   return *this; 
        
               idx++; 
        
               FileNode n(fs, blockIdx, ofs); 
        
               ofs += n.rawSize(); 
        
               if( ofs >= blockSize ) 
        
               { 
        
                   fs->normalizeNodeOfs(blockIdx, ofs); 
        
                   blockSize = fs->fs_data_blksz[blockIdx]; 
        
               } 
        
               return *this; 
        
           }

Bug is that rawSize method still return 4 for any integer. I haven't figured out a way how to get variable raw size for integer in this method. I made raw size for integer is constant and equal to 8.

Yes, after this patch memory consumption for integers will increase, but I don't know a better way to do it yet. At least this fixes bug and implementation becomes more correct

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

dkurt · 2025-01-27T07:08:17Z

Yes, after this patch memory consumption for integers will increase, but I don't know a better way to do it yet. At least this fixes bug and implementation becomes more correct

Probably, this fix may break compatibility on reading raw data serialized before with 4 bytes per int, doesn't it?

dkurt · 2025-01-27T07:16:47Z

@MaximSmolskiy, May I also ask you to open a PR with similar patch to 5.x branch? In such way we will test CV_64S Mat read/write too: https://github.com/opencv/opencv/pull/26399/files#diff-cf43289d2308f02754d618a5d975c44123ebb553fe074ccff700c7328a49badfR2078

modules/core/test/test_io.cpp

MaximSmolskiy · 2025-01-27T20:40:31Z

Yes, after this patch memory consumption for integers will increase, but I don't know a better way to do it yet. At least this fixes bug and implementation becomes more correct

Probably, this fix may break compatibility on reading raw data serialized before with 4 bytes per int, doesn't it?

Yes, it is. But I can't think of any example code to do that. Can you please provide one?

MaximSmolskiy · 2025-01-27T23:12:31Z

@MaximSmolskiy, May I also ask you to open a PR with similar patch to 5.x branch? In such way we will test CV_64S Mat read/write too: https://github.com/opencv/opencv/pull/26399/files#diff-cf43289d2308f02754d618a5d975c44123ebb553fe074ccff700c7328a49badfR2078

#26852

dkurt · 2025-01-28T07:59:49Z

Yes, after this patch memory consumption for integers will increase, but I don't know a better way to do it yet. At least this fixes bug and implementation becomes more correct

Probably, this fix may break compatibility on reading raw data serialized before with 4 bytes per int, doesn't it?

Yes, it is. But I can't think of any example code to do that. Can you please provide one?

Never mind, I just checked that both 5.x and PR produces the same base64 raw data outputs for int32. Thanks for the patch!

@staticmethod
    def get_normal_2d_mat():
        rows = 10
        cols = 20
        cn = 3
        image = np.zeros((rows, cols, cn), np.int32)
        image[:] = (1, 2, 127)
        for i in range(rows):
            for j in range(cols):
                image[i, j, 1] = (i + j) % 256
        return image
def write_base64_json(self, fname):
        fname = "/home/d.kurtaev/test_file.json"
        fs = cv.FileStorage(fname, cv.FileStorage_WRITE_BASE64)
        mats = {'normal_2d_mat': self.get_normal_2d_mat()}
        for name, mat in mats.items():
            fs.write(name, mat)
        fs.release()

0c16e58d13286e0d1b489e71be614f7b2a0fa57313ee36dd9b6329b7c6383976  /home/d.kurtaev/test_file_pr.json
0c16e58d13286e0d1b489e71be614f7b2a0fa57313ee36dd9b6329b7c6383976  /home/d.kurtaev/test_file_5.x.json

However seems like we should fix int64 base64 write mode later (in a separate PR for sure):

diff --git a/modules/python/test/test_filestorage_io.py b/modules/python/test/test_filestorage_io.py
index 01e0a72300..5ef0fd26ee 100644
--- a/modules/python/test/test_filestorage_io.py
+++ b/modules/python/test/test_filestorage_io.py
@@ -124,7 +124,7 @@ class filestorage_io_test(NewOpenCVTests):
         cols = 20
         cn = 3
 
-        image = np.zeros((rows, cols, cn), np.uint8)
+        image = np.zeros((rows, cols, cn), np.int64)
         image[:] = (1, 2, 127)
 
         for i in range(rows):

Testing OpenCV 5.0.0-pre
Local repo path: None
F...
======================================================================
FAIL: test_base64 (__main__.filestorage_io_test.test_base64)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/d.kurtaev/opencv/modules/python/test/test_filestorage_io.py", line 118, in test_base64
    self.write_base64_json(fname)
  File "/home/d.kurtaev/opencv/modules/python/test/test_filestorage_io.py", line 202, in write_base64_json
    self.assertEqual(buffer, self.decode(data[name]['data']))
AssertionError: b'\x0[26 chars]x00\x00\x00\x00\x00\x00\x00\x00\x00\x7f\x00\x0[19061 chars]\x00' != b'\x0[26 chars]x00\x7f\x00\x00\x00\x01\x00\x00\x00\x01\x00\x0[9461 chars]\x00'
----------------------------------------------------------------------
Ran 4 tests in 0.009s
FAILED (failures=1)

…test_base64-for-64-bit-integer-types Support 64-bit integer types for FileStorage Base64 #26871 ### Pull Request Readiness Checklist Related to #26846 (comment) OpenCV extra: opencv/opencv_extra#1232 See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake

…_support_for_FileStorage Fix bug with int64 support for FileStorage opencv#26846 ### Pull Request Readiness Checklist Fix opencv#26829, opencv/opencv-python#1078 In current implementation of `int64` support raw size of recorded integer is variable (`4` or `8` bytes depending on value). But then we iterate over nodes we need to know it exact value https://github.com/opencv/opencv/blob/dfad11aae7ef3b3a0643379266bc363b1a9c3d40/modules/core/src/persistence.cpp#L2596-L2609 Bug is that `rawSize` method still return `4` for any integer. I haven't figured out a way how to get variable raw size for integer in this method. I made raw size for integer is constant and equal to `8`. Yes, after this patch memory consumption for integers will increase, but I don't know a better way to do it yet. At least this fixes bug and implementation becomes more correct See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake

Fix bug with int64 support for FileStorage

56ddb98

MaximSmolskiy force-pushed the fix_bug_with_int64_support_for_FileStorage branch from dfad11a to 56ddb98 Compare January 27, 2025 05:15

asmorkalov requested a review from dkurt January 27, 2025 07:06

dkurt self-assigned this Jan 27, 2025

asmorkalov added this to the 4.12.0 milestone Jan 27, 2025

asmorkalov reviewed Jan 27, 2025

View reviewed changes

modules/core/test/test_io.cpp Outdated Show resolved Hide resolved

modules/core/test/test_io.cpp Show resolved Hide resolved

opencv-alalek added bug category: core labels Jan 27, 2025

Fix review issue

dd293da

MaximSmolskiy mentioned this pull request Jan 27, 2025

Fix bug with int64 support for FileStorage #26852

Merged

6 tasks

MaximSmolskiy requested a review from asmorkalov January 27, 2025 23:12

dkurt mentioned this pull request Jan 28, 2025

Regression in opencv 4.11 when reading double precision matrix data from yaml files opencv/opencv-python#1078

Closed

dkurt approved these changes Jan 28, 2025

View reviewed changes

asmorkalov closed this Jan 28, 2025

asmorkalov reopened this Jan 28, 2025

asmorkalov merged commit 08a24ba into opencv:4.x Jan 28, 2025
30 of 31 checks passed

MaximSmolskiy deleted the fix_bug_with_int64_support_for_FileStorage branch January 28, 2025 10:43

opencv-alalek added the port/backport done Label for maintainers. Authors of PR can ignore this label Jan 29, 2025

MaximSmolskiy mentioned this pull request Feb 2, 2025

Support 64-bit integer types for FileStorage Base64 #26871

Merged

6 tasks

asmorkalov mentioned this pull request Feb 19, 2025

5.x merge 4.x #26939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix bug with int64 support for FileStorage #26846

Fix bug with int64 support for FileStorage #26846

Uh oh!

MaximSmolskiy commented Jan 26, 2025 •

edited by dkurt

Loading

Uh oh!

dkurt commented Jan 27, 2025

Uh oh!

dkurt commented Jan 27, 2025

Uh oh!

Uh oh!

Uh oh!

MaximSmolskiy commented Jan 27, 2025 •

edited

Loading

Uh oh!

MaximSmolskiy commented Jan 27, 2025

Uh oh!

dkurt commented Jan 28, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

	FileNodeIterator& FileNodeIterator::operator ++ ()
	{
	if( idx == nodeNElems \|\| !fs )
	return *this;
	idx++;
	FileNode n(fs, blockIdx, ofs);
	ofs += n.rawSize();
	if( ofs >= blockSize )
	{
	fs->normalizeNodeOfs(blockIdx, ofs);
	blockSize = fs->fs_data_blksz[blockIdx];
	}
	return *this;
	}

Uh oh!

Fix bug with int64 support for FileStorage #26846

Fix bug with int64 support for FileStorage #26846

Uh oh!

Conversation

MaximSmolskiy commented Jan 26, 2025 • edited by dkurt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

dkurt commented Jan 27, 2025

Uh oh!

dkurt commented Jan 27, 2025

Uh oh!

Uh oh!

Uh oh!

MaximSmolskiy commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaximSmolskiy commented Jan 27, 2025

Uh oh!

dkurt commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaximSmolskiy commented Jan 26, 2025 •

edited by dkurt

Loading

MaximSmolskiy commented Jan 27, 2025 •

edited

Loading

dkurt commented Jan 28, 2025 •

edited

Loading