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

[TODO] Memory limit feature #198

Open
syoyo opened this issue Jun 27, 2023 · 4 comments
Open

[TODO] Memory limit feature #198

syoyo opened this issue Jun 27, 2023 · 4 comments

Comments

@syoyo
Copy link
Owner

syoyo commented Jun 27, 2023

Describe the issue

Currently TinyEXR does not support capping memory consumption.
Without such feature, the app may trigger OOM for large EXR images.

To Reproduce

Run poc2 and poc2 in issue 197.

Expected behavior

Add feature to specify the maximum memory to use. This requires

  • Define new API(current API does not support providing any loader options)
  • Set the maximum memory in define macro, e.g.
    • `#define TINYEXR_MAX_MAMORY (102410241024*64)

It also require implementing tracking memory consumption in TinyEXR .

Environment

N/A

@jspohr
Copy link

jspohr commented Feb 13, 2024

Hi, I'm currently looking into adding custom allocator support to TinyEXR. I started by simply replacing malloc/free with TINYEXR_MALLOC / TINYEXR_FREE macros that I can define within the cpp implementation file, to route allocations to a custom global allocator. This is a rather unintrusive change, easy to whip up a small PR for.
However, the STB libraries implement allocator overrides with a user-supplied context void *, which allows to use a different allocator depending on use case / thread index / etc. That would be my preferred way to handle it in TinyEXR. Doing that would mean passing down the pointer to all functions that allocate memory. Would you agree that it would be worthwhile? Maybe this is something you thought about doing anyway for this particular issue?
Since I don't want to maintain my own fork, I'd rather not do this work without a chance that it can be contributed back.
Looking forward to hearing your thoughts, and thanks for your work on the library!

@syoyo
Copy link
Owner Author

syoyo commented Feb 13, 2024

👍 > I'm currently looking into adding custom allocator support to TinyEXR

the STB libraries implement allocator overrides with a user-supplied context void *

Where can I find such allocator override in STB project?
I have glimpsed stb_image.h but allocators used there is macro-based approach(STB_MALLOC, STB_FREE, ...).

https://github.com/nothings/stb/blob/master/stb_image.h

Anyway, yes, having userdata pointer is the preferred approach.
So we'd be better to introduce callback functions for memory allocations, something like...

struct EXRMemoryAllocatorCallback
{
  void* (*malloc)(size_t n, void *userdata);
  void* (*realloc)(void *p, size_t newn, void *userdata);
  void (*free)(void *ptr, void *userdata);
};

(FYI stb_image uses this approach in io callback https://github.com/nothings/stb/blob/ae721c50eaf761660b4f90cc590453cdb0c2acd0/stb_image.h#L410 )

We'll then need to introduce new API which takes this callback in its argument

bool load_exr(..., EXRMemoryAllocatorCallback *cb, void *userdata);
void exr_free_image(..., EXRMemoryAllocatorCallback *cb, void *userdata);

For backward compatibility, existing API uses default allocator.

@jspohr
Copy link

jspohr commented Feb 14, 2024

Where can I find such allocator override in STB project? I have glimpsed stb_image.h but allocators used there is macro-based approach(STB_MALLOC, STB_FREE, ...).

You're right, it's not consistent in all the STB libs. I saw this in stb_image_resize2.h:
https://github.com/nothings/stb/blob/ae721c50eaf761660b4f90cc590453cdb0c2acd0/stb_image_resize2.h#L69

So we'd be better to introduce callback functions for memory allocations, something like...

That's definitely the cleanest approach! realloc isn't used in TinyEXR though, correct? I'd be careful about adding it as a required part of the callback since some custom allocators don't support it, and in my experience there are subtleties to how realloc is expected to behave in various cases. But it could be optional, ie. if it's not supplied emulate it using malloc -> memcpy -> free. As long as you don't see a need for it though, maybe just stick to malloc and free?

@syoyo
Copy link
Owner Author

syoyo commented Feb 15, 2024

Oh, Thanks! > I saw this in stb_image_resize2.h

realloc isn't used in TinyEXR though, correct?

Yes, realloc is not used in TinyEXR, so we can omit it. just malloc and free!

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

No branches or pull requests

2 participants