Add support for decoding and encoding of TGA images #1026

brianpopow · 2019-10-13T16:09:30Z

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

As the title say already: This will add support for TGA images.

Still work in progress, because some unit test are failing.

… when there is insufficient data

brianpopow · 2019-10-27T16:59:35Z

@JimBobSquarePants I think this is ready for review.

I noticed there is maybe an issue with InternalDetectFormat.
There a buffer will be allocated with a length of maxHeaderSize. This buffer will be passed to DetectFormat. In all DetectFormat methods of the different formats,
there is usally a check for a minimum length of the buffer. Since the buffer will be always maxHeaderSize this checks will alway be true.

For example you could pass in the two bytes stream 0xFFD8 and it will passs as a valid JPEG file, even though the length check indicates it should be at least 11 bytes.

JimBobSquarePants · 2019-10-28T12:58:54Z

Thanks @brianpopow I'm amazed how good you are at working with all these formats!

I'll have a good read of the supplied specification (thanks for adding that) and go through your code.

On the point of the format detector. Yes, that sounds dangerous, we should review it. 👍

codecov · 2019-10-28T13:03:02Z

Codecov Report

Merging #1026 into master will increase coverage by 0.04%.
The diff coverage is 92.94%.

@@            Coverage Diff             @@
##           master    #1026      +/-   ##
==========================================
+ Coverage   89.89%   89.94%   +0.04%     
==========================================
  Files        1107     1123      +16     
  Lines       49156    49768     +612     
  Branches     3447     3515      +68     
==========================================
+ Hits        44191    44764     +573     
- Misses       4253     4279      +26     
- Partials      712      725      +13

Impacted Files	Coverage Δ
tests/ImageSharp.Tests/TestImages.cs	`100% <ø> (ø)`	⬆️
...ImageSharp.Tests/Formats/Bmp/BmpFileHeaderTests.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp/Formats/Tga/TgaThrowHelper.cs	`0% <0%> (ø)`
src/ImageSharp/Formats/Tga/TgaConstants.cs	`100% <100%> (ø)`
...ogramEqualizationSlidingWindowProcessor{TPixel}.cs	`100% <100%> (ø)`	⬆️
...ts/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs	`100% <100%> (ø)`	⬆️
...c/ImageSharp/Formats/Tga/TgaConfigurationModule.cs	`100% <100%> (ø)`
src/ImageSharp/Common/Helpers/ImageMaths.cs	`87.5% <100%> (+0.32%)`	⬆️
src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs	`94.38% <100%> (+0.01%)`	⬆️
...ts/ImageSharp.Tests/Formats/Tga/TgaEncoderTests.cs	`100% <100%> (ø)`
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0049ca2...d183a51. Read the comment docs.

JimBobSquarePants

Really great work!! 👍

Just a few comments and what looks like a bug to me.

src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs

src/ImageSharp/Formats/Tga/ITgaEncoderOptions.cs

src/ImageSharp/Formats/Tga/TgaDecoderCore.cs

JimBobSquarePants · 2019-11-05T00:50:16Z

src/ImageSharp/Formats/Tga/TgaDecoderCore.cs

+                    }
+
+                    int colorMapPixelSizeInBytes = this.fileHeader.CMapDepth / 8;
+                    var palette = new byte[this.fileHeader.CMapLength * colorMapPixelSizeInBytes];


What's the maximum size here? Maybe we should stackalloc a Span<byte> or if too large use pooling?

a typical size would be between 512 and 768, but the specification states its possible to have a CmapLength of 4096 with 8 bit for each entry.

src/ImageSharp/Formats/Tga/TgaDecoderCore.cs

JimBobSquarePants · 2019-11-05T01:12:49Z

src/ImageSharp/Formats/Tga/TgaImageFormatDetector.cs

