Skip to content

Commit

Permalink
Fix int16_t for glyphs, const glyphs, and clarify glyph loop.
Browse files Browse the repository at this point in the history
Several places in the PDF code are using int16_t for glyphs. With
newer NotoSans fonts, all possible glyph ids are being used, so this
can lead to problems.

The PDF glyphs from text code returns the text for the glyphs if the
encoding is for glyphs. However, it returns this using an unsafe const
cast which is hiding possible bugs and preventing correct use of const
in other places.

The way the glyph loop in SkPDFDevice::drawPosText is written uses a
'--i' in the loop, which makes it appear this can loop forever. I don't
believe it can, but it is an unecessary code folding. We should also at
least assert the forward progress correctness here.

Review URL: https://codereview.chromium.org/626613002

Cherry-pick: 22edc83
Approval: https://code.google.com/p/chromium/issues/detail?id=418939#c30
  • Loading branch information
bungeman committed Oct 7, 2014
1 parent f14866d commit f3ffa8b
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 19 deletions.
33 changes: 18 additions & 15 deletions src/pdf/SkPDFDevice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ typedef SkAutoSTMalloc<128, uint16_t> SkGlyphStorage;

static int force_glyph_encoding(const SkPaint& paint, const void* text,
size_t len, SkGlyphStorage* storage,
uint16_t** glyphIDs) {
const uint16_t** glyphIDs) {
// Make sure we have a glyph id encoding.
if (paint.getTextEncoding() != SkPaint::kGlyphID_TextEncoding) {
int numGlyphs = paint.textToGlyphs(text, len, NULL);
Expand All @@ -131,8 +131,7 @@ static int force_glyph_encoding(const SkPaint& paint, const void* text,
// For user supplied glyph ids we need to validate them.
SkASSERT((len & 1) == 0);
int numGlyphs = SkToInt(len / 2);
const uint16_t* input =
reinterpret_cast<uint16_t*>(const_cast<void*>((text)));
const uint16_t* input = static_cast<const uint16_t*>(text);

int maxGlyphID = max_glyphid_for_typeface(paint.getTypeface());
int validated;
Expand All @@ -142,7 +141,7 @@ static int force_glyph_encoding(const SkPaint& paint, const void* text,
}
}
if (validated >= numGlyphs) {
*glyphIDs = reinterpret_cast<uint16_t*>(const_cast<void*>((text)));
*glyphIDs = static_cast<const uint16_t*>(text);
return numGlyphs;
}

Expand Down Expand Up @@ -1115,7 +1114,7 @@ void SkPDFDevice::drawText(const SkDraw& d, const void* text, size_t len,
}

SkGlyphStorage storage(0);
uint16_t* glyphIDs = NULL;
const uint16_t* glyphIDs = NULL;
int numGlyphs = force_glyph_encoding(paint, text, len, &storage, &glyphIDs);
textPaint.setTextEncoding(SkPaint::kGlyphID_TextEncoding);

Expand All @@ -1128,8 +1127,9 @@ void SkPDFDevice::drawText(const SkDraw& d, const void* text, size_t len,
while (numGlyphs > consumedGlyphCount) {
updateFont(textPaint, glyphIDs[consumedGlyphCount], content.entry());
SkPDFFont* font = content.entry()->fState.fFont;
//TODO: the const_cast here is a bug if the encoding started out as glyph encoding.
int availableGlyphs =
font->glyphsToPDFFontEncoding(glyphIDs + consumedGlyphCount,
font->glyphsToPDFFontEncoding(const_cast<uint16_t*>(glyphIDs) + consumedGlyphCount,
numGlyphs - consumedGlyphCount);
fFontGlyphUsage->noteGlyphUsage(font, glyphIDs + consumedGlyphCount,
availableGlyphs);
Expand Down Expand Up @@ -1160,9 +1160,8 @@ void SkPDFDevice::drawPosText(const SkDraw& d, const void* text, size_t len,
}

SkGlyphStorage storage(0);
uint16_t* glyphIDs = NULL;
size_t numGlyphs = force_glyph_encoding(paint, text, len, &storage,
&glyphIDs);
const uint16_t* glyphIDs = NULL;
size_t numGlyphs = force_glyph_encoding(paint, text, len, &storage, &glyphIDs);
textPaint.setTextEncoding(SkPaint::kGlyphID_TextEncoding);

SkDrawCacheProc glyphCacheProc = textPaint.getDrawCacheProc();
Expand All @@ -1172,19 +1171,23 @@ void SkPDFDevice::drawPosText(const SkDraw& d, const void* text, size_t len,
SkPDFFont* font = content.entry()->fState.fFont;
uint16_t encodedValue = glyphIDs[i];
if (font->glyphsToPDFFontEncoding(&encodedValue, 1) != 1) {
// The current pdf font cannot encode the current glyph.
// Try to get a pdf font which can encode the current glyph.
updateFont(textPaint, glyphIDs[i], content.entry());
i--;
continue;
font = content.entry()->fState.fFont;
if (font->glyphsToPDFFontEncoding(&encodedValue, 1) != 1) {
SkDEBUGFAIL("PDF could not encode glyph.");
continue;
}
}

fFontGlyphUsage->noteGlyphUsage(font, &encodedValue, 1);
SkScalar x = pos[i * scalarsPerPos];
SkScalar y = scalarsPerPos == 1 ? constY : pos[i * scalarsPerPos + 1];
align_text(glyphCacheProc, textPaint, glyphIDs + i, 1, &x, &y);
set_text_transform(x, y, textPaint.getTextSkewX(),
&content.entry()->fContent);
set_text_transform(x, y, textPaint.getTextSkewX(), &content.entry()->fContent);
SkString encodedString =
SkPDFString::FormatString(&encodedValue, 1,
font->multiByteGlyphs());
SkPDFString::FormatString(&encodedValue, 1, font->multiByteGlyphs());
content.entry()->fContent.writeText(encodedString.c_str());
content.entry()->fContent.writeText(" Tj\n");
}
Expand Down
4 changes: 2 additions & 2 deletions src/pdf/SkPDFFont.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1028,7 +1028,7 @@ bool SkPDFFont::addCommonFontDescriptorEntries(int16_t defaultWidth) {
return true;
}

void SkPDFFont::adjustGlyphRangeForSingleByteEncoding(int16_t glyphID) {
void SkPDFFont::adjustGlyphRangeForSingleByteEncoding(uint16_t glyphID) {
// Single byte glyph encoding supports a max of 255 glyphs.
fFirstGlyphID = glyphID - (glyphID - 1) % 255;
if (fLastGlyphID > fFirstGlyphID + 255 - 1) {
Expand Down Expand Up @@ -1425,7 +1425,7 @@ SkPDFType3Font::SkPDFType3Font(const SkAdvancedTypefaceMetrics* info,

SkPDFType3Font::~SkPDFType3Font() {}

bool SkPDFType3Font::populate(int16_t glyphID) {
bool SkPDFType3Font::populate(uint16_t glyphID) {
SkPaint paint;
paint.setTypeface(typeface());
paint.setTextSize(1000);
Expand Down
2 changes: 1 addition & 1 deletion src/pdf/SkPDFFont.h
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ class SkPDFFont : public SkPDFDict {
/** Set fFirstGlyphID and fLastGlyphID to span at most 255 glyphs,
* including the passed glyphID.
*/
void adjustGlyphRangeForSingleByteEncoding(int16_t glyphID);
void adjustGlyphRangeForSingleByteEncoding(uint16_t glyphID);

// Generate ToUnicode table according to glyph usage subset.
// If subset is NULL, all available glyph ids will be used.
Expand Down
2 changes: 1 addition & 1 deletion src/pdf/SkPDFFontImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class SkPDFType3Font : public SkPDFFont {
SkPDFType3Font(const SkAdvancedTypefaceMetrics* info,
SkTypeface* typeface, uint16_t glyphID);

bool populate(int16_t glyphID);
bool populate(uint16_t glyphID);
};

#endif

0 comments on commit f3ffa8b

Please sign in to comment.