Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix font glyph path #757

Merged
merged 2 commits into from
Jan 20, 2024
Merged

Fix font glyph path #757

merged 2 commits into from
Jan 20, 2024

Conversation

BobLd
Copy link
Collaborator

@BobLd BobLd commented Jan 15, 2024

This replaces #742

Sorry for the long PR. Many file modified are just tidy up.

Regarding how font matrices are actually supposed to work, this is not clear as per the documenation. have a look here if you want to see some discussion:

All my changes are backed by visual tests (for both glyph paths and bounding boxes)

Added test documents either come from:

@BobLd BobLd force-pushed the fix-font-glyph-path branch from 6c70852 to 41dbb39 Compare January 16, 2024 10:27
@BobLd
Copy link
Collaborator Author

BobLd commented Jan 16, 2024

@EliotJones I've pushed a commit to fix a compilation warning.

Regarding your point about sealed classes, the only 2 public ones I sealed I could find are CompactFontFormatFontCollection and GlyphList, let me know if you want me to unseal them (I'm not sure how could people derive and use classes based on those, but I really could be wrong)

Copy link
Member

@EliotJones EliotJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a couple of places becoming sealed that would change the public API, otherwise LGTM even though I'm still not 100% on what's happening but if the tests look good then it should be right

src/UglyToad.PdfPig.Fonts/GlyphList.cs Outdated Show resolved Hide resolved
for (int c = 0; c < values.Length; c++)
{
int i = 2 * c;
values[c] = PdfFunction.ClipToRange(values[c], Range[i], Range[i + 1]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest what is happening here?

Copy link
Collaborator Author

@BobLd BobLd Jan 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tested in RepresentativeWindEnergyDeals() in the GenerateLetterGlyphImages. Document is GHOSTSCRIPT-698168-0.pdf. You can try to open it in Acrobat reader (supports ICC) and Firefox/pdf.js (does not support ICC) and you will see the difference.

In this particular test, the color space is ICC, but we don't support it. So we use the Alternate CC. The input values are not within the RGB range (which is the alternate CC).

In the case of ICC, the Alternate CC is defined as (I've put in bold the part that justifies this change):

(Optional) An alternate colour space that shall be used in case the one specified in the stream data is not supported. PDF readers should not use this colour space. The alternate space may be any valid colour space (except a Pattern colour space) that has the number of components specified by N. If this entry is omitted and the PDF reader does not understand the ICC profile data, the colour space that shall be used is DeviceGray, DeviceRGB, or DeviceCMYK, depending on whether the value of N is 1, 3, or 4, respectively. There shall not be conversion of source colour values, such as a tint transformation, when using the alternate colour space. Colour values within the range of the ICCBased colour space might not be within the range of the alternate colour space. In this case and after constraining to the ICCBased range, the nearest values within the range of the alternate space shall be substituted without error indication.

Do let me know if you understand that in a different way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is how they do in PdfBox, where they clamp values too.

https://github.com/apache/pdfbox/blob/33ebd39096a650df157a846d9e8c631a1e3631ab/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/color/PDICCBased.java#L291C13-L291C75

Again, happy for you to tell me if you understand that differently

@BobLd BobLd force-pushed the fix-font-glyph-path branch from 95eebc8 to dd693f0 Compare January 20, 2024 16:13
@BobLd
Copy link
Collaborator Author

BobLd commented Jan 20, 2024

I agree with you, this is not exactly clear what happens. And reading the discussions in the links I've put, it seems we are not the only ones. I tried to document the code to reflect that

@BobLd BobLd merged commit 6103cf1 into UglyToad:master Jan 20, 2024
1 check passed
@BobLd BobLd deleted the fix-font-glyph-path branch January 20, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants