Repository navigation
#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 😉 |
| : base(image, memoryAllocator, configuration, entriesCollector) | ||
| { | ||
| using IQuantizer<TPixel> frameQuantizer = quantizer.CreatePixelSpecificQuantizer<TPixel>(this.Configuration); | ||
| this.quantized = frameQuantizer.BuildPaletteAndQuantizeFrame(image, image.Bounds()); |
There was a problem hiding this comment.
Palette encoder also use hard operations.
I didn't change this code, stayed as is.
There was a problem hiding this comment.
This should utilize whatever bit depth property we store.
There was a problem hiding this comment.
@JimBobSquarePants Sorry, I don't understand, what did you mean?
There was a problem hiding this comment.
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.
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.
default use multi strip method
| /// <summary> | ||
| /// Gets the maximum size of strip (bytes). | ||
| /// </summary> | ||
| int MaxStripBytes { get; } |
There was a problem hiding this comment.
this is a limit of one strip length (the field also can be used for tiles)
There was a problem hiding this comment.
this field also can be renamed 🙂
example MaxStripLength
There was a problem hiding this comment.
i think MaxStripBytes is an ok name, its clear what it does.
There was a problem hiding this comment.
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.
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.
Pardon my ignorance but what do the different strip count length mean in terms of encoding?
There was a problem hiding this comment.
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.
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.
Thanks... I guess we can determine a sensible default by benchmarking.
|
@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. Here is an example output: |
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. |
brianpopow
left a comment
brianpopow
left a comment
There was a problem hiding this comment.
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.
3 participants
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.