Skip to content
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

Merged
merged 16 commits into from
Feb 15, 2021

Conversation

IldarKhayrutdinov
Copy link
Contributor

@IldarKhayrutdinov IldarKhayrutdinov commented Feb 7, 2021

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

  • Refactory color encoders and data compressors - it will make it easier to add new color modes (4-bit gray, CMYK, RGB 565/555, YCbCr), compressions (CcittGroup4Fax, Jpeg) and multi page encoding.
  • Support multi strip encoding.
  • Improve performance and memory usage of encoders and small improve of decoders.
  • [fixed by @brianpopow] Report palette lzw bug

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.

@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Feb 7, 2021

Attention 1

@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Feb 7, 2021

Attention 2

In current ImageSharp:tiff-format branch following tests generate black output images. I don't understand, why they are successful:

[Theory]
[WithFile(Calliphora_PaletteUncompressed, PixelTypes.Rgba32)]
public void TiffEncoder_EncodeColorPalette_WithLzwCompression_Works<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
var encoder = new TiffEncoder { Mode = TiffEncodingMode.ColorPalette, Compression = TiffEncoderCompression.Lzw };
this.TiffEncoderPaletteTest(provider, encoder);
}
[Theory]
[WithFile(Calliphora_PaletteUncompressed, PixelTypes.Rgba32)]
public void TiffEncoder_EncodeColorPalette_WithLzwCompressionAndPredictor_Works<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
var encoder = new TiffEncoder { Mode = TiffEncodingMode.ColorPalette, Compression = TiffEncoderCompression.Lzw, UseHorizontalPredictor = true };
this.TiffEncoderPaletteTest(provider, encoder);
}

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:
5fc3d37#diff-916bcb855ed09d4491b4ccc51fee218af1aa197ec22d499cc4dddc31f6942a20R210-R237

@brianpopow
Copy link
Collaborator

Attention 2

In current ImageSharp:tiff-format branch following tests generate black output images. I don't understand, why they are successful:

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.
Instead we load the encoded image with a reference decoder and compare against that. Not sure how we can improve that.

I tried to figure out the issue, but so far i cannot see any obvious bug. tiffinfo shows no error with the encoded image:

TIFF Directory at offset 0x1c1e (7198)
  Image Width: 804 Image Length: 1198
  Resolution: 96, 96 pixels/inch
  Bits/Sample: 8
  Compression Scheme: LZW
  Photometric Interpretation: palette color (RGB from colormap)
  Samples/Pixel: 1
  Rows/Strip: 1198
  Planar Configuration: single image plane
  Color Map: (present)
  DocumentName: .\Calliphora_palette_uncompressed.tiff
  ImageDescription: File source: http://commons.wikimedia.org/wiki/File:Calliphora_sp_Portrait.jpg
  Software: ImageSharp

@IldarKhayrutdinov
Copy link
Contributor Author

@brianpopow there are my outputs EncodeColorPalette_WithLzw.zip
from \ImageSharp\tests\Images\ActualOutput\TiffEncoderTests\ path.

There may be a problem on my computer, please check your output?

@brianpopow
Copy link
Collaborator

@brianpopow there are my outputs EncodeColorPalette_WithLzw.zip
from \ImageSharp\tests\Images\ActualOutput\TiffEncoderTests\ path.

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.

@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Feb 7, 2021

@brianpopow there are my outputs EncodeColorPalette_WithLzw.zip
from \ImageSharp\tests\Images\ActualOutput\TiffEncoderTests\ path.
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.

ok, I want to note that this problem was before this PR

@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Feb 7, 2021

Encoder performance report

Spoiler
Method Runtime Compression Mean Ratio Allocated Improve performance Improve memory allocation
'System.Drawing Tiff' .NET 4.7.2 None 6.520 ms 1.00** 11570062 B
'ImageSharp Tiff' .NET 4.7.2 None 5.698 ms 0.87 9919288 B
PR 'ImageSharp Tiff' .NET 4.7.2 None 4.844 ms 0.73 7445922 B 6% 25%
'System.Drawing Tiff' .NET Core 2.1 None 6.851 ms 1.00 11562768 B
'ImageSharp Tiff' .NET Core 2.1 None 4.294 ms 0.63 9918144 B
PR 'ImageSharp Tiff' .NET Core 2.1 None 3.189 ms 0.46 7444718 B 27% 25%
'System.Drawing Tiff' .NET Core 3.1 None 5.835 ms 1.00 8672224 B
'ImageSharp Tiff' .NET Core 3.1 None 5.167 ms 0.89 9918112 B
PR 'ImageSharp Tiff' .NET Core 3.1 None 3.342 ms 0.57 7444631 B 36% 25%
'System.Drawing Tiff' .NET 4.7.2 Deflate NA ? -
'ImageSharp Tiff' .NET 4.7.2 Deflate 125.909 ms ? 11167960 B
PR 'ImageSharp Tiff' .NET 4.7.2 Deflate 87.815 ms ? 6617521 B 30% 40%
'System.Drawing Tiff' .NET Core 2.1 Deflate NA ? -
'ImageSharp Tiff' .NET Core 2.1 Deflate 125.041 ms ? 11164792 B
PR 'ImageSharp Tiff' .NET Core 2.1 Deflate 84.005 ms ? 6605507 B 33% 40%
'System.Drawing Tiff' .NET Core 3.1 Deflate NA ? -
'ImageSharp Tiff' .NET Core 3.1 Deflate 125.139 ms ? 11168428 B
PR 'ImageSharp Tiff' .NET Core 3.1 Deflate 81.102 ms ? 6604792 B 35% 40%
'System.Drawing Tiff' .NET 4.7.2 Lzw 49.024 ms 1.00 10673371 B
'ImageSharp Tiff' .NET 4.7.2 Lzw 411.728 ms 8.41 23265464 B
PR 'ImageSharp Tiff' .NET 4.7.2 Lzw 125.569 ms 2.66 8423760 B 69% 64%
'System.Drawing Tiff' .NET Core 2.1 Lzw 47.288 ms 1.00 10668688 B
'ImageSharp Tiff' .NET Core 2.1 Lzw 201.643 ms 4.26 27451168 B
PR 'ImageSharp Tiff' .NET Core 2.1 Lzw 96.217 ms 2.03 8422488 B 52% 70%
'System.Drawing Tiff' .NET Core 3.1 Lzw 46.526 ms 1.00 8001741 B
'ImageSharp Tiff' .NET Core 3.1 Lzw 170.276 ms 3.66 27451445 B
PR 'ImageSharp Tiff' .NET Core 3.1 Lzw 93.635 ms 2.02 8422504 B 44% 70%
'System.Drawing Tiff' .NET 4.7.2 PackBits NA ? -
'ImageSharp Tiff' .NET 4.7.2 PackBits 28.948 ms ? 9943858 B
PR 'ImageSharp Tiff' .NET 4.7.2 PackBits 27.449 ms ? 7453052 B 5% 25%
'System.Drawing Tiff' .NET Core 2.1 PackBits NA ? -
'ImageSharp Tiff' .NET Core 2.1 PackBits 22.611 ms ? 9942792 B
PR 'ImageSharp Tiff' .NET Core 2.1 PackBits 19.935 ms ? 7451912 B 12% 25%
'System.Drawing Tiff' .NET Core 3.1 PackBits NA ? -
'ImageSharp Tiff' .NET Core 3.1 PackBits 23.465 ms ? 9942772 B
PR 'ImageSharp Tiff' .NET Core 3.1 PackBits 19.664 ms ? 7451974 B 16% 25%
'System.Drawing Tiff' .NET 4.7.2 CcittGroup3Fax 43.618 ms 1.00 1169683 B
'ImageSharp Tiff' .NET 4.7.2 CcittGroup3Fax 191.602 ms 4.39 24829048 B
PR 'ImageSharp Tiff' .NET 4.7.2 CcittGroup3Fax 191.413 ms 4.42 22714336 B -1% 8%
'System.Drawing Tiff' .NET Core 2.1 CcittGroup3Fax 43.258 ms 1.00 1169200 B
'ImageSharp Tiff' .NET Core 2.1 CcittGroup3Fax 177.930 ms 4.11 24772997 B
PR 'ImageSharp Tiff' .NET Core 2.1 CcittGroup3Fax 180.059 ms 4.13 22658509 B - 8%
'System.Drawing Tiff' .NET Core 3.1 CcittGroup3Fax 43.330 ms 1.00 850189 B
'ImageSharp Tiff' .NET Core 3.1 CcittGroup3Fax 168.846 ms 3.90 24774571 B
PR 'ImageSharp Tiff' .NET Core 3.1 CcittGroup3Fax 171.370 ms 3.94 22658261 B -1% 8%
'System.Drawing Tiff' .NET 4.7.2 ModifiedHuffman 17.106 ms 1.00 11561706 B
'ImageSharp Tiff' .NET 4.7.2 ModifiedHuffman 192.530 ms 11.27 24826163 B
PR 'ImageSharp Tiff' .NET 4.7.2 ModifiedHuffman 191.066 ms 11.18 22710384 B <1% 8%
'System.Drawing Tiff' .NET Core 2.1 ModifiedHuffman 16.988 ms 1.00 11555088 B
'ImageSharp Tiff' .NET Core 2.1 ModifiedHuffman 180.265 ms 10.61 24769453 B
PR 'ImageSharp Tiff' .NET Core 2.1 ModifiedHuffman 177.379 ms 10.41 22656395 B 1% 8%
'System.Drawing Tiff' .NET Core 3.1 ModifiedHuffman 15.989 ms 1.00 8666467 B
'ImageSharp Tiff' .NET Core 3.1 ModifiedHuffman 181.295 ms 11.34 24770275 B
PR 'ImageSharp Tiff' .NET Core 3.1 ModifiedHuffman 167.231 ms 10.49 22659275 B 7% 8%

@IldarKhayrutdinov IldarKhayrutdinov marked this pull request as ready for review February 7, 2021 16:00
@IldarKhayrutdinov IldarKhayrutdinov mentioned this pull request Feb 7, 2021
17 tasks
@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Feb 7, 2021

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());
Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Feb 7, 2021

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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:
Copy link
Contributor Author

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; }
Copy link
Contributor Author

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)

Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Feb 7, 2021

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

Copy link
Collaborator

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.

Copy link
Member

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

Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Feb 11, 2021

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.

Copy link
Member

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?

Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Feb 12, 2021

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?

Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Feb 12, 2021

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

Copy link
Member

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.

@brianpopow
Copy link
Collaborator

@IldarKhayrutdinov there seems to be an issue now with the packedbits compression. While the thumbnail of the tiffs look fine, if you open the image it looks broken. See for example in the Actual output folder of the tests: TiffEncoder_EncodeColorPalette_WithPackBitsCompression_Works_Rgba32_Calliphora_palette_uncompressed.tiff

Maybe we should add a CompareToReference output step in the tests to cover those cases.

@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Feb 13, 2021

@IldarKhayrutdinov there seems to be an issue now with the packedbits compression. While the thumbnail of the tiffs look fine, if you open the image it looks broken. See for example in the Actual output folder of the tests: TiffEncoder_EncodeColorPalette_WithPackBitsCompression_Works_Rgba32_Calliphora_palette_uncompressed.tiff

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?
TiffEncoder_EncodeColorPalette_WithPackBitsCompression_Works_Rgba32_Calliphora_palette_uncompressed.zip

I think adding CompareToReference is a good idea.

@brianpopow
Copy link
Collaborator

@IldarKhayrutdinov there seems to be an issue now with the packedbits compression. While the thumbnail of the tiffs look fine, if you open the image it looks broken. See for example in the Actual output folder of the tests: TiffEncoder_EncodeColorPalette_WithPackBitsCompression_Works_Rgba32_Calliphora_palette_uncompressed.tiff
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?
TiffEncoder_EncodeColorPalette_WithPackBitsCompression_Works_Rgba32_Calliphora_palette_uncompressed.zip

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:
TiffEncoder_EncodeColorPalette_WithPackBitsCompression_Works_Rgba32_Calliphora_palette_uncompressed tiff

@IldarKhayrutdinov
Copy link
Contributor Author

I doubt that the output can be different on different computers. We do not use any hardware intrinsics when decoding tiffs.

yes, it confused me too...

now I also saw through IrfanView (earlier I looked through the standard windows viewer and MS Paint)

@IldarKhayrutdinov
Copy link
Contributor Author

@brianpopow I investigated the lzw issue and finally understood, cause of decoder affecting for encoder test.
The encoder test method:

image.Save(memStream, encoder);
memStream.Position = 0;
using var encodedImage = (Image<TPixel>)Image.Load(this.configuration, memStream);
var encodedImagePath = provider.Utility.SaveTestOutputFile(encodedImage, "tiff", encoder);

line using var encodedImage = (Image<TPixel>)Image.Load(this.configuration, memStream);

here output image reload and resave, so in output directory tests\Images\ActualOutput\TiffEncoderTests placed resaved image.

@brianpopow
Copy link
Collaborator

@brianpopow I investigated the lzw issue and finally understood, cause of decoder affecting for encoder test.
The encoder test method:

image.Save(memStream, encoder);
memStream.Position = 0;
using var encodedImage = (Image<TPixel>)Image.Load(this.configuration, memStream);
var encodedImagePath = provider.Utility.SaveTestOutputFile(encodedImage, "tiff", encoder);

line using var encodedImage = (Image<TPixel>)Image.Load(this.configuration, memStream);

here output image reload and resave, so in output directory tests\Images\ActualOutput\TiffEncoderTests placed resaved image.

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.

@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Feb 14, 2021

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 tiff-format branch

I researching loading issue with IrfanView

@JimBobSquarePants
Copy link
Member

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 TPixel is Rgba32

@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Feb 14, 2021

Why the cast here and not using the generic version directly? That cast will only work if TPixel is Rgba32

@JimBobSquarePants now this method has removed, now use TestTiffEncoderCore method:

image.VerifyEncoder(provider, "tiff", bitsPerPixel, encoder, useExactComparer ? ImageComparer.Exact : ImageComparer.Tolerant(compareTolerance), referenceDecoder: ReferenceDecoder);

@IldarKhayrutdinov
Copy link
Contributor Author

@brianpopow I fixed PackBits bug, please check, is everything fixed?

@brianpopow
Copy link
Collaborator

@brianpopow I fixed PackBits bug, please check, is everything fixed?

@IldarKhayrutdinov great job in finding this issue! I will try to get this reviewed tomorrow.

Copy link
Collaborator

@brianpopow brianpopow left a 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!

@brianpopow brianpopow merged commit 6774c57 into SixLabors:tiff-format Feb 15, 2021
@IldarKhayrutdinov IldarKhayrutdinov deleted the tiff-format branch February 15, 2021 15:43
JimBobSquarePants pushed a commit that referenced this pull request Feb 17, 2021
#12 Support multi strip encoding. Improve performance and memory usage.
JimBobSquarePants pushed a commit that referenced this pull request Feb 17, 2021
#12 Support multi strip encoding. Improve performance and memory usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants