Png - Preserve Pixel Format for Non-generic decode. #1861

JimBobSquarePants · 2021-11-30T06:48:47Z

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

Fixes #1813 by determining the most appropriate pixel format to decode to based upon the detected color type and bit depth.

I could have avoided the identify/reset/decode route and gained a little more performance but Identify is so quick anyway it seemed like the simplest approach.

codecov · 2021-11-30T07:17:15Z

Codecov Report

Merging #1861 (f6b8383) into master (f45e184) will decrease coverage by 0%.
The diff coverage is 86%.

@@          Coverage Diff           @@
##           master   #1861   +/-   ##
======================================
- Coverage      87%     87%   -1%     
======================================
  Files         957     957           
  Lines       50391   50474   +83     
  Branches     6264    6291   +27     
======================================
+ Hits        44180   44252   +72     
- Misses       5180    5185    +5     
- Partials     1031    1037    +6

Flag	Coverage Δ
unittests	`87% <86%> (-1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/ImageSharp/Formats/Png/PngDecoderCore.cs	`90% <83%> (-1%)`	⬇️
src/ImageSharp/Formats/Png/PngDecoder.cs	`87% <88%> (+4%)`	⬆️

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 82e8c0d...f6b8383. Read the comment docs.

src/ImageSharp/Formats/Png/PngDecoderCore.cs

Co-authored-by: Günther Foidl <gue@korporal.at>

antonfirsov

but Identify is so quick anyway it seemed like the simplest approach.

I'm not sure if it's always true. There is some unnecessary IO cost reading the stream twice when the file is large and/or there are *Text chunks not being skipped on ignoreMetadata. This may hurt scalability, and it doesn't seem to be very hard to fix:

We can introduce a field in PngDecoderCore that asks to skip everything except the absolute minimum needed to determine the pixel type (Header + Transparency?), and stop when the essential chunks are processed or when a Data chunk is detected.

Later we can improve the IImageDecoderInternals infrastructure to support these scenarios, but that's far beyond the essential feature work we need for Png.

JimBobSquarePants · 2021-12-07T08:38:10Z

I'm not sure if it's always true. There is some unnecessary IO cost reading the stream twice when the file is large and/or there are *Text chunks not being skipped on ignoreMetadata. This may hurt scalability, and it doesn't seem to be very hard to fix:

I've changed the behavior of the chunk reader to avoid reading data from anything other than the header and transparency chunks when required. This also allows skipping the entire chunk. It also now stops as soon as we have the info we require.

It's a little hacky but should be fast enough for now.

antonfirsov · 2021-12-13T13:16:23Z

src/ImageSharp/Formats/Png/PngDecoderCore.cs

+                                    this.SkipChunkDataAndCrc(chunk);
+                                    break;
+                                }
+
                                this.ReadGammaChunk(pngMetadata, chunk.Data.GetSpan());
                                break;
                            case PngChunkType.Data:
                                this.SkipChunkDataAndCrc(chunk);


Spec says:

When present, the tRNS chunk must precede the first IDAT chunk, and must follow the PLTE chunk, if any.

It means that if we reach Data, we are free to quit the loop. Forwarding the file position beyond the data chunk may have IO overhead we can easily avoid here.

antonfirsov · 2021-12-13T13:18:14Z

src/ImageSharp/Formats/Png/PngDecoderCore.cs

+                                this.paletteAlpha = alpha;
+                                this.AssignTransparentMarkers(alpha, pngMetadata);
+
+                                if (this.colorMetadataOnly && this.isHeaderChunkReached)


According to the spec, ~~IDAT~~ IHDR is always the first, so - in theory - we don't need to track if we reached it. I wonder if there are malformed PNG-s in the wild which do not follow this rule.

@brianpopow any thoughts?

You mean IHDR is always first I guess? I will go through my collection of "weird and broken images" to check if i can find any which has not IHDR first.

You mean IHDR is always first I guess?

Yes, sorry for the typo.

I could not find any png which do not start with IHDR. Nonetheless I am still in favor of keeping it as it is, just to be sure, because in theory such png can be created.

antonfirsov · 2021-12-13T13:19:10Z

src/ImageSharp/Formats/Png/PngDecoderCore.cs

+                                if (this.colorMetadataOnly && this.isHeaderChunkReached)
+                                {
+                                    // Quick exit
+                                    this.isEndChunkReached = true;


I wonder if we should just break out of the loop instead of abusing this flag?

goto baby; goto.

JimBobSquarePants · 2021-12-14T06:31:10Z

@antonfirsov @brianpopow I'll just follow the strict specification here and optimize accordingly. The worst that can happen is that they decode to the incorrect pixel format.

antonfirsov

LGTM. I think it's possible to achieve the same without goto will do it in a separate cleanup PR.

JimBobSquarePants · 2021-12-14T12:11:49Z

LGTM. I think it's possible to achieve the same without goto will do it in a separate cleanup PR.

Whole thing needs a cleanup. We should be skipping all chunks we don't currently read on decode but are instead reading the data.

JimBobSquarePants added 2 commits November 30, 2021 11:56

Stub out approach

61c30de

Update PngDecoder.cs

65bd0de

JimBobSquarePants added enhancement formats:png breaking Signifies a binary breaking change. labels Nov 30, 2021

JimBobSquarePants added this to the 2.0.0 milestone Nov 30, 2021

Merge branch 'master' into js/png-decode-to-type

ee4876b

gfoidl reviewed Nov 30, 2021

View reviewed changes

src/ImageSharp/Formats/Png/PngDecoderCore.cs Outdated Show resolved Hide resolved

JimBobSquarePants and others added 2 commits December 1, 2021 23:05

Update src/ImageSharp/Formats/Png/PngDecoderCore.cs

d7032fe

Co-authored-by: Günther Foidl <gue@korporal.at>

Merge branch 'master' into js/png-decode-to-type

509f254

JimBobSquarePants requested a review from a team December 1, 2021 12:13

JimBobSquarePants changed the title ~~Png - Preserve Pixel Format for Non-generic decode.~~ WIP - Png - Preserve Pixel Format for Non-generic decode. Dec 1, 2021

JimBobSquarePants marked this pull request as draft December 1, 2021 12:51

JimBobSquarePants added 2 commits December 2, 2021 00:37

Fix transparency chunk identification

8fd9b72

Update PngDecoderTests.cs

eb68bb2

JimBobSquarePants marked this pull request as ready for review December 1, 2021 13:37

JimBobSquarePants changed the title ~~WIP - Png - Preserve Pixel Format for Non-generic decode.~~ Png - Preserve Pixel Format for Non-generic decode. Dec 1, 2021

Merge branch 'master' into js/png-decode-to-type

8a20f3f

antonfirsov reviewed Dec 4, 2021

View reviewed changes

JimBobSquarePants added 2 commits December 7, 2021 18:42

Merge branch 'master' into js/png-decode-to-type

c9f2c9e

Optimize chunk reading to maximize performance

c214af5

Faster exit

7429547

JimBobSquarePants requested a review from antonfirsov December 12, 2021 11:44

JimBobSquarePants added 2 commits December 12, 2021 22:44

Merge branch 'master' into js/png-decode-to-type

3a76e34

Merge branch 'master' into js/png-decode-to-type

1157710

antonfirsov reviewed Dec 13, 2021

View reviewed changes

Merge branch 'master' into js/png-decode-to-type

a63bf58

Update PngDecoderCore.cs

f6b8383

antonfirsov approved these changes Dec 14, 2021

View reviewed changes

JimBobSquarePants merged commit ecd3db2 into master Dec 14, 2021

JimBobSquarePants deleted the js/png-decode-to-type branch December 14, 2021 12:12

JimBobSquarePants mentioned this pull request Jan 24, 2022

PNG file bit depth is not recognized correctly #1954

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Png - Preserve Pixel Format for Non-generic decode. #1861

Png - Preserve Pixel Format for Non-generic decode. #1861

JimBobSquarePants commented Nov 30, 2021

codecov bot commented Nov 30, 2021 •

edited

Loading

antonfirsov left a comment •

edited

Loading

JimBobSquarePants commented Dec 7, 2021 •

edited

Loading

antonfirsov Dec 13, 2021

antonfirsov Dec 13, 2021 •

edited

Loading

brianpopow Dec 13, 2021

antonfirsov Dec 13, 2021

brianpopow Dec 14, 2021

antonfirsov Dec 13, 2021

JimBobSquarePants Dec 14, 2021

JimBobSquarePants commented Dec 14, 2021

antonfirsov left a comment

JimBobSquarePants commented Dec 14, 2021

Png - Preserve Pixel Format for Non-generic decode. #1861

Png - Preserve Pixel Format for Non-generic decode. #1861

Conversation

JimBobSquarePants commented Nov 30, 2021

Prerequisites

Description

codecov bot commented Nov 30, 2021 • edited Loading

Codecov Report

antonfirsov left a comment • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants commented Dec 7, 2021 • edited Loading

antonfirsov Dec 13, 2021

Choose a reason for hiding this comment

antonfirsov Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

brianpopow Dec 13, 2021

Choose a reason for hiding this comment

antonfirsov Dec 13, 2021

Choose a reason for hiding this comment

brianpopow Dec 14, 2021

Choose a reason for hiding this comment

antonfirsov Dec 13, 2021

Choose a reason for hiding this comment

JimBobSquarePants Dec 14, 2021

Choose a reason for hiding this comment

JimBobSquarePants commented Dec 14, 2021

antonfirsov left a comment

Choose a reason for hiding this comment

JimBobSquarePants commented Dec 14, 2021

codecov bot commented Nov 30, 2021 •

edited

Loading

antonfirsov left a comment •

edited

Loading

JimBobSquarePants commented Dec 7, 2021 •

edited

Loading

antonfirsov Dec 13, 2021 •

edited

Loading