Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23001007:
Remove the call to getFontMetrics from SkBBoxRecord (Closed)
|
Created:
7 years, 4 months ago by sglez Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionRemove the call to getFontMetrics from SkBBoxRecord
Committed: http://code.google.com/p/skia/source/detail?r=10876
Patch Set 1 : #
Total comments: 8
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 15 (0 generated)
Link to a graph that shows the speedup: (x axis: skps, y axis: recording time in milliseconds) https://docs.google.com/a/google.com/file/d/0ByjCOkTma8-LQ252TmxQNVdmYVE
https://codereview.chromium.org/23001007/diff/8001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/8001/src/core/SkBBoxRecord.cpp#... src/core/SkBBoxRecord.cpp:187: if (numChars > 0) { minor: if numChars == 0, we should abort everything (before we call getFlatPaintData). https://codereview.chromium.org/23001007/diff/8001/src/core/SkBBoxRecord.cpp#... src/core/SkBBoxRecord.cpp:203: if (flatPaintData->isTopBotWritten()) { Is there (can there be) a common helper method, perhaps on flatPaintData, that computes topBot as needed? Could the baseclass drawPosText use that as well? https://codereview.chromium.org/23001007/diff/8001/src/core/SkPictureRecord.cpp File src/core/SkPictureRecord.cpp (right): https://codereview.chromium.org/23001007/diff/8001/src/core/SkPictureRecord.c... src/core/SkPictureRecord.cpp:1109: drawPosTextHImpl(text, byteLength, xpos, constY, paint, NULL); Might be clearer if we called getFlatPaintData here, and passed that ptr. That way we look more parallel to the bbox version, and the impl doesn't need to check for null.
https://codereview.chromium.org/23001007/diff/8001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/8001/src/core/SkBBoxRecord.cpp#... src/core/SkBBoxRecord.cpp:202: SkScalar bottom = 0; no need to initialize top, bottom https://codereview.chromium.org/23001007/diff/8001/src/core/SkBBoxRecord.cpp#... src/core/SkBBoxRecord.cpp:206: pad = top - bottom; move pad declaration + assignment below 'if' https://codereview.chromium.org/23001007/diff/8001/src/core/SkPictureRecord.h File src/core/SkPictureRecord.h (right): https://codereview.chromium.org/23001007/diff/8001/src/core/SkPictureRecord.h... src/core/SkPictureRecord.h:168: void addFlatPaint(const SkFlatData* flatPaint); looks like the surrounding calls are alphabetized -- this one might as well be too
https://codereview.chromium.org/23001007/diff/8001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/8001/src/core/SkBBoxRecord.cpp#... src/core/SkBBoxRecord.cpp:203: if (flatPaintData->isTopBotWritten()) { On 2013/08/20 17:59:23, reed1 wrote: > Is there (can there be) a common helper method, perhaps on flatPaintData, that > computes topBot as needed? Could the baseclass drawPosText use that as well? Added helper method WriteTopBot that wraps what the base class does to ensure topBot is written. https://codereview.chromium.org/23001007/diff/8001/src/core/SkPictureRecord.h File src/core/SkPictureRecord.h (right): https://codereview.chromium.org/23001007/diff/8001/src/core/SkPictureRecord.h... src/core/SkPictureRecord.h:168: void addFlatPaint(const SkFlatData* flatPaint); On 2013/08/20 20:11:34, caryclark wrote: > looks like the surrounding calls are alphabetized -- this one might as well be > too I would prefer for the sake of clarity to have all the add*Paint* functions together, see addIRect below. If you think the other way is better, I will change it.
https://codereview.chromium.org/23001007/diff/22001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/22001/src/core/SkBBoxRecord.cpp... src/core/SkBBoxRecord.cpp:197: for (size_t i = 1; i < numChars; ++i) { what if xpos[0] > bbox.fRight ?
https://codereview.chromium.org/23001007/diff/22001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/22001/src/core/SkBBoxRecord.cpp... src/core/SkBBoxRecord.cpp:197: for (size_t i = 1; i < numChars; ++i) { On 2013/08/21 17:54:09, caryclark wrote: > what if xpos[0] > bbox.fRight ? i starts at 1. If numChars == 1, then fLeft == fRight, but that will change below when we add the padding.
https://codereview.chromium.org/23001007/diff/22001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/22001/src/core/SkBBoxRecord.cpp... src/core/SkBBoxRecord.cpp:197: for (size_t i = 1; i < numChars; ++i) { On 2013/08/21 18:11:56, sglez wrote: > On 2013/08/21 17:54:09, caryclark wrote: > > what if xpos[0] > bbox.fRight ? > > i starts at 1. > > If numChars == 1, then fLeft == fRight, but that will change below when we add > the padding. suppose numChars == 2, xpos[0] = 3, xpos[1] = 2: bbox.fLeft = xpos[0] = 3; bbox.fRight = xpos[1] = 2; for (size_t i = 1; i < 2; ++i) { if (xpos[1] (==2) < bbox.fLeft (==3)) { bboxfLeft = xpos[1] = 2; if (xpos[1] (==2) > bbox.fRight (==2)) { ... nothing happens } bbox.fLeft == bbox.fRight == 2
https://codereview.chromium.org/23001007/diff/22001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/22001/src/core/SkBBoxRecord.cpp... src/core/SkBBoxRecord.cpp:197: for (size_t i = 1; i < numChars; ++i) { On 2013/08/21 18:27:55, caryclark wrote: > On 2013/08/21 18:11:56, sglez wrote: > > On 2013/08/21 17:54:09, caryclark wrote: > > > what if xpos[0] > bbox.fRight ? > > > > i starts at 1. > > > > If numChars == 1, then fLeft == fRight, but that will change below when we add > > the padding. > > suppose numChars == 2, xpos[0] = 3, xpos[1] = 2: > > bbox.fLeft = xpos[0] = 3; > bbox.fRight = xpos[1] = 2; > for (size_t i = 1; i < 2; ++i) { > if (xpos[1] (==2) < bbox.fLeft (==3)) { > bboxfLeft = xpos[1] = 2; > if (xpos[1] (==2) > bbox.fRight (==2)) { > ... nothing happens > } > > bbox.fLeft == bbox.fRight == 2 Assuming bbox.fLeft == bbox.fRight, And assuming pad != 0; Below, we have: bbox.fLeft += pad; bbox.fRight -= pad; This will leave bbox.fLeft != bbox.fRight, which is fine, because the call to transformBounds(..) will call bbox.sort(), leaving bbox.fLeft < bbox.fRight If pad == 0, then top == bottom and there's something seriously wrong with the paint.
My example was poor. I was trying to suggest that if xpos[0] > xpos[1], then this logic does not set bbox.fRight to xpos[0]. Repeat my example, with pos[0] set to 1000 -- hopefully this will make sense then.
On 2013/08/21 18:58:33, caryclark wrote: > My example was poor. I was trying to suggest that if xpos[0] > xpos[1], then > this logic does not set bbox.fRight to xpos[0]. > Repeat my example, with pos[0] set to 1000 -- hopefully this will make sense > then. Touche...
https://codereview.chromium.org/23001007/diff/28001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/28001/src/core/SkBBoxRecord.cpp... src/core/SkBBoxRecord.cpp:197: I think that calling min and max fixes the bug.
lgtm https://codereview.chromium.org/23001007/diff/28001/src/core/SkBBoxRecord.cpp File src/core/SkBBoxRecord.cpp (right): https://codereview.chromium.org/23001007/diff/28001/src/core/SkBBoxRecord.cpp... src/core/SkBBoxRecord.cpp:197: On 2013/08/21 19:33:28, sglez wrote: > I think that calling min and max fixes the bug. It works. I would have written it as bbox.fLeft = SK_ScalarMax; bbox.fRight = SK_ScalarMin; for (size_t i = 0; i < numChars; ++i) { ...
On 2013/08/21 19:58:01, caryclark wrote: > lgtm > > https://codereview.chromium.org/23001007/diff/28001/src/core/SkBBoxRecord.cpp > File src/core/SkBBoxRecord.cpp (right): > > https://codereview.chromium.org/23001007/diff/28001/src/core/SkBBoxRecord.cpp... > src/core/SkBBoxRecord.cpp:197: > On 2013/08/21 19:33:28, sglez wrote: > > I think that calling min and max fixes the bug. > > It works. I would have written it as > > bbox.fLeft = SK_ScalarMax; > bbox.fRight = SK_ScalarMin; > > for (size_t i = 0; i < numChars; ++i) { > ... Thanks, that's a better solution
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sglez@google.com/23001007/45001
Message was sent while issue was closed.
Change committed as 10876 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
