Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions flow/display_list.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "third_party/skia/include/core/SkPathEffect.h"
#include "third_party/skia/include/core/SkPicture.h"
#include "third_party/skia/include/core/SkShader.h"
#include "third_party/skia/include/core/SkTextBlob.h"
#include "third_party/skia/include/core/SkVertices.h"

// The Flutter DisplayList mechanism encapsulates a persistent sequence of
Expand Down
23 changes: 23 additions & 0 deletions flow/display_list_canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,34 @@ void DisplayListCanvasDispatcher::drawAtlas(const sk_sp<SkImage> atlas,
void DisplayListCanvasDispatcher::drawPicture(const sk_sp<SkPicture> picture,
const SkMatrix* matrix,
bool render_with_attributes) {
if (cache_ && !render_with_attributes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I understanding this correctly? It looks like this is trying to raster cache sub-pictures every time. That will be expensive on memory and processing power...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On what basis do you make that claim?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see now. I was misreading this and thought you were skipping a call to Prepare. I was confused about which class/method was doing what. I can see now the logic I was looking for is in https://github.com/flutter/engine/pull/29155/files#diff-3854149b0d2e47aedb825a57720ae5dbd70e7b0ab78a460bb8b31d0c4f8211dcR147

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, this is just setting up a new scan of the DL ops with the same logic applied to the primary DL/Picture.

SkAutoCanvasRestore save(canvas_, true);
if (matrix) {
canvas_->concat(*matrix);
}
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
canvas_->setMatrix(
RasterCache::GetIntegralTransCTM(canvas_->getTotalMatrix()));
#endif
if (cache_->Draw(*picture, *canvas_)) {
return;
}
}
canvas_->drawPicture(picture, matrix,
render_with_attributes ? &paint() : nullptr);
}
void DisplayListCanvasDispatcher::drawDisplayList(
const sk_sp<DisplayList> display_list) {
if (cache_) {
SkAutoCanvasRestore save(canvas_, true);
#ifndef SUPPORT_FRACTIONAL_TRANSLATION
canvas_->setMatrix(
RasterCache::GetIntegralTransCTM(canvas_->getTotalMatrix()));
#endif
if (cache_->Draw(*display_list, *canvas_)) {
return;
}
}
int save_count = canvas_->save();
{
DisplayListCanvasDispatcher dispatcher(canvas_);
Expand Down
6 changes: 5 additions & 1 deletion flow/display_list_canvas.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include "flutter/flow/display_list.h"
#include "flutter/flow/display_list_utils.h"
#include "flutter/flow/raster_cache.h"
#include "flutter/fml/logging.h"

#include "third_party/skia/include/core/SkCanvasVirtualEnforcer.h"
Expand All @@ -27,7 +28,9 @@ namespace flutter {
class DisplayListCanvasDispatcher : public virtual Dispatcher,
public SkPaintDispatchHelper {
public:
DisplayListCanvasDispatcher(SkCanvas* canvas) : canvas_(canvas) {}
DisplayListCanvasDispatcher(SkCanvas* canvas,
const RasterCache* cache = nullptr)
: canvas_(canvas), cache_(cache) {}

void save() override;
void restore() override;
Expand Down Expand Up @@ -115,6 +118,7 @@ class DisplayListCanvasDispatcher : public virtual Dispatcher,

private:
SkCanvas* canvas_;
const RasterCache* cache_;
};

// Receives all methods on SkCanvas and sends them to a DisplayListBuilder
Expand Down
63 changes: 63 additions & 0 deletions flow/display_list_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
// IgnoreAttributeDispatchHelper:
// IgnoreClipDispatchHelper:
// IgnoreTransformDispatchHelper
// IgnoreRenderingDispatchHelper
// Empty overrides of all of the associated methods of Dispatcher
// for dispatchers that only track some of the rendering operations
//
Expand Down Expand Up @@ -91,6 +92,68 @@ class IgnoreTransformDispatchHelper : public virtual Dispatcher {
// clang-format on
};

class IgnoreRenderingDispatchHelper : public virtual Dispatcher {
void drawColor(SkColor color, SkBlendMode mode) override {}
void drawPaint() override {}
void drawLine(const SkPoint& p0, const SkPoint& p1) override {}
void drawRect(const SkRect& rect) override {}
void drawOval(const SkRect& bounds) override {}
void drawCircle(const SkPoint& center, SkScalar radius) override {}
void drawRRect(const SkRRect& rrect) override {}
void drawDRRect(const SkRRect& outer, const SkRRect& inner) override {}
void drawPath(const SkPath& path) override {}
void drawArc(const SkRect& oval_bounds,
SkScalar start_degrees,
SkScalar sweep_degrees,
bool use_center) override {}
void drawPoints(SkCanvas::PointMode mode,
uint32_t count,
const SkPoint points[]) override {}
void drawVertices(const sk_sp<SkVertices> vertices,
SkBlendMode mode) override {}
void drawImage(const sk_sp<SkImage> image,
const SkPoint point,
const SkSamplingOptions& sampling,
bool render_with_attributes) override {}
void drawImageRect(const sk_sp<SkImage> image,
const SkRect& src,
const SkRect& dst,
const SkSamplingOptions& sampling,
bool render_with_attributes,
SkCanvas::SrcRectConstraint constraint) override {}
void drawImageNine(const sk_sp<SkImage> image,
const SkIRect& center,
const SkRect& dst,
SkFilterMode filter,
bool render_with_attributes) override {}
void drawImageLattice(const sk_sp<SkImage> image,
const SkCanvas::Lattice& lattice,
const SkRect& dst,
SkFilterMode filter,
bool render_with_attributes) override {}
void drawAtlas(const sk_sp<SkImage> atlas,
const SkRSXform xform[],
const SkRect tex[],
const SkColor colors[],
int count,
SkBlendMode mode,
const SkSamplingOptions& sampling,
const SkRect* cull_rect,
bool render_with_attributes) override {}
void drawPicture(const sk_sp<SkPicture> picture,
const SkMatrix* matrix,
bool render_with_attributes) override {}
void drawDisplayList(const sk_sp<DisplayList> display_list) override {}
void drawTextBlob(const sk_sp<SkTextBlob> blob,
SkScalar x,
SkScalar y) override {}
void drawShadow(const SkPath& path,
const SkColor color,
const SkScalar elevation,
bool transparent_occluder,
SkScalar dpr) override {}
};

// A utility class that will monitor the Dispatcher methods relating
// to the rendering attributes and accumulate them into an SkPaint
// which can be accessed at any time via paint().
Expand Down
34 changes: 33 additions & 1 deletion flow/layers/display_list_layer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ void DisplayListLayer::Preroll(PrerollContext* context,
TRACE_EVENT0("flutter", "DisplayListLayer::RasterCache (Preroll)");
cache->Prepare(context, disp_list, is_complex_, will_change_, matrix,
offset_);
if (!matrix.hasPerspective()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no.

Technically we probably shouldn't be doing any caching under perspective transforms. We've been living with it for a while for picture layers and display list layers and I haven't seen any developers note that it is causing problems, mainly because really who does a lot of perspective transforms in UI code. Without a specific test case I don't want to go modifying that code path just yet.

For this new code that test is partially "aiming for more correctness" in that regard, but also in a practical matter the setup for a perspective matrix (working under the naive assumption that it might be worthwhile) would be more involved. So, to simplify the new code by avoiding a more complicated setup that would likely end in tears, I added this check to avoid the recursion and bad output that might ensue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this into a separate PR? It seems like it's worth doing, but it should be landed separately to make bisecting easier, and to avoid needing to revert this change for an unrelated reason.

Copy link
Contributor Author

@flar flar Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should make a new Issue for it, but if I remove it from this code, this code itself gets more complicated for a reason that will likely prove unfeasible.

I can add to the issue something along the lines of "if it turns out we can cache under perspective transforms, then the following code needs to remove its test and adjust".

DisplayListCacheDispatcher dispatcher(context, cache);
dispatcher.transform2DAffine(matrix[0], matrix[1], matrix[2], //
matrix[3], matrix[4], matrix[5]);
disp_list->Dispatch(dispatcher);
}
}

SkRect bounds = disp_list->bounds().makeOffset(offset_.x(), offset_.y());
Expand All @@ -121,7 +127,33 @@ void DisplayListLayer::Paint(PaintContext& context) const {
return;
}

display_list()->RenderTo(context.leaf_nodes_canvas);
DisplayListCanvasDispatcher dispatcher(context.leaf_nodes_canvas,
context.raster_cache);
display_list()->Dispatch(dispatcher);
}

void DisplayListCacheDispatcher::drawPicture(const sk_sp<SkPicture> picture,
const SkMatrix* picture_matrix,
bool render_with_attributes) {
if (render_with_attributes) {
return;
}
TRACE_EVENT0("flutter",
"DisplayListLayer::EmbeddedPicture::RasterCache (Preroll)");
SkMatrix cache_matrix = matrix();
if (picture_matrix != nullptr) {
cache_matrix.preConcat(*picture_matrix);
}
cache_->Prepare(context_, picture.get(), false, false, cache_matrix,
SkPoint::Make(0, 0));
}

void DisplayListCacheDispatcher::drawDisplayList(
const sk_sp<DisplayList> display_list) {
TRACE_EVENT0("flutter",
"DisplayListLayer::EmbeddedDisplayList::RasterCache (Preroll)");
cache_->Prepare(context_, display_list.get(), false, false, matrix(),
SkPoint::Make(0, 0));
}

} // namespace flutter
26 changes: 26 additions & 0 deletions flow/layers/display_list_layer.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,37 @@
#define FLUTTER_FLOW_LAYERS_DISPLAY_LIST_LAYER_H_

#include "flutter/flow/display_list.h"
#include "flutter/flow/display_list_utils.h"
#include "flutter/flow/layers/layer.h"
#include "flutter/flow/skia_gpu_object.h"

namespace flutter {

class DisplayListCacheDispatcher : public virtual Dispatcher,
public SkMatrixDispatchHelper,
public IgnoreAttributeDispatchHelper,
public IgnoreClipDispatchHelper,
public IgnoreRenderingDispatchHelper {
public:
DisplayListCacheDispatcher(PrerollContext* context, RasterCache* cache)
: context_(context), cache_(cache) {}

void save() override { SkMatrixDispatchHelper::save(); }
void saveLayer(const SkRect* bounds, bool restore_with_paint) override {
SkMatrixDispatchHelper::save();
}
void restore() override { SkMatrixDispatchHelper::restore(); }

void drawPicture(const sk_sp<SkPicture> picture,
const SkMatrix* matrix,
bool render_with_attributes) override;
void drawDisplayList(const sk_sp<DisplayList> display_list) override;

private:
PrerollContext* context_;
RasterCache* cache_;
};

class DisplayListLayer : public Layer {
public:
static constexpr size_t kMaxBytesToCompare = 10000;
Expand Down