Adding option in SkPicture to record device-space bounds of draw commands.

src/core/SkPictureRecord.cpp

Index: src/core/SkPictureRecord.cpp
===================================================================
--- src/core/SkPictureRecord.cpp	(revision 8010)
+++ src/core/SkPictureRecord.cpp	(working copy)
@@ -62,10 +62,56 @@
 
 ///////////////////////////////////////////////////////////////////////////////
 

[robertphillips, 2013/03/07 18:37:40]

Do you want to also make this one static inline?

[reed1, 2013/03/07 20:36:44]

Maybe its time to create a static uint8_t array that holds a bitfield of
attributes for each op-code, so we don't have to executed any switch-statements
when trying to classify an draw-op.

+bool SkPictureRecord::canRecordBounds(DrawType op) {

[Justin Novosad, 2013/03/07 20:25:47]

It needs to be accessible from SkPictureBitmap.

+    SkASSERT(op != DRAW_POS_TEXT_TOP_BOTTOM);  // Should never be used when encoding bounds
+    SkASSERT(op != DRAW_POS_TEXT_H_TOP_BOTTOM);
+    switch (op) {
+        case DRAW_BITMAP:
+        case DRAW_BITMAP_MATRIX:
+        case DRAW_BITMAP_NINE:
+        case DRAW_BITMAP_RECT_TO_RECT:
+        case DRAW_OVAL:
+        case DRAW_PATH:
+        case DRAW_POINTS:
+        case DRAW_POS_TEXT:
+        case DRAW_POS_TEXT_H:
+        case DRAW_RECT:
+        case DRAW_RRECT:
+        case DRAW_TEXT:
+        case DRAW_TEXT_ON_PATH:
+        case DRAW_VERTICES:
+            return true;
+        default:
+            return false;
+    }
+}
+
+static inline bool modifiesState(DrawType op) {
+    switch (op) {
+        case CLIP_PATH:
+        case CLIP_REGION:
+        case CLIP_RECT:
+        case CLIP_RRECT:
+        case CONCAT:
+        case DRAW_PICTURE:
+        case RESTORE:
+        case ROTATE:
+        case SAVE:
+        case SAVE_LAYER:
+        case SCALE:
+        case SET_MATRIX:
+        case SKEW:
+        case TRANSLATE:
+            return true;
+        default:
+            return false;
+    }
+}
+
 // Return the offset of the paint inside a given op's byte stream. A zero
 // return value means there is no paint (and you really shouldn't be calling
 // this method)
-static inline uint32_t getPaintOffset(DrawType op, uint32_t opSize) {
+static inline uint32_t getPaintOffset(DrawType op, uint32_t opSize, bool recordBounds) {
     // These offsets are where the paint would be if the op size doesn't overflow
     static const uint8_t gPaintOffsets[LAST_DRAWTYPE_ENUM + 1] = {
         0,  // UNUSED - no paint
@@ -116,6 +162,9 @@
         // after the op code
         overflow = sizeof(uint32_t);
     }
+    if (recordBounds && SkPictureRecord::canRecordBounds(op)) {
+        overflow += sizeof(SkIRect);
+    }
 
     if (SAVE_LAYER == op) {
         static const uint32_t kSaveLayerNoBoundsPaintOffset = 2 * kUInt32Size;
@@ -146,6 +195,7 @@
     // op + flags
     uint32_t size = 2 * kUInt32Size;
     uint32_t initialOffset = this->addDraw(SAVE, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addInt(flags);
 
     validate(initialOffset, size);
@@ -169,8 +219,9 @@
     SkASSERT(kSaveLayerNoBoundsSize == size || kSaveLayerWithBoundsSize == size);
 
     uint32_t initialOffset = this->addDraw(SAVE_LAYER, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addRectPtr(bounds);
-    SkASSERT(initialOffset+getPaintOffset(SAVE_LAYER, size) == fWriter.size());
+    SkASSERT(initialOffset + getPaintOffset(SAVE_LAYER, size, false) == fWriter.size());
     addPaintPtr(paint);
     addInt(flags);
 
@@ -235,7 +286,7 @@
  * where the saveLayer's color can be moved into the drawBitmap*'s paint
  */
 static bool remove_save_layer1(SkWriter32* writer, int32_t offset,
-                               SkPaintDictionary* paintDict) {
+                               SkPaintDictionary* paintDict, bool recordBounds) {
 
 #ifdef SK_IGNORE_PICTURE_RECORD_SAVE_LAYER_OPT
     return false;
@@ -283,8 +334,8 @@
         return false;       // something else between the dbm* and the restore
     }
 
-    uint32_t dbmPaintOffset = getPaintOffset(op, dbmSize);
-    uint32_t slPaintOffset = getPaintOffset(SAVE_LAYER, saveLayerSize);
+    uint32_t dbmPaintOffset = getPaintOffset(op, dbmSize, recordBounds);
+    uint32_t slPaintOffset = getPaintOffset(SAVE_LAYER, saveLayerSize, false);
 
     // we have a match, now we need to get the paints involved
     int dbmPaintId = *((int32_t*)writer->peek32(dbmOffset+dbmPaintOffset));
@@ -418,7 +469,8 @@
 
     uint32_t initialOffset, size;
     if (!collapseSaveClipRestore(&fWriter, fRestoreOffsetStack.top()) &&
-        !remove_save_layer1(&fWriter, fRestoreOffsetStack.top(), &fPaints)) {
+        !remove_save_layer1(&fWriter, fRestoreOffsetStack.top(), &fPaints,
+        this->recordBounds())) {
         fillRestoreOffsetPlaceholdersForCurrentStackLevel((uint32_t)fWriter.size());
         // op
         size = 1 * kUInt32Size;
@@ -427,6 +479,7 @@
         size = 0;
         initialOffset = fWriter.size();
     }
+    SkASSERT(kInvalidOffset != initialOffset);
 
     fRestoreOffsetStack.pop();
 
@@ -434,10 +487,81 @@
     return this->INHERITED::restore();
 }
 
+uint32_t SkPictureRecord::addDraw(DrawType drawType, uint32_t* size,
+                                  const SkPaint* paint, const SkRect* localBounds) {
+    bool writeBounds = this->recordBounds() && canRecordBounds(drawType);
+    SkIRect clippedDevBounds;  // Bounds of the clipped draw in device space
+    SkIRect devClipBounds;  // Bounds of the current clip in device space
+    if (!modifiesState(drawType) && !this->getClipDeviceBounds(&devClipBounds)) {
+        // Optimize-out calls that are clipped-out.  State modifying commands
+        // can not be optimized-out because they may have an impact on future calls
+        // if the clip is ever expanded. For example, with a union clip op. 
+        return kInvalidOffset;
+    }

[robertphillips, 2013/03/07 18:37:40]

mv clippedDevBounds down to here?

+    if (writeBounds) {
+        SkASSERT(!modifiesState(drawType)); // Ensure devClipBounds is initialized
+        if (NULL == localBounds || (NULL != paint && !paint->canComputeFastBounds())) {
+            clippedDevBounds = devClipBounds;
+        } else {
+            SkRect paintBounds;
+            SkRect rectifiedBounds = *localBounds;
+            rectifiedBounds.sort();
+            if (NULL != paint) {
+                if (drawType == DRAW_POINTS) {
+                    paintBounds = paint->computeFastStrokeBounds(rectifiedBounds, &paintBounds);
+                } else {
+                    paintBounds = paint->computeFastBounds(rectifiedBounds, &paintBounds);
+                }
+            } else {
+                paintBounds = *localBounds;
+            }
+            if (paintBounds.isEmpty()) {
+                // Area affected by draw op is empty even after accounting
+                // for stroke width.
+                return kInvalidOffset;
+            }
+            SkRect devBounds;
+            this->getTotalMatrix().mapRect(&devBounds, paintBounds);
+            devBounds.roundOut(&clippedDevBounds);
+            if (!clippedDevBounds.intersect(devClipBounds)) {
+                // Draw lies outside of clip. The clip can only get further
+                // restricted at playback time, so it is safe to reject during
+                // record stage.
+                return kInvalidOffset;
+            }
+        }
+        *size += sizeof(SkIRect);
+    }
+
+    uint32_t offset = fWriter.size();
+
+    this->predrawNotify();
+
+    #ifdef SK_DEBUG_TRACE
+        SkDebugf("add %s\n", DrawTypeToString(drawType));
+    #endif
+    SkASSERT(0 != *size);
+    SkASSERT(((uint8_t) drawType) == drawType);
+    if (0 != (*size & ~MASK_24) || *size == MASK_24) {
+        fWriter.writeInt(PACK_8_24(drawType, MASK_24));
+        *size += 1;
+        fWriter.writeInt(*size);
+    } else {
+        fWriter.writeInt(PACK_8_24(drawType, *size));
+    }
+
+    if (writeBounds) {
+        fWriter.write(&clippedDevBounds, sizeof(clippedDevBounds));
+    }
+
+    return offset;
+}
+
 bool SkPictureRecord::translate(SkScalar dx, SkScalar dy) {
     // op + dx + dy
     uint32_t size = 1 * kUInt32Size + 2 * sizeof(SkScalar);
     uint32_t initialOffset = this->addDraw(TRANSLATE, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addScalar(dx);
     addScalar(dy);
     validate(initialOffset, size);
@@ -448,6 +572,7 @@
     // op + sx + sy
     uint32_t size = 1 * kUInt32Size + 2 * sizeof(SkScalar);
     uint32_t initialOffset = this->addDraw(SCALE, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addScalar(sx);
     addScalar(sy);
     validate(initialOffset, size);
@@ -458,6 +583,7 @@
     // op + degrees
     uint32_t size = 1 * kUInt32Size + sizeof(SkScalar);
     uint32_t initialOffset = this->addDraw(ROTATE, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addScalar(degrees);
     validate(initialOffset, size);
     return this->INHERITED::rotate(degrees);
@@ -467,6 +593,7 @@
     // op + sx + sy
     uint32_t size = 1 * kUInt32Size + 2 * sizeof(SkScalar);
     uint32_t initialOffset = this->addDraw(SKEW, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addScalar(sx);
     addScalar(sy);
     validate(initialOffset, size);
@@ -478,6 +605,7 @@
     // op + matrix index
     uint32_t size = 2 * kUInt32Size;
     uint32_t initialOffset = this->addDraw(CONCAT, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addMatrix(matrix);
     validate(initialOffset, size);
     return this->INHERITED::concat(matrix);
@@ -488,6 +616,7 @@
     // op + matrix index
     uint32_t size = 2 * kUInt32Size;
     uint32_t initialOffset = this->addDraw(SET_MATRIX, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addMatrix(matrix);
     validate(initialOffset, size);
     this->INHERITED::setMatrix(matrix);
@@ -569,6 +698,7 @@
         size += kUInt32Size;
     }
     uint32_t initialOffset = this->addDraw(CLIP_RECT, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addRect(rect);
     addInt(ClipParams_pack(op, doAA));
     recordRestoreOffsetPlaceholder(op);
@@ -590,6 +720,7 @@
         size += kUInt32Size;
     }
     uint32_t initialOffset = this->addDraw(CLIP_RRECT, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addRRect(rrect);
     addInt(ClipParams_pack(op, doAA));
     recordRestoreOffsetPlaceholder(op);
@@ -618,6 +749,7 @@
         size += kUInt32Size;
     }
     uint32_t initialOffset = this->addDraw(CLIP_PATH, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addPath(path);
     addInt(ClipParams_pack(op, doAA));
     recordRestoreOffsetPlaceholder(op);
@@ -640,6 +772,7 @@
         size += kUInt32Size;
     }
     uint32_t initialOffset = this->addDraw(CLIP_REGION, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addRegion(region);
     addInt(ClipParams_pack(op, false));
     recordRestoreOffsetPlaceholder(op);
@@ -652,6 +785,9 @@
     // op + color
     uint32_t size = 2 * kUInt32Size;
     uint32_t initialOffset = this->addDraw(DRAW_CLEAR, &size);
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
     addInt(color);
     validate(initialOffset, size);
 }
@@ -660,7 +796,10 @@
     // op + paint index
     uint32_t size = 2 * kUInt32Size;
     uint32_t initialOffset = this->addDraw(DRAW_PAINT, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_PAINT, size) == fWriter.size());
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_PAINT, size, false) == fWriter.size());
     addPaint(paint);
     validate(initialOffset, size);
 }
@@ -669,8 +808,19 @@
                                  const SkPaint& paint) {
     // op + paint index + mode + count + point data
     uint32_t size = 4 * kUInt32Size + count * sizeof(SkPoint);
-    uint32_t initialOffset = this->addDraw(DRAW_POINTS, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_POINTS, size) == fWriter.size());
+    uint32_t initialOffset;
+    if (this->recordBounds()) {
+        SkRect bbox;
+        bbox.set(pts, count);
+        initialOffset = this->addDraw(DRAW_POINTS, &size, &paint, &bbox);
+    } else {
+        initialOffset = this->addDraw(DRAW_POINTS, &size);
+    }
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_POINTS, size, this->recordBounds()) \
+        == fWriter.size());
     addPaint(paint);
     addInt(mode);
     addInt(count);
@@ -681,8 +831,12 @@
 void SkPictureRecord::drawOval(const SkRect& oval, const SkPaint& paint) {
     // op + paint index + rect
     uint32_t size = 2 * kUInt32Size + sizeof(oval);
-    uint32_t initialOffset = this->addDraw(DRAW_OVAL, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_OVAL, size) == fWriter.size());
+    uint32_t initialOffset = this->addDraw(DRAW_OVAL, &size, &paint, &oval);
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_OVAL, size, this->recordBounds()) \
+        == fWriter.size());
     addPaint(paint);
     addRect(oval);
     validate(initialOffset, size);
@@ -691,45 +845,47 @@
 void SkPictureRecord::drawRect(const SkRect& rect, const SkPaint& paint) {
     // op + paint index + rect
     uint32_t size = 2 * kUInt32Size + sizeof(rect);
-    uint32_t initialOffset = this->addDraw(DRAW_RECT, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_RECT, size) == fWriter.size());
+    uint32_t initialOffset = this->addDraw(DRAW_RECT, &size, &paint, &rect);
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_RECT, size, this->recordBounds()) \
+        == fWriter.size());
     addPaint(paint);
     addRect(rect);
     validate(initialOffset, size);
 }
 
 void SkPictureRecord::drawRRect(const SkRRect& rrect, const SkPaint& paint) {
-    uint32_t initialOffset, size;
     if (rrect.isRect()) {
-        // op + paint index + rect
-        size = 2 * kUInt32Size + sizeof(SkRect);
-        initialOffset = this->addDraw(DRAW_RECT, &size);
-        SkASSERT(initialOffset+getPaintOffset(DRAW_RECT, size) == fWriter.size());
-        addPaint(paint);
-        addRect(rrect.getBounds());
+        this->drawRect(rrect.getBounds(), paint);
     } else if (rrect.isOval()) {
-        // op + paint index + rect
-        size = 2 * kUInt32Size + sizeof(SkRect);
-        initialOffset = this->addDraw(DRAW_OVAL, &size);
-        SkASSERT(initialOffset+getPaintOffset(DRAW_OVAL, size) == fWriter.size());
-        addPaint(paint);
-        addRect(rrect.getBounds());
+        this->drawOval(rrect.getBounds(), paint);
     } else {
         // op + paint index + rrect
-        size = 2 * kUInt32Size + SkRRect::kSizeInMemory;
-        initialOffset = this->addDraw(DRAW_RRECT, &size);
-        SkASSERT(initialOffset+getPaintOffset(DRAW_RRECT, size) == fWriter.size());
+        uint32_t size = 2 * kUInt32Size + SkRRect::kSizeInMemory;
+        uint32_t initialOffset = this->addDraw(DRAW_RRECT, &size, &paint, &rrect.getBounds());
+        if (kInvalidOffset == initialOffset) {
+            return;
+        }
+        SkASSERT(initialOffset + getPaintOffset(DRAW_RRECT, size, this->recordBounds()) \
+            == fWriter.size());
         addPaint(paint);
         addRRect(rrect);
+        validate(initialOffset, size);
     }
-    validate(initialOffset, size);
 }
 
 void SkPictureRecord::drawPath(const SkPath& path, const SkPaint& paint) {
     // op + paint index + path index
     uint32_t size = 3 * kUInt32Size;
-    uint32_t initialOffset = this->addDraw(DRAW_PATH, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_PATH, size) == fWriter.size());
+    const SkRect* bounds = path.isInverseFillType() ? NULL : &path.getBounds();
+    uint32_t initialOffset = this->addDraw(DRAW_PATH, &size, &paint, bounds);
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_PATH, size, this->recordBounds()) \
+        == fWriter.size());
     addPaint(paint);
     addPath(path);
     validate(initialOffset, size);
