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

Apply AllocationLimitMegabytes against the total size of multi-frame images before attempting to decode them #2850

Open
antonfirsov opened this issue Dec 17, 2024 Discussed in #2849 · 2 comments

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Dec 17, 2024

Discussed in #2849

Originally posted by antonfirsov December 17, 2024

Configuration.Default.MemoryAllocator = MemoryAllocator.Create(new MemoryAllocatorOptions()
{
    AllocationLimitMegabytes = 16
});

// Should throw InvalidMemoryOperationException when the total size of all frames
// to allocate for this image would be over 16MB, even if the individual frame sizes are below 16MB.
var img = Image.Load("multiframe.gif");

Even though strictly speaking this would be a hack since the allocation limit is originally meant to be per-buffer, #2848 made me think this is something users want us to do. For built-in decoders, this is possible to implement by adding an internal validation method toMemoryAllocator; we can figure out later whether we want to make the feature available to external decoders.

@JimBobSquarePants
Copy link
Member

I think we should avoid the hack and add a new property.

  • AllocationLimitMegabytes
  • AccumulativeAllocationLimitMegabytes

I thought all allocators must inherit MemoryAllocator; can we not add tracking there? I'm happy to break the inheritance pattern to do this.

@antonfirsov
Copy link
Member Author

antonfirsov commented Dec 18, 2024

The problem with an extra property is that it adds complexity while I find it unlikely that users would want to configure the values separately; or do you see use cases where they would?

I thought all allocators must inherit MemoryAllocator

They all do. Note that the limit properties live in MemoryAllocatorOptions.

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

No branches or pull requests

2 participants