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

[git/2.1 regression] Fix opj_write_tile() failure when numresolutions=1 #690

Merged

Conversation

rouault
Copy link
Collaborator

@rouault rouault commented Jan 8, 2016

When trying the GDAL OpenJPEG driver against openjpeg current master HEAD,
I get failures when trying to create .jp2 files. The driver uses
opj_write_tile() and in some tests numresolutions = 1.

In openjp2/dwt.c:410, l_data_size = opj_dwt_max_resolution( tilec->resolutions,tilec->numresolutions) * (OPJ_UINT32)sizeof(OPJ_INT32);
is called and returns l_data_size = 0. Now in git opj_malloc() has a special case
for 0 to return a NULL pointer whereas previously it relied on system malloc(),
which in my case didn't return NULL.

So only test the pointer value if l_data_size != 0. This makes the GDAL
autotest suite to pass again.

When trying the GDAL OpenJPEG driver against openjpeg current master HEAD,
I get failures when trying to create .jp2 files. The driver uses
opj_write_tile() and in some tests numresolutions = 1.

In openjp2/dwt.c:410, l_data_size = opj_dwt_max_resolution( tilec->resolutions,tilec->numresolutions) * (OPJ_UINT32)sizeof(OPJ_INT32);
is called and returns l_data_size = 0. Now in git opj_malloc() has a special case
for 0 to return a NULL pointer whereas previously it relied on system malloc(),
which in my case didn't return NULL.

So only test the pointer value if l_data_size != 0. This makes the GDAL
autotest suite to pass again.
@stweil
Copy link
Contributor

stweil commented Jan 8, 2016

Maybe it would be better to return a pointer to a fixed address which points to const memory instead of returning NULL. Then callers would not need special handling.

@rouault
Copy link
Collaborator Author

rouault commented Jan 8, 2016

Maybe it would be better to return a pointer to a fixed address which points to const memory instead of returning NULL. Then callers would not need special handling.

Well you'd have also to make a special case in opj_free() to not free this address. Another solution would be to do malloc(1) when opj_malloc(0) is called

@stweil
Copy link
Contributor

stweil commented Jan 8, 2016

Sure, opj_free must ignore that address. Using malloc would only be an option if it had the same defined behavior on all platforms when called with 0.

@rouault
Copy link
Collaborator Author

rouault commented Jan 8, 2016

I suggested using malloc(1)

@mayeut
Copy link
Collaborator

mayeut commented Jan 8, 2016

Isn't that linked to #239 , #215 ? I think that there's something wrong other than malloc returning NULL for size 0 which is, I think, sane behavior. c.f. #635 for the sane behavior of opj_malloc

@rouault
Copy link
Collaborator Author

rouault commented Jan 9, 2016

@mayeut I don't think it's connected with #239. Whe comparing HEAD with 2.1, both return l_data_size = 0 but the difference is that HEAD now fails because malloc(0) now returns NULL which is considered as an error.

@malaterre
Copy link
Collaborator

Could you please handle the case l_data_size == 0 && bj == NULL

@rouault
Copy link
Collaborator Author

rouault commented Jan 9, 2016

@malaterre Not sure to understand what you mean. It is the nominal case handled by the code after the if (l_data_size != 0 && ! bj) check. What I know is that with my patch things work again as in 2.1

@rouault
Copy link
Collaborator Author

rouault commented Jan 9, 2016

Hum, or perhaps do you mean that if tilec->numresolutions == 1 we could do an early exit return OPJ_TRUE at the beginning of the function ? (since when looking closer the code after the if (l_data_size != 0 && ! bj) check ends up not entering the while (i--) loop )

@malaterre
Copy link
Collaborator

@rouault my bad. Could you please add comment stating that bj will not be used when l_data_size == 0.

@rouault
Copy link
Collaborator Author

rouault commented Jan 9, 2016

Comment added per 6a1974d

malaterre added a commit that referenced this pull request Jan 9, 2016
…cedure

[git/2.1 regression] Fix opj_write_tile() failure when numresolutions=1
@malaterre malaterre merged commit cb33ff4 into uclouvain:master Jan 9, 2016
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

4 participants