@@ -739,8 +895,13 @@
                         const SkPaint* paint = NULL) {
     // op + paint index + bitmap index + left + top
     uint32_t size = 3 * kUInt32Size + 2 * sizeof(SkScalar);
-    uint32_t initialOffset = this->addDraw(DRAW_BITMAP, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_BITMAP, size) == fWriter.size());
+    SkRect bounds = SkRect::MakeXYWH(left, top, bitmap.width(), bitmap.height());
+    uint32_t initialOffset = this->addDraw(DRAW_BITMAP, &size, paint, &bounds);
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_BITMAP, size, this->recordBounds()) \
+        == fWriter.size());
     addPaintPtr(paint);
     addBitmap(bitmap);
     addScalar(left);
@@ -757,8 +918,12 @@
     }
     size += sizeof(dst);        // + rect
 
-    uint32_t initialOffset = this->addDraw(DRAW_BITMAP_RECT_TO_RECT, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_BITMAP_RECT_TO_RECT, size) == fWriter.size());
+    uint32_t initialOffset = this->addDraw(DRAW_BITMAP_RECT_TO_RECT, &size, paint, &dst);
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_BITMAP_RECT_TO_RECT, size, this->recordBounds()) \
+             == fWriter.size());
     addPaintPtr(paint);
     addBitmap(bitmap);
     addRectPtr(src);  // may be null
