Create a semi-stable API for capturing the state of an SkCanvas and reconstructing that state acros…

src/utils/SkCanvasUtils.cpp

+/*
+ * Copyright 2013 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkCanvasUtils.h"
+#include "SkCanvas.h"
+#include "SkDevice.h"
+#include "SkNWayCanvas.h"
+#include "SkSurface.h"
+#include "SkTScopedPtr.h"

[mtklein, 2013/08/28 18:20:46]

Ben will scream.  I think the Ben-friendly version in SkAutoTDelete?

[djsollen, 2013/08/29 15:09:19]

Done.

+#include "SkWriter32.h"
+
+#define CANVAS_STATE_VERSION 1
+
+enum RasterConfigs {
+  kRGB_565_RasterConfig   = 4,

[scroggo, 2013/08/28 17:22:01]

Maybe a comment explaining why these start with 4?

[djsollen, 2013/08/29 15:09:19]

Done.

+  kARGB_4444_RasterConfig = 5,
+  kARGB_8888_RasterConfig = 6
+};
+typedef int32_t RasterConfig;

[mtklein, 2013/08/28 18:20:46]

Can you plaster a few warnings around to remind the reader that we're trying to
keep a stable ABI here, and that's why we do things like typedef int32_t
RasterConfig instead of just using the enum?

[djsollen, 2013/08/29 15:09:19]

Done.

+
+enum CanvasBackends {
+    kUnknown_CanvasBackend = 0,
+    kRaster_CanvasBackend  = 1,
+    kGPU_CanvasBackend     = 2,
+    kPDF_CanvasBackend     = 3
+};
+typedef int32_t CanvasBackend;
+
+struct SkMCState {
+    // The Matrix and Clip are relative to |pixels|, not the source canvas.
+    float matrix[9];      // The matrix currently in effect on the canvas.
+
+    // this only works for non-antialiased clips
+    int32_t clipRectCount; // Number of rects in |clip_rects|.
+    int32_t* clipRects;    // Clip area: 4 ints per rect in {x,y,w,h} format.

[mtklein, 2013/08/28 18:20:46]

Can't hurt to have a struct for this, right?  Then we don't have to remember the
4x multiplier when we reconstruct the canvas.

[djsollen, 2013/08/29 15:09:19]

Done.

+

[mtklein, 2013/08/28 18:20:46]

stray newline

[djsollen, 2013/08/29 15:09:19]

Done.

+};
+
+// NOTE: If you add more members, bump CanvasState::version.
+struct SkCanvasLayerState {
+    CanvasBackend type;   // |pixel| format: a value from AwPixelConfig.

[mtklein, 2013/08/28 18:20:46]

Comment is confusing?

[djsollen, 2013/08/29 15:09:19]

Done.

+    int32_t x, y;
+    int32_t width;        // In pixels.
+    int32_t height;       // In pixels.
+
+    SkMCState mcState;
+
+    union {
+        struct {
+            RasterConfig config; // |pixel| format: a value from RasterConfigs.
+            int32_t rowBytes;   // Number of bytes from start of one line to next.

[scroggo, 2013/08/28 17:22:01]

nit: alignment of //'s

Also, should rowBytes be size_t like in SkBitmap?

[djsollen, 2013/08/29 15:09:19]

No size_t is not guaranteed to always be 32 bits. For example, it can be 64 bits
on a 64 bit system. However we probably should make the a uint_32t since there
can never be a negative number and it will more closely match the common size_t
implementation.

+            void* pixels;        // The pixels, all (height * row_bytes) of them.
+        } raster;
+        struct {
+            int32_t textureID;
+        } gpu;
+    } content;

[mtklein, 2013/08/28 18:20:46]

You may find it more readable to remove the name "content", and just work with
raster or gpu directly.  Pretty sure you can do that right?

[djsollen, 2013/08/29 15:09:19]

Done.

+};
+
+class SkCanvasState {

[scroggo, 2013/08/28 17:22:01]

Should this inherit from SkNoncopyable and hide the default constructor, or not
worry about it since it's a private class?

[djsollen, 2013/08/29 15:09:19]

This needs to stay as close to a simple struct as possible.  I don't want to
inherit from anything to keep the ABI stable.  In the event that those inherited
classes were to add virtual methods or member variables compatibility would be
broken.

+public:
+    SkCanvasState(const SkISize& size) {
+        version = CANVAS_STATE_VERSION;
+        width = size.width();
+        height = size.height();
+        layerCount = 0;
+        layers = NULL;
+    }
+
+    ~SkCanvasState() {
+        // loop through the layers and free the data allocated to the clipRects
+        for (int i = 0; i < layerCount; ++i) {
+            sk_free(layers[i].mcState.clipRects);
+        }
+
+        sk_free(mcState.clipRects);

[scroggo, 2013/08/28 17:22:01]

Should we set this to NULL in the constructor?

[mtklein, 2013/08/28 18:20:46]

More than that, what about initializing the structs generally?  Or is the idea,
there's no way to get one of these without filling it in from an SkCanvas, so
delay initialization until then?  I'd rather go whole-hog one way or the
other... make this thing's constructor and destructor comprehensive, or just
make SkCanvasState a struct and do both the init and cleanup externally.

+        sk_free(layers);
+    }
+
+    int32_t version;  // The CanvasState version this struct was built with.

[scroggo, 2013/08/28 17:22:01]

Is there a reason these don't follow our convention of starting with 'f' for
fields? Same with the rest of the classes/structs in this file; although I think
we don't always follow the convention for structs?

[mtklein, 2013/08/28 18:20:46]

Given that they're public members, I personally like it this way.

[mtklein, 2013/08/28 18:20:46]

Add a reminder that we can never reorder around the version field?  I that too
obvious?

+    int32_t width;
+    int32_t height;
+
+    SkMCState mcState;
+
+    int32_t layerCount;
+    SkCanvasLayerState* layers;
+};
+
+////////////////////////////////////////////////////////////////////////////////
+
+class ClipValidator : public SkCanvas::ClipVisitor {
+ public:

[scroggo, 2013/08/28 17:22:01]

nit: spacing. We typically have no spaces before "public"

[djsollen, 2013/08/29 15:09:19]

Done.

+  ClipValidator() : failed_(false) {}
+  bool failed() { return failed_; }
+
+  // ClipVisitor
+  virtual void clipRect(const SkRect& rect, SkRegion::Op op, bool antialias) {

[scroggo, 2013/08/28 17:22:01]

SK_OVERRIDE?

[djsollen, 2013/08/29 15:09:19]

Done.

+    failed_ |= antialias;
+  }
+
+  virtual void clipPath(const SkPath&, SkRegion::Op, bool antialias) {

[scroggo, 2013/08/28 17:22:01]

SK_OVERRIDE?

[djsollen, 2013/08/29 15:09:19]

Done.

+    failed_ |= antialias;
+  }
+
+ private:

[scroggo, 2013/08/28 17:22:01]

Same spacing nit.

[djsollen, 2013/08/29 15:09:19]

Done.

+  bool failed_;
+};
+
+static void setup_MC_state(SkMCState* state, const SkMatrix& matrix, const SkRegion& clip) {
+    // initialize the struct
+    state->clipRectCount = 0;
+
+    // capture the matrix
+    for (int i = 0; i < 9; i++) {
+        state->matrix[i] = matrix.get(i);
+    }
+
+    // capture the clip
+    const int clipBufferSize = 3 * sizeof(SkIRect);
+    char clipBuffer[clipBufferSize];
+    SkWriter32 clipWriter(sizeof(SkIRect), clipBuffer, clipBufferSize);
+
+    if (!clip.isEmpty()) {
+        // only returns the b/w clip so aa clips fail
+        SkRegion::Iterator clip_iterator(clip);
+        for (; !clip_iterator.done(); clip_iterator.next()) {
+            clipWriter.writeIRect(clip_iterator.rect());
+            state->clipRectCount++;
+        }
+    }
+
+    // allocate memory for the clip then and copy them to the struct
+    state->clipRects = (int32_t*) sk_malloc_throw(clipWriter.size());
+    clipWriter.flatten(state->clipRects);
+}
+
+
+
+SkCanvasState* SkCanvasUtils::CaptureCanvasState(SkCanvas* canvas) {

[scroggo, 2013/08/28 17:22:01]

Should we check for a NULL canvas?

[djsollen, 2013/08/29 15:09:19]

Done.

+    // Check the clip can be decomposed into simple rectangles.

[scroggo, 2013/08/28 17:22:01]

Comment seems inaccurate; the validator checks for antialiasing, but allows
arbitrary paths.

[djsollen, 2013/08/29 15:09:19]

non-antialiased arbitrary paths are OK as the get decomposed into a bunch of
different rects.

+    ClipValidator validator;
+    canvas->replayClips(&validator);
+    if (validator.failed()) {
+        SkDEBUGF(("CaptureCanvasState does not currently support canvases with antialiased clips"));
+        return NULL;
+    }
+
+    const SkISize canvasSize = canvas->getDeviceSize();
+    SkTScopedPtr<SkCanvasState> canvasState(SkNEW_ARGS(SkCanvasState, (canvasSize)));

[scroggo, 2013/08/28 17:22:01]

Aren't we moving away from SkTScopedPtr?

[djsollen, 2013/08/29 15:09:19]

Done.

+
+    // decompose the total matrix and clip
+    setup_MC_state(&canvasState->mcState, canvas->getTotalMatrix(), canvas->getTotalClip());
+
+    // decompose the layers
+    const int layerBufferSize = 3 * sizeof(SkCanvasLayerState);

[mtklein, 2013/08/28 18:20:46]

Remind us where 3 comes from?   Just a guess about how many layers are likely?

[djsollen, 2013/08/29 15:09:19]

Done.

+    char layerBuffer[layerBufferSize];
+    SkWriter32 layerWriter(sizeof(SkCanvasLayerState), layerBuffer, layerBufferSize);
+
+    for (SkCanvas::LayerIter layer(canvas, false); !layer.done(); layer.next()) {

[mtklein, 2013/08/28 18:20:46]

can you add an inline "false /*meaning*/" comment here?  Or name it?  Just helps
clueless guys like me read new code.

[djsollen, 2013/08/29 15:09:19]

Done.

+        // if the layer is clipped out then we can skip it
+        if (layer.clip().isEmpty()) {
+            continue;
+        }
+
+        // we currently only work for a bitmap backed devices

[scroggo, 2013/08/28 17:22:01]

nit: The word 'a' is not needed here

[djsollen, 2013/08/29 15:09:19]

Done.

+        const SkBitmap& bitmap = layer.device()->accessBitmap(true);

[mtklein, 2013/08/28 18:20:46]

Same for true?

[djsollen, 2013/08/29 15:09:19]

Done.

+        if (bitmap.empty() || bitmap.isNull() || !bitmap.lockPixelsAreWritable()) {
+            return NULL;
+        }
+
+        SkCanvasLayerState* layerState = (SkCanvasLayerState*) layerWriter.reserve(sizeof(SkCanvasLayerState));

[scroggo, 2013/08/28 17:22:01]

100 chars.

+        layerState->type = kRaster_CanvasBackend;
+        layerState->x = layer.x();
+        layerState->y = layer.y();
+        layerState->width = bitmap.width();
+        layerState->height = bitmap.height();
+
+        layerState->content.raster.config =

[scroggo, 2013/08/28 17:22:01]

nit: I find a switch statement much more readable than a cascading ? : (may just
be a personal preference though)

