When we read from a RemoteFontStream (might happen maybe with other streams), stream's data might b…

When we read from a RemoteFontStream (might happen maybe with other streams), stream's data might be partially consumed. If so, when we try to read the whole content of the font, we fail, because we read only part of the content.
Committed: https://code.google.com/p/skia/source/detail?r=8291

[edisonn, 2013-03-21 00:47:10]

Please prioritization this review, it is a P1 M27 blocker. Thanks!

[vandebo (ex-Chrome), 2013-03-21 00:55:11]

https://codereview.chromium.org/12926004/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/12926004/diff/1/src/pdf/SkPDFFont.cpp#newcode551
src/pdf/SkPDFFont.cpp:551: SkAutoTUnref<SkStream>
fontData(typeface->openStream(&ttcIndex));
Shouldn't typeface->openStream() return a stream at the beginning?

[bungeman-skia, 2013-03-21 03:25:12]

Do we need to clarify and review use of SkStream? It's SkRefCnt, but it isn't
really share-able, since it has mutable  state (the data may be immutable, but
the seek is). Do we need something like 'lock/unlock stream' to return a thread
local copy of the state?

https://codereview.chromium.org/12926004/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/12926004/diff/1/src/pdf/SkPDFFont.cpp#newcode551
src/pdf/SkPDFFont.cpp:551: SkAutoTUnref<SkStream>
fontData(typeface->openStream(&ttcIndex));
On 2013/03/21 00:55:11, vandebo wrote:
> Shouldn't typeface->openStream() return a stream at the beginning?

I have to agree, this is an issue with reed's changes to
SkFontHost_fontconfig.cpp::open_stream. We shouldn't try to band-aid this here
and leave this to still happen elsewhere.

It looks like on Windows and Mac we always create a new stream on OpenStream.
With the fontconfig font host, it looks like we're often just ref-ing and
returning. This seems iffy, the old code made a copy, maybe this is why? It
seems what we want there is a shallow stream copy.

[edisonn, 2013-03-21 13:33:08]

https://codereview.chromium.org/12926004/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/12926004/diff/1/src/pdf/SkPDFFont.cpp#newcode551
src/pdf/SkPDFFont.cpp:551: SkAutoTUnref<SkStream>
fontData(typeface->openStream(&ttcIndex));
done; added issue 1176 for the thread safety. right now we will fix simply by
rewinding in open_stream

On 2013/03/21 03:25:12, bungeman1 wrote:
> On 2013/03/21 00:55:11, vandebo wrote:
> > Shouldn't typeface->openStream() return a stream at the beginning?
> 
> I have to agree, this is an issue with reed's changes to
> SkFontHost_fontconfig.cpp::open_stream. We shouldn't try to band-aid this here
> and leave this to still happen elsewhere.
> 
> It looks like on Windows and Mac we always create a new stream on OpenStream.
> With the fontconfig font host, it looks like we're often just ref-ing and
> returning. This seems iffy, the old code made a copy, maybe this is why? It
> seems what we want there is a shallow stream copy.

https://codereview.chromium.org/12926004/diff/1/src/pdf/SkPDFFont.cpp#newcode552
src/pdf/SkPDFFont.cpp:552: fontData->rewind();
Steve's comments:
1) openStream should give already a stream at the beginning, I agree
2) multithreading ... multiple readers would interfere reading the data! I
suggested a lock, but Steve thinks that we need independent seek pointers, but
this might require a new interface for all SkStream classes

[reed1, 2013-03-21 13:35:25]

lgtm

I vote for the shallow-copy API on SkStream. That said, if this is an important
fix, lets land this work-around now, and then revisit it when we have this new
shallow api.

[edisonn, 2013-03-21 13:54:41]

Committed patchset #2 manually as r8291 (presubmit successful).