@@ -770,8 +935,19 @@
                                        const SkPaint* paint) {
     // id + paint index + bitmap index + matrix index
     uint32_t size = 4 * kUInt32Size;
-    uint32_t initialOffset = this->addDraw(DRAW_BITMAP_MATRIX, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_BITMAP_MATRIX, size) == fWriter.size());
+    uint32_t initialOffset;
+    if (this->recordBounds()) {
+        SkRect bounds = SkRect::MakeWH(bitmap.width(), bitmap.height());
+        matrix.mapRect(&bounds);
+        initialOffset = this->addDraw(DRAW_BITMAP_MATRIX, &size, paint, &bounds);
+    } else {
+        initialOffset = this->addDraw(DRAW_BITMAP_MATRIX, &size);
+    }
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_BITMAP_MATRIX, size, this->recordBounds()) \
+        == fWriter.size());
     addPaintPtr(paint);
     addBitmap(bitmap);
     addMatrix(matrix);
@@ -782,8 +958,12 @@
                                      const SkRect& dst, const SkPaint* paint) {
     // op + paint index + bitmap id + center + dst rect
     uint32_t size = 3 * kUInt32Size + sizeof(center) + sizeof(dst);
-    uint32_t initialOffset = this->addDraw(DRAW_BITMAP_NINE, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_BITMAP_NINE, size) == fWriter.size());
+    uint32_t initialOffset = this->addDraw(DRAW_BITMAP_NINE, &size, paint, &dst);
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_BITMAP_NINE, size, this->recordBounds()) == \
+        fWriter.size());
     addPaintPtr(paint);
     addBitmap(bitmap);
     addIRect(center);
@@ -792,11 +972,16 @@
 }
 
 void SkPictureRecord::drawSprite(const SkBitmap& bitmap, int left, int top,
-                        const SkPaint* paint = NULL) {
+                                 const SkPaint* paint) {
     // op + paint index + bitmap index + left + top
     uint32_t size = 5 * kUInt32Size;
-    uint32_t initialOffset = this->addDraw(DRAW_SPRITE, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_SPRITE, size) == fWriter.size());
+    uint32_t initialOffset;

[robertphillips, 2013/03/07 18:37:40]

Could you expand on this with a "because ..."

+    // Note: Bounds encoding not supported.

[robertphillips, 2013/03/07 18:37:40]

Move uint32_t down to line 980?

+    initialOffset = this->addDraw(DRAW_SPRITE, &size);
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_SPRITE, size, false) == fWriter.size());
     addPaintPtr(paint);
     addBitmap(bitmap);
     addInt(left);
@@ -831,7 +1016,10 @@
 
 void SkPictureRecord::drawText(const void* text, size_t byteLength, SkScalar x,
                       SkScalar y, const SkPaint& paint) {
-    bool fast = !paint.isVerticalText() && paint.canComputeFastBounds();
+    // Note: The DRAW_TEXT_TOP_BOTTOM optimization is redundant if we are
+    // already recording full bounds.
+    bool fast = !paint.isVerticalText() && paint.canComputeFastBounds() &&

[robertphillips, 2013/03/07 18:37:40]

spaces

+        !this->recordBounds();
 
     // op + paint index + length + 'length' worth of chars + x + y
     uint32_t size = 3 * kUInt32Size + SkAlign4(byteLength) + 2 * sizeof(SkScalar);
@@ -840,8 +1028,57 @@
     }
 
     DrawType op = fast ? DRAW_TEXT_TOP_BOTTOM : DRAW_TEXT;
-    uint32_t initialOffset = this->addDraw(op, &size);
-    SkASSERT(initialOffset+getPaintOffset(op, size) == fWriter.size());
+    uint32_t initialOffset;
+    if (this->recordBounds()) {
+        SkRect bbox;
+        paint.measureText(text, byteLength, &bbox);
+        SkPaint::FontMetrics metrics;
+        paint.getFontMetrics(&metrics);
+
+        // Vertical and aligned text need to be offset
+        if (paint.isVerticalText()) {
+            SkScalar h = bbox.fBottom - bbox.fTop;
+            if (paint.getTextAlign() == SkPaint::kCenter_Align) {
+                bbox.fTop    -= h / 2;
+                bbox.fBottom -= h / 2;
+            }
+            // Pad top and bottom with max extents from FontMetrics
+            bbox.fBottom += metrics.fBottom;
+            bbox.fTop += metrics.fTop;
+        } else {
+            SkScalar w = bbox.fRight - bbox.fLeft;
+            if (paint.getTextAlign() == SkPaint::kCenter_Align) {
+                bbox.fLeft  -= w / 2;
+                bbox.fRight -= w / 2;
+            } else if (paint.getTextAlign() == SkPaint::kRight_Align) {
+                bbox.fLeft  -= w;
+                bbox.fRight -= w;
+            }
+            // Set vertical bounds to max extents from font metrics
+            bbox.fTop = metrics.fTop;
+            bbox.fBottom = metrics.fBottom;
+        }
+
+        // Pad horizontal bounds on each side by half of max vertical extents (this is sort of

[reed1, 2013/03/07 20:36:44]

80-cols in this file

+        // arbitrary, but seems to produce reasonable results, if there were a way of getting max
+        // glyph X-extents to pad by, that may be better here, but FontMetrics fXMin and fXMax seem
+        // incorrect on most platforms (too small in Linux, never even set in Windows).

[reed1, 2013/03/07 20:36:44]

yikes! please file a bug!

[Justin Novosad, 2013/03/08 14:49:14]

This was copy-pasted from SkBBoxRecord (r5423 by rileya).  Not sure what the
history behind this comment is. I feel this should be made more conservative due
to its fuzzy nature. I think I should remove the divide by two.

+        SkScalar pad = (metrics.fBottom - metrics.fTop) / 2;
+        bbox.fLeft  -= pad;
+        bbox.fRight += pad;
+
+        bbox.fLeft += x;
+        bbox.fRight += x;
+        bbox.fTop += y;
+        bbox.fBottom += y;
+        initialOffset = this->addDraw(op, &size, &paint, &bbox);
+    } else {
+        initialOffset = this->addDraw(op, &size);
+    }
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(op, size, this->recordBounds()) == fWriter.size());
     const SkFlatData* flatPaintData = addPaint(paint);
     SkASSERT(flatPaintData);
     addText(text, byteLength);
