Preserve color profile when encoding PNG images #2110
Conversation
|
|
||
| using (var memoryStream = new MemoryStream(compressedData.ToArray())) | ||
| using (var bufferedStream = new BufferedReadStream(this.Configuration, memoryStream)) | ||
| using (var inflateStream = new ZlibInflateStream(bufferedStream)) |
There was a problem hiding this comment.
I think we can avoid the allocation here.
| outputBytes[bytesWritten++] = 0; // Null separator. | ||
| outputBytes[bytesWritten++] = 0; // Compression. |
There was a problem hiding this comment.
Here the above "trick" doesn't make sense, as for slicing there's an argument validation, so we just would exchange one comparison for the bound-check against the argument validation. So no net-win.
Co-authored-by: Günther Foidl <gue@korporal.at>
|
|
||
| var uncompressedBytes = new List<byte>(compressedData.Length); | ||
|
|
||
| // Note: this uses a buffer which is only 4 bytes long to read the stream, maybe allocating a larger buffer makes sense here. |
There was a problem hiding this comment.
Should we use the memory allocator to to create a bigger buffer to read the uncompressed data into here? using the scratch buffer feels like it could be very inefficient.
There was a problem hiding this comment.
Yeah, lot's of small allocations. I would use the allocator to allocate a buffer of length Configuration.StreamProcessingBufferSize.
There was a problem hiding this comment.
I was thinking about this for a while now, but if we always use the allocator this could be also inefficient, if we have alot of small compressed text string's.
Maybe something like this:
Span<byte> destBuffer = this.buffer.AsSpan();
using IMemoryOwner<byte> foo = compressedData.Length > 1024 ? this.memoryAllocator.Allocate<byte>(1024) : null;
if (compressedData.Length > 1024)
{
destBuffer = foo.GetSpan();
}
For the the buffer to uncomress into?
There was a problem hiding this comment.
uncompressedBytes.AddRange(this.buffer.AsSpan(0, bytesRead).ToArray());It's actually this line that concerns me the most. Zlib compression is pretty good and you could end up with a large number of ToArray() calls. That's why I favour a much larger array and simply slicing it. Pooling is pretty cheap.
There was a problem hiding this comment.
I dont see how this ToArray call can be avoided, though. We cannot pass a Span to AddRange, see dotnet1530
There was a problem hiding this comment.
Maybe MemoryStream?
Can be used with spans and can expand like a list.
No virt calls as we would work with direct type.
Should be relatively fast as it uses Buffer.InternalBlockCopy for big buffers.
|
I wanted to merge this now, but two tests for I kind of doubt this is related to the PR changes.
|
|
Definitely not us. There’s been a new .NET7 preview release. We should check the output and report any issues. |
I will merge this PR and create a tracking issue for the .NET7 issue: #2117 |
Prerequisites
Description
This PR changes the PNG encoder to preserve the color profile when encoding an image.
Related issue: #2107