+            {
+                // There is no magick bytes in a tga file, so at least the image type
+                // and the colormap type in the header will be checked for a valid value.
+                if (header[1] != 0 && header[1] != 1)


Astonishing that there are no identifiers!

yes its really strange. As far as i know its the only image format without some sort of magick bytes

JimBobSquarePants · 2019-11-05T01:14:26Z

src/ImageSharp/Formats/Tga/TgaImageType.cs

+    {
+        /// <summary>
+        /// No image data included.
+        /// Not sure what this is used for.


Let's remove this comment from the summary. I don't think the type is ever used but it's in the spec so no need for the comment.

JimBobSquarePants · 2019-11-05T01:17:07Z

src/ImageSharp/Formats/Tga/TgaImageTypeExtensions.cs

+        {
+            byte imageTypeVal = (byte)imageType;
+
+            switch (imageTypeVal)


Why cast and switch?

good question, does not make sense. Its changed now.

JimBobSquarePants · 2019-11-05T01:17:50Z

src/ImageSharp/PixelFormats/PixelOperations{TPixel}.Generated.tt

        /// Converts all pixels in 'source` span of <see cref="<#=pixelType#>"/> into a span of <typeparamref name="TPixel"/>-s.
        /// </summary>
-        /// <param name="configuration">A <see cref="Configuration"/> to configure internal operations</param>
+        /// <param name="configuration">A <see cref="Configuration"/> to configure internal operations.</param>


Well spotted 😄

JimBobSquarePants · 2019-11-05T01:19:07Z

tests/ImageSharp.Tests/ImageSharp.Tests.csproj


  <ItemGroup>
-    <PackageReference Include="Magick.NET-Q16-AnyCPU" />
+    <PackageReference Include="Magick.NET-Q16-AnyCPU" Version="7.14.4" />


Don't add the version here. You have to add it to the Directory.Targets file.

…ion to pixelDataStart

JimBobSquarePants

Perfect! 👏

brianpopow · 2019-11-12T20:20:27Z

@JimBobSquarePants i think this is done now and can be merged (if you dont see any more issues).
appveyor is failing for net472, but i dont think this is related to my changes.

JimBobSquarePants · 2019-11-12T21:06:37Z

And we're in! Thanks so much for your help!

brianpopow and others added 30 commits October 6, 2019 12:23

Add decoding of 24 bit targa files

4a5e075

Add support for decoding 8, 16, 32 bit tga files

10ff875

Add tga decoder tests

2e1a6ff

Fix decoding 16 bit tga files by making them opaque

a2c4636

Add RLE test images

5037380

Add decoding of 24bit RLE tga images

f6cce78

Add decoding of 32 bit RLE

f69048f

Avoid calculating the row start index inside the loop

914d191

Unified reading rle images

f1bfba6

Add decoding of 16 and 8 bit rle images

e18481b

Add support for encoding 24 bit tga files

e7fe670

Support for encoding 8, 16 and 32 bit tga files

5aaf845

Add support for decoding tga image with palette

bb92179

Add support for decoding rle tga with palette

d13b646

Change test images, add additional tests

391cb44

Add tests for topleft origin

6fda29b

Add support for images with top left origin

b4a8778

Treat bgra5551 pixels as opaque

94b4f35

Add support for encoding RLE tga images

6720f10

Add support for encoding rle 8, 16 and bit tga images

28222b5

Add test for the tga encoder

f8854c7

Skip palette bytes if image type indicates its no palette image

8dfe6da

A little cleanup and comments

b1572ae

Add CompareToOriginal at the end of Issue1014 test

d4b7587

Set expected default configuration count to 5

ca6dcb3

Fix orientation of RLE images

a0728d9

Fix encoding of 16 tga files

56150f1

Using tolerant comparer for 16 and 8 bit

45a6a6b

Add tga specification

30291a2

Merge branch 'master' into feature/tga

178761a

brianpopow added 4 commits October 26, 2019 21:22

Fix build error in Release mode

10c9867

Add check for valid tga image type in the format detector

5b478c5

Add tests for bmp and tga header to throw UnknownImageFormatException…

b1e8202

… when there is insufficient data

Throw ImageFormatException when width or height is 0

5789599

brianpopow changed the title ~~WIP: Add support for decoding and encoding of TGA images~~ Add support for decoding and encoding of TGA images Oct 27, 2019

Merge branch 'master' into feature/tga

32cd7a3

JimBobSquarePants requested changes Nov 5, 2019

View reviewed changes

brianpopow and others added 6 commits November 8, 2019 21:18

Code review changes

1b04fbe

Fix converting pixel to gray in histogram equalization

7681c6a

Add benchmarks for tga images

43fcf57

Merge branch 'master' into feature/tga

917c1e4

Add width and height as parameter for makeopaque, change currentPosit…

e8e1e52

…ion to pixelDataStart

Avoid second iteration over the stream in ReadBgra16 to make it opaque

d3f1d3c

JimBobSquarePants approved these changes Nov 10, 2019

View reviewed changes

brianpopow added 3 commits November 12, 2019 19:22

Update external for issue 984 testimages

006491f

Add GetBT709Luminance with vector test

45cd31a

Merge remote-tracking branch 'upstream/master' into feature/tga

d183a51

JimBobSquarePants merged commit 133d908 into SixLabors:master Nov 12, 2019

JimBobSquarePants added formats:tga enhancement area:formats labels Nov 12, 2019

JimBobSquarePants added this to the 1.0.0-rc1 milestone Nov 12, 2019

JimBobSquarePants mentioned this pull request Dec 4, 2019

Allow returning the image format with the info. #1059

Merged

4 tasks

ghost mentioned this pull request Dec 8, 2019

Support loading TGA format? #629

Closed

4 tasks

Uh oh!

Uh oh!

Add support for decoding and encoding of TGA images #1026

Add support for decoding and encoding of TGA images #1026

Uh oh!

Conversation

brianpopow commented Oct 13, 2019

Prerequisites

Description

Uh oh!

brianpopow commented Oct 27, 2019

Uh oh!

JimBobSquarePants commented Oct 28, 2019

Uh oh!

codecov bot commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

brianpopow commented Nov 12, 2019

Uh oh!

JimBobSquarePants commented Nov 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 28, 2019 •

edited

Loading