Fixing invalid text clipping on SkPicture playback

Fixing invalid text clipping on SkPicture playback

The bug was caused by an invalid assumption that a flattend object's
index is related to its array index in SkFlatDictionary::fData.  
The data in SkFlatDictionary is sorted by flattened data content,
not by index number. Problem was solved by passing down the SkFlatData*
through addPaint, rather than the index value. The bug was causing
SkPictureRecord::addFontMetricsTopBottom to use cached font metrics
from the wrong SkPaint instance.

BUG=https://code.google.com/p/chromium/issues/detail?id=170964
Committed: https://code.google.com/p/skia/source/detail?r=7312

[junov, 2013-01-21 20:25:33]

PTAL

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (left):

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureFlat.h#oldcod...
src/core/SkPictureFlat.h:658: SkFlatData* writableFlatData(int index) {
Removing this method.  It was not doing the right thing.
This code was falsely assuming that (*this)[index]->fIndex - 1 == index

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureFlat.h#newcod...
src/core/SkPictureFlat.h:361: mutable SkScalar fTopBot[2];
Mutable so we don't have to remove const all over the place or  use const_cast. 
Since this member is used for caching the result of a computation, I think it is
fine to use mutable here.

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (left):

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureRecord.cpp#ol...
src/core/SkPictureRecord.cpp:953: 
This diff is just housekeeping. There was a pre-submit warning  that file ended
with two empty lines instead of one.

[junov, 2013-01-21 20:26:30]

I'll add a unit test and re-upload a patch in a bit...

[reed1, 2013-01-22 15:31:21]

Derek and I are confused. Can you have a brief VC with us to explain?

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureFlat.h#newcod...
src/core/SkPictureFlat.h:478: */
Seems like this guy should just return const SkFlatData*, since anyone can call
data->index() if they need that...

Do we want a 2nd helper, findIndex, that returns just the index, for those
callers?

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.appspot.com/7178045/diff/1/src/core/SkPictureRecord.cpp#ne...
src/core/SkPictureRecord.cpp:537: 
const SkFlatData& instead of *

[junov1, 2013-01-22 16:06:46]

More explanation: the fIndex member is the Id used to reference the flattened
paint. These numbers are assigned sequentially starting from 1. Flattened paints
are stored in an array in SkFlatDictionary.  The contents of this array are
sorted by flattened data contents, and not by fIndex.  This accelerates the
search for duplicated paint objects, but makes it inconvenient to search by
fIndex.  On playback, the unflattened objects get ordered by fIndex, so it
becomes convenient to search by index at that time.
This CL passes down the SkFlatData* to avoid having to search for it by index
number, which would be an O(N) search.

[reed1, 2013-01-22 16:29:05]

Note to selves:

In a future CL, it might help if we remove the ability to random-access the
dictionary (e.g. hide []?), and possibly comment the "index" field to note that
it is NOT an index into any data structure during record, but more of a
monotonic ID recording the order that the items were created.

[junov, 2013-01-22 16:50:56]

New patch

https://codereview.appspot.com/7178045/diff/7001/src/core/SkPictureFlat.h
File src/core/SkPictureFlat.h (right):

https://codereview.appspot.com/7178045/diff/7001/src/core/SkPictureFlat.h#new...
src/core/SkPictureFlat.h:497: const SkFlatData* findAndReturnFlat(const T&
element) {
Diff is misrepresenting what happened here. I moved findAndReturnFlat up into
the public section of the class definition

[reed1, 2013-01-22 16:54:13]

looks good. did you get a unittest to trigger the old bug?

[junov, 2013-01-22 17:41:31]

On 2013/01/22 16:54:13, reed1 wrote:
> looks good. did you get a unittest to trigger the old bug?

I'm having a hard time coming up with a good unit test. I think I'm gonna just
peek at the bytes written to the command stream to validate that the metrics are
correct.

[reed1, 2013-01-22 17:43:05]

Hmmm, that sounds (a) fragile, and (b) hard to verify in the test itself. I'm OK
w/ no unittest for this.

[reed1, 2013-01-22 17:43:14]

lgtm

[junov, 2013-01-22 17:48:54]

On 2013/01/22 17:43:14, reed1 wrote:
> lgtm

Sounds good. Besides the current code has decent test coverage already. It's
just a checksum/order coincidence that existing tests did not fail before this
CL.

[junov, 2013-01-22 17:48:54]

On 2013/01/22 17:43:14, reed1 wrote:
> lgtm

Sounds good. Besides the current code has decent test coverage already. It's
just a checksum/order coincidence that existing tests did not fail before this
CL.