Allocate clean buffer in AnalyzeEntropy #1873
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1873 +/- ##
======================================
- Coverage 87% 87% -1%
======================================
Files 935 935
Lines 48939 48939
Branches 6086 6086
======================================
- Hits 42872 42814 -58
- Misses 5059 5115 +56
- Partials 1008 1010 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I have added a test to check for the expected file size, but I am not sure if that will trigger the actual issue. The issue #1872 will only happen, if the allocator will allocate unclean buffers, which i believe will not be the case when i create a new instance of |
| using var memoryStream = new MemoryStream(); | ||
| image.Save(memoryStream, encoder); | ||
|
|
||
| Assert.Equal(memoryStream.Length, expectedBytes); |
There was a problem hiding this comment.
Do we need strict equality? Doesn't the spec allow some space for the implementation?
There was a problem hiding this comment.
No the spec is not that strict. The analyses step could have choosen different transformation, e.g. here the analyses step has choosen to use the following features PREDICTION CROSS-COLOR-TRANSFORM SUBTRACT-GREEN. If one of those transformations would not been applied, the image would be still valid, but a bit larger.
Nonetheless i think we should try to aim to be as close to the reference implementation as possible and this is what libwebp would have encoded to.
There was a problem hiding this comment.
Ok then, we can relax this restriction later if it will become necessary for some reason in the future.
Prerequisites
Description
This PR fixes a bug in
AnalyzeEntropywhich did not allocate a clean buffer from the memory allocator, resulting in possible unpredictable results.Closes #1872