Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix bug with int64 support for FileStorage#26846

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@MaximSmolskiy
Copy link
Contributor

@MaximSmolskiyMaximSmolskiy commentedJan 26, 2025•
edited by dkurt
Loading

Pull Request Readiness Checklist

Fix#26829,opencv/opencv-python#1078

In current implementation ofint64 support raw size of recorded integer is variable (4 or8 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++;
FileNoden(fs, blockIdx, ofs);
ofs += n.rawSize();
if( ofs >= blockSize )
{
fs->normalizeNodeOfs(blockIdx, ofs);
blockSize = fs->fs_data_blksz[blockIdx];
}
return *this;
}

Bug is thatrawSize method still return4 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 to8.

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 athttps://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

@MaximSmolskiyMaximSmolskiyforce-pushed thefix_bug_with_int64_support_for_FileStorage branch fromdfad11a to56ddb98CompareJanuary 27, 2025 05:15
@dkurt
Copy link
Member

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?

@dkurtdkurt self-assigned thisJan 27, 2025
@dkurt
Copy link
Member

@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

@asmorkalovasmorkalov added this to the4.12.0 milestoneJan 27, 2025
@MaximSmolskiy
Copy link
ContributorAuthor

MaximSmolskiy commentedJan 27, 2025•
edited
Loading

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?

dkurt reacted with thumbs up emoji

@MaximSmolskiy
Copy link
ContributorAuthor

@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
Member

dkurt commentedJan 28, 2025•
edited
Loading

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!

@staticmethoddefget_normal_2d_mat():rows=10cols=20cn=3image=np.zeros((rows,cols,cn),np.int32)image[:]= (1,2,127)foriinrange(rows):forjinrange(cols):image[i,j,1]= (i+j)%256returnimagedefwrite_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()}forname,matinmats.items():fs.write(name,mat)fs.release()
0c16e58d13286e0d1b489e71be614f7b2a0fa57313ee36dd9b6329b7c6383976  /home/d.kurtaev/test_file_pr.json0c16e58d13286e0d1b489e71be614f7b2a0fa57313ee36dd9b6329b7c6383976  /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.pyindex 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-preLocal repo path: NoneF...======================================================================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.009sFAILED (failures=1)

@asmorkalovasmorkalov merged commit08a24ba intoopencv:4.xJan 28, 2025
30 of 31 checks passed
@MaximSmolskiyMaximSmolskiy deleted the fix_bug_with_int64_support_for_FileStorage branchJanuary 28, 2025 10:43
@opencv-alalekopencv-alalek added the port/backport doneLabel for maintainers. Authors of PR can ignore this labelJan 29, 2025
asmorkalov pushed a commit that referenced this pull requestFeb 6, 2025
…test_base64-for-64-bit-integer-typesSupport 64-bit integer types for FileStorage Base64#26871### Pull Request Readiness ChecklistRelated to#26846 (comment)OpenCV extra:opencv/opencv_extra#1232See details athttps://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
@asmorkalovasmorkalov mentioned this pull requestFeb 19, 2025
NanQin555 pushed a commit to NanQin555/opencv that referenced this pull requestFeb 24, 2025
…_support_for_FileStorageFix bug with int64 support for FileStorageopencv#26846### Pull Request Readiness ChecklistFixopencv#26829,opencv/opencv-python#1078In 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 valuehttps://github.com/opencv/opencv/blob/dfad11aae7ef3b3a0643379266bc363b1a9c3d40/modules/core/src/persistence.cpp#L2596-L2609Bug 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 correctSee details athttps://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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dkurtdkurtdkurt approved these changes

@asmorkalovasmorkalovAwaiting requested review from asmorkalov

Assignees

@dkurtdkurt

Labels

bugcategory: coreport/backport doneLabel for maintainers. Authors of PR can ignore this

Projects

None yet

Milestone

4.12.0

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

@MaximSmolskiy@dkurt@asmorkalov@opencv-alalek
[8]ページ先頭
©2009-2025 Movatter.jp