[PDF] Handle invalid glyph IDs on drawText methods.

[PDF] Handle invalid glyph IDs on drawText methods.

Committed: https://code.google.com/p/skia/source/detail?r=7401

[Steve VanDeBogart, 2013-01-23 01:53:03]

Ok, this fixes both issues.  I don't like how it silently drops invalid draw
arguments, but that's the interface.

[edisonn, 2013-01-23 14:14:07]

Mike/Ben, Steve insists on fixing the callers, this is the proposed fix.

Steve, have you run it with the test in https://codereview.appspot.com/7127056/
?

[reed1, 2013-01-23 14:39:48]

These are just performance comments, that apply equally to the old and new code.

1. we need a cheaper way to get glyph-count, one that doesn't involve allocating
memory if possible. That is something I can work on, trying to add it (perhaps)
as a field in SkTypeface.

2. Often I bet the number of glyphs in a single draw-call is small (~100). Seems
like we could use SkAutoSTArray<128, uint16_t> to save on the call to sk_malloc
for those easy cases.

[Steve VanDeBogart, 2013-01-23 22:21:37]

On 2013/01/23 14:39:48, reed1 wrote:
> These are just performance comments, that apply equally to the old and new
code.
> 
> 1. we need a cheaper way to get glyph-count, one that doesn't involve
allocating
> memory if possible. That is something I can work on, trying to add it
(perhaps)
> as a field in SkTypeface.

This may be difficult since a single character may expand to multiple glyphs or
visa versa. 
 
> 2. Often I bet the number of glyphs in a single draw-call is small (~100).
Seems
> like we could use SkAutoSTArray<128, uint16_t> to save on the call to
sk_malloc
> for those easy cases.

I tries using SkAutoSTArray, but it was a very messy change. We don't know the
size at construction time, so I added a reset method.  But then we may drop some
glyphs if they aren't valid, so I started to add a trim() method.  But then
fCount could transition between the local storage and the malloced memory, so it
needs another internal flag to indicate which storage is used.  Let me know if
you think these changes to SkAutoSTArray are worth it and I'll do that.

[reed1, 2013-01-23 22:32:27]

On 2013/01/23 22:21:37, Steve VanDeBogart wrote:
> On 2013/01/23 14:39:48, reed1 wrote:
> > These are just performance comments, that apply equally to the old and new
> code.
> > 
> > 1. we need a cheaper way to get glyph-count, one that doesn't involve
> allocating
> > memory if possible. That is something I can work on, trying to add it
> (perhaps)
> > as a field in SkTypeface.
> 
> This may be difficult since a single character may expand to multiple glyphs
or
> visa versa. 

Bad wording on my part. I mean "numberOfGlyphsInTheTypeface".

>  
> > 2. Often I bet the number of glyphs in a single draw-call is small (~100).
> Seems
> > like we could use SkAutoSTArray<128, uint16_t> to save on the call to
> sk_malloc
> > for those easy cases.
> 
> I tries using SkAutoSTArray, but it was a very messy change. We don't know the
> size at construction time, so I added a reset method.  But then we may drop
some
> glyphs if they aren't valid, so I started to add a trim() method.  But then
> fCount could transition between the local storage and the malloced memory, so
it
> needs another internal flag to indicate which storage is used.  Let me know if
> you think these changes to SkAutoSTArray are worth it and I'll do that.

We use the following pattern in other parts of Skia. Might be simpler.

void* fill_buffer(void* storage, size_t sizeOfStorage, ...) {
    void* buff;
    if (sizeOfStorage is big_enough) {
        buff = storage;
    } else {
        buff = sk_malloc(...);
    }
    ...
    return buff;
}

The caller can compare the returned ptr w/ storage, to know if it should call
free on it.

Alternatively, SkAutoSTMalloc seems to already have a reset() method, if you
wanted to use that.

[Steve VanDeBogart, 2013-01-23 22:54:27]

On 2013/01/23 22:32:27, reed1 wrote:
> On 2013/01/23 22:21:37, Steve VanDeBogart wrote:
> > On 2013/01/23 14:39:48, reed1 wrote:
> > > These are just performance comments, that apply equally to the old and new
> > code.
> > > 
> > > 1. we need a cheaper way to get glyph-count, one that doesn't involve
> > allocating
> > > memory if possible. That is something I can work on, trying to add it
> > (perhaps)
> > > as a field in SkTypeface.
> > 
> > This may be difficult since a single character may expand to multiple glyphs
> or
> > visa versa. 
> 
> Bad wording on my part. I mean "numberOfGlyphsInTheTypeface".
> 
> >  
> > > 2. Often I bet the number of glyphs in a single draw-call is small (~100).
> > Seems
> > > like we could use SkAutoSTArray<128, uint16_t> to save on the call to
> > sk_malloc
> > > for those easy cases.
> > 
> > I tries using SkAutoSTArray, but it was a very messy change. We don't know
the
> > size at construction time, so I added a reset method.  But then we may drop
> some
> > glyphs if they aren't valid, so I started to add a trim() method.  But then
> > fCount could transition between the local storage and the malloced memory,
so
> it
> > needs another internal flag to indicate which storage is used.  Let me know
if
> > you think these changes to SkAutoSTArray are worth it and I'll do that.
> 
> We use the following pattern in other parts of Skia. Might be simpler.
> 
> void* fill_buffer(void* storage, size_t sizeOfStorage, ...) {
>     void* buff;
>     if (sizeOfStorage is big_enough) {
>         buff = storage;
>     } else {
>         buff = sk_malloc(...);
>     }
>     ...
>     return buff;
> }
> 
> The caller can compare the returned ptr w/ storage, to know if it should call
> free on it.
> 
> Alternatively, SkAutoSTMalloc seems to already have a reset() method, if you
> wanted to use that.

Hmm, that works better.  Done.

[reed1, 2013-01-24 12:51:24]

1.  Why do we always allocate/copy the input in the glyphID case? It seems to me
that 99% of the time, the inputs will be valid, and so the copy is unnecessary.
This function could return its buffer (either the original input buffer in this
99% case, or the memory in the STMalloc guy. It seems you could rip-through the
input, and *if* you detect something out of range, backup, make a copy and
correct the input.

2. Why do we also check for valid glyphIDs in the non-glyph case? Since skia is
returning the glyphIDs from textToGlyphs(), we shouldn't need to spend the time
in that loop.

[Steve VanDeBogart, 2013-01-24 22:25:12]

On 2013/01/24 12:51:24, reed1 wrote:
> 1.  Why do we always allocate/copy the input in the glyphID case? It seems to
me
> that 99% of the time, the inputs will be valid, and so the copy is
unnecessary.
> This function could return its buffer (either the original input buffer in
this
> 99% case, or the memory in the STMalloc guy. It seems you could rip-through
the
> input, and *if* you detect something out of range, backup, make a copy and
> correct the input.

I only did it this way to simplify the code. Most of the time, the length of the
text will be short... Done.

> 2. Why do we also check for valid glyphIDs in the non-glyph case? Since skia
is
> returning the glyphIDs from textToGlyphs(), we shouldn't need to spend the
time
> in that loop.

Again, for simplicity and to validate all the input.  If we trust that the fonts
are well formed, then there's no need to validate it.  Done.

[reed1, 2013-01-25 13:23:06]

Thanks for both optimizations.

It looks like the validation process just skips bad glyphs? In the case of
drawPosText (for example) we will now get out of sync between glyphs and
positions.

Could we instead just write 0 into the output buffer for bad glyphs? This will
keep things insync, and will actually behave the same as when we are given
out-of-range character codes: they are mapped to glyph 0.

[Steve VanDeBogart, 2013-01-25 18:10:39]

On 2013/01/25 13:23:06, reed1 wrote:
> Thanks for both optimizations.
> 
> It looks like the validation process just skips bad glyphs? In the case of
> drawPosText (for example) we will now get out of sync between glyphs and
> positions.
> 
> Could we instead just write 0 into the output buffer for bad glyphs? This will
> keep things insync, and will actually behave the same as when we are given
> out-of-range character codes: they are mapped to glyph 0.

Done.

Interesting... I initially just checked drawText and it seemed that the raster
case simply skipped bad glyph ids.  But as you say it leaves space for them in
drawPosText.  Glyph id 0 is reserved for "unknown glyph" so what you've
suggested makes sense and I've done it, but the result is still not consistent
with the raster case.  Some fonts draw a box for glyph id 0 and that's the
result I got in my test; both the drawText and drawPosText case drew a box for
the invalid glyph id.  I wonder if the raster case should do the same thing -
draw glyph 0 for invalid glyphs.

[reed1, 2013-01-25 18:57:47]

lgtm