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

Issue3258 #1438

Closed
wants to merge 20 commits into from
Closed

Issue3258 #1438

wants to merge 20 commits into from

Conversation

ramiromagalhaes
Copy link

This pach forces the coertion to size_t of certain arithmetic operations that determined the buffer sizes of a Matrix while cv::imread'ing a big PGM file I have.

Also, I changed the name of a local variable named "readed" to read. This variable simply counts the amount of bytes read from a RLByteStream.

{
uchar* data = (uchar*)buffer;
int readed = 0;
assert( count >= 0 );
Copy link
Author

Choose a reason for hiding this comment

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

Since size_t is unsigned, this assertion became useless.

@ghost ghost assigned vpisarev Sep 12, 2013
@asmorkalov
Copy link
Contributor

@SpecLad, @vpisarev Please look at it.

int m_block_size;
int m_block_pos;
size_t m_block_size;
size_t m_block_pos;
Copy link

Choose a reason for hiding this comment

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

This isn't quite correct, because m_block_pos is used as an offset for fseek, which only accepts a long.

I also question the necessity of changing m_block_size to size_t, given that it's only ever set to BS_DEF_BLOCK_SIZE, which is well within the range of int.

…e of size_t type. On the other hand all tests I made showed that the image only loads correctly with m_block_pos being of type size_t. I could not figure out if there is some other change in the code that can be done to avoid changing the the type of m_block_size.
@ramiromagalhaes
Copy link
Author

@SpecLad I made a couple changes addressing your comments. Indeed, m_block_size has no need to be of type size_t and I did test this with my big image. However, when I changed m_block_pos back back to int for testing purposes, altough the cv::imread method did run without crashing, the image displayed misplaced lines.

@SpecLad
Copy link

SpecLad commented Sep 13, 2013

Try long.

@ramiromagalhaes
Copy link
Author

@SpecLad long works fine too.

@ramiromagalhaes
Copy link
Author

But seriously, I think that using long is not a good thing since its size depends on the platform we're compiling on.

@alalek
Copy link
Member

alalek commented Sep 19, 2013

Please fix these warnings (Win x64, MSVS 2010):

    13>..\..\..\opencv\modules\highgui\src\grfmt_pxm.cpp(171): warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data [C:\buildslave\precommit_OCL-win64-vc10\build\modules\highgui\opencv_highgui.vcxproj]

    13>..\..\..\opencv\modules\highgui\src\grfmt_sunras.cpp(121): warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data [C:\buildslave\precommit_OCL-win64-vc10\build\modules\highgui\opencv_highgui.vcxproj]

    13>..\..\..\opencv\modules\highgui\src\grfmt_sunras.cpp(134): warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data [C:\buildslave\precommit_OCL-win64-vc10\build\modules\highgui\opencv_highgui.vcxproj]

    13>..\..\..\opencv\modules\highgui\src\bitstrm.cpp(177): warning C4267: '=' : conversion from 'size_t' to 'long', possible loss of data [C:\buildslave\precommit_OCL-win64-vc10\build\modules\highgui\opencv_highgui.vcxproj]

    13>..\..\..\opencv\modules\highgui\src\bitstrm.cpp(237): warning C4267: '+=' : conversion from 'size_t' to 'int', possible loss of data [C:\buildslave\precommit_OCL-win64-vc10\build\modules\highgui\opencv_highgui.vcxproj]

@ramiromagalhaes
Copy link
Author

Will check that pretty soon.

@SpecLad
Copy link

SpecLad commented Sep 30, 2013

But seriously, I think that using long is not a good thing since its size depends on the platform we're compiling on.

Point is, fseek accepts a long, so using a size_t won't make a difference in the end. But I guess you can use size_t if you want.

@@ -211,7 +211,7 @@ void Mat::create(int d, const int* _sizes, int _type)
#endif
if( !allocator )
{
size_t totalsize = alignSize(step.p[0]*size.p[0], (int)sizeof(*refcount));
size_t totalsize = alignSize(((size_t)step.p[0])*size.p[0], (int)sizeof(*refcount));
Copy link
Contributor

Choose a reason for hiding this comment

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

step.p[0] is already size_t, the change is not needed

@ramiromagalhaes
Copy link
Author

@alalek I might have some problem to deal with those warnings since I do not have a Windows machine to try and fix those warnings. Is there a gcc warning that I might turn on so that I get a similar output?

@SpecLad
Copy link

SpecLad commented Oct 1, 2013

@ramiromagalhaes I think it's -Wconversion. I don't see any warnings right now, though, perhaps you've already fixed them by accident.

@mdim
Copy link
Contributor

mdim commented Oct 18, 2013

@vpisarev Could you please back to review this PR?

@BKNio
Copy link
Contributor

BKNio commented Nov 6, 2013

@vpisarev just remind you to review :)

@snosov1
Copy link
Contributor

snosov1 commented Nov 19, 2013

@vpisarev ping

@Daniil-Osokin
Copy link

@vpisarev Just a reminder.

@BKNio
Copy link
Contributor

BKNio commented Mar 3, 2014

@ramiromagalhaes 10x PR! unfortunately there are build warnings: http://pullrequest.opencv.org

@ramiromagalhaes
Copy link
Author

Ok, I'll try to get back to this soon (this time, for real). I'll check those warnings pointed by @alalek and @BKNio following @SpecLad 's advice of using (-Wconversion).

@vpisarev
Copy link
Contributor

vpisarev commented Apr 8, 2014

as I see, there is no progress on PR, long and size_t are still used where they should not be. Actually, the images could be converted to, e.g., png format, so it's not a huge problem. Let's close this PR for now as we are cleaning the list of opened PR. @ramiromagalhaes, when you have time to address the pointed out issues, please, feel free to re-open the PR.

@vpisarev vpisarev closed this Apr 8, 2014
@marisvali
Copy link

Hello. We are using opencv in a commercial project and we work with huge tif files (a little over 4gb). As a workaround to this issue, we are using a different library to read the huge tif file, then we convert the data to a Mat. It would be great if this issue would be solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants