Implement SkScalerContext_Windows::generateGlyphCount() and provide access vi...

Implement SkScalerContext_Windows::generateGlyphCount() and provide access via SkGlyphCache interface.
* This will be used by PDF font code.


Committed: http://code.google.com/p/skia/source/detail?r=888

[Chris Guillory, 2011-03-03 00:07:53]

Brett can you take a quick peek at src/ports/SkFontHost_win.cpp?

[reed1, 2011-03-03 12:45:59]

Does the PDF code have access to SkGlyphCache?

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
File src/ports/SkFontHost_ascender.cpp (right):

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
src/ports/SkFontHost_ascender.cpp:105: unsigned
SkScalerContext_Ascender::generateGlyphCount()
Since we don't have a real impl, lets not edit this file, and rely on the base
class impl (returning 0 I guess).

[Chris Guillory, 2011-03-03 17:55:17]

On 2011/03/03 12:45:59, reed1 wrote:
> Does the PDF code have access to SkGlyphCache?
PDF Type3 support will use SkGlyphCache for obtaining glyph metrics and paths.
http://codereview.appspot.com/4247043/

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
File src/ports/SkFontHost_ascender.cpp (right):

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
src/ports/SkFontHost_ascender.cpp:105: unsigned
SkScalerContext_Ascender::generateGlyphCount()
On 2011/03/03 12:45:59, reed1 wrote:
> Since we don't have a real impl, lets not edit this file, and rely on the base
> class impl (returning 0 I guess).
I think returning 0 might cause problems for SkScalerContent functions that
attempt to verify that that an input glyph ID is within a valid range.

[reed1, 2011-03-03 18:14:39]

Then perhaps we should make the base class not implement it?

On Thu, Mar 3, 2011 at 12:55 PM, <ctguil@chromium.org> wrote:

> On 2011/03/03 12:45:59, reed1 wrote:
>
>> Does the PDF code have access to SkGlyphCache?
>>
> PDF Type3 support will use SkGlyphCache for obtaining glyph metrics and
> paths.
> http://codereview.appspot.com/4247043/
>
>
>
>
>
http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
> File src/ports/SkFontHost_ascender.cpp (right):
>
>
>
http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
> src/ports/SkFontHost_ascender.cpp:105: unsigned
> SkScalerContext_Ascender::generateGlyphCount()
> On 2011/03/03 12:45:59, reed1 wrote:
>
>> Since we don't have a real impl, lets not edit this file, and rely on
>>
> the base
>
>> class impl (returning 0 I guess).
>>
> I think returning 0 might cause problems for SkScalerContent functions
> that attempt to verify that that an input glyph ID is within a valid
> range.
>
>
> http://codereview.appspot.com/4261042/
>

[Chris Guillory, 2011-03-03 18:26:29]

On 2011/03/03 18:14:39, reed1 wrote:
> Then perhaps we should make the base class not implement it?
The base class (SkScalerContent) actually doesn't implement it. The function is
pure virtual. I was looking at SkScalerContext_Empty which returns 0.
> 
> On Thu, Mar 3, 2011 at 12:55 PM, <mailto:ctguil@chromium.org> wrote:
> 
> > On 2011/03/03 12:45:59, reed1 wrote:
> >
> >> Does the PDF code have access to SkGlyphCache?
> >>
> > PDF Type3 support will use SkGlyphCache for obtaining glyph metrics and
> > paths.
> > http://codereview.appspot.com/4247043/
> >
> >
> >
> >
> >
>
http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
> > File src/ports/SkFontHost_ascender.cpp (right):
> >
> >
> >
>
http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
> > src/ports/SkFontHost_ascender.cpp:105: unsigned
> > SkScalerContext_Ascender::generateGlyphCount()
> > On 2011/03/03 12:45:59, reed1 wrote:
> >
> >> Since we don't have a real impl, lets not edit this file, and rely on
> >>
> > the base
> >
> >> class impl (returning 0 I guess).
> >>
> > I think returning 0 might cause problems for SkScalerContent functions
> > that attempt to verify that that an input glyph ID is within a valid
> > range.
> >
> >
> > http://codereview.appspot.com/4261042/
> >

[reed1, 2011-03-03 18:29:38]

Ah, thanks.

On Thu, Mar 3, 2011 at 1:26 PM, <ctguil@chromium.org> wrote:

> On 2011/03/03 18:14:39, reed1 wrote:
>
>> Then perhaps we should make the base class not implement it?
>>
> The base class (SkScalerContent) actually doesn't implement it. The
> function is pure virtual. I was looking at SkScalerContext_Empty which
> returns 0.
>
>
>  On Thu, Mar 3, 2011 at 12:55 PM, <mailto:ctguil@chromium.org> wrote:
>>
>
>  > On 2011/03/03 12:45:59, reed1 wrote:
>> >
>> >> Does the PDF code have access to SkGlyphCache?
>> >>
>> > PDF Type3 support will use SkGlyphCache for obtaining glyph metrics
>>
> and
>
>> > paths.
>> > http://codereview.appspot.com/4247043/
>> >
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
>
>> > File src/ports/SkFontHost_ascender.cpp (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_ascender...
>
>> > src/ports/SkFontHost_ascender.cpp:105: unsigned
>> > SkScalerContext_Ascender::generateGlyphCount()
>> > On 2011/03/03 12:45:59, reed1 wrote:
>> >
>> >> Since we don't have a real impl, lets not edit this file, and rely
>>
> on
>
>> >>
>> > the base
>> >
>> >> class impl (returning 0 I guess).
>> >>
>> > I think returning 0 might cause problems for SkScalerContent
>>
> functions
>
>> > that attempt to verify that that an input glyph ID is within a valid
>> > range.
>> >
>> >
>> > http://codereview.appspot.com/4261042/
>> >
>>
>
>
>
> http://codereview.appspot.com/4261042/
>

[brettw, 2011-03-03 18:43:05]

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

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:252: unsigned
SkScalerContext_Windows::generateGlyphCount() {
I think the way this is written, it will return the highest valid glyph index,
which is also the count because the indices are 1-based. Just making sure this
is what you want.

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:260: if (GetGlyphOutlineW(fDDC, mid, GGO_METRICS |
GGO_GLYPH_INDEX, &gm,
I think I figured out the "right" way to find the glyph count:
  GetFontData for the 'maxp' table (which I think should be 0x6d617870), and
it's the 4th and 5th byte in the resulting data:
http://www.microsoft.com/typography/otspec/maxp.htm

Note that this value is probably 0-based.

[reed1, 2011-03-03 18:45:10]

The glyphCount in the maxp table is the count, not the highest index.

i.e. if a font has glyphs 0....9, the glyphCount will be set to 10

On Thu, Mar 3, 2011 at 1:43 PM, <brettw@chromium.org> wrote:

>
>
> http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp
> File src/ports/SkFontHost_win.cpp (right):
>
>
>
http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
> src/ports/SkFontHost_win.cpp:252: unsigned
> SkScalerContext_Windows::generateGlyphCount() {
> I think the way this is written, it will return the highest valid glyph
> index, which is also the count because the indices are 1-based. Just
> making sure this is what you want.
>
>
>
http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
> src/ports/SkFontHost_win.cpp:260: if (GetGlyphOutlineW(fDDC, mid,
>
> GGO_METRICS | GGO_GLYPH_INDEX, &gm,
> I think I figured out the "right" way to find the glyph count:
>  GetFontData for the 'maxp' table (which I think should be 0x6d617870),
> and it's the 4th and 5th byte in the resulting data:
> http://www.microsoft.com/typography/otspec/maxp.htm
>
> Note that this value is probably 0-based.
>
>
> http://codereview.appspot.com/4261042/
>

[reed1, 2011-03-03 20:39:29]

I fine with this, modulo brett approving or amending the impl in fonthost_win

[Chris Guillory, 2011-03-03 21:09:14]

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

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:252: unsigned
SkScalerContext_Windows::generateGlyphCount() {
On 2011/03/03 18:43:05, brettw1 wrote:
> I think the way this is written, it will return the highest valid glyph index,
> which is also the count because the indices are 1-based. Just making sure this
> is what you want.
It's written to return the first invalid glyph index; one greater than the
highest valid glyph index.

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:260: if (GetGlyphOutlineW(fDDC, mid, GGO_METRICS |
GGO_GLYPH_INDEX, &gm,
On 2011/03/03 18:43:05, brettw1 wrote:
> I think I figured out the "right" way to find the glyph count:
>   GetFontData for the 'maxp' table (which I think should be 0x6d617870), and
> it's the 4th and 5th byte in the resulting data:
> http://www.microsoft.com/typography/otspec/maxp.htm
> 
> Note that this value is probably 0-based.
Just tried that but it is returning a much larger value than expected. Maybe
because "This table establishes the memory requirements for this font"?
Also font's like Type1 don't have this table so I think we'd need this search or
some other fallback. I can try to see how freetype is calculating this value on
linux.

[brettw, 2011-03-03 21:39:49]

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

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:252: unsigned
SkScalerContext_Windows::generateGlyphCount() {
Let's say 3 is the last valid glyph ID. Say min is 3 and max is 4.

mid will be 3 andGetGlyphOutline will return true. Then you'll set high to 3 and
the while loop will terminate since low is no longer < high. Then you'll return
low, which is 3, which at the beginning is the last valid value.

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:260: if (GetGlyphOutlineW(fDDC, mid, GGO_METRICS |
GGO_GLYPH_INDEX, &gm,
Maybe Mike can shed more light on this. There are a number of other values, like
the size of the hmetrics table, which we would possibly use instead.

[reed1, 2011-03-03 21:49:52]

Not sure why we want to look at other tables like hdmx. If its a truetype or
opentype font, it must have a maxp, afaik.

[Chris Guillory, 2011-03-03 22:16:31]

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

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:252: unsigned
SkScalerContext_Windows::generateGlyphCount() {
On 2011/03/03 21:39:49, brettw1 wrote:
> Let's say 3 is the last valid glyph ID. Say min is 3 and max is 4.
> 
> mid will be 3 andGetGlyphOutline will return true. Then you'll set high to 3
and
> the while loop will terminate since low is no longer < high. Then you'll
return
> low, which is 3, which at the beginning is the last valid value.
In this case GetGlyphOutline will not return GDI_ERROR so low will be set to
mid+1, 4.

[brettw, 2011-03-03 22:37:37]

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

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:252: unsigned
SkScalerContext_Windows::generateGlyphCount() {
You win :) And this value is the one that Skia expects? It's actually one
greater than the number of glyphs.

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:260: if (GetGlyphOutlineW(fDDC, mid, GGO_METRICS |
GGO_GLYPH_INDEX, &gm,
I guess it would be interesting to see if there is a more reliable way that
freetype uses.

[Chris Guillory, 2011-03-04 01:49:24]

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

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:252: unsigned
SkScalerContext_Windows::generateGlyphCount() {
On 2011/03/03 22:37:37, brettw wrote:
> You win :) And this value is the one that Skia expects? It's actually one
> greater than the number of glyphs.
Yes this is the value skia expects. Had to check with Steve on that.

http://codereview.appspot.com/4261042/diff/3001/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:260: if (GetGlyphOutlineW(fDDC, mid, GGO_METRICS |
GGO_GLYPH_INDEX, &gm,
On 2011/03/03 22:37:37, brettw wrote:
> I guess it would be interesting to see if there is a more reliable way that
> freetype uses.
Freetype converts this value from big endian. I've updated the change so we do
too.

[brettw, 2011-03-04 06:06:04]

LGTM (I only looked at the _win file)