Skip to content

Commit

Permalink
Hide analysis canvas from PaintOps
Browse files Browse the repository at this point in the history
Overview of changes:
- PaintOp::SerializeOptions.canvas is removed entirely.
- PaintOpBufferSerializer::Serialize() variants create a new SkCanvas at
  their start, instead of asserting the canvas was left in the initial
  state.
- The Serializer uses this canvas for text analysis and tracking of the
  total canvas matrix, but does not expose the canvas to any of the ops
  being serialized.
- PaintOp::Serialize adds a parameter for the current ctm; up to this
  point it was the only piece of information ever accessed from the
  SerializeOptions's canvas by paint ops.
- PaintOpWriter's Write() functions for PaintFlags, shaders, and filters
  add a parameter for the current ctm since these are the only types
  that need to modify what's being serialized based on the transform.

Rational:
- I wanted to improve ownership/lifetime management of the analysis
  canvas.
- I wanted to clarify what state from the canvas was important during
  serialization.
- I wanted to reduce the future chance of adding additional dependencies
  on the canvas's state beyond just the current transform.
- I wanted to ensure that PaintOps did not modify the canvas's state
  during serialization outside of the controlled playback handled by
  PaintOpBufferSerializer::PlaybackOnAnalysisCanvas.

I had put together another CL:
https://chromium-review.googlesource.com/c/chromium/src/+/2800511
that tried to achieve the same goals outlined in rational but did it
by moving the analysis canvas creation and ownership into
SerializeOptions (vs. removing it from Options entirely). This was a
much smaller change but preserves the status quo where PaintOps can
access/use the canvas during serialization, and leads to a somewhat
tricky lifetime issue where the Options has to live longer than the
Serializer using the options.

Bug: 1194701
Change-Id: I065253b9be89469f964ba6836ba6a995be7578b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2845730
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Cr-Commit-Position: refs/heads/master@{#877058}
  • Loading branch information
lhkbob authored and Chromium LUCI CQ committed Apr 28, 2021
1 parent 6d87665 commit 4a741e8
Show file tree
Hide file tree
Showing 15 changed files with 336 additions and 278 deletions.
3 changes: 1 addition & 2 deletions cc/paint/image_transfer_cache_entry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,7 @@ bool ClientImageTransferCacheEntry::Serialize(base::span<uint8_t> data) const {
DCHECK_GE(data.size(), SerializedSize());
// We don't need to populate the SerializeOptions here since the writer is
// only used for serializing primitives.
PaintOp::SerializeOptions options(nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, false, false, 0);
PaintOp::SerializeOptions options;
PaintOpWriter writer(data.data(), data.size(), options);
writer.Write(plane_config_);

Expand Down
73 changes: 51 additions & 22 deletions cc/paint/paint_op_buffer.cc

Large diffs are not rendered by default.

13 changes: 7 additions & 6 deletions cc/paint/paint_op_buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@ class CC_PAINT_EXPORT SharedImageProvider {
// don't write the 4 byte type/skip header because they don't know how much
// data they will need to write. PaintOp::Serialize itself must update it.
#define HAS_SERIALIZATION_FUNCTIONS() \
static size_t Serialize(const PaintOp* op, void* memory, size_t size, \
const SerializeOptions& options, \
const PaintFlags* flags_to_serialize, \
const SkM44& original_ctm); \
static size_t Serialize( \
const PaintOp* op, void* memory, size_t size, \
const SerializeOptions& options, const PaintFlags* flags_to_serialize, \
const SkM44& current_ctm, const SkM44& original_ctm); \
static PaintOp* Deserialize(const volatile void* input, size_t input_size, \
void* output, size_t output_size, \
const DeserializeOptions& options)
Expand Down Expand Up @@ -154,10 +154,10 @@ class CC_PAINT_EXPORT PaintOp {
bool operator!=(const PaintOp& other) const { return !(*this == other); }

struct CC_PAINT_EXPORT SerializeOptions {
SerializeOptions();
SerializeOptions(ImageProvider* image_provider,
TransferCacheSerializeHelper* transfer_cache,
ClientPaintCache* paint_cache,
SkCanvas* canvas,
SkStrikeServer* strike_server,
sk_sp<SkColorSpace> color_space,
bool can_use_lcd_text,
Expand All @@ -171,7 +171,6 @@ class CC_PAINT_EXPORT PaintOp {
ImageProvider* image_provider = nullptr;
TransferCacheSerializeHelper* transfer_cache = nullptr;
ClientPaintCache* paint_cache = nullptr;
SkCanvas* canvas = nullptr;
SkStrikeServer* strike_server = nullptr;
sk_sp<SkColorSpace> color_space = nullptr;
bool can_use_lcd_text = false;
Expand Down Expand Up @@ -219,11 +218,13 @@ class CC_PAINT_EXPORT PaintOp {
// If the op can be serialized to |memory| in no more than |size| bytes,
// then return the number of bytes written. If it won't fit, return 0.
// If |flags_to_serialize| is non-null, it overrides any flags within the op.
// |current_ctm| is the transform that will affect the op when rasterized.
// |original_ctm| is the transform that SetMatrixOps must be made relative to.
size_t Serialize(void* memory,
size_t size,
const SerializeOptions& options,
const PaintFlags* flags_to_serialize,
const SkM44& current_ctm,
const SkM44& original_ctm) const;

// Deserializes a PaintOp of this type from a given buffer |input| of
Expand Down
Loading

0 comments on commit 4a741e8

Please sign in to comment.