+          bitmap.config() == SkBitmap::kARGB_8888_Config ? kARGB_8888_RasterConfig :
+          bitmap.config() == SkBitmap::kARGB_4444_Config ? kARGB_4444_RasterConfig :

[mtklein, 2013/08/28 18:20:46]

Can't we just screw 4444 now?

+          bitmap.config() == SkBitmap::kRGB_565_Config ? kRGB_565_RasterConfig : -1;

[mtklein, 2013/08/28 18:20:46]

return NULL on the failure case?

[djsollen, 2013/08/29 15:09:19]

Done.

+        layerState->content.raster.rowBytes = bitmap.rowBytes();
+        layerState->content.raster.pixels = bitmap.getPixels();
+
+        if (layerState->content.raster.config < 0) {

[mtklein, 2013/08/28 18:20:46]

Oh.  :)  Maybe move this up with the rest of raster.config?

+          return NULL;
+        }
+
+        setup_MC_state(&layerState->mcState, layer.matrix(), layer.clip());
+        canvasState->layerCount++;
+    }
+
+    // allocate memory for the layers and then and copy them to the struct
+    canvasState->layers = (SkCanvasLayerState*) sk_malloc_throw(layerWriter.size());

[mtklein, 2013/08/28 18:20:46]

Slight paranoia... add an assert that layerWriter.size() %
sizeof(SkCanvasLayerState) == 0? 

[djsollen, 2013/08/29 15:09:19]

Done.

+    layerWriter.flatten(canvasState->layers);
+
+    // for now, just ignore any client supplied DrawFilter.
+    if (canvas->getDrawFilter()) {
+        SkDEBUGF(("CaptureCanvasState will ignore the canvas' draw filter."));
+    }
+
+    return canvasState.release();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+static void setup_canvas_from_MC_state(const SkMCState& state, SkCanvas* canvas) {
+    // reconstruct the matrix
+    SkMatrix matrix;
+    for (int i = 0; i < 9; i++) {
+        matrix.set(i, state.matrix[i]);
+    }
+
+    // reconstruct the clip
+    SkRegion clip;
+    for (int i = 0; i < state.clipRectCount; ++i) {
+        const int32_t rect_offset = i * 4;
+        clip.op(SkIRect::MakeLTRB(state.clipRects[rect_offset + 0],
+                                  state.clipRects[rect_offset + 1],
+                                  state.clipRects[rect_offset + 2],
+                                  state.clipRects[rect_offset + 3]),
+                                  SkRegion::kUnion_Op);

[mtklein, 2013/08/28 18:20:46]

dedent SkRegion::kUnion_Op to line up with SkIRect::... ?

[djsollen, 2013/08/29 15:09:19]

Done.

+    }
+
+    canvas->setMatrix(matrix);
+    canvas->setClipRegion(clip);
+}
+
+static SkCanvas* create_canvas_from_canvas_layer(const SkCanvasLayerState& layerState) {
+    SkASSERT(kRaster_CanvasBackend == layerState.type);
+
+    SkBitmap bitmap;
+    SkBitmap::Config config =
+        layerState.content.raster.config == kARGB_8888_RasterConfig ? SkBitmap::kARGB_8888_Config :
+        layerState.content.raster.config == kRGB_565_RasterConfig ? SkBitmap::kRGB_565_Config :
+        SkBitmap::kNo_Config;

[mtklein, 2013/08/28 18:20:46]

Why the asymmetry for 4444?  We let a 4444 canvas be passed across the line but
not interacted with on the other side?

[djsollen, 2013/08/29 15:09:19]

Done.

+
+    if (config == SkBitmap::kNo_Config) {
+        return NULL;
+    }
+
+    bitmap.setConfig(config, layerState.width, layerState.height,
+                     layerState.content.raster.rowBytes);
+    bitmap.setPixels(layerState.content.raster.pixels);
+
+    SkASSERT(!bitmap.empty());
+    SkASSERT(!bitmap.isNull());
+
+    // create a device & canvas
+    SkAutoTUnref<SkDevice> device(SkNEW_ARGS(SkDevice, (bitmap)));
+    SkAutoTUnref<SkCanvas> canvas(SkNEW_ARGS(SkCanvas, (device.get())));

[mtklein, 2013/08/28 18:20:46]

Given that you don't otherwise use the device, and we never don't detach canvas,
might be clearer to write as

SkCanvas* canvas = SkNEW_ARGS(SkCanvas, (SkNEW_ARGS(SkDevice, (bitmap))));
setup_canvas...(..., canvas);
return canvas;

?

[scroggo, 2013/08/28 20:42:02]

The SkCanvas constructor will ref the device, so we still need to deref it.

+
+    // setup the matrix and clip
+    setup_canvas_from_MC_state(layerState.mcState, canvas.get());
+
+    return canvas.detach();
+}
+
+SkCanvas* SkCanvasUtils::CreateFromCanvasState(SkCanvasState* state) {

[scroggo, 2013/08/28 17:22:01]

Should we check for a NULL state?

+
+    // check that the versions match
+    if (CANVAS_STATE_VERSION != state->version) {
+        SkDebugf("CreateFromCanvasState version does not match the one use to create the input");
+        return NULL;
+    }
+
+    if (state->layerCount < 1)

[scroggo, 2013/08/28 17:22:01]

Braces.

+      return NULL;

[scroggo, 2013/08/28 17:22:01]

4 space tabs

+
+    SkAutoTUnref<SkNWayCanvas> canvas(SkNEW_ARGS(SkNWayCanvas, (state->width, state->height)));
+
+    // setup the matrix and clip on the n-way canvas
+    setup_canvas_from_MC_state(state->mcState, canvas);
+
+    // Iterate over the layers and add them to the n-way canvas
+    for (int i = 0; i < state->layerCount; ++i) {
+        SkAutoTUnref<SkCanvas> canvasLayer(create_canvas_from_canvas_layer(state->layers[i]));
+        if (!canvasLayer.get()) {
+            return NULL;
+        }
+        canvas->addCanvas(canvasLayer.get());

[mtklein, 2013/08/28 18:20:46]

Would it be effectively the same to call canvasLayer.detach() here?  If so, it'd
make it clearer that we're really doing an exclusive ownership transfer.

[scroggo, 2013/08/28 20:42:02]

No, addCanvas refs it, so we want canvasLayer to get unref'ed

[djsollen, 2013/08/29 15:09:19]

the detach would run first which would push the refCnt to zero before we could
add it to the main canvas (which will increment the refCnt).

+    }
+
+    return canvas.detach();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+void SkCanvasUtils::ReleaseCanvasState(SkCanvasState* state) {
+    SkDELETE(state);
+}