Skip to content

fix (openjpeg): Change the malloc method to dlmalloc to fix memory issue#40

Merged
wayfarer3130 merged 2 commits into
cornerstonejs:mainfrom
emelalkim:main
Feb 6, 2024
Merged

fix (openjpeg): Change the malloc method to dlmalloc to fix memory issue#40
wayfarer3130 merged 2 commits into
cornerstonejs:mainfrom
emelalkim:main

Conversation

@emelalkim
Copy link
Copy Markdown
Contributor

This change fixes the DX decompression problem that occurs in some j2k images with or without the latest changes in the openjpeg extern library and it has very minimal effect on the size of the distribution js files.

The current version of the codecs library cannot decompress some j2k DX images. See the issue for more information. Further investigation narrowed down the issue to malloc which is currently using emmalloc. While trying to allocate 595*4 bytes of memory with opj_aligned_malloc at this line in openjpeg library, it keeps increasing the size of the heap and crashes when it exceeds 2GB.

Changes:

  • Changes the malloc method to dlmalloc
  • Adds tests for problematic (memory crashing) DX images

Notes:

…sue during decompression of some j2k images

Changes the malloc method and adds tests for problematic (memory crashing) DX images
@emelalkim
Copy link
Copy Markdown
Contributor Author

@wayfarer3130, @sedghi who would be a good person to review this?

@sedghi
Copy link
Copy Markdown
Member

sedghi commented Feb 6, 2024

CC @chafey

@wayfarer3130 wayfarer3130 requested a review from chafey February 6, 2024 13:32
@wayfarer3130
Copy link
Copy Markdown
Contributor

Chris - any chance you could take a look? @chafey

@chafey
Copy link
Copy Markdown

chafey commented Feb 6, 2024

A few comments:

  1. I haven't done much with EMSCRIPTEN for years so am way out of date.
  2. I don't recall making a choice between the different allocators so no concern about changing it.
  3. Based on the documentation dmalloc seems like a better choice for potential speed reasons alone.
  4. It doesn't seem like we understand the root cause of why emalloc had a problem. The EMSCRIPTEN docs doesn't mention anything about a limitation like this between the two. Suggest we open an issue with EMSCRIPTEN reporting this situation so they can investigate or update docs if this is a known limitation

@emelalkim
Copy link
Copy Markdown
Contributor Author

A few comments:

  1. I haven't done much with EMSCRIPTEN for years so am way out of date.
  2. I don't recall making a choice between the different allocators so no concern about changing it.
  3. Based on the documentation dmalloc seems like a better choice for potential speed reasons alone.
  4. It doesn't seem like we understand the root cause of why emalloc had a problem. The EMSCRIPTEN docs doesn't mention anything about a limitation like this between the two. Suggest we open an issue with EMSCRIPTEN reporting this situation so they can investigate or update docs if this is a known limitation

Thank you @chafey, Happy to open an issue to report it. Would this repository be the correct place to open the issue?

@chafey
Copy link
Copy Markdown

chafey commented Feb 6, 2024

Yes I think so

@emelalkim
Copy link
Copy Markdown
Contributor Author

emelalkim commented Feb 6, 2024

@chafey I created the issue. Let me know if you this it needs improvement.
I changed the order of the tests so that failure can be tested if built with emmalloc
Does anyone have a problem merging the PR?

The problematic DX file fails only when it is in the beginning
@wayfarer3130 wayfarer3130 merged commit 05dbef9 into cornerstonejs:main Feb 6, 2024
@chafey
Copy link
Copy Markdown

chafey commented Feb 6, 2024

@emelalkim issue looks good, thanks for that. Ill add some comments. BTW - impressive debugging work, this stuff is really complicated!

@emelalkim
Copy link
Copy Markdown
Contributor Author

@emelalkim issue looks good, thanks for that. Ill add some comments. BTW - impressive debugging work, this stuff is really complicated!

Thank you @chafey. It was hard indeed! I must confess I needed to call reinforcements :) I got help from my brother @erdemalkim who is the actual c developer in the family

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.

4 participants