Fix run-length encode compression implementation mistake in tga encoder #2172

l1nxy · 2022-07-13T08:47:13Z

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 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

According to the Tga format spec page 24:

Run-length Packets should never encode pixels from more than one scan line. Even if the end of one
scan line and the beginning of the next contain pixels of the same value, the two should be encoded
as separate packets. In other words, Run-length Packets should not wrap from one line to another.
This scheme allows software to create and use a scan line table for rapid, random access of
individual lines. Scan line tables are discussed in further detail in the Extension Area section of this
document.

It should not cross multiple lines when using Run-length encode compression. Some libraries can not fix the rle mistake when importing images saved by ImageSharp, and will throw an exception.

CLAassistant · 2022-07-13T08:47:20Z

All committers have signed the CLA.

brianpopow · 2022-07-13T09:12:22Z

@l1nxy thanks for providing a fix for this.

Some questions I have:

Can you provide a test image where this error happens?
Does ImageMagick detect this error? If so, we should add a test for this case (we do use ImageMagick as a ReferenceDecoder for unit tests).

l1nxy · 2022-07-13T09:30:20Z

@l1nxy thanks for providing a fix for this.

Some questions I have:

Can you provide a test image where this error happens?

Does ImageMagick detect this error? If so, we should add a test for this case (we do use ImageMagick as a ReferenceDecoder for unit tests).

ImageMagick thinks the old cross multiple scan line format and new format which do not cross multiple scan lines all of those are correct, and I was find this microsoft/DirectXTex#251.
Maybe should i add a new option to control this?

l1nxy · 2022-07-13T09:57:29Z

I will close this pr and make a new one, which should add a new option to control which RLE compression is selected.

brianpopow · 2022-07-13T10:07:56Z

I will close this pr and make a new one, which should add a new option to control which RLE compression is selected.

Mhm, I dont think this is a good idea. Why would someone want to have an option to write a invalid tga? If its against the spec, we should not do it.

l1nxy · 2022-07-13T10:17:43Z

I will close this pr and make a new one, which should add a new option to control which RLE compression is selected.

Mhm, I dont think this is a good idea. Why would someone want to have an option to write a invalid tga? If its against the spec, we should not do it.

But the legacy format is used widely, ImageMagick and Many picture viewers can detect the legacy format.
What do you think about it?

brianpopow · 2022-07-13T10:21:35Z

I will close this pr and make a new one, which should add a new option to control which RLE compression is selected.

Mhm, I dont think this is a good idea. Why would someone want to have an option to write a invalid tga? If its against the spec, we should not do it.

But the legacy format is used widely, ImageMagick and Many picture viewers can detect the legacy format. What do you think about it?

I think its ok, if we can decode such image, but I dont think we should encode such tga which do not follow the spec. Can we actually decode such images? You have not provided a test image yet.

edit: Also note you can add commits to this PR without the need to re-open a new one.

l1nxy · 2022-07-13T10:56:32Z

I will close this pr and make a new one, which should add a new option to control which RLE compression is selected.

Mhm, I dont think this is a good idea. Why would someone want to have an option to write a invalid tga? If its against the spec, we should not do it.

But the legacy format is used widely, ImageMagick and Many picture viewers can detect the legacy format. What do you think about it?

I think its ok, if we can decode such image, but I dont think we should encode such tga which do not follow the spec. Can we actually decode such images? You have not provided a test image yet.

edit: Also note you can add commits to this PR without the need to re-open a new one.

OK, thank you very much. i will provide some test pic later. and test the encoder.

JimBobSquarePants · 2022-07-14T13:14:26Z

Just to confirm.

We should aim to be able to decode both old and new specification.
We should only encode to the latest specification.

brianpopow · 2022-07-14T13:17:37Z

Just to confirm.

We should aim to be able to decode both old and new specification.
We should only encode to the latest specification.

Yes, that is what I would suggest.

l1nxy · 2022-07-18T09:27:22Z

There is the test_pic.zip.
I downloaded the GitHub logo and translated it to TGA format using ImageMagick, Then, I wrote a demo to read the TGA file by DirectxTex, which can be read correctly.

And I wrote another demo that used ImageSharp to read this TGA file and save it directly to another TGA file which named Github_legacy.tga, It can not be read by DirectxTex which means ImageSharp generated a legacy TGA format file.

Repeat this procedure using modified code, It can be read by DirectxTex correctly.

The TGA decoder can read these three files, so the decoder implementation is correct, But there is a wired thing is the image size generated by ImageSharp bigger than the file generated by ImageMagick.

brianpopow

I would like two unit tests to be added to make sure it works now as it should. (I would do it by myself, but for some weird reason gitlfs does not let me push to this PR)

One test for decoding the legacy format (even though we can decode it already, a test to make sure would be nice).
Add the following to TgaDecoderTests.cs

[Theory]
[WithFile(Github_RLE_legacy, PixelTypes.Rgba32)]
public void TgaDecoder_CanDecode_LegacyFormat<TPixel>(TestImageProvider<TPixel> provider)
    where TPixel : unmanaged, IPixel<TPixel>
{
    using (Image<TPixel> image = provider.GetImage(TgaDecoder))
    {
        image.DebugSave(provider);
        ImageComparingUtils.CompareWithReferenceDecoder(provider, image);
    }
}

and the Github_RLE_legacy to TestImages.cs.

Make sure the encoded bytes now matches the expected size. Add to TgaEncoderTests.cs

// Run length encoded pixels should not exceed row boundaries.
// https://github.com/SixLabors/ImageSharp/pull/2172
[Theory]
[MemberData(nameof(TgaBitsPerPixelFiles))]
public void TgaEncoder_RunLengthDoesNotCrossRowBoundaries(string imagePath, TgaBitsPerPixel bmpBitsPerPixel)
{
    var options = new TgaEncoder() { Compression = TgaCompression.RunLength };

    var testFile = TestFile.Create(imagePath);
    using (Image<Rgba32> input = testFile.CreateRgba32Image())
    {
        using (var memStream = new MemoryStream())
        {
            input.Save(memStream, options);
            // TODO assert the current encoded bytes match.
        }
    }
}

brianpopow · 2022-07-19T08:26:21Z

src/ImageSharp/Formats/Tga/TgaEncoderCore.cs

-            bool firstRow = true;
-            TPixel startPixel = pixels[xStart, yStart];
-            for (int y = yStart; y < pixels.Height; y++)
+            TPixel startPixel = pixels[xStart, yPos];


Instead of using the indexer, you could use DangerousGetRowSpan, which should be faster. Like this:

Span<TPixel> pixelRow = pixels.DangerousGetRowSpan(yPos).Slice(xStart);

JimBobSquarePants · 2022-07-19T12:27:55Z

I would do it by myself, but for some weird reason gitlfs does not let me push to this PR

There's a whole heap of issues related to GitHub, Git LFS and forks. Looks like they're finally on to figuring out a cause though which is ace.

git-lfs/git-lfs#5001

I use a workaround for pushing to forks when there is no LFS files to be added but that won't help in this case.

src/ImageSharp/Formats/Tga/TgaEncoderCore.cs

brianpopow · 2022-07-21T09:05:12Z

But there is a wired thing is the image size generated by ImageSharp bigger than the file generated by ImageMagick.

I had some more time looking into this. The size difference currently fort the given testimage is:

ImageSharp (main): 50.2 kB
ImageMagick: 35.6 kB

The reason for that is, that we always write Run-Length Packets. If consecutive pixels are not the same, this will add an additional byte overhead to each pixel data. This will add up alot, if there are not many pixels equal. (note: the test image looks good for RLE, but it has transparency).

So to fix this, we need to change this to not always write RLE packets.
This does not have to be part of this PR and can be a follow up PR.

brianpopow · 2022-07-30T14:21:36Z

I will go ahead and merge this. Code itself looks good, just tests are missing. I will do a follow up PR to add tests and fix the image size issue, since I do not know how to work around the git lfs issues

JimBobSquarePants · 2022-07-30T14:45:57Z

Code itself looks good

It's still using the indexer to access pixels which will be slow. Best make sure your follow up fixes that.

Fix rle compression mistake in tga encoder.

4b28eb6

l1nxy changed the title ~~Fix run-length encode compression mistake in tga encoder.~~ Fix run-length encode compression implementation mistake in tga encoder. Jul 13, 2022

l1nxy changed the title ~~Fix run-length encode compression implementation mistake in tga encoder.~~ Fix run-length encode compression implementation mistake in tga encoder Jul 13, 2022

brianpopow added formats:tga bug labels Jul 13, 2022

l1nxy closed this Jul 13, 2022

l1nxy deleted the fix-tga-rle-encoder branch July 13, 2022 09:57

l1nxy restored the fix-tga-rle-encoder branch July 13, 2022 10:15

l1nxy reopened this Jul 13, 2022

brianpopow requested changes Jul 19, 2022

View reviewed changes

brianpopow reviewed Jul 19, 2022

View reviewed changes

src/ImageSharp/Formats/Tga/TgaEncoderCore.cs Show resolved Hide resolved

Merge branch 'main' into fix-tga-rle-encoder

ab7f384

brianpopow merged commit 7dacf4f into SixLabors:main Jul 30, 2022

brianpopow mentioned this pull request Aug 3, 2022

TGA Encoder/Decoder Improvements #2197

Merged

4 tasks

brianpopow added a commit that referenced this pull request Aug 4, 2022

Add another test case for #2172

343b4af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix run-length encode compression implementation mistake in tga encoder #2172

Fix run-length encode compression implementation mistake in tga encoder #2172

l1nxy commented Jul 13, 2022 •

edited

Loading

CLAassistant commented Jul 13, 2022 •

edited

Loading

brianpopow commented Jul 13, 2022

l1nxy commented Jul 13, 2022

l1nxy commented Jul 13, 2022

brianpopow commented Jul 13, 2022

l1nxy commented Jul 13, 2022

brianpopow commented Jul 13, 2022 •

edited

Loading

l1nxy commented Jul 13, 2022

JimBobSquarePants commented Jul 14, 2022

brianpopow commented Jul 14, 2022

l1nxy commented Jul 18, 2022

brianpopow left a comment •

edited

Loading

brianpopow Jul 19, 2022

JimBobSquarePants commented Jul 19, 2022

brianpopow commented Jul 21, 2022

brianpopow commented Jul 30, 2022

JimBobSquarePants commented Jul 30, 2022

Fix run-length encode compression implementation mistake in tga encoder #2172

Fix run-length encode compression implementation mistake in tga encoder #2172

Conversation

l1nxy commented Jul 13, 2022 • edited Loading

Prerequisites

Description

CLAassistant commented Jul 13, 2022 • edited Loading

brianpopow commented Jul 13, 2022

l1nxy commented Jul 13, 2022

l1nxy commented Jul 13, 2022

brianpopow commented Jul 13, 2022

l1nxy commented Jul 13, 2022

brianpopow commented Jul 13, 2022 • edited Loading

l1nxy commented Jul 13, 2022

JimBobSquarePants commented Jul 14, 2022

brianpopow commented Jul 14, 2022

l1nxy commented Jul 18, 2022

brianpopow left a comment • edited Loading

Choose a reason for hiding this comment

brianpopow Jul 19, 2022

Choose a reason for hiding this comment

JimBobSquarePants commented Jul 19, 2022

brianpopow commented Jul 21, 2022

brianpopow commented Jul 30, 2022

JimBobSquarePants commented Jul 30, 2022

l1nxy commented Jul 13, 2022 •

edited

Loading

CLAassistant commented Jul 13, 2022 •

edited

Loading

brianpopow commented Jul 13, 2022 •

edited

Loading

brianpopow left a comment •

edited

Loading