Add support for glyphs stored in CFF formats - #265
Conversation
Codecov Report
@@ Coverage Diff @@
## main #265 +/- ##
======================================
- Coverage 83% 81% -2%
======================================
Files 188 216 +28
Lines 9856 11264 +1408
Branches 1589 1750 +161
======================================
+ Hits 8222 9233 +1011
- Misses 1266 1607 +341
- Partials 368 424 +56
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
brianpopow
left a comment
brianpopow
left a comment
There was a problem hiding this comment.
Good work, this is a really huge PR. This will be a great improvement for the Fonts library!
| { | ||
| Span<byte> slice = bufferSpan.Slice(0, length); | ||
| int actualRead = reader.BaseStream.Read(slice); | ||
| #if DEBUG |
There was a problem hiding this comment.
Why is this only thrown when debugging? Is this not considered an error in any case?
| switch (entry.Operator.Name) | ||
| { | ||
| default: | ||
| #if DEBUG |
There was a problem hiding this comment.
I dont like those #if DEBUG statements much.
Maybe we should introduce a proper logger for such cases? I really like serilog for such things. I am a big fan of structured logging. I can really help finding bugs/issues.
| { | ||
| return reader.ReadInt16(); | ||
| } | ||
| else if (b0 == 29) |
There was a problem hiding this comment.
nit: I would omit those redundant else's
| // Note 6 The charstring itself may end with a call(g)subr; the subroutine must | ||
| // then end with an endchar operator | ||
| // endchar | ||
| throw new Exception("invalid end byte?"); |
There was a problem hiding this comment.
again why only throw in Debug mode? If not only in debug mode, maybe a more specific exception/exception message .
| this.currentGlyphIndex = 0; | ||
| this.currentSelectedRangeIndex = 0; | ||
|
|
||
| if (ranges != null) |
There was a problem hiding this comment.
| if (ranges != null) | |
| if (ranges?.Length > 0) |
|
Maybe we could use some of those fonts here for testing: https://v-fonts.com/tags/C107 |
| byte[] header = reader.ReadBytes(4); | ||
| byte major = header[0]; | ||
| byte minor = header[1]; | ||
| byte hdrSize = header[2]; |
There was a problem hiding this comment.
The spec recommends using the hdrSize to locate the Name INDEX.
| private static void Register(Dictionary<int, CFFOperator> dictionary, byte b0, string operatorName, OperatorOperandKind opopKind) | ||
| => dictionary.Add(b0, new CFFOperator(operatorName, b0, 0, opopKind)); | ||
|
|
||
| #if DEBUG |
There was a problem hiding this comment.
Maybe use DebuggerDisplay attribute instead?
There was a problem hiding this comment.
Yeah, just a chore that's all....
| /// <param name="encoding">The encoding.</param> | ||
| /// <param name="buffer">The buffer.</param> | ||
| /// <returns>The string.</returns> | ||
| public static string GetString(this Encoding encoding, ReadOnlySpan<byte> buffer) |
There was a problem hiding this comment.
The ReadString and ReadTag methods in BigEndianBinaryReader could maybe make use of this extension method
| } | ||
| } | ||
|
|
||
| private void ReadCharsetsFormat0(BigEndianBinaryReader reader, string[] stringIndex, CffGlyphData[] glyphs) |
There was a problem hiding this comment.
ReadCharsetsFormat0 and ReadCharsetsFormat1 are not covered by tests according to the code coverage report. Would be good, if we have tests for those cases.
There was a problem hiding this comment.
I'll double check. The should be getting called.
|
@brianpopow thanks for the review so far! I've got some experimental code locally this is pinches fingers "this close" to working that allows me to delete a ton of code that is creating second set of commands rather than the raw literals used for rendering the output. I'll do a whole heap of cleanup once it's working. |
| /// A Type 2 charstring program is a sequence of unsigned 8-bit bytes that encode numbers and operators. | ||
| /// The byte value specifies a operator, a number, or subsequent bytes that are to be interpreted in a specific manner | ||
| /// </remarks> | ||
| internal ref struct CffEvaluationEngine |
There was a problem hiding this comment.
@brianpopow this is a complete rewrite of the evaluator based upon the Fontkit implementation and sense checked against PDFPig. It's much, much more efficient that the original implementation saving a secondary buffer allocation for each set of operand instructions in the font.
| bool a; | ||
| bool b; | ||
| byte twoByteOperator = reader.ReadByte(); | ||
| if (twoByteOperator < 38) |
There was a problem hiding this comment.
The nesting makes it kind of hard to read. Maybe invert the if statement here and throw ThrowInvalidOperator(twoByteOperator) to reduce one level?
| { | ||
| ThrowInvalidOperator(twoByteOperator); | ||
| } | ||
| else |
There was a problem hiding this comment.
The intention was to remove the else nesting completely now, since twoByteOperator < 38, but that's just a nitpick. Leave it like it is, if you want.
There was a problem hiding this comment.
Of course, sorry! Brains elsewhere just now
brianpopow
left a comment
brianpopow
left a comment
There was a problem hiding this comment.
Looks good, I think its time to get this into the main branch 👍
|
@JimBobSquarePants I think we missed to update the readme. We can now remove the Limitations section and add CFF glyph's to the feature list |
|
Yup. Missed that! We’ll spotted! |
2 participants
Prerequisites
Description
Fixes #12
Adds support for CFF fonts.
NOTE: This does not add support for CCF2 variable fonts.
There's a lot going on here so I thought best to open a draft so review can be a gradual process. I had to do significant refactoring in order to allow both TTF and CFF glyph formats.
I'm basing the work primarily on LayoutFarm Typograph with cleanup and missing features based upon UglyToad PdfPig
Initial rendering experiments seems to be working well and the new architecture allows shaping for both formats.
TODO: