Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2276653002:
Add fallback fonts in pdfium (Closed)
DescriptionAllow CPDF_Font to use fallback fonts
Added a vector of pointers to CFX_Fonts in the class CPDF_Font, so that
fallback fonts may be used. In CPDF_CharPosList::Load, the glyphs for each
character are calculated. When m_Font does not support a character, a fallback
font is selected and the character is rendered using that font. This meant
adding an attribute to FXTEXT_CHARPOS so it knows which font renders it.
Also, methods in fpdf_render_text.cpp now may need to call device drawing
methods multiple times because these only support one font at a time. In
CPDF_TextRenderer::DrawNormalText and in CPDF_TextRenderer::DrawTextPath, the
device drawing method is called as few times as possible by grouping contiguous
characters rendered by the same font. In
CPDF_RenderStatus::DrawTextPathWithPattern, drawing was already done one
character at a time, but precalculating CFX_FaceCache. Now, the face cache is
precalculated for all of the fallback fonts.
The list of fallback fonts does not include tha main font. Otherwise the list
would be of raw pointers to avoid double free problems. For now, the font
Arial is used as fallback. This should fix the issue of not seeing Latin
characters displayed when bad fonts are used. However, this should be improved.
Tested manually using the file in the bug, plus a font directory containing a
font that supports Hangul but not Latin. This font is chosen as the substitute
font, but Latin characters are now being rendered.
Design proposal: go/pdfium_fallbackfonts
BUG=pdfium:358
Committed: https://pdfium.googlesource.com/pdfium/+/b107193e9780b4a50e85d54c1ffbd2303263e193
Patch Set 1 #Patch Set 2 : Nits #
Total comments: 12
Patch Set 3 : Only fallbacks in vector #
Total comments: 28
Patch Set 4 : Address comments #
Total comments: 10
Patch Set 5 : Comments #
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Add fallback fonts in pdfium BUG= ========== to ========== Allow CPDF_Font to use fallback fonts Added a vector of pointers to CFX_Fonts in the class CPDF_Font, so that fallback fonts may be used. In CPDF_CharPosList::Load, the glyphs for each character are calculated. When m_Font does not support a character, a fallback font is selected and the character is rendered using that font. This meant adding an attribute to FXTEXT_CHARPOS so it knows which font renders it. Also, methods in fpdf_render_text.cpp now may need to call device drawing methods multiple times because these only support one font at a time. In CPDF_TextRenderer::DrawNormalText and in CPDF_TextRenderer::DrawTextPath, the device drawing method is called as few times as possible by grouping contiguous characters rendered by the same font. In CPDF_RenderStatus::DrawTextPathWithPattern, drawing was already done one character at a time, but precalculating CFX_FaceCache. Now, the face cache is precalculated for all of the fallback fonts. Pointers are used instead of unique_ptrs because apparently one the destruction of the fonts is already being explicitly handled, so replacing with unique_ptrs will require more code changes. For now, the font Arial is used as fallback. This should fix the issue of not seeing Latin characters displayed when bad fonts are used. However, this should be improved. Design proposal: go/pdfium_fallbackfonts BUG=pdfium:358 ==========
npm@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org, tsepez@chromium.org, weili@chromium.org
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Allow CPDF_Font to use fallback fonts Added a vector of pointers to CFX_Fonts in the class CPDF_Font, so that fallback fonts may be used. In CPDF_CharPosList::Load, the glyphs for each character are calculated. When m_Font does not support a character, a fallback font is selected and the character is rendered using that font. This meant adding an attribute to FXTEXT_CHARPOS so it knows which font renders it. Also, methods in fpdf_render_text.cpp now may need to call device drawing methods multiple times because these only support one font at a time. In CPDF_TextRenderer::DrawNormalText and in CPDF_TextRenderer::DrawTextPath, the device drawing method is called as few times as possible by grouping contiguous characters rendered by the same font. In CPDF_RenderStatus::DrawTextPathWithPattern, drawing was already done one character at a time, but precalculating CFX_FaceCache. Now, the face cache is precalculated for all of the fallback fonts. Pointers are used instead of unique_ptrs because apparently one the destruction of the fonts is already being explicitly handled, so replacing with unique_ptrs will require more code changes. For now, the font Arial is used as fallback. This should fix the issue of not seeing Latin characters displayed when bad fonts are used. However, this should be improved. Design proposal: go/pdfium_fallbackfonts BUG=pdfium:358 ========== to ========== Allow CPDF_Font to use fallback fonts Added a vector of pointers to CFX_Fonts in the class CPDF_Font, so that fallback fonts may be used. In CPDF_CharPosList::Load, the glyphs for each character are calculated. When m_Font does not support a character, a fallback font is selected and the character is rendered using that font. This meant adding an attribute to FXTEXT_CHARPOS so it knows which font renders it. Also, methods in fpdf_render_text.cpp now may need to call device drawing methods multiple times because these only support one font at a time. In CPDF_TextRenderer::DrawNormalText and in CPDF_TextRenderer::DrawTextPath, the device drawing method is called as few times as possible by grouping contiguous characters rendered by the same font. In CPDF_RenderStatus::DrawTextPathWithPattern, drawing was already done one character at a time, but precalculating CFX_FaceCache. Now, the face cache is precalculated for all of the fallback fonts. Pointers are used instead of unique_ptrs because apparently one the destruction of the fonts is already being explicitly handled, so replacing with unique_ptrs will require more code changes. For now, the font Arial is used as fallback. This should fix the issue of not seeing Latin characters displayed when bad fonts are used. However, this should be improved. Tested manually using the file in the bug, plus a font directory containing a font that supports Hangul but not Latin. This font is chosen as the substitute font, but Latin characters are now being rendered. Design proposal: go/pdfium_fallbackfonts BUG=pdfium:358 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/cpdf_font.cpp (right): https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:455: uint32_t CPDF_Font::FallbackFontFromCharcode(uint32_t charcode, This only ever returns 1, can the return value just be removed? https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:458: m_FontPlusFallbacks.push_back(new CFX_Font()); This font is going to end up leaking. The other font your adding is owned by someone else. I think we need to work out the ownership here so that either, a) this font is created elsewhere and added to the map, or b) the other font is owned by this vector. https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/include/cpdf_font.h (right): https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/include/cpdf_font.h:74: virtual uint32_t FallbackFontFromCharcode(uint32_t charcode, Are either of these overridden by subclasses? If not, lets remove the virtual for now. https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_rende... File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right): https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:565: pFont->m_FontPlusFallbacks.push_back(&pFont->m_Font); Actually, can you just remove this font from the list and have it strictly a fallback font list? You're always returning 1 as the index below, so this font never gets picked up as the fallback font, which is fine. The code to look for the font would then return >-1 for found fallback font. https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:578: if (charpos.m_GlyphIndex != (uint32_t)-1) { static_cast<uint32_t>(-1) https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:647: uint32_t pFontPosition = CharPosList.m_pCharPos[0].m_FallbackFontPosition; This bit would have to become a bit smarter to handle the font that isn't in the fallback list.
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/cpdf_font.cpp (right): https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:455: uint32_t CPDF_Font::FallbackFontFromCharcode(uint32_t charcode, On 2016/08/23 19:41:18, dsinclair wrote: > This only ever returns 1, can the return value just be removed? The intention is that the return value may be different once we use more than one fallback font. https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:458: m_FontPlusFallbacks.push_back(new CFX_Font()); On 2016/08/23 19:41:18, dsinclair wrote: > This font is going to end up leaking. The other font your adding is owned by > someone else. I think we need to work out the ownership here so that either, a) > this font is created elsewhere and added to the map, or b) the other font is > owned by this vector. Ok, a vector of unique pointers using only the fallbacks works. https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/include/cpdf_font.h (right): https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/include/cpdf_font.h:74: virtual uint32_t FallbackFontFromCharcode(uint32_t charcode, On 2016/08/23 19:41:18, dsinclair wrote: > Are either of these overridden by subclasses? If not, lets remove the virtual > for now. Done. https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_rende... File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right): https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:565: pFont->m_FontPlusFallbacks.push_back(&pFont->m_Font); On 2016/08/23 19:41:18, dsinclair wrote: > Actually, can you just remove this font from the list and have it strictly a > fallback font list? You're always returning 1 as the index below, so this font > never gets picked up as the fallback font, which is fine. The code to look for > the font would then return >-1 for found fallback font. Done. https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:578: if (charpos.m_GlyphIndex != (uint32_t)-1) { On 2016/08/23 19:41:18, dsinclair wrote: > static_cast<uint32_t>(-1) Done. https://codereview.chromium.org/2276653002/diff/20001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:647: uint32_t pFontPosition = CharPosList.m_pCharPos[0].m_FallbackFontPosition; On 2016/08/23 19:41:18, dsinclair wrote: > This bit would have to become a bit smarter to handle the font that isn't in the > fallback list. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right): https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:645: int32_t pFontPosition = CharPosList.m_pCharPos[0].m_FallbackFontPosition; can m_pCharPos length be 0? https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:649: if (pFontPosition != pCurFontPosition) { if (pFontPosition == pCurFontPosition) continue; https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:771: int32_t pFontPosition = CharPosList.m_pCharPos[0].m_FallbackFontPosition; Same comments as above.
https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/cpdf_font.cpp (right): https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:457: if (m_FontFallbacks.size() < 1) { m_FontFallbacks.empty() https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:458: m_FontFallbacks.push_back(std::unique_ptr<CFX_Font>(new CFX_Font())); We have our own WrapUnique() if you don't want to write out CFX_Font 2x. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:463: return 0; So always return 0? https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:468: bool* pVertGlyph) { not used? https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:470: FXFT_Get_Char_Index(m_FontFallbacks[fallbackFont]->GetFace(), charcode); Can |fallbackFont| go out of bound? https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/include/cpdf_font.h (right): https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/include/cpdf_font.h:74: uint32_t FallbackFontFromCharcode(uint32_t charcode, bool* pVertGlyph); Keep these separate from the virtual methods? https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right): https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:374: // TODO(npm) Font fallback for type 3 fonts? (Completely separate code!!) TODO(username): Comment. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:644: FX_BOOL bDraw = TRUE; Use bool? https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:646: uint32_t pStartIndex = 0; Not a pointer, ditto for |pFontPosition| above. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:650: auto font = pFontPosition == -1 Is |font| a pointer? Use auto* https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:827: std::vector<CFX_FaceCache*> pFaceCaches; Also not a pointer, thus the p-prefix is slightly misleading.
PTAL https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/cpdf_font.cpp (right): https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:457: if (m_FontFallbacks.size() < 1) { On 2016/08/24 04:43:10, Lei Zhang wrote: > m_FontFallbacks.empty() Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:458: m_FontFallbacks.push_back(std::unique_ptr<CFX_Font>(new CFX_Font())); On 2016/08/24 04:43:10, Lei Zhang wrote: > We have our own WrapUnique() if you don't want to write out CFX_Font 2x. Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:463: return 0; On 2016/08/24 04:43:10, Lei Zhang wrote: > So always return 0? For now, just having a single fallback font https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:468: bool* pVertGlyph) { On 2016/08/24 04:43:10, Lei Zhang wrote: > not used? The vert parameter is in GlyphFromCharcode so it might be needed later on. But for now I see no use for it. Should I delete then? https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:470: FXFT_Get_Char_Index(m_FontFallbacks[fallbackFont]->GetFace(), charcode); On 2016/08/24 04:43:10, Lei Zhang wrote: > Can |fallbackFont| go out of bound? This should not happen if the two methods are implemented properly, but I've added a check https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/include/cpdf_font.h (right): https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/include/cpdf_font.h:74: uint32_t FallbackFontFromCharcode(uint32_t charcode, bool* pVertGlyph); On 2016/08/24 04:43:11, Lei Zhang wrote: > Keep these separate from the virtual methods? Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right): https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:374: // TODO(npm) Font fallback for type 3 fonts? (Completely separate code!!) On 2016/08/24 04:43:11, Lei Zhang wrote: > TODO(username): Comment. Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:644: FX_BOOL bDraw = TRUE; On 2016/08/24 04:43:11, Lei Zhang wrote: > Use bool? DrawTextPath returns FX_BOOL https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:645: int32_t pFontPosition = CharPosList.m_pCharPos[0].m_FallbackFontPosition; On 2016/08/23 20:58:02, dsinclair wrote: > can m_pCharPos length be 0? Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:646: uint32_t pStartIndex = 0; On 2016/08/24 04:43:11, Lei Zhang wrote: > Not a pointer, ditto for |pFontPosition| above. Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:649: if (pFontPosition != pCurFontPosition) { On 2016/08/23 20:58:01, dsinclair wrote: > if (pFontPosition == pCurFontPosition) > continue; Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:650: auto font = pFontPosition == -1 On 2016/08/24 04:43:11, Lei Zhang wrote: > Is |font| a pointer? Use auto* Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:771: int32_t pFontPosition = CharPosList.m_pCharPos[0].m_FallbackFontPosition; On 2016/08/23 20:58:01, dsinclair wrote: > Same comments as above. Done. https://codereview.chromium.org/2276653002/diff/40001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:827: std::vector<CFX_FaceCache*> pFaceCaches; On 2016/08/24 04:43:11, Lei Zhang wrote: > Also not a pointer, thus the p-prefix is slightly misleading. Done.
https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/cpdf_font.cpp (right): https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:471: fallbackFont >= static_cast<int>(m_FontFallbacks.size())) { nit: pdfium::CollectionSize<int>(m_FontFallbacks) is preferred over the cast, since it checks for overflow. https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right): https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:644: FX_BOOL bDraw = TRUE; nit: this should be a bool if we are going to &= it, otherwise we're doing bitwise ops against an underlying int type. Generally prefer if (!somthing) bDraw = false; down at line 654 just in case it becomes an fx_bool again. https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:651: auto* font = fontPosition == -1 Is there a reason to separate default fonts from fallback fonts instead of always putting the default at index [0] and just walking the array until we're happy? https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:772: FX_BOOL bDraw = TRUE; nit: same here. https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:833: for (auto& font : pFont->m_FontFallbacks) { nit: can this be const auto& font ?
PTAL https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/cpdf_font.cpp (right): https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/cpdf_font.cpp:471: fallbackFont >= static_cast<int>(m_FontFallbacks.size())) { On 2016/08/24 16:23:49, Tom Sepez wrote: > nit: pdfium::CollectionSize<int>(m_FontFallbacks) is preferred over the cast, > since it checks for overflow. Done. https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right): https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:644: FX_BOOL bDraw = TRUE; On 2016/08/24 16:23:49, Tom Sepez wrote: > nit: this should be a bool if we are going to &= it, otherwise we're doing > bitwise ops against an underlying int type. Generally prefer > > if (!somthing) bDraw = false; > > down at line 654 just in case it becomes an fx_bool again. Done. https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:651: auto* font = fontPosition == -1 On 2016/08/24 16:23:49, Tom Sepez wrote: > Is there a reason to separate default fonts from fallback fonts instead of > always putting the default at index [0] and just walking the array until we're > happy? CPDF_Font already has m_Font for its main font. If it is added to this list (as I had in first patchset), there will be ownership issues with this particular font, the vector will have to contain pointers instead of unique_ptrs, and the fallbacks will probably leak. https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:772: FX_BOOL bDraw = TRUE; On 2016/08/24 16:23:49, Tom Sepez wrote: > nit: same here. Done. https://codereview.chromium.org/2276653002/diff/60001/core/fpdfapi/fpdf_rende... core/fpdfapi/fpdf_render/fpdf_render_text.cpp:833: for (auto& font : pFont->m_FontFallbacks) { On 2016/08/24 16:23:49, Tom Sepez wrote: > nit: can this be const auto& font ? Done.
Description was changed from ========== Allow CPDF_Font to use fallback fonts Added a vector of pointers to CFX_Fonts in the class CPDF_Font, so that fallback fonts may be used. In CPDF_CharPosList::Load, the glyphs for each character are calculated. When m_Font does not support a character, a fallback font is selected and the character is rendered using that font. This meant adding an attribute to FXTEXT_CHARPOS so it knows which font renders it. Also, methods in fpdf_render_text.cpp now may need to call device drawing methods multiple times because these only support one font at a time. In CPDF_TextRenderer::DrawNormalText and in CPDF_TextRenderer::DrawTextPath, the device drawing method is called as few times as possible by grouping contiguous characters rendered by the same font. In CPDF_RenderStatus::DrawTextPathWithPattern, drawing was already done one character at a time, but precalculating CFX_FaceCache. Now, the face cache is precalculated for all of the fallback fonts. Pointers are used instead of unique_ptrs because apparently one the destruction of the fonts is already being explicitly handled, so replacing with unique_ptrs will require more code changes. For now, the font Arial is used as fallback. This should fix the issue of not seeing Latin characters displayed when bad fonts are used. However, this should be improved. Tested manually using the file in the bug, plus a font directory containing a font that supports Hangul but not Latin. This font is chosen as the substitute font, but Latin characters are now being rendered. Design proposal: go/pdfium_fallbackfonts BUG=pdfium:358 ========== to ========== Allow CPDF_Font to use fallback fonts Added a vector of pointers to CFX_Fonts in the class CPDF_Font, so that fallback fonts may be used. In CPDF_CharPosList::Load, the glyphs for each character are calculated. When m_Font does not support a character, a fallback font is selected and the character is rendered using that font. This meant adding an attribute to FXTEXT_CHARPOS so it knows which font renders it. Also, methods in fpdf_render_text.cpp now may need to call device drawing methods multiple times because these only support one font at a time. In CPDF_TextRenderer::DrawNormalText and in CPDF_TextRenderer::DrawTextPath, the device drawing method is called as few times as possible by grouping contiguous characters rendered by the same font. In CPDF_RenderStatus::DrawTextPathWithPattern, drawing was already done one character at a time, but precalculating CFX_FaceCache. Now, the face cache is precalculated for all of the fallback fonts. The list of fallback fonts does not include tha main font. Otherwise the list would be of raw pointers to avoid double free problems. For now, the font Arial is used as fallback. This should fix the issue of not seeing Latin characters displayed when bad fonts are used. However, this should be improved. Tested manually using the file in the bug, plus a font directory containing a font that supports Hangul but not Latin. This font is chosen as the substitute font, but Latin characters are now being rendered. Design proposal: go/pdfium_fallbackfonts BUG=pdfium:358 ==========
lgtm
The CQ bit was checked by npm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow CPDF_Font to use fallback fonts Added a vector of pointers to CFX_Fonts in the class CPDF_Font, so that fallback fonts may be used. In CPDF_CharPosList::Load, the glyphs for each character are calculated. When m_Font does not support a character, a fallback font is selected and the character is rendered using that font. This meant adding an attribute to FXTEXT_CHARPOS so it knows which font renders it. Also, methods in fpdf_render_text.cpp now may need to call device drawing methods multiple times because these only support one font at a time. In CPDF_TextRenderer::DrawNormalText and in CPDF_TextRenderer::DrawTextPath, the device drawing method is called as few times as possible by grouping contiguous characters rendered by the same font. In CPDF_RenderStatus::DrawTextPathWithPattern, drawing was already done one character at a time, but precalculating CFX_FaceCache. Now, the face cache is precalculated for all of the fallback fonts. The list of fallback fonts does not include tha main font. Otherwise the list would be of raw pointers to avoid double free problems. For now, the font Arial is used as fallback. This should fix the issue of not seeing Latin characters displayed when bad fonts are used. However, this should be improved. Tested manually using the file in the bug, plus a font directory containing a font that supports Hangul but not Latin. This font is chosen as the substitute font, but Latin characters are now being rendered. Design proposal: go/pdfium_fallbackfonts BUG=pdfium:358 ========== to ========== Allow CPDF_Font to use fallback fonts Added a vector of pointers to CFX_Fonts in the class CPDF_Font, so that fallback fonts may be used. In CPDF_CharPosList::Load, the glyphs for each character are calculated. When m_Font does not support a character, a fallback font is selected and the character is rendered using that font. This meant adding an attribute to FXTEXT_CHARPOS so it knows which font renders it. Also, methods in fpdf_render_text.cpp now may need to call device drawing methods multiple times because these only support one font at a time. In CPDF_TextRenderer::DrawNormalText and in CPDF_TextRenderer::DrawTextPath, the device drawing method is called as few times as possible by grouping contiguous characters rendered by the same font. In CPDF_RenderStatus::DrawTextPathWithPattern, drawing was already done one character at a time, but precalculating CFX_FaceCache. Now, the face cache is precalculated for all of the fallback fonts. The list of fallback fonts does not include tha main font. Otherwise the list would be of raw pointers to avoid double free problems. For now, the font Arial is used as fallback. This should fix the issue of not seeing Latin characters displayed when bad fonts are used. However, this should be improved. Tested manually using the file in the bug, plus a font directory containing a font that supports Hangul but not Latin. This font is chosen as the substitute font, but Latin characters are now being rendered. Design proposal: go/pdfium_fallbackfonts BUG=pdfium:358 Committed: https://pdfium.googlesource.com/pdfium/+/b107193e9780b4a50e85d54c1ffbd2303263... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://pdfium.googlesource.com/pdfium/+/b107193e9780b4a50e85d54c1ffbd2303263... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
