Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
472333003:
Directly compute glyphToUnicode array in DirectWrite. (Closed)
|
Created:
6 years, 4 months ago by bungeman-skia Modified:
6 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionDirectly compute glyphToUnicode array in DirectWrite.
This new code is still rather bad, but has fewer steps.
The motivation for looking at this is that on Windows 8 the previous code
was occasionally asserting in such a way as to imply that GetGlyphIndices
might return different data the second time through the font. This avoids
that particular issue by only going through the font once.
Committed: https://skia.googlesource.com/skia/+/df2ec358be78820550f334bc7224ae18e3d25390
Patch Set 1 #Patch Set 2 : Remove tabs. #
Total comments: 6
Patch Set 3 : Do less work. #
Total comments: 4
Patch Set 4 : Put boots on the fenceposts. #
Total comments: 2
Messages
Total messages: 11 (0 generated)
I'm not sure we have any good tests to make sure a change like this doesn't break anything, but I produced some PDFs with this and they look to have the same copy behavior as they did previously. https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... File src/ports/SkTypeface_win_dw.cpp (left): https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:283: // of calling GetFontUnicodeRange(). This comment was imported from the GDI implementation, and makes no sense here. Also, these's no way to implement this TODO anyway. https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:9: #undef GetGlyphIndices This pattern is being repeated from SkScalerContext_win.cpp. The issue is that GDI #defines GetGlyphIndices to GetGlyphIndicesA or GetGlyphIndicesW, but IDWriteFontFace has a method called GetGlyphIndices. Since we're not going to be using GDI in this file, this makes things less confusing.
https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:9: #undef GetGlyphIndices On 2014/08/18 19:25:30, bungeman1 wrote: > This pattern is being repeated from SkScalerContext_win.cpp. The issue is that > GDI #defines GetGlyphIndices to GetGlyphIndicesA or GetGlyphIndicesW, but > IDWriteFontFace has a method called GetGlyphIndices. Since we're not going to be > using GDI in this file, this makes things less confusing. Can't hurt to copy that explanation into a comment here. https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:291: UINT16 glyphToUnicodeCount = glyphToUnicode->count(); Build into an SkAutoMalloc, reallocing 2x as needed, then copy into the output all at once?
https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:9: #undef GetGlyphIndices On 2014/08/18 21:21:50, mtklein wrote: > On 2014/08/18 19:25:30, bungeman1 wrote: > > This pattern is being repeated from SkScalerContext_win.cpp. The issue is that > > GDI #defines GetGlyphIndices to GetGlyphIndicesA or GetGlyphIndicesW, but > > IDWriteFontFace has a method called GetGlyphIndices. Since we're not going to > be > > using GDI in this file, this makes things less confusing. > > Can't hurt to copy that explanation into a comment here. Done. https://codereview.chromium.org/472333003/diff/20001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:291: UINT16 glyphToUnicodeCount = glyphToUnicode->count(); On 2014/08/18 21:21:50, mtklein wrote: > Build into an SkAutoMalloc, reallocing 2x as needed, then copy into the output > all at once? Actually, this can only have a size up to glyphCount, and we expect that most glyphs will be reachable, so we can just allocate this at a size of glyphCount, never resize, and shrink at the end.
https://codereview.chromium.org/472333003/diff/40001/src/ports/SkTypeface_win... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/472333003/diff/40001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:307: glyphToUnicode->swap(SkTDArray<SkUnichar>(glyphToUni, maxGlyph)); It's the same to do glyphToUnicode->rewind(); glyphToUnicode->append(maxGlyph, glyphToUni) right? https://codereview.chromium.org/472333003/diff/40001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:307: glyphToUnicode->swap(SkTDArray<SkUnichar>(glyphToUni, maxGlyph)); Don't we need to copy [0, maxGlyph] ? It looks like we're doing [0, maxGlyph).
https://codereview.chromium.org/472333003/diff/40001/src/ports/SkTypeface_win... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/472333003/diff/40001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:307: glyphToUnicode->swap(SkTDArray<SkUnichar>(glyphToUni, maxGlyph)); On 2014/08/20 18:15:01, mtklein wrote: > It's the same to do > > glyphToUnicode->rewind(); > glyphToUnicode->append(maxGlyph, glyphToUni) > > right? Nope, that would end up using 1/4x more space. https://codereview.chromium.org/472333003/diff/40001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:307: glyphToUnicode->swap(SkTDArray<SkUnichar>(glyphToUni, maxGlyph)); On 2014/08/20 18:15:01, mtklein wrote: > Don't we need to copy [0, maxGlyph] ? It looks like we're doing [0, maxGlyph). Done.
lgtm
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bungeman@google.com/472333003/60001
Message was sent while issue was closed.
Committed patchset #4 (60001) as df2ec358be78820550f334bc7224ae18e3d25390
Message was sent while issue was closed.
(I don't have a windows box at hand, else I'd send you a patch instead) https://codereview.chromium.org/472333003/diff/60001/src/ports/SkTypeface_win... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/472333003/diff/60001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:307: glyphToUnicode->swap(SkTDArray<SkUnichar>(glyphToUni, maxGlyph + 1)); Our clang/win fyi bot doesn't like this: ..\..\third_party\skia\src\ports\SkTypeface_win_dw.cpp(307,26) : error(clang): non-const lvalue reference to type 'SkTDArray<[...]>' cannot bind to a temporary of type 'SkTDArray<[...]>' glyphToUnicode->swap(SkTDArray<SkUnichar>(glyphToUni, maxGlyph + 1)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ..\..\third_party\skia\include\core\SkTDArray.h(76,29) : note(clang): passing argument to parameter 'other' here void swap(SkTDArray<T>& other) { ^ 1 error generated. I think the usual way to empty vectors is to write SkTDArray<SkUnichar>(glyphToUni, maxGlyph + 1).swap(*glyphToUnicode); instead – could that work here?
Message was sent while issue was closed.
See also Issue 487533004 and http://skbug.com/2679 . https://codereview.chromium.org/472333003/diff/60001/src/ports/SkTypeface_win... File src/ports/SkTypeface_win_dw.cpp (right): https://codereview.chromium.org/472333003/diff/60001/src/ports/SkTypeface_win... src/ports/SkTypeface_win_dw.cpp:307: glyphToUnicode->swap(SkTDArray<SkUnichar>(glyphToUni, maxGlyph + 1)); On 2014/08/25 03:18:59, Nico (afk vacation til Aug 24) wrote: > Our clang/win fyi bot doesn't like this: > > ..\..\third_party\skia\src\ports\SkTypeface_win_dw.cpp(307,26) : error(clang): > non-const lvalue reference to type 'SkTDArray<[...]>' cannot bind to a temporary > of type 'SkTDArray<[...]>' > glyphToUnicode->swap(SkTDArray<SkUnichar>(glyphToUni, maxGlyph + 1)); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ..\..\third_party\skia\include\core\SkTDArray.h(76,29) : note(clang): passing > argument to parameter 'other' here > void swap(SkTDArray<T>& other) { > ^ > 1 error generated. > > I think the usual way to empty vectors is to write > > SkTDArray<SkUnichar>(glyphToUni, maxGlyph + 1).swap(*glyphToUnicode); > > instead – could that work here? Hmmmm, yes, C++ specifically forbids all of this temporaries binding to non-const references silliness, but vc++ doesn't seem to even give a warning about it. I shall take your suggestion to to flip it around and see if that makes clang happier. |
