Update 2 to memory management #356

[slmistry]

I have applied the issue356-r2997.patch, a number of hunks failed and had to be applied manually. I then corrected a number of errors/warnings and a memory leak in the image destroy functionality.

This code has successfully passed through all the regression files in the Ghostscript repository.

[mayeut]

527 errors in OpenJpeg test suite with this PR: http://my.cdash.org/buildSummary.php?buildid=823368

[slmistry]

The last update shows 17 test failures but they appear to be unrelated to this code change. Please can you confirm if this is ok or if you require further input from me.

[julienmalik]

See #356

[malaterre]

@slmistry could you confirm that you tried to preserved the old API and provided a new one (let's call it the 'manager') ?

[slmistry]

The code was checked with/without the manager to ensure it would continue to work as is.

[malaterre]

@slmistry did you provide a mechanism to detect case where an application mixes both API, eg:

         l_codec = opj_manager_create_compress(l_manager, OPJ_CODEC_JP2);
[...]
         l_codec = opj_create_compress(OPJ_CODEC_JP2);

[slmistry]

There is no mechanism in place to check for the user mixing API calls but it does seem highly improbable.

If you start with opj_create_compress, then you would not suddenly create an instance of l_manager for your remaining function calls.

Conversely if you start with opj_manager_create_compress, you have already made an instance of l_manager and it should be apparent that you need to keep using it.

[malaterre]

So @slmistry confirms that there are two dual (conflicting API) in this patch. I suggest two way to improve this patchet:

• hard path: remove the deprecated API (really clean update of API, make sure to remove any leftover from existing apps out there),
• soft path: move the deprecated API in between OPJ_USE_MEMORY_MANAGER idef(s) , update the documentation to mention the deprecated API is only there not to break API (allow simple recompilation for current application).

I do not buy the argument that "people wont mix API" at all (why would anyone update an existing opj_image_destroy with opj_manager_image_destroy when it compiles), and I do not want OpenJPEG team to maintain a dual API where it is not clear where the benefit of the old one is.

As mentioned in another channel, this patchset changes the ABI, so it would makes it natural to bump the API version at the same time, if one picks the hard path suggestion.

[slmistry]

Just for reference, this memory management update has been over 3 years in the making.

It started on 12/Jan/2014 in issue #253 (https://github.com/uclouvain/openjpeg/issues/253)

It then progressed into issue #356 (https://github.com/uclouvain/openjpeg/issues/356) a period of over a year were it was reworked numerous times exactly as your team requested.

Now nearly 2 years after it was finally accepted onto your milestone list you appear to be requesting even more changes!

[slmistry]

Is it possible to get an update on this? I am happy to implement the hard path if you can confirm that it will potentially go into a release fairly soon.