Resizing operation throws InvalidOperationException with exotic image resolution

[br3aker]

• I have written a descriptive issue title
• I have verified that I am running the latest version of ImageSharp
• I have verified if the problem exist in both DEBUG and RELEASE mode
• I have searched open and closed issues to ensure it has not already been reported

Description

Custom debug logs from MemoryGroup<T>.Allocate(...) call during steps stated below:

// Loading & decoding initial image from disk, 10x10=100 pixels - ok
// Minimal contiguous memory line 40 bytes - ok
Requesting Rgba32[100] (400 bytes) with stride of 40/1000 bytes
Allocation of 1 buffers 100 elements/400 bytes each

// Image cloning with bigger size for resizing, 15x15=225 pixels - ok
// Minimal contiguous memory line for 60 bytes - ok
Requesting Rgba32[225] (900 bytes) with stride of 60/1000 bytes
Allocation of 1 buffers 225 elements/900 bytes each

// Something related to resizing I guess - ok
Requesting Single[35] (140 bytes) with stride of 20/1000 bytes
Allocation of 1 buffers 35 elements/140 bytes each

// Something related to resizing I guess - ok
Requesting Single[35] (140 bytes) with stride of 20/1000 bytes
Allocation of 1 buffers 35 elements/140 bytes each

// That's where everything breaks due to non-contiguous memory allocation
Requesting Vector4[150] (2400 bytes) with stride of 160/1000 bytes
Allocation of 3 buffers 60 elements/960 bytes each
<-- After this call exception occurs -->

Last allocation from log allocates 150 Vector4 elements with a stride of 10 elements which is done by allocating 3 separate buffers of 60 elements each which fullfils requested "contiguous chunks of 10 elements each", so far so good.

Real problem lies inside ResizeWorker class:

ImageSharp/src/ImageSharp/Processing/Processors/Transforms/Resize/ResizeWorker.cs

Lines 117 to 118 in 993f96e

	// When creating transposedFirstPassBuffer, we made sure it's contiguous:
	Span<Vector4> transposedFirstPassBufferSpan = this.transposedFirstPassBuffer.GetSingleSpan();

Although comment states that buffer is ensured to be contiguous it's not:

ImageSharp/src/ImageSharp/Processing/Processors/Transforms/Resize/ResizeWorker.cs

Lines 88 to 91 in 993f96e

	this.transposedFirstPassBuffer = configuration.MemoryAllocator.Allocate2D<Vector4>(
	this.workerHeight,
	destWidth,
	AllocationOptions.Clean);

Allocate2D only ensures that requested memory is aligned by at least chunks of width parameter:

ImageSharp/src/ImageSharp/Memory/MemoryAllocatorExtensions.cs

Lines 25 to 35 in 993f96e

	public static Buffer2D<T> Allocate2D<T>(
	this MemoryAllocator memoryAllocator,
	int width,
	int height,
	AllocationOptions options = AllocationOptions.None)
	where T : struct
	{
	long groupLength = (long)width * height;
	MemoryGroup<T> memoryGroup = memoryAllocator.AllocateGroup<T>(groupLength, width, options);
	return new Buffer2D<T>(memoryGroup, width, height);
	}

Which is what happened in logs, ResizeWorker requested 10 strides of 15 elements with alignment of 15.

Atm given Buffer2D API doesn't provide any way to ensure that requested memory occupies single contiguous buffer but I suppose it should be something like this:

public static Buffer2D<T> AllocateContiguous2D<T>(
    this MemoryAllocator memoryAllocator,
    int width,
    int height,
    AllocationOptions options = AllocationOptions.None)
    where T : struct
{
    long groupLength = (long)width * height;
    MemoryGroup<T> memoryGroup = memoryAllocator.AllocateGroup<T>(
       groupLength, // passing total size as buffer size
       groupLength, // passing total size as alignment
       options);
    return new Buffer2D<T>(memoryGroup, width, height);
}

Steps to Reproduce

Use this code snippet:

Configuration.Default.MemoryAllocator =
    new ImageSharp.Memory.ArrayPoolMemoryAllocator(
        maxPoolSizeInBytes: 1000,
        poolSelectorThresholdInBytes: 1, // [not relevant]
        maxArraysPerBucketLargePool: 1000,
        maxArraysPerBucketNormalPool: 1, // [not relevant]
        bufferCapacityInBytes: 1000
    );
var image = Image.Load("IMAGE_PATH");
image.Mutate(op => op.Resize(15, 15));

With this 10x10 pixels plain black png image:
https://user-images.githubusercontent.com/20967409/117883361-fdb01e80-b2b3-11eb-850f-10c66309430d.png

Yes, it can be broken even with current default config allocator but with a bit more extreme example:

var image = Image.Load("IMAGE_PATH");
image.Mutate(op => op.Resize(10000, 1));

With this 1x4096 pixels plain black png image:
https://user-images.githubusercontent.com/20967409/117890174-56d08000-b2bd-11eb-93f3-ad625787f0be.png

[br3aker]

@antonfirsov you were right, MemoryGroup<T>.Allocate() indeed allocates non-contiguous memory with proper allocator settings (I still have questions about that code but it's for another discussion).

Problem lies inside processor itself and who knows what other proccessors might fail due the same problem with allocations.

[br3aker]

ResizeWorker has some sort of checking for contiguous memory but it doesn't affect allocated buffer in any way:

ImageSharp/src/ImageSharp/Processing/Processors/Transforms/Resize/ResizeWorker.cs

Lines 76 to 79 in 993f96e

	// We need to make sure the working buffer is contiguous:
	int workingBufferLimitHintInBytes = Math.Min(
	configuration.WorkingBufferSizeHintInBytes,
	configuration.MemoryAllocator.GetBufferCapacityInBytes());

I don't think that providing such internal detail about allocator is a good thing, memory user should work with size of the given memory, not with some configuration value from entierly different class This is error prone and introduce dependency from underlying implementation.

Anyway, even if logic is correct, code fails on calling GetSingleSpan() on Buffer2D which is strange, shouldn't this code allocate Memory<T> directly via allocator.Allocate(totalSize)?

P.S.
Ashamed saying it but I don't have a slightest idea of what's going on in that processing code even in resize processor, are there any articles or study behind all of this so I can acquire necessary knowledge? :P

[antonfirsov]

@br3aker nice catch, thanks for the report! Repro confirmed. I hope I will be able to look into this next week, this one is completely busy with other stuff.

[saucecontrol]

Ashamed saying it but I don't have a slightest idea of what's going on in that processing code even in resize processor, are there any articles or study behind all of this so I can acquire necessary knowledge?

@br3aker resizing is one of the more complicated processors, but it's just based on convolution in the end, as are things like blur, sharpen, and edge detection. This is probably the best intro to the math behind resizing I've seen: http://entropymine.com/imageworsener/resample/

[br3aker]

@saucecontrol thanks a lot! Will definetely take a look.

[JimBobSquarePants]

We'll need to look at this again now the memory allocator works differently.

[JimBobSquarePants]

@antonfirsov It appears to me that we can ensure the allocation is contiguous now via the new Allocate2D overload

ImageSharp/src/ImageSharp/Memory/MemoryAllocatorExtensions.cs

Lines 26 to 32 in bab74dc

	public static Buffer2D<T> Allocate2D<T>(
	this MemoryAllocator memoryAllocator,
	int width,
	int height,
	bool preferContiguosImageBuffers,
	AllocationOptions options = AllocationOptions.None)
	where T : struct

Passing true as the 3rd parameter.

ImageSharp/src/ImageSharp/Processing/Processors/Transforms/Resize/ResizeWorker.cs

Lines 88 to 91 in bab74dc

	this.transposedFirstPassBuffer = configuration.MemoryAllocator.Allocate2D<Vector4>(
	this.workerHeight,
	destWidth,
	AllocationOptions.Clean);

We could then drop the calculation, and property here and just enforce a 1MB limit internally.

ImageSharp/src/ImageSharp/Processing/Processors/Transforms/Resize/ResizeWorker.cs

Lines 76 to 79 in bab74dc

	// We need to make sure the working buffer is contiguous:
	int workingBufferLimitHintInBytes = Math.Min(
	configuration.WorkingBufferSizeHintInBytes,
	configuration.MemoryAllocator.GetBufferCapacityInBytes());

Is that correct?

[antonfirsov]

@JimBobSquarePants yeah that would solve the problem, I will file a PR to fix this soon. Note that this is a corner case, you can't trigger it with sane code, especially hard with the new API.