@@ -877,7 +1114,10 @@
         }
     }
 
-    bool fastBounds = !paint.isVerticalText() && paint.canComputeFastBounds();
+    // Note: The DRAW_TEXT_[H_]TOP_BOTTOM optimization is redundant if we are
+    // already recording full bounds.
+    bool fastBounds = !paint.isVerticalText() && paint.canComputeFastBounds() &&

[robertphillips, 2013/03/07 18:37:40]

spaces

+        !this->recordBounds();
     bool fast = canUseDrawH && fastBounds;
 
     // op + paint index + length + 'length' worth of data + num points
@@ -906,8 +1146,27 @@
     } else {
         op = DRAW_POS_TEXT;
     }
-    uint32_t initialOffset = this->addDraw(op, &size);
-    SkASSERT(initialOffset+getPaintOffset(op, size) == fWriter.size());
+    uint32_t initialOffset;
+    if (this->recordBounds()) {
+        SkRect bbox;
+        bbox.set(pos, paint.countText(text, byteLength));
+        SkPaint::FontMetrics metrics;
+        paint.getFontMetrics(&metrics);
+        bbox.fTop += metrics.fTop;
+        bbox.fBottom += metrics.fBottom;
+
+        // pad on left and right by half of max vertical glyph extents
+        SkScalar pad = (metrics.fTop - metrics.fBottom) / 2;
+        bbox.fLeft += pad;
+        bbox.fRight -= pad;
+        initialOffset = addDraw(op, &size, &paint, &bbox);
+    } else {
+        initialOffset = this->addDraw(op, &size);
+    }
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(op, size, this->recordBounds()) == fWriter.size());
     const SkFlatData* flatPaintData = addPaint(paint);
     SkASSERT(flatPaintData);
     addText(text, byteLength);
@@ -944,7 +1203,10 @@
     if (0 == points)
         return;
 
-    bool fast = !paint.isVerticalText() && paint.canComputeFastBounds();
+    // Note: The DRAW_POS_TEXT_H_TOP_BOTTOM optimization is redundant if we are
+    // already recording full bounds.
+    bool fast = !paint.isVerticalText() && paint.canComputeFastBounds() &&

[robertphillips, 2013/03/07 18:37:40]

spaces

+        !this->recordBounds();
 
     // op + paint index + length + 'length' worth of data + num points
     uint32_t size = 3 * kUInt32Size + SkAlign4(byteLength) + 1 * kUInt32Size;
@@ -954,8 +1216,41 @@
     // + y + the actual points
     size += 1 * kUInt32Size + points * sizeof(SkScalar);
 
-    uint32_t initialOffset = this->addDraw(fast ? DRAW_POS_TEXT_H_TOP_BOTTOM : DRAW_POS_TEXT_H,
-                                           &size);
+    uint32_t initialOffset;
+    if (this->recordBounds()) {
+        size_t numChars = paint.countText(text, byteLength);

[robertphillips, 2013/03/07 18:37:40]

We should probably bail somewhere (or add an assert) if numChar == 0

[Justin Novosad, 2013/03/07 20:25:47]

Actually, I just realized this is redundant with 'points', computed above.

+        SkRect bbox;
+        bbox.fLeft  = xpos[0];

[robertphillips, 2013/03/07 18:37:40]

shouldn't this be 0 rather than numChars-1?

[Justin Novosad, 2013/03/07 20:25:47]

For the algorithm to work, it actually does not matter.  Initializing like this
is more likely to be optimal for perf (assuming we usually write from left to
right) because we will never update bbox in the loop below in the well
conditioned cases.

[robertphillips, 2013/03/07 20:57:37]

Since we have no guarantee of monotonicity, what if xpos[0] is the rightmost?

+        bbox.fRight = xpos[numChars - 1];
+        // if we had a guarantee that these will be monotonically increasing, this could be sped up
+        for (size_t i = 1; i < numChars; ++i) {
+            if (xpos[i] < bbox.fLeft) {
+                bbox.fLeft = xpos[i];
+            }
+            if (xpos[i] > bbox.fRight) {
+                bbox.fRight = xpos[i];
+            }
+        }
+        SkPaint::FontMetrics metrics;

[reed1, 2013/03/07 20:36:44]

Some amount of this code looks awefully similar to drawPosText and drawText. Can
we do refactoring to share some?

+        paint.getFontMetrics(&metrics);
+
+        // pad horizontally by max glyph height
+        SkScalar pad = (metrics.fTop - metrics.fBottom);
+        bbox.fLeft  += pad;
+        bbox.fRight -= pad;
+
+        bbox.fTop    = metrics.fTop + constY;
+        bbox.fBottom = metrics.fBottom + constY;
+        initialOffset = this->addDraw(DRAW_POS_TEXT_H, &size, &paint, &bbox);
+    } else {
+        initialOffset = this->addDraw(fast ? DRAW_POS_TEXT_H_TOP_BOTTOM : DRAW_POS_TEXT_H,
+                                      &size);
+    }
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_TEXT_ON_PATH, size, this->recordBounds()) \
+             == fWriter.size());
     const SkFlatData* flatPaintData = addPaint(paint);
     SkASSERT(flatPaintData);
     addText(text, byteLength);
@@ -981,8 +1276,26 @@
                             const SkPaint& paint) {
     // op + paint index + length + 'length' worth of data + path index + matrix index
     uint32_t size = 3 * kUInt32Size + SkAlign4(byteLength) + 2 * kUInt32Size;
-    uint32_t initialOffset = this->addDraw(DRAW_TEXT_ON_PATH, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_TEXT_ON_PATH, size) == fWriter.size());
+    uint32_t initialOffset;
+    if (this->recordBounds()) {
+        SkRect bbox = path.getBounds();
+        SkPaint::FontMetrics metrics;
+        paint.getFontMetrics(&metrics);
+        // pad out all sides by the max glyph height above baseline
+        SkScalar pad = metrics.fTop;
+        bbox.fLeft += pad;
+        bbox.fRight -= pad;
+        bbox.fTop += pad;
+        bbox.fBottom -= pad;
+        initialOffset = this->addDraw(DRAW_TEXT_ON_PATH, &size, &paint, &bbox);
+    } else {
+        initialOffset = this->addDraw(DRAW_TEXT_ON_PATH, &size);
+    }
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_TEXT_ON_PATH, size, this->recordBounds()) \
+        == fWriter.size());
     addPaint(paint);
     addText(text, byteLength);
     addPath(path);
@@ -994,6 +1307,7 @@
     // op + picture index
     uint32_t size = 2 * kUInt32Size;
     uint32_t initialOffset = this->addDraw(DRAW_PICTURE, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addPicture(picture);
     validate(initialOffset, size);
 }
@@ -1027,8 +1341,19 @@
         size += 1 * kUInt32Size + SkAlign4(indexCount * sizeof(uint16_t));
     }
 
-    uint32_t initialOffset = this->addDraw(DRAW_VERTICES, &size);
-    SkASSERT(initialOffset+getPaintOffset(DRAW_VERTICES, size) == fWriter.size());
+    uint32_t initialOffset;
+    if (this->recordBounds()) {
+        SkRect bbox;
+        bbox.set(vertices, vertexCount);
+        initialOffset = this->addDraw(DRAW_VERTICES, &size, &paint, &bbox);
+    } else {
+        initialOffset = this->addDraw(DRAW_VERTICES, &size);
+    }
+    if (kInvalidOffset == initialOffset) {
+        return;
+    }
+    SkASSERT(initialOffset + getPaintOffset(DRAW_VERTICES, size, this->recordBounds()) \
+        == fWriter.size());
     addPaint(paint);
     addInt(flags);
     addInt(vmode);
@@ -1051,6 +1376,7 @@
     // op + length + 'length' worth of data
     uint32_t size = 2 * kUInt32Size + SkAlign4(length);
     uint32_t initialOffset = this->addDraw(DRAW_DATA, &size);
+    SkASSERT(kInvalidOffset != initialOffset);
     addInt(length);
     fWriter.writePad(data, length);
     validate(initialOffset, size);