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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICC Improvements #588

Merged
merged 6 commits into from May 23, 2018

Conversation

Projects
None yet
2 participants
@JBildstein
Copy link
Contributor

JBildstein commented May 22, 2018

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

This PR improves the handling of ICC profiles, specifically:

  • fixes profile ID calculation
  • add check for the validity of a profile and use it in the jpeg decoders
  • change the ToByteArray method to use the underlying buffer instead of parsing and then writing the profile
  • make parsing of profile data more defensive to guard against corrupt data

That last point is not extensive and could need some more work. Currently only the tag table is checked for invalid values but it would make sense for many values to check them during parsing as well.
For example, often a number of array items or a position within the profile is read. I'm not sure how to best handle it yet and may need some medium changes and a separate PR.
Until the profiles are actually used for color management, it's not really relevant though. The issues that surfaced with corrupt profiles only occured because of the way ToByteArray was implemented.

This PR fixes #578, #559 and #562

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 22, 2018

Codecov Report

Merging #588 into master will decrease coverage by 0.03%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #588      +/-   ##
==========================================
- Coverage    88.8%   88.77%   -0.04%     
==========================================
  Files         853      854       +1     
  Lines       36100    36168      +68     
  Branches     2607     2610       +3     
==========================================
+ Hits        32059    32108      +49     
- Misses       3267     3287      +20     
+ Partials      774      773       -1
Impacted Files Coverage 螖
...p/Formats/Jpeg/GolangPort/GolangJpegDecoderCore.cs 75.46% <0%> (-0.57%) 猬囷笍
.../MetaData/Profiles/ICC/DataReader/IccDataReader.cs 94.11% <100%> (酶) 猬嗭笍
...arp.Tests/MetaData/Profiles/ICC/IccProfileTests.cs 100% <100%> (酶)
...mageSharp.Tests/TestDataIcc/IccTestDataProfiles.cs 100% <100%> (酶) 猬嗭笍
...arp/Formats/Jpeg/PdfJsPort/PdfJsJpegDecoderCore.cs 83.71% <50%> (-0.55%) 猬囷笍
src/ImageSharp/MetaData/Profiles/ICC/IccReader.cs 91.93% <72.22%> (-8.07%) 猬囷笍
src/ImageSharp/MetaData/Profiles/ICC/IccProfile.cs 91.3% <88.46%> (-1.56%) 猬囷笍
...C/TagDataEntries/IccTextDescriptionTagDataEntry.cs 48.48% <0%> (-6.07%) 猬囷笍
...files/ICC/DataWriter/IccDataWriter.TagDataEntry.cs 77.14% <0%> (-1.43%) 猬囷笍
...rofiles/ICC/TagDataEntries/IccCurveTagDataEntry.cs 48.27% <0%> (酶) 猬嗭笍
... and 3 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 90272b4...df7fa40. Read the comment docs.

@JimBobSquarePants
Copy link
Member

JimBobSquarePants left a comment

THE KING OF ICC!!!

Looking good, just some minor changes to ensure we don't affect performance.

@@ -345,6 +345,11 @@ public void ParseStream(Stream stream, bool metadataOnly = false)
// Read on.
fileMarker = FindNextFileMarker(this.markerBuffer, this.InputStream);
}

if (this.MetaData.IccProfile?.CheckIsValid() == false)

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 23, 2018

Member

Can we move this to AssignResolution and rename that to InitDerivedMetaDataProperties, The return above prevents us reading more than we need to when we are identifying images.

This comment has been minimized.

@JBildstein

JBildstein May 23, 2018

Contributor

I initially wanted to put it there, but it's only called from Decode and Identify and not in ParseStream. In the go port it's called at the end of the ParseStream method (and by extension in Decode and Identify).
Should we move AssignResolution there as well? I changed the return you mentioned to a break so things after the while loop will be executed.

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 23, 2018

Member

Both Decode and Identify call ParseStream in both decoders. We want to return at that point in PdfJs decoder to avoid reading the entire SOS segment when we don鈥檛 need to. So using break slows us down.

I can move the method if you like? Save you the bother.

This comment has been minimized.

@JBildstein

JBildstein May 23, 2018

Contributor

yes they do, but ParseStream is public so it can be called directly, that's the only reason I wanted to do it there.
Sorry about the return thing, I thought break behaves differently at that point. We could use a
goto to jump out of the while (it's evil, I know :P)

and no worries, it's a small change, I can do it.

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 23, 2018

Member

馃憤 It's only public as we use it for tests. The class is internal so not available to consumers.

This comment has been minimized.

@JBildstein

JBildstein May 23, 2018

Contributor

ok great, then there's no reason to change anything in that regard. Will push changes in a couple minutes.

@@ -413,6 +413,11 @@ public void ParseStream(Stream stream, bool metadataOnly = false)
}

this.InitDerivedMetaDataProperties();

if (this.MetaData.IccProfile?.CheckIsValid() == false)

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants May 23, 2018

Member

Can we move this to InitDerivedMetaDataProperties

@JimBobSquarePants JimBobSquarePants merged commit 41e23b8 into SixLabors:master May 23, 2018

5 checks passed

codecov/patch 90% of diff hit (target 88.8%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +1.19% compared to 90272b4
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@JBildstein JBildstein deleted the JBildstein:icc-improvements branch May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment