Update 2 to memory management

[GoogleCodeExporter]

This is the next update to bug 253 
(https://code.google.com/p/openjpeg/issues/detail?id=253) and patches on top of 
OpenJPEG2.1.

This patch propagates the codec to all areas where memory management is used 
and also means that the existing code can be further extended to swap out 
fprintf statements for the correct callback message.

The patch has been regression tested using the ghostscript test files.

Original issue reported on code.google.com by slmis...@gmail.com on 13 Jun 2014 at 10:52

Attachments:

• mem21_jun_13.patch

[GoogleCodeExporter]

Patch to update user memory management in OpenJPEG2.1

Original comment by slmis...@gmail.com on 13 Jun 2014 at 11:01

Attachments:

• mem21_jun_13.patch

[GoogleCodeExporter]

Original comment by antonin on 15 Jul 2014 at 8:27

• Changed state: Accepted
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Hi Shailesh,
Your patch drastically modifies the OpenJPEG API ... So at first glance I must 
say I'm reluctant to apply it. Could you give details on what are the benefits 
of the new memory management you propose and if there is a way to get (some of) 
the same benefits without modifying so much the API ?
Thanks

Original comment by antonin on 17 Jul 2014 at 8:46

[GoogleCodeExporter]

The existing memory management code allows the user to swap out generic
function calls for user specific calls but does not account for the memory
being accessed. This patch allows he user to define their own chunk of
memory so that they have greater control and this in turn means that the
code is thread safe.

A side benefit of this is that all routes that have been updated because of
this can now use the error printing functionality that has only partially
been implemented by yourselves.

Drop me a line if you have further questions, please note I am off on
holiday from the 25th July till the 3rd of August.

Shailesh

Original comment by slmis...@gmail.com on 21 Jul 2014 at 8:44

[GoogleCodeExporter]

Hi All,

It has been a month since our last contact, can you tell me if this patch
is still progressing or do you need some more information?

Shailesh

Original comment by slmis...@gmail.com on 18 Aug 2014 at 7:31

[GoogleCodeExporter]

Hi All,

Is it possible to get some kind of confirmation that you are still looking
into this?

Shailesh

Original comment by slmis...@gmail.com on 26 Aug 2014 at 12:57

[GoogleCodeExporter]

Original comment by antonin on 16 Sep 2014 at 3:55

[GoogleCodeExporter]

We (the Ghostscript dev team - Shailesh has been looking at this on our behalf) 
are getting to the point where we feel it's going to be necessary to fork the 
OpenJPEG code to satisfy our memory management requirements.

Whilst that's not a route we take lightly, we're not seeing much progress on 
(nor enthusiasm for) moving this forward "properly".

Chris

Original comment by chris.j....@googlemail.com on 26 Sep 2014 at 9:14

[GoogleCodeExporter]

@Chris: a few elements from the OpenJPEG side:
 - we are very few people managing the contributions from outside, with very little time available. Moreover we were almost offline for two months during july and august. I know that sounds a bit weird in our times but that's the way it is. So it's not a question of enthusiasm, it's a question of available free time (free, as in "non-paid"). 
 - we definitely want OpenJPEG to move forward, that's why I asked in September for people willing to review the provided patches. Matthieu answered and already helped a lot. In the mean time, we had several security vulnerabilities provided by Foxit that are kept private on the issue tracker, for obvious reasons.
 - as I wrote earlier, the patch provided by Shailesh drastically modifies the OpenJPEG API. Either there is a way to provide a patch that does not modify so much the API, either I need more review on the original patch before applying it.
 - I you cannot wait any longer, I perfectly understand, please fork: I still hope you will merge back in the future.
 - A way to boost OpenJPEG development in the past was to fund an engineer during a few days to speed-up progress on all high-priority issues. Mathieu (Malaterre) did this already very successfully. If you have any way to fund the project, feel free to contact me.

Original comment by antonin on 29 Sep 2014 at 9:41

[GoogleCodeExporter]

@Chris,

Would the API proposed in either of the attach patch be ok ?
I'd rather stick with optB than optA.

Original comment by m.darb...@gmail.com on 1 Oct 2014 at 9:13

Attachments:

• issue356.optA.patch
• issue356.optB.patch

[GoogleCodeExporter]

Original comment by m.darb...@gmail.com on 9 Oct 2014 at 6:09

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

I have reviewed these and patch B does look workable.

With regards to the manager typedef, you only need one context reference so 
opj_manager_create could be defined as 

OPJ_API opj_manager_t OPJ_CALLCONV opj_manager_create(
               void*                       context,
               opj_malloc_callback         malloc_callback,
               opj_calloc_callback         calloc_callback,
               opj_realloc_callback        realloc_callback,
               opj_free_callback           free_callback,
               opj_aligned_malloc_callback aligned_malloc_callback,
               opj_aligned_free_callback   aligned_free_callback,
       );

I am still not sure how this will all work with memory allocation at the lower 
level such as in opj_bio_create(void), will these and related functions be 
updated to handle the opj_manager_t?

Would it be possible to get some working source code so that I can try a full 
working system with the ghostscript code and pass it through their regression 
test suite?

Shailesh

Original comment by slmis...@gmail.com on 22 Oct 2014 at 9:20

[GoogleCodeExporter]

@Shailesh,

Here's a working patch against r2924.
It's been test for non regression against OpenJpeg test suite.

You'll have to be careful & use the same manager for images than for codec 
otherwise you'll end up mixing allocator functions (non matching malloc/free).

Early tests on OpenJpeg binaries showed that malloc/free are called directly on 
opj_image_t members (or opj_image_comp_t) leading to non matching malloc/free & 
thus a crash. Custom allocators can be enabled in opj_compress (not working as 
it would require changes to convert.c which allocates the image with default 
manager while the codec tries to free component data with the custom one) & 
opj_decompress ( working except for some icc profiles & all yuv images - 
conversion reallocates component with the wrong allocator).

When not using custom allocators (2.1 API), full non regression using 
unmodified OpenJpeg binaries.

The patch also contains issue 283 & issue 392 fixes (it was more convenient for 
my baseline regression testing)

Original comment by m.darb...@gmail.com on 7 Nov 2014 at 10:22

Attachments:

• issue356.patch

[GoogleCodeExporter]

Updated with issue 428 patch.

Original comment by m.darb...@gmail.com on 15 Nov 2014 at 1:24

• Changed state: Started

Attachments:

• issue356.patch

[GoogleCodeExporter]

Updated with issue 433

Original comment by m.darb...@gmail.com on 15 Nov 2014 at 2:24

Attachments:

• issue356.patch

[GoogleCodeExporter]

Apologies for taking so long getting back to you but I now have an allocated 
chunk of time to get this completed.

I have just tried the patch against r2993 but I get too many rejected chunks in 
j2k.c, jp2.c, t2.c, tcd.c and tcd.h. Is it possible to get this patch updated?

Shailesh

Original comment by slmis...@gmail.com on 20 Jan 2015 at 10:14

[GoogleCodeExporter]

@Shailesh,

Before I start updating this (it's quite a big patch), can you give feedback 
using it on r2924 ? 

Original comment by m.darb...@gmail.com on 25 Jan 2015 at 6:24

[GoogleCodeExporter]

I have taken in the r2924 code and applied the patch.

I notice that in some places you use "assert(manager != NULL);" but in others 
you use "if (manager == NULL) {return NULL;}". Please can you confirm if I 
should be using one over the other?

The main reason I ask is because my compiler is complaining about having this 
check before a declaration and gives the standard error 

src/lib/openjp2/function_list.c:45:2: error: ISO C90 forbids mixed declarations 
and code [-Werror=declaration-after-statement]
  opj_procedure_list_t * l_validation = (opj_procedure_list_t *) opj_manager_calloc(manager, 1, sizeof(opj_procedure_list_t));
  ^

This error is given in numerous places so I am trying to avoid making too many 
changes.

Original comment by slmis...@gmail.com on 27 Jan 2015 at 9:46

[GoogleCodeExporter]

Hi

|I notice that in some places you use "assert(manager != NULL);" but in others 
you use "if (manager == NULL) {return NULL;}". Please can you confirm if I 
should be using one over the other?

In places where argument checks should be done, I use "if (manager == NULL) 
{return NULL;}"
In places where previous calls/checks should never lead to (manager == null), I 
use "assert(manager != NULL);" (which is not called in Release)

I used a C99 compiler so I guess I missed those "mixed declarations and code" 
(which I avoid generally since I also use C90 compilers). This is an error on 
my end.

When you get such an error, you should modify the code to correct this. In the 
specific sample you mention :

    opj_procedure_list_t * l_validation = NULL;

    /* preconditions */
    assert(manager != NULL);

    /* memory allocation */
    l_validation = (opj_procedure_list_t *) opj_manager_calloc(manager, 1, sizeof(opj_procedure_list_t));

Regards
Matthieu

Original comment by m.darb...@gmail.com on 28 Jan 2015 at 1:34

[GoogleCodeExporter]

Hi, 

I have built the code with r2924 patched, it is currently going through 
regression testing with ALLOC_PERF_OPT turned off and then I will run again 
with it on.

There were a number of files that needed updating but it could be this is 
resolved in the HEAD version. The problems encountered were :-

In opj_j2k_post_write_tile, l_tcd is defined but not used

In opj_jpip_write_iptr, OPJ_OFF_T j2k_codestream_exit set but not used

In opj_jpip_write_iptr, p_manager is used but undeclared

In opj_jpip_write_fidx, p_manager is used but undeclared

In opj_jpip_write_cidx, p_manager is used but undeclared

In opj_includes.h, the check for __STDC_VERSION__ caused a number of warnings 
that can be quashed by using
#if !defined(__STDC_VERSION__) || (__STDC_VERSION__ != 199901L)

Some of the files call opj_calloc but do not include opj_malloc.h

The build/regression takes a while so I will get back to you with the results 
another day.

Shailesh

Original comment by slmis...@gmail.com on 28 Jan 2015 at 11:28

[GoogleCodeExporter]

I have built in the manager code using the ghostscript memory management 
routines and the regression test suite passed fine.

Please can you update me once this has been merged into the main line.

Original comment by slmis...@gmail.com on 1 Feb 2015 at 10:03

[GoogleCodeExporter]

Here's an updated patch against r2997. It's OK against the test suite.

Can you please try this one before it gets into the trunk ?

Original comment by m.darb...@gmail.com on 8 Feb 2015 at 4:50

Attachments:

• issue356-r2997.patch

[GoogleCodeExporter]

I have updated to r2997 and patched accordingly but the following still cause 
problems.

1) In opj_includes.h, the check for __STDC_VERSION__ caused a number of 
warnings that can be quashed by using
#if !defined(__STDC_VERSION__) || (__STDC_VERSION__ != 199901L)

2) Some of the files call opj_calloc but do not include opj_malloc.h, the files 
are cidx_manager.c, phix_manager.c, ppix_manager.c and thix_manager.c

3) The "ISO C90 forbids mixed declarations..." error still exists in j2k.c for 
opj_j2k_create_compress and opj_j2k_create_decompress

The regression suite is running right now and I will come back to you with the 
results.

Original comment by slmis...@gmail.com on 11 Feb 2015 at 9:35

[GoogleCodeExporter]

The patches for r2924 and r2997 both pass the regression test but they do show 
some blurriness in the output. Attached are some images showing the output with 
the patches on the left and the original output on the right, this can be fixed 
by applying the original patch in the following bug report.

https://code.google.com/p/openjpeg/issues/detail?id=235

Original comment by slmis...@gmail.com on 13 Feb 2015 at 9:40

Attachments:

• Bug691816.JPG
• Bug691843.JPG
• IA3Z3225.JPG

[GoogleCodeExporter]

Hi All,

Is there any update on this bug? If the patch referred to in #24 above is 
proving to be a stumbling block then please go ahead with the memory patch as 
it exists and Ghostscript can always apply this patch in their own code.

Shailesh

Original comment by slmis...@gmail.com on 7 Mar 2015 at 1:11

[GoogleCodeExporter]

Hi Shailesh,

I will keep this patch up to date but it won't get into the next version of 
openjpeg. If patch from issue 235 gets into the trunk, I'll merge it to the 
memory management patch.

I'll try to update the patch with your latest comments.

Matthieu

Original comment by m.darb...@gmail.com on 17 Mar 2015 at 3:48

[GoogleCodeExporter]

Hi Matthieu,

Do you have a rough idea which version the memory patch might be merged back in 
or maybe a rough timescale? I would like to update the GS team to manage their 
expectation of when this might get resolved.

Shailesh

Original comment by slmis...@gmail.com on 17 Mar 2015 at 8:48