ICC Improvements #588
ICC Improvements #588
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| fileMarker = FindNextFileMarker(this.markerBuffer, this.InputStream); | ||
| } | ||
|
|
||
| if (this.MetaData.IccProfile?.CheckIsValid() == false) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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’t need to. So using break slows us down.
I can move the method if you like? Save you the bother.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
👍 It's only public as we use it for tests. The class is internal so not available to consumers.
There was a problem hiding this comment.
ok great, then there's no reason to change anything in that regard. Will push changes in a couple minutes.
|
|
||
| this.InitDerivedMetaDataProperties(); | ||
|
|
||
| if (this.MetaData.IccProfile?.CheckIsValid() == false) |
There was a problem hiding this comment.
Can we move this to InitDerivedMetaDataProperties
2 participants
Prerequisites
Description
This PR improves the handling of ICC profiles, specifically:
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