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

Support files with very large data size #730

Closed
wants to merge 1 commit into from

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Mar 25, 2016

The maximum supported data size is a limiting factor for the maximum
size of allowed images. As it is possible to allocate size_t bytes,
this data type must also be used for the data size.

This modification also fixes issue #432. That file is now decoded on
64 bit hosts with enough RAM, so the regression test had to be fixed.

Update also some comments in the test suite.

Signed-off-by: Stefan Weil [email protected]

@stweil
Copy link
Contributor Author

stweil commented Mar 25, 2016

The modified code in tests/nonregression/test_suite.ctest.in assumes a 64 bit host system (size_t > 32 bits) with enough RAM. Is it possible to use different test code for 32 bit hosts?

@stweil
Copy link
Contributor Author

stweil commented Mar 25, 2016

CI fails on some hosts when running the tests for issue #432. I think this must be fixed by updating the test code. As I am not familiar with ctest, I need help for this.

My test result is available here: http://my.cdash.org/buildSummary.php?buildid=933150 (how can I set the build name?).

@stweil
Copy link
Contributor Author

stweil commented Mar 25, 2016

Updated PR, test for issue #432 is now temporarily disabled.

@stweil
Copy link
Contributor Author

stweil commented May 8, 2016

Ping? What is needed to get large file support integrated? The PR was now re-based on latest git version.

@mayeut
Copy link
Collaborator

mayeut commented May 8, 2016

@stweil, there are many places throughout the code where data is allocated with UINT32 size, or operations are performed on 32-bits size width * height.
To merge a PR for large images, all of those shall use size_t with proper overflow check when needed.
This is what I would expect. With no modification at all in bin folder, I doubt this will be properly working.

@stweil
Copy link
Contributor Author

stweil commented May 11, 2016

The limit which is addressed by this PR is width * height * 4 < 32 bit or width * height < 1073741824. I did not find more code with the same limit. Of course there are other code locations with different limitations, for example width * height < 32 bit. Maybe someone has a test image between those two limits, so we could test whether the new code works?

@jmichel-otb
Copy link

I recently ran into the same issue. Is there any way to move this forward ?

@stweil
Copy link
Contributor Author

stweil commented May 24, 2016

@jmichel-otb, the current question is whether this pull request alone is useful or not. There are code limitations which are not fixed here, so a general solution for large files needs more changes. If you have a valid example where this pull request helps, I think it will be applied. Maybe you can provide your example then for the test cases?

@jmichel-otb
Copy link

Well, what I can do is check if this branch fixes my problem. The test data is quite large (more than 4 Gb) and I think I can not give it away, but maybe I can generate a similar dataset. Nevertheless, I can also try to solve the other code limitations.

@stweil
Copy link
Contributor Author

stweil commented May 25, 2016

Checking would be great. Solving more code limitations would be even better. What are your image dimensions (width, height)?

@jmichel-otb
Copy link

I need to check because I do not have the image here, but it was roughly 40 000 x 160 000 pixels, so bigger than the width * height < 1073741824 limit.

@malaterre
Copy link
Collaborator

malaterre commented May 25, 2016

For the old timers, there used to be a script for generating such images in SVN: https://groups.google.com/forum/#!msg/openjpeg/zGAIlZgxePE/Jx3L3y2u1kYJ

@stweil
Copy link
Contributor Author

stweil commented May 3, 2017

I rebased this pull request now to latest git master.

@stweil
Copy link
Contributor Author

stweil commented Aug 12, 2017

PR rebased again to fix merge conflicts.

@pedros007
Copy link

It would be great to have large file support for OpenJPEG!

FWIW, I get a warning when building the stweil:largefiles branch via Docker with a debian:jessie base image:

git clone -b largefiles https://github.com/stweil/openjpeg.git /tmp/openjpeg
mkdir /tmp/openjpeg/build
cd /tmp/openjpeg/build
cmake .. -DCMAKE_INSTALL_PREFIX=/usr
make
[ 26%] Building C object src/lib/openjp2/CMakeFiles/openjp2.dir/tcd.c.o
/tmp/openjpeg/src/lib/openjp2/tcd.c: In function ‘opj_tcd_init_tile’:
/tmp/openjpeg/src/lib/openjp2/tcd.c:796:23: warning: conversion to ‘size_t’ from ‘OPJ_INT32’ may change the sign of the result [-Wsign-conversion]
         l_data_size = l_tilec->x1 - l_tilec->x0;
                       ^
/tmp/openjpeg/src/lib/openjp2/tcd.c:833:41: warning: conversion to ‘OPJ_UINT32’ from ‘size_t’ may alter its value [-Wconversion]
             l_tilec->resolutions_size = l_data_size;
                                         ^
/tmp/openjpeg/src/lib/openjp2/tcd.c:849:41: warning: conversion to ‘OPJ_UINT32’ from ‘size_t’ may alter its value [-Wconversion]
             l_tilec->resolutions_size = l_data_size;
                                         ^
/tmp/openjpeg/src/lib/openjp2/tcd.c: In function ‘opj_tcd_code_block_enc_allocate_data’:
/tmp/openjpeg/src/lib/openjp2/tcd.c:1191:70: warning: conversion to ‘long unsigned int’ from ‘OPJ_INT32’ may change the sign of the result [-Wsign-conversion]
                                (p_code_block->y1 - p_code_block->y0) * sizeof(OPJ_UINT32));
                                                                      ^
/tmp/openjpeg/src/lib/openjp2/tcd.c:1203:35: warning: conversion to ‘OPJ_UINT32’ from ‘size_t’ may alter its value [-Wconversion]
         p_code_block->data_size = l_data_size;
                                   ^

The maximum supported data size is a limiting factor for the maximum
size of allowed images. As it is possible to allocate size_t bytes,
this data type must also be used for the data size.

This modification also fixes issue uclouvain#432. That file is now decoded on
64 bit hosts with enough RAM, so the regression test had to be fixed.
Until it is possible to use different test cases for 32 and 64 bit hosts,
the test for issue uclouvain#432 is disabled.

Handle also a potential division by zero when l_data_size is 0.
This fixes issue uclouvain#733.

Update also some comments in the test suite.

Signed-off-by: Stefan Weil <[email protected]>
@stweil
Copy link
Contributor Author

stweil commented Aug 24, 2017

Pull request was rebased again, and warnings are fixed now.

@rouault
Copy link
Collaborator

rouault commented Aug 24, 2017

This PR would only help for cases where number of pixels in tiles is < 4 billion. I'll integrate that, as well as other needed changes, in ongoing work for subtile decoding (not merging that right now to avoid merge conflicts)

@pedros007
Copy link

@rouault are you talking about #1010? Sounds promising -- nice work!

@rouault
Copy link
Collaborator

rouault commented Sep 1, 2017

@pedros007 Yes, exactly. This PR is included in d5153ba , 008a12d and 98b9310 of #1010

@stweil
Copy link
Contributor Author

stweil commented Sep 1, 2017

Nice indeed, and good to see those improvements.

@rouault, maybe you can also replace the error message Not enough memory for tile data by Size of tile data exceeds system limits. There exist 32 bit systems which have more than 4 GiB RAM, but the system limit is nevertheless 32 bits.

rouault added a commit to rouault/openjpeg that referenced this pull request Sep 1, 2017
rouault added a commit to rouault/openjpeg that referenced this pull request Sep 1, 2017
@rouault
Copy link
Collaborator

rouault commented Sep 5, 2017

#1010 has just been merged. This obsoletes this PR

@rouault rouault closed this Sep 5, 2017
pedros007 added a commit to pedros007/debian-gdal that referenced this pull request Feb 19, 2018
Jasper failed to read a large jp2 file & OpenJPEG will get large file
support.  @rouault is making more OpenJPEG improvements, [including
support for files with > 4 billion
pixels](uclouvain/openjpeg#730 (comment)).

Build against OpenJPEG master. Eventually it may work with the jp2
files I have.
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.

6 participants