-
-
Notifications
You must be signed in to change notification settings - Fork 860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#12 Support multi strip encoding. Improve performance and memory usage. #1537
#12 Support multi strip encoding. Improve performance and memory usage. #1537
Conversation
… usage of decoders and encoders. # Conflicts: # tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs
Attention 1
|
Attention 2 In current ImageSharp:tiff-format branch following tests generate black output images. I don't understand, why they are successful: ImageSharp/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs Lines 183 to 201 in c57664d
Also I tested this bug in revision befor merging #1527, the bug was there too. This bug I found during multistrip tests. Therefore I add workaround for this tests and add bug reprodusion test: |
The test do not fail because with the palette image tests, we do not compare the result against the input image. There is a quantization step involved to create the palette, which means the encoded image will always be slightly different from the original. I tried to figure out the issue, but so far i cannot see any obvious bug.
|
@brianpopow there are my outputs EncodeColorPalette_WithLzw.zip There may be a problem on my computer, please check your output? |
@IldarKhayrutdinov i can reproduce the isssue, i do see those black images, too. I just dont know yet why this is happening. |
bbbb465
to
f1e1e21
Compare
ok, I want to note that this problem was before this PR |
Encoder performance report Spoiler
|
There were a lot of changes, don't hesitate to comments 😉 |
{ | ||
using IQuantizer<TPixel> frameQuantizer = quantizer.CreatePixelSpecificQuantizer<TPixel>(this.Configuration); | ||
this.quantized = frameQuantizer.BuildPaletteAndQuantizeFrame(image, image.Bounds()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Palette
encoder also use hard operations.
I didn't change this code, stayed as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should utilize whatever bit depth property we store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JimBobSquarePants Sorry, I don't understand, what did you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing a passed IQuantizer
instance but also hardcoded ColorsPerChannel
values which indicate 256 colors each time. I would assume that there is some sort of BitDepth
property that is captured during decoding and can be optionally set to dictate the encoding bit depth. There should be consistency in the palette size and desired bit depth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's required to bit depth settings, but at the moment support only 8 bit palette and grayscale.
I would like add bit depth later, in future PR.
{ | ||
default: | ||
case TiffEncoderPixelStorageMethod.Auto: | ||
case TiffEncoderPixelStorageMethod.MultiStrip: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default use multi strip method
/// <summary> | ||
/// Gets the maximum size of strip (bytes). | ||
/// </summary> | ||
int MaxStripBytes { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a limit of one strip length (the field also can be used for tiles)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this field also can be renamed 🙂
example MaxStripLength
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think MaxStripBytes
is an ok name, its clear what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that interface public? I’m curious as to what the property means as a configuration value to the average user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface is internal
, but TiffEncoder
class that inherit this interface is public
.
I looked StripByteCounts
on different tiff
files and watched that they have different lengths, not only 8Kb, in some cases it reaches 1Mb. So I figured that the user might want to set the strip size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon my ignorance but what do the different strip count length mean in terms of encoding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In simple terms this is size of encoder (and decoder when read) buffer, the image is divided into strips
and encoding one by one, so there is no need to allocate large buffers for full image size. It seems that there is no single agreement, there is only a recommendation about 8Kb
(also I read about 64Kb
), I think most users will be satisfied with default size. But maybe someone wants to adjust the strip size himself. Maybe @brianpopow will give more complete information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frankly, for modern computers, I think it won't be a problem using 1Mb
buffers, but 8Kb
is safer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks... I guess we can determine a sensible default by benchmarking.
src/ImageSharp/Formats/Tiff/Compression/Compressors/DeflateCompressor.cs
Outdated
Show resolved
Hide resolved
@IldarKhayrutdinov there seems to be an issue now with the Maybe we should add a CompareToReference output step in the tests to cover those cases. |
@brianpopow here is my output, it seems it looks normal, maybe different results on different computers? I think adding CompareToReference is a good idea. |
I doubt that the output can be different on different computers. We do not use any hardware intrinsics when decoding tiffs. After digging a bit deeper and that you mentioned it does not look corrupted on your pc, it seems an issue with IrfanView, gimp and XnView. On the other hand viewing it wit paint works. I definitely think there is a regression introduced with this PR and PackedBits compression, as i dont see this issue with the current tiff branch. I will try to improve on the tests so we will spot those issues. |
yes, it confused me too... now I also saw through |
@brianpopow I investigated the ImageSharp/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs Lines 222 to 226 in ed6078e
line here output image reload and resave, so in output directory |
yeah that was an unlucky workaround for the fact that the quantization was changing the output image slightly. I have changed that part now and use a TolerantComparer for the color palette tests: a8f75b0. This should fail now if the output has too much different from the input. |
👍 after, I'll sync with I researching loading issue with |
using var encodedImage = (Image<TPixel>)Image.Load(this.configuration, memStream); Why the cast here and not using the generic version directly? That cast will only work if |
@JimBobSquarePants now this method has removed, now use
|
@brianpopow I fixed |
@IldarKhayrutdinov great job in finding this issue! I will try to get this reviewed tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, @IldarKhayrutdinov thank you once again for your help!
#12 Support multi strip encoding. Improve performance and memory usage.
#12 Support multi strip encoding. Improve performance and memory usage.
Prerequisites
Description
Encoing performance improved: 6-69%, memory usage: 8-70%.
Decoder performance stayed ±10%, maybe big tests will show better results.
I don't provided profiling, I think with profiling there is potential for more improvements.