Improve generation of glyph advance array.

Improve generation of glyph advance array.

[arthurhsu, 2011-08-10 03:34:23]

This is the change to reduce native API calling and doubles PDF generation speed
on Windows.

Chris please focus on FontHost that you've worked on.
Steve please help with the rest.

Thanks.

[reed1, 2011-08-10 13:55:20]

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h
File include/core/SkFontHost.h (right):

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h#newcod...
include/core/SkFontHost.h:196: */
Does this new API need to be on FontHost, or can it just be a method on the
SkAdvancedTypefaceMetrics class?

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h#newcod...
include/core/SkFontHost.h:200: const SkTDArray<uint32_t>* glyphIDs);
Why are we passing a TDArray instead of just a const uint32_t* + length? The
latter is more general and less restrictive on the caller.

Need more documentation on how to interpret the uint32_t. e.g. Where is glyph 0
stored? LSB or MSB?

[reed1, 2011-08-10 14:10:26]

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_mac_coretex...
File src/ports/SkFontHost_mac_coretext.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_mac_coretex...
src/ports/SkFontHost_mac_coretext.cpp:867: }
Why do we call SafeRelease here, instead of just release? We are not checking
for NULL above (e.g. in GetGlyphCount)

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:900: HFONT savefont = (HFONT)SelectObject(hdc,
font);
Do we need designFont?

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:910: } else {
Can we move all of the GDI object create/deletion (hdc, font, safefont) into
this else clause?

[Steve VanDeBogart, 2011-08-10 18:11:14]

Lets talk about the approach to this problem.

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h
File include/core/SkFontHost.h (right):

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h#newcod...
include/core/SkFontHost.h:197: static bool UpdateAdvancedTypefaceMetrics(
Do we even need a new method?  The amount of redundant work is pretty small if
we first call GetAdvancedTypefaceMetrics with no per-glyph info requested and
call it a second time to get the per-glyph info. Or a subsequent call could do
an update...

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
src/core/SkAdvancedTypefaceMetrics.cpp:95: }  // Do not call getAdvance if not
in glyph id.
If we don't want the advance for a glyph ID, it means we don't care what the PDF
render thinks the advance for that glyph ID.  Maybe we should set it to
SK_MinS16 or some other sentinal, and treat that as a don't care in the rest of
the calculation below.  i.e. don't break up a repeat range for a don't care id,
etc?

http://codereview.appspot.com/4830068/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/pdf/SkPDFFont.cpp#newcode769
src/pdf/SkPDFFont.cpp:769: SkFontHost::UpdateAdvancedTypefaceMetrics(
This isn't thread safe.  If there are two threads generating two subsets at the
same time... even if they don't clobber each other, one will get the wrong set
of advance data.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_FreeType.cpp
File src/ports/SkFontHost_FreeType.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_FreeType.cp...
src/ports/SkFontHost_FreeType.cpp:578: SkAutoMutexAcquire ac(gFTMutex);
You can't recursively aquire a mutex.

[Chris Guillory, 2011-08-10 19:35:08]

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:74: *	Since LOGFONT wants its textsize as an int,
and we support fractional sizes,
Please remove this tab.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:860: // UpdateAdvancedTypefaceMetrics is no good
because it does not worth
does -> is

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:893:
(LogFontTypeface*)SkTypefaceCache::FindByID(info->fFontID);
Use C++ cast.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:900: HFONT savefont = (HFONT)SelectObject(hdc,
font);
C++ cast.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:906: appendRange(&info->fGlyphWidths, 0);
Optional: You might want to create another function for these lines of code as
they are used in multiple places.

[arthurhsu, 2011-08-11 01:55:05]

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h
File include/core/SkFontHost.h (right):

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h#newcod...
include/core/SkFontHost.h:196: */
On 2011/08/10 13:55:21, reed1 wrote:
> Does this new API need to be on FontHost, or can it just be a method on the
> SkAdvancedTypefaceMetrics class?

No longer new API but alter the signature due to thread safety fixes.

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h#newcod...
include/core/SkFontHost.h:197: static bool UpdateAdvancedTypefaceMetrics(
On 2011/08/10 18:11:14, Steve VanDeBogart wrote:
> Do we even need a new method?  The amount of redundant work is pretty small if
> we first call GetAdvancedTypefaceMetrics with no per-glyph info requested and
> call it a second time to get the per-glyph info. Or a subsequent call could do
> an update...

Done.

http://codereview.appspot.com/4830068/diff/1/include/core/SkFontHost.h#newcod...
include/core/SkFontHost.h:200: const SkTDArray<uint32_t>* glyphIDs);
On 2011/08/10 13:55:21, reed1 wrote:
> Why are we passing a TDArray instead of just a const uint32_t* + length? The
> latter is more general and less restrictive on the caller.
> 
> Need more documentation on how to interpret the uint32_t. e.g. Where is glyph
0
> stored? LSB or MSB?

Done.

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
src/core/SkAdvancedTypefaceMetrics.cpp:95: }  // Do not call getAdvance if not
in glyph id.
On 2011/08/10 18:11:14, Steve VanDeBogart wrote:
> If we don't want the advance for a glyph ID, it means we don't care what the
PDF
> render thinks the advance for that glyph ID.  Maybe we should set it to
> SK_MinS16 or some other sentinal, and treat that as a don't care in the rest
of
> the calculation below.  i.e. don't break up a repeat range for a don't care
id,
> etc?

Yes, that's why 0 is chosen as default value because special optimization
against 0 is performed.  Also, 0 is don't care.

http://codereview.appspot.com/4830068/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/pdf/SkPDFFont.cpp#newcode769
src/pdf/SkPDFFont.cpp:769: SkFontHost::UpdateAdvancedTypefaceMetrics(
On 2011/08/10 18:11:14, Steve VanDeBogart wrote:
> This isn't thread safe.  If there are two threads generating two subsets at
the
> same time... even if they don't clobber each other, one will get the wrong set
> of advance data.

Done.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_FreeType.cpp
File src/ports/SkFontHost_FreeType.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_FreeType.cp...
src/ports/SkFontHost_FreeType.cpp:578: SkAutoMutexAcquire ac(gFTMutex);
On 2011/08/10 18:11:14, Steve VanDeBogart wrote:
> You can't recursively aquire a mutex.

Done.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_mac_coretex...
File src/ports/SkFontHost_mac_coretext.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_mac_coretex...
src/ports/SkFontHost_mac_coretext.cpp:867: }
On 2011/08/10 14:10:28, reed1 wrote:
> Why do we call SafeRelease here, instead of just release? We are not checking
> for NULL above (e.g. in GetGlyphCount)

Function no longer exists after thread-safety fix.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:74: *	Since LOGFONT wants its textsize as an int,
and we support fractional sizes,
On 2011/08/10 19:35:10, Chris Guillory wrote:
> Please remove this tab.

Done.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:860: // UpdateAdvancedTypefaceMetrics is no good
because it does not worth
On 2011/08/10 19:35:10, Chris Guillory wrote:
> does -> is

Done.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:893:
(LogFontTypeface*)SkTypefaceCache::FindByID(info->fFontID);
On 2011/08/10 19:35:10, Chris Guillory wrote:
> Use C++ cast.

Done.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:900: HFONT savefont = (HFONT)SelectObject(hdc,
font);
On 2011/08/10 14:10:28, reed1 wrote:
> Do we need designFont?

Ditto.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:900: HFONT savefont = (HFONT)SelectObject(hdc,
font);
On 2011/08/10 14:10:28, reed1 wrote:
> Do we need designFont?

Function no longer exists after thread-safety fix.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:906: appendRange(&info->fGlyphWidths, 0);
On 2011/08/10 19:35:10, Chris Guillory wrote:
> Optional: You might want to create another function for these lines of code as
> they are used in multiple places.

Ditto.

http://codereview.appspot.com/4830068/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:910: } else {
On 2011/08/10 14:10:28, reed1 wrote:
> Can we move all of the GDI object create/deletion (hdc, font, safefont) into
> this else clause?

Ditto.

[reed1, 2011-08-11 03:34:18]

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
File include/core/SkAdvancedTypefaceMetrics.h (left):

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
include/core/SkAdvancedTypefaceMetrics.h:138: int num_glyphs,
Lets document what these two parameters are:

e.g.
subsetGlyphIDs: bit array, MSB is glyphID=0 (or LSB?)
subsetGlphIDLength: bit count or uint32_t count?

nit:
the length parameter should be int (if its an element count) or size_t (if its a
byte count)

Before the glyph bit array was const. Is it still?

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
File include/core/SkAdvancedTypefaceMetrics.h (right):

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
include/core/SkAdvancedTypefaceMetrics.h:116: // Font ID used for updating this
structure.
SkFontID fFontID;

http://codereview.appspot.com/4830068/diff/9001/include/core/SkFontHost.h
File include/core/SkFontHost.h (right):

http://codereview.appspot.com/4830068/diff/9001/include/core/SkFontHost.h#new...
include/core/SkFontHost.h:192: SkAdvancedTypefaceMetrics::PerGlyphInfo
perGlyphInfo,
See first header for my request for documentation of these two fields

[Chris Guillory, 2011-08-11 18:04:48]

Win FontHost part LGTM.

http://codereview.appspot.com/4830068/diff/9001/src/core/SkAdvancedTypefaceMe...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/9001/src/core/SkAdvancedTypefaceMe...
src/core/SkAdvancedTypefaceMetrics.cpp:96: }  // Do not call getAdvance if not
in glyph id.
Nit: Can you move this comment above the if statement.

http://codereview.appspot.com/4830068/diff/9001/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

http://codereview.appspot.com/4830068/diff/9001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:748: uint32_t glyphIDsLength) {
Optional: glyphIDsLength -> glyphIDsCount

[arthurhsu, 2011-08-11 18:48:08]

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
File include/core/SkAdvancedTypefaceMetrics.h (left):

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
include/core/SkAdvancedTypefaceMetrics.h:138: int num_glyphs,
On 2011/08/11 03:34:19, reed1 wrote:
> Lets document what these two parameters are:
> 
> e.g.
> subsetGlyphIDs: bit array, MSB is glyphID=0 (or LSB?)
> subsetGlphIDLength: bit count or uint32_t count?
> 
> nit:
> the length parameter should be int (if its an element count) or size_t (if its
a
> byte count)
> 
> Before the glyph bit array was const. Is it still?

Each element (i.e. uint32_t) denotes a glyph id of the corresponding font. 
There's nothing related to LSB/MSB.

I've added comments and make the array const.

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
File include/core/SkAdvancedTypefaceMetrics.h (right):

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
include/core/SkAdvancedTypefaceMetrics.h:116: // Font ID used for updating this
structure.
On 2011/08/11 03:34:19, reed1 wrote:
> SkFontID fFontID;

Done.

http://codereview.appspot.com/4830068/diff/9001/include/core/SkFontHost.h
File include/core/SkFontHost.h (right):

http://codereview.appspot.com/4830068/diff/9001/include/core/SkFontHost.h#new...
include/core/SkFontHost.h:192: SkAdvancedTypefaceMetrics::PerGlyphInfo
perGlyphInfo,
On 2011/08/11 03:34:19, reed1 wrote:
> See first header for my request for documentation of these two fields

Done.

http://codereview.appspot.com/4830068/diff/9001/src/core/SkAdvancedTypefaceMe...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/9001/src/core/SkAdvancedTypefaceMe...
src/core/SkAdvancedTypefaceMetrics.cpp:96: }  // Do not call getAdvance if not
in glyph id.
On 2011/08/11 18:04:48, Chris Guillory wrote:
> Nit: Can you move this comment above the if statement.

Done.

[Steve VanDeBogart, 2011-08-11 20:22:33]

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
src/core/SkAdvancedTypefaceMetrics.cpp:95: }  // Do not call getAdvance if not
in glyph id.
On 2011/08/11 01:55:05, arthurhsu wrote:
> On 2011/08/10 18:11:14, Steve VanDeBogart wrote:
> > If we don't want the advance for a glyph ID, it means we don't care what the
> PDF
> > render thinks the advance for that glyph ID.  Maybe we should set it to
> > SK_MinS16 or some other sentinal, and treat that as a don't care in the rest
> of
> > the calculation below.  i.e. don't break up a repeat range for a don't care
> id,
> > etc?
> 
> Yes, that's why 0 is chosen as default value because special optimization
> against 0 is performed.  Also, 0 is don't care.

0 is not a don't care, it's what we set the default advance to.  glyphs we skip
are truely don't care - they need not be defined or can have any value we
choose.  For example, if we're in a range of repeats, then setting it to
lastAdvance would make more sense.

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
File include/core/SkAdvancedTypefaceMetrics.h (right):

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
include/core/SkAdvancedTypefaceMetrics.h:142: uint32_t* subsetGlyphIDs,
uint16_t* ?

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
File include/core/SkAdvancedTypefaceMetrics.h (right):

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:20: typedef uint32_t SkFontID;
This is in SkTypeface.h  Typeface is a much more common include, best to leave
the definition there.

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:118: // Font ID used for updating this
structure.
Is this still needed?

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:153: const uint32_t* subsetGlyphIDs,
const uint16_t* ?

http://codereview.appspot.com/4830068/diff/14002/include/core/SkFontHost.h
File include/core/SkFontHost.h (right):

http://codereview.appspot.com/4830068/diff/14002/include/core/SkFontHost.h#ne...
include/core/SkFontHost.h:193: const uint32_t* glyphIDs,
const uint16_t* ?

http://codereview.appspot.com/4830068/diff/14002/src/core/SkAdvancedTypefaceM...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/14002/src/core/SkAdvancedTypefaceM...
src/core/SkAdvancedTypefaceMetrics.cpp:89: Data advance = 0;  // Give a default
value.
Redundant comment.

http://codereview.appspot.com/4830068/diff/14002/src/core/SkAdvancedTypefaceM...
src/core/SkAdvancedTypefaceMetrics.cpp:91: // If specified subset, do not call
getAdvance if glyph id is not in
Complete sentences please.

http://codereview.appspot.com/4830068/diff/14002/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

http://codereview.appspot.com/4830068/diff/14002/src/core/SkTypeface.cpp#newc...
src/core/SkTypeface.cpp:95: SkAdvancedTypefaceMetrics*
SkTypeface::getAdvancedTypefaceMetrics(
Do we not want to expose the new API all the way up the stack?  We have to do it
in a non-breaking way since Webkit uses this method, but a temporary
implementation that does what you've done here would work... as would making the
subset default arguments.

http://codereview.appspot.com/4830068/diff/14002/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4830068/diff/14002/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:990: if (fontInfo()->fType ==
SkAdvancedTypefaceMetrics::kTrueType_Font) {
Why not defer this until inside the CIDFont, then you can export the glyph set
to an array just once?

[arthurhsu, 2011-08-11 23:50:24]

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
src/core/SkAdvancedTypefaceMetrics.cpp:95: }  // Do not call getAdvance if not
in glyph id.
On 2011/08/11 20:22:34, Steve VanDeBogart wrote:
> On 2011/08/11 01:55:05, arthurhsu wrote:
> > On 2011/08/10 18:11:14, Steve VanDeBogart wrote:
> > > If we don't want the advance for a glyph ID, it means we don't care what
the
> > PDF
> > > render thinks the advance for that glyph ID.  Maybe we should set it to
> > > SK_MinS16 or some other sentinal, and treat that as a don't care in the
rest
> > of
> > > the calculation below.  i.e. don't break up a repeat range for a don't
care
> > id,
> > > etc?
> > 
> > Yes, that's why 0 is chosen as default value because special optimization
> > against 0 is performed.  Also, 0 is don't care.
> 
> 0 is not a don't care, it's what we set the default advance to.  glyphs we
skip
> are truely don't care - they need not be defined or can have any value we
> choose.  For example, if we're in a range of repeats, then setting it to
> lastAdvance would make more sense.

Done.

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
File include/core/SkAdvancedTypefaceMetrics.h (right):

http://codereview.appspot.com/4830068/diff/9001/include/core/SkAdvancedTypefa...
include/core/SkAdvancedTypefaceMetrics.h:142: uint32_t* subsetGlyphIDs,
On 2011/08/11 20:22:34, Steve VanDeBogart wrote:
> uint16_t* ?

BitSet exports to uint32_t so we use uint32_t here.

http://codereview.appspot.com/4830068/diff/9001/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

http://codereview.appspot.com/4830068/diff/9001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:748: uint32_t glyphIDsLength) {
On 2011/08/11 18:04:48, Chris Guillory wrote:
> Optional: glyphIDsLength -> glyphIDsCount

Done.

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
File include/core/SkAdvancedTypefaceMetrics.h (right):

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:20: typedef uint32_t SkFontID;
On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> This is in SkTypeface.h  Typeface is a much more common include, best to leave
> the definition there.

Due to compilation issue, moved to SkTypes.h.

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:118: // Font ID used for updating this
structure.
On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> Is this still needed?

Used in delay population.

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:153: const uint32_t* subsetGlyphIDs,
On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> const uint16_t* ?

Bitset exported to uint32_t so we use as is.

http://codereview.appspot.com/4830068/diff/14002/include/core/SkFontHost.h
File include/core/SkFontHost.h (right):

http://codereview.appspot.com/4830068/diff/14002/include/core/SkFontHost.h#ne...
include/core/SkFontHost.h:193: const uint32_t* glyphIDs,
On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> const uint16_t* ?

same, bitset exported to uint32_t.

http://codereview.appspot.com/4830068/diff/14002/src/core/SkAdvancedTypefaceM...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/14002/src/core/SkAdvancedTypefaceM...
src/core/SkAdvancedTypefaceMetrics.cpp:89: Data advance = 0;  // Give a default
value.
On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> Redundant comment.

Done.

http://codereview.appspot.com/4830068/diff/14002/src/core/SkAdvancedTypefaceM...
src/core/SkAdvancedTypefaceMetrics.cpp:91: // If specified subset, do not call
getAdvance if glyph id is not in
On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> Complete sentences please.

Done.

http://codereview.appspot.com/4830068/diff/14002/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

http://codereview.appspot.com/4830068/diff/14002/src/core/SkTypeface.cpp#newc...
src/core/SkTypeface.cpp:95: SkAdvancedTypefaceMetrics*
SkTypeface::getAdvancedTypefaceMetrics(
On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> Do we not want to expose the new API all the way up the stack?  We have to do
it
> in a non-breaking way since Webkit uses this method, but a temporary
> implementation that does what you've done here would work... as would making
the
> subset default arguments.

Done.

http://codereview.appspot.com/4830068/diff/14002/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4830068/diff/14002/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:990: if (fontInfo()->fType ==
SkAdvancedTypefaceMetrics::kTrueType_Font) {
On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> Why not defer this until inside the CIDFont, then you can export the glyph set
> to an array just once?

Font info is a private member of parent class and was not expected to be set.

[reed1, 2011-08-12 12:46:13]

regarding MSB/LSB, got it, each element is a glyph ID.

Why are these 32bit instead of 16bit?

[arthurhsu, 2011-08-12 17:32:57]

On 2011/08/12 12:46:13, reed1 wrote:
> regarding MSB/LSB, got it, each element is a glyph ID.
> 
> Why are these 32bit instead of 16bit?

SkBitSet exports 32bit integers so that it can pass to font subsetter without
conversion.  We just take what SkBitSet exports.

[Steve VanDeBogart, 2011-08-12 19:01:59]

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
src/core/SkAdvancedTypefaceMetrics.cpp:95: }  // Do not call getAdvance if not
in glyph id.
On 2011/08/11 23:50:24, arthurhsu wrote:
> On 2011/08/11 20:22:34, Steve VanDeBogart wrote:
> > On 2011/08/11 01:55:05, arthurhsu wrote:
> > > On 2011/08/10 18:11:14, Steve VanDeBogart wrote:
> > > > If we don't want the advance for a glyph ID, it means we don't care what
> the
> > > PDF
> > > > render thinks the advance for that glyph ID.  Maybe we should set it to
> > > > SK_MinS16 or some other sentinal, and treat that as a don't care in the
> rest
> > > of
> > > > the calculation below.  i.e. don't break up a repeat range for a don't
> care
> > > id,
> > > > etc?
> > > 
> > > Yes, that's why 0 is chosen as default value because special optimization
> > > against 0 is performed.  Also, 0 is don't care.
> > 
> > 0 is not a don't care, it's what we set the default advance to.  glyphs we
> skip
> > are truely don't care - they need not be defined or can have any value we
> > choose.  For example, if we're in a range of repeats, then setting it to
> > lastAdvance would make more sense.
> 
> Done.

No, that was just an example... another example is if you're in the middle of a
range, then a don't care is probably best encoded as 0.  If your not in either a
run or a range, then we shouldn't output anything.

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
File include/core/SkAdvancedTypefaceMetrics.h (right):

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:118: // Font ID used for updating this
structure.
On 2011/08/11 23:50:25, arthurhsu wrote:
> On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> > Is this still needed?
> 
> Used in delay population.

Can't you use typeface()->uniqueID() instead?

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:153: const uint32_t* subsetGlyphIDs,
On 2011/08/11 23:50:25, arthurhsu wrote:
> On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> > const uint16_t* ?
> 
> Bitset exported to uint32_t so we use as is.

Do you think it would make sense to templatize this function so that you could
get arrays of uint8_t's, uint16_t's, or uint32_t's ?  Even if you do, probably
left to a different CL.

http://codereview.appspot.com/4830068/diff/14002/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4830068/diff/14002/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:990: if (fontInfo()->fType ==
SkAdvancedTypefaceMetrics::kTrueType_Font) {
On 2011/08/11 23:50:25, arthurhsu wrote:
> On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> > Why not defer this until inside the CIDFont, then you can export the glyph
set
> > to an array just once?
> 
> Font info is a private member of parent class and was not expected to be set.

You could change that if you think this suggestion makes sense.

[arthurhsu, 2011-08-16 19:06:26]

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
File src/core/SkAdvancedTypefaceMetrics.cpp (right):

http://codereview.appspot.com/4830068/diff/1/src/core/SkAdvancedTypefaceMetri...
src/core/SkAdvancedTypefaceMetrics.cpp:95: }  // Do not call getAdvance if not
in glyph id.
On 2011/08/12 19:01:59, Steve VanDeBogart wrote:
> On 2011/08/11 23:50:24, arthurhsu wrote:
> > On 2011/08/11 20:22:34, Steve VanDeBogart wrote:
> > > On 2011/08/11 01:55:05, arthurhsu wrote:
> > > > On 2011/08/10 18:11:14, Steve VanDeBogart wrote:
> > > > > If we don't want the advance for a glyph ID, it means we don't care
what
> > the
> > > > PDF
> > > > > render thinks the advance for that glyph ID.  Maybe we should set it
to
> > > > > SK_MinS16 or some other sentinal, and treat that as a don't care in
the
> > rest
> > > > of
> > > > > the calculation below.  i.e. don't break up a repeat range for a don't
> > care
> > > > id,
> > > > > etc?
> > > > 
> > > > Yes, that's why 0 is chosen as default value because special
optimization
> > > > against 0 is performed.  Also, 0 is don't care.
> > > 
> > > 0 is not a don't care, it's what we set the default advance to.  glyphs we
> > skip
> > > are truely don't care - they need not be defined or can have any value we
> > > choose.  For example, if we're in a range of repeats, then setting it to
> > > lastAdvance would make more sense.
> > 
> > Done.
> 
> No, that was just an example... another example is if you're in the middle of
a
> range, then a don't care is probably best encoded as 0.  If your not in either
a
> run or a range, then we shouldn't output anything.

As discussed offline, this is best done in another CL since current CL is not
platform agnostic.

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
File include/core/SkAdvancedTypefaceMetrics.h (right):

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:118: // Font ID used for updating this
structure.
On 2011/08/12 19:01:59, Steve VanDeBogart wrote:
> On 2011/08/11 23:50:25, arthurhsu wrote:
> > On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> > > Is this still needed?
> > 
> > Used in delay population.
> 
> Can't you use typeface()->uniqueID() instead?

Done.

http://codereview.appspot.com/4830068/diff/14002/include/core/SkAdvancedTypef...
include/core/SkAdvancedTypefaceMetrics.h:153: const uint32_t* subsetGlyphIDs,
On 2011/08/12 19:01:59, Steve VanDeBogart wrote:
> On 2011/08/11 23:50:25, arthurhsu wrote:
> > On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> > > const uint16_t* ?
> > 
> > Bitset exported to uint32_t so we use as is.
> 
> Do you think it would make sense to templatize this function so that you could
> get arrays of uint8_t's, uint16_t's, or uint32_t's ?  Even if you do, probably
> left to a different CL.

Another CL it shall be.

http://codereview.appspot.com/4830068/diff/14002/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4830068/diff/14002/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:990: if (fontInfo()->fType ==
SkAdvancedTypefaceMetrics::kTrueType_Font) {
On 2011/08/12 19:01:59, Steve VanDeBogart wrote:
> On 2011/08/11 23:50:25, arthurhsu wrote:
> > On 2011/08/11 20:22:35, Steve VanDeBogart wrote:
> > > Why not defer this until inside the CIDFont, then you can export the glyph
> set
> > > to an array just once?
> > 
> > Font info is a private member of parent class and was not expected to be
set.
> 
> You could change that if you think this suggestion makes sense.

Done.

[Steve VanDeBogart, 2011-08-16 20:42:51]

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:755: if (fontMetrics->fType !=
SkAdvancedTypefaceMetrics::kTrueType_Font) {
What happens if we don't have a subsetter, i.e. compile without sfntly ?

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:1076: SkASSERT(subset);
Can't we pass a null subset in order to get the entire thing?

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFontImpl.h
File src/pdf/SkPDFFontImpl.h (right):

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFontImpl.h#newc...
src/pdf/SkPDFFontImpl.h:50: const SkTDArray<uint32_t>* subset);
why const * instead of const & ?

[arthurhsu, 2011-08-16 21:42:26]

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:755: if (fontMetrics->fType !=
SkAdvancedTypefaceMetrics::kTrueType_Font) {
On 2011/08/16 20:42:52, Steve VanDeBogart wrote:
> What happens if we don't have a subsetter, i.e. compile without sfntly ?

Done.

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:1076: SkASSERT(subset);
On 2011/08/16 20:42:52, Steve VanDeBogart wrote:
> Can't we pass a null subset in order to get the entire thing?

Done.

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFontImpl.h
File src/pdf/SkPDFFontImpl.h (right):

http://codereview.appspot.com/4830068/diff/25001/src/pdf/SkPDFFontImpl.h#newc...
src/pdf/SkPDFFontImpl.h:50: const SkTDArray<uint32_t>* subset);
On 2011/08/16 20:42:52, Steve VanDeBogart wrote:
> why const * instead of const & ?

Subset can be NULL to represent all glyphs.

[Steve VanDeBogart, 2011-08-16 22:14:47]

LGTM, but doesn't apply cleanly to trunk.  Rebase and I'll commit.

[arthurhsu, 2011-08-16 22:32:15]

On 2011/08/16 22:14:47, Steve VanDeBogart wrote:
> LGTM, but doesn't apply cleanly to trunk.  Rebase and I'll commit.

Done.

[Steve VanDeBogart, 2011-08-16 22:46:56]

Committed as r2126