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

Provide memory manager overriding api #325

Closed
wants to merge 1 commit into from
Closed

Provide memory manager overriding api #325

wants to merge 1 commit into from

Conversation

dandedrick
Copy link

I know that, with the state of funding for this project at the moment, this might
not be considered but I thought I would share this anyway.

This provides a new interface for creating compress and decompress
streams with an already initialized memory manager. By adding a new
function this doesn't break compatability with old clients. The new api
assumes that either mem has been initialized and is ready to go or that
it is NULL and will be initialized as normal.

Without this it is problematic to actually fully override the default
memory manager. The main issue is that the memory manager is that the
jpeg_create_* functions initialize the default memory manager and then
use it to allocate some items before returning. If the memory manager is
replaced after this the original memory manager will never have the
destruct called. If the destruct is called manually it will free memory
that is still in use by structures in cinfo.

This provides a new interface for creating compress and decompress
streams with an already initialized memory manager. By adding a new
function this doesn't break compatability with old clients. The new api
assumes that either mem has been initialized and is ready to go or that
it is NULL and will be initialized as normal.

Without this it is problematic to actually fully override the default
memory manager. The main issue is that the memory manager is that the
jpeg_create_* functions initialize the default memory manager and then
use it to allocate some items before returning. If the memory manager is
replaced after this the original memory manager will never have the
destruct called. If the destruct is called manually it will free memory
that is still in use by structures in cinfo.
@dcommander
Copy link
Member

I strongly dislike the introduction of new *_create_* functions. That's just poor API design, but due to the aforementioned lack of project funding, I don't have the resources right now to dig into this issue and figure out a better solution. Do you really need to override the entire memory manager? Or are you just trying to override jpeg_get_large()/jpeg_get_small(), as in #308?

Also refer to my comments in #311 and #313 regarding the fact that the libjpeg API is very old and brittle, and I'm largely uninterested in any disruptive modifications to it. I would be much more interested in extending the TurboJPEG API to cover the necessary features of the libjpeg API that it doesn't already cover.

That being said, I think there is probably a way to solve this problem by extending the libjpeg API with a single function that allows a custom memory manager to be specified. It just may require restructuring a few things. Given that the newly-established general fund only pays for about 8 hours of labor a month, and that has already been exhausted for this month, it may be a while before I can look at this in more detail.

@dandedrick
Copy link
Author

Overriding jpeg_get_large()/jpeg_get_small() would probably be enough. I don't want to override these globally but on a per-jpeg basis. The basic use case is for time critical uses a pool would be pre-allocated and during the libjpeg processing no actual allocations would occur but instead just be handed out by the pool. The pool management could be done in the memory manager or just be collected into the jpeg_get_large()/``jpeg_get_small``` functions.

I do think that exposing the memory manager would be more powerful than just
``jpeg_get_large/jpeg_get_small``` and the interfaces area already exposed. You could almost just overwrite mem after ```jpeg_create_*``` with one significant issue. That issue is that ```jpeg_create_compress() ``` uses mem to allocate several items. So you can't just destruct the memory manager and replace because that would free structures like the ```jpeg_marker_reader``` and if you don't destruct it you are leaking memory. All of this on top of the fact that those allocation were done with the default allocator.

Ideally I would have just liked the current jpeg_create_* functions to not overwrite the mem value but that would break clients who currently leave that uninitialized. I was trying to avoid adding new data to the structure as that would cause breaks in version compatibility.

@dcommander
Copy link
Member

Unless I'm missing something, I see no reason why the call to jinit_memory_mgr() cannot be deferred from jpeg_CreateCompress() until the point that the memory manager is actually needed. At that point, jinit_memory_mgr() could be called only if cinfo->mem == NULL (it is set to NULL within the both of jpeg_CreateCompress().) As far as jpeg_CreateDecompress(), that's trickier, and I'll have to look at how best to solve it.

@dandedrick
Copy link
Author

Agreed deferring it for Compress would be a reasonable solution but I don't know what could be done for Decompress where allocation occurs in the create.

@dcommander
Copy link
Member

dcommander commented Jan 25, 2019

I need to look at that, because I'm the one who moved that allocation into the jpeg_CreateDecompress() function. Can't say without spending a few hours digging into it.

@dandedrick
Copy link
Author

I'm closing this request because I will no longer be working for the company I logged this on behalf of in a weeks time and I don't have any personal interest in this particular change to pursue it any further.

@dandedrick dandedrick closed this Apr 23, 2019
@dcommander dcommander reopened this Apr 23, 2019
@dcommander
Copy link
Member

This issue is of general interest. You aren't the first person who has requested it.

@dcommander dcommander force-pushed the master branch 6 times, most recently from f79d239 to c8e5274 Compare May 9, 2019 23:10
@dcommander dcommander closed this May 12, 2021
@dcommander dcommander deleted the branch libjpeg-turbo:master May 12, 2021 16:00
dcommander added a commit that referenced this pull request Aug 9, 2021
This issue was introduced in 5557fd2
due to an oversight, so it has existed in libjpeg-turbo since the
project's inception.  However, the issue is effectively a non-issue.
Although #325 proposes allowing programs to override jpeg_get_*() and
jpeg_free_*() externally, there is currently no way to override those
functions without modifying the libjpeg-turbo source code.
libjpeg-turbo only includes the malloc()/free() memory manager from
libjpeg, and the implementation of jpeg_free_*() in that memory manager
ignores the size argument.  libjpeg had several additional memory
managers for legacy systems (MS-DOS, System 7, etc.), but those memory
managers ignored the size argument to jpeg_free_*() as well.  Thus, this
issue would have only potentially affected custom memory managers in
downstream libjpeg-turbo forks, and since no one has complained until
now, apparently those are rare.

Fixes #542
dcommander added a commit that referenced this pull request Nov 19, 2021
This issue was introduced in 5557fd2
due to an oversight, so it has existed in libjpeg-turbo since the
project's inception.  However, the issue is effectively a non-issue.
Although #325 proposes allowing programs to override jpeg_get_*() and
jpeg_free_*() externally, there is currently no way to override those
functions without modifying the libjpeg-turbo source code.
libjpeg-turbo only includes the malloc()/free() memory manager from
libjpeg, and the implementation of jpeg_free_*() in that memory manager
ignores the size argument.  libjpeg had several additional memory
managers for legacy systems (MS-DOS, System 7, etc.), but those memory
managers ignored the size argument to jpeg_free_*() as well.  Thus, this
issue would have only potentially affected custom memory managers in
downstream libjpeg-turbo forks, and since no one has complained until
now, apparently those are rare.

Fixes #542
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants