Implement Median Blur processor #2219
Conversation
Update from upstream
…into median-filter
| { | ||
| var k = xOffsets[baseXOffsetIndex + z]; | ||
| var pixel = row[k]; | ||
| kernelBuffer[index + z] = pixel.ToVector4(); |
There was a problem hiding this comment.
Can we optimize this? Would be useful to assign a buffer large enough to allow the bulk operation like we do in the ConvolutionProcessor<T>
There was a problem hiding this comment.
Because the z loops runs over neighboring rows, this buffer would need to contain at least several rows of the source image.
Do you feel that is worth the memory?
There was a problem hiding this comment.
We can still read/convert the source row slice one at a time though if you're sampling one kernel row at a time?
There was a problem hiding this comment.
Please see in the comments below how this is done.
src/ImageSharp/Processing/Processors/Convolution/MedianRowOperation{TPixel}.cs
Outdated
Show resolved
Hide resolved
| { | ||
| var k = xOffsets[baseXOffsetIndex + z]; | ||
| var pixel = row[k]; | ||
| kernelBuffer[index + z] = pixel.ToVector4(); |
There was a problem hiding this comment.
Perhaps we should Make ReadOnlyKernel Kernel and add setters. That way you can use a DenseMatrix<T> and use all the existing APIs for Kernel access.
There was a problem hiding this comment.
The ConvolutionProcessor uses a clever trick, where it builds up the target value row-by-row. This is allowed for ReadOnlyKernel type of RowOperation classes, where we're calculating weighted sums basically. These weighted sums can indeed be split arbitrary, as is done by clearing the target buffer before the loop and using += in line 152
However, the operation of finding a median, cannot be split up like this (or at least not without lots of caching). It mandates the traversal of the entire kernel in one pass, to get a single Span of pixel values to Sort. Hence the different logic here, compared to ConvolutionProcessor.
I'll update the code to use Unsafe.Add iso indexers.
There was a problem hiding this comment.
Ah no, I didn't mean the processors themselves I just meant the ConvolutionState struct. It's designed to track all the offsets you're manually tracking yourself. ReadOnlyKernel could become Kernel with a read/write indexer (maybe ref based).
There was a problem hiding this comment.
Made a separate MedianConvolutionState struct for this, as it's using Vector4 iso float.
src/ImageSharp/Processing/Processors/Convolution/MedianRowOperation{TPixel}.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // We use a rectangle with width set to 2 * kernelSize^2 + width, to allocate a buffer big enough | ||
| // for kernel source and target bulk pixel conversion. | ||
| Rectangle operationBounds = new Rectangle(interest.X, interest.Y, (2 * (kernelSize * kernelSize)) + interest.Width, interest.Height); |
There was a problem hiding this comment.
If you use 2x interest width here you can use the extra buffer space to allow bulk per-row ToVector4() against the source row in the operation.
new Rectangle(interest.X, interest.Y, (2 * (kernelSize * kernelSize)) + (2 * interest.Width), interest.Height);There was a problem hiding this comment.
Well, it's not that simple I think, as we need more rows for a single kernel.
I think I get your point about caching the Vector4 converted pixels, I'll take the challenge of making it work!
There was a problem hiding this comment.
On the multiple source rows that Median needs, the conversion is now done with BulkOperations. I think this is what you meant.
src/ImageSharp/Processing/Processors/Convolution/MedianRowOperation{TPixel}.cs
Outdated
Show resolved
Hide resolved
src/ImageSharp/Processing/Processors/Convolution/MedianRowOperation{TPixel}.cs
Outdated
Show resolved
Hide resolved
src/ImageSharp/Processing/Processors/Convolution/MedianRowOperation{TPixel}.cs
Outdated
Show resolved
Hide resolved
Prerequisites
Description
This PR aims to solve #814 by implementing a Median Blur processor using the .NET runtime Span.Sort(). Results are comparable to Gimp.