Skip to content

Fix bug with int64 support for FileStorage#26846

Merged
asmorkalov merged 2 commits intoopencv:4.xfrom
MaximSmolskiy:fix_bug_with_int64_support_for_FileStorage
Jan 28, 2025
Merged

Fix bug with int64 support for FileStorage#26846
asmorkalov merged 2 commits intoopencv:4.xfrom
MaximSmolskiy:fix_bug_with_int64_support_for_FileStorage

Conversation

@MaximSmolskiy
Copy link
Copy Markdown
Contributor

@MaximSmolskiy MaximSmolskiy commented Jan 26, 2025

Pull Request Readiness Checklist

Fix #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

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

@MaximSmolskiy MaximSmolskiy force-pushed the fix_bug_with_int64_support_for_FileStorage branch from dfad11a to 56ddb98 Compare January 27, 2025 05:15
@asmorkalov asmorkalov requested a review from dkurt January 27, 2025 07:06
@dkurt
Copy link
Copy Markdown
Member

dkurt commented Jan 27, 2025

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 dkurt self-assigned this Jan 27, 2025
@dkurt
Copy link
Copy Markdown
Member

dkurt commented Jan 27, 2025

@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

@asmorkalov asmorkalov added this to the 4.12.0 milestone Jan 27, 2025
@MaximSmolskiy
Copy link
Copy Markdown
Contributor Author

MaximSmolskiy commented Jan 27, 2025

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
Copy link
Copy Markdown
Contributor Author

@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
Copy link
Copy Markdown
Member

dkurt commented Jan 28, 2025

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)

@asmorkalov asmorkalov closed this Jan 28, 2025
@asmorkalov asmorkalov reopened this Jan 28, 2025
@asmorkalov asmorkalov merged commit 08a24ba into opencv:4.x Jan 28, 2025
@MaximSmolskiy MaximSmolskiy deleted the fix_bug_with_int64_support_for_FileStorage branch January 28, 2025 10:43
@opencv-alalek opencv-alalek added the port/backport done Label for maintainers. Authors of PR can ignore this label Jan 29, 2025
asmorkalov pushed a commit that referenced this pull request Feb 6, 2025
…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
@asmorkalov asmorkalov mentioned this pull request Feb 19, 2025
NanQin555 pushed a commit to NanQin555/opencv that referenced this pull request Feb 24, 2025
…_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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug category: core port/backport done Label for maintainers. Authors of PR can ignore this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug in cv::FileStorage when the XML contains integer values larger INT_MAX

4 participants