Make sure encoding 4bit paletted tiff rows are byte aligned#1646
Make sure encoding 4bit paletted tiff rows are byte aligned#1646brianpopow merged 4 commits intomasterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1646 +/- ##
==========================================
+ Coverage 84.11% 84.12% +0.01%
==========================================
Files 812 812
Lines 35583 35592 +9
Branches 4144 4146 +2
==========================================
+ Hits 29931 29943 +12
+ Misses 4834 4831 -3
Partials 818 818
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| using IMemoryOwner<byte> rows4bitBuffer = this.MemoryAllocator.Allocate<byte>(pixels.Length / 2); | ||
| int width = this.Image.Width; | ||
| int excess = (width % 2) * height; | ||
| int rows4BitBufferLength = indexedPixels.Length + excess; |
There was a problem hiding this comment.
it seems that here you can:
int rows4BitBufferLength = (width / 2) * height + excess;
There was a problem hiding this comment.
yeah your right, this was a bit too much
| { | ||
| using IMemoryOwner<byte> rows4bitBuffer = this.MemoryAllocator.Allocate<byte>(pixels.Length / 2); | ||
| int width = this.Image.Width; | ||
| int excess = (width % 2) * height; |
There was a problem hiding this comment.
| int excess = (width % 2) * height; | |
| int excess = (width & 1) * height; |
This produces better machine code than the modulo operation here (as it's signed arithmetics).
| for (int i = 0; i < rows4bit.Length; i++) | ||
| int idxPixels = 0; | ||
| int idx4bitRows = 0; | ||
| int halfWidth = width / 2; |
There was a problem hiding this comment.
| int halfWidth = width / 2; | |
| int halfWidth = width >> 1; |
And move it before L63, so this variable can be re-used in rows4BitBufferLength.
Note: instead of the bit-operations (here and comment above) you could use unsigned-arthmetics, as there the JIT does these "tricks" on your behalf.
There was a problem hiding this comment.
Wow, I'm surprised JIT still can't optimize this .
@brianpopow in case you change these, I would recommend a form like
int excess = (width & 1) * height; // (width % 2) * heightto keep the intention clear.
There was a problem hiding this comment.
I'm surprised JIT still can't optimize this .
Please don't blame the JIT here. It's signed arithmetics that need to work for negative values too, and the JIT can't conclude that width is positive. So one could use uint to give the JIT the necessary hint(s) and produce bit-operations, or do it manually.
GCC or Clang can't handle this too (because of signed arithmetics).
There was a problem hiding this comment.
It's signed arithmetics that need to work for negative values too, and the JIT can't conclude that width is positive.
Missed that aspect, makes sense, thanks!
5 participants
Prerequisites
Description
This PR makes sure to write padding when encoding 4-bit paletted tiff's with uneven width.