Skip to content

Commit

Permalink
Revert "SceneBuilder.addPicture returns the layer (flutter#26074)" (f…
Browse files Browse the repository at this point in the history
  • Loading branch information
dnfield authored May 13, 2021
1 parent 247d1d9 commit 7e34f1f
Show file tree
Hide file tree
Showing 11 changed files with 30 additions and 96 deletions.
28 changes: 4 additions & 24 deletions lib/ui/compositing.dart
Original file line number Diff line number Diff line change
Expand Up @@ -188,15 +188,6 @@ class PhysicalShapeEngineLayer extends _EngineLayerWrapper {
PhysicalShapeEngineLayer._(EngineLayer nativeLayer) : super._(nativeLayer);
}

/// An opaque handle to a picture engine layer.
///
/// Instances of this class are created by [SceneBuilder.addPicture].
///
/// {@macro dart.ui.sceneBuilder.oldLayerCompatibility}
class PictureEngineLayer extends _EngineLayerWrapper {
PictureEngineLayer._(EngineLayer nativeLayer) : super._(nativeLayer);
}

/// Builds a [Scene] containing the given visuals.
///
/// A [Scene] can then be rendered using [FlutterView.render].
Expand Down Expand Up @@ -708,29 +699,18 @@ class SceneBuilder extends NativeFieldWrapperClass2 {
/// Adds a [Picture] to the scene.
///
/// The picture is rasterized at the given offset.
///
/// {@macro dart.ui.sceneBuilder.oldLayer}
PictureEngineLayer addPicture(
void addPicture(
Offset offset,
Picture picture, {
bool isComplexHint = false,
bool willChangeHint = false,
PictureEngineLayer? oldLayer,
}) {
assert(_debugCheckCanBeUsedAsOldLayer(oldLayer, 'addPicture'));
final EngineLayer engineLayer = EngineLayer._();
final int hints = (isComplexHint ? 1 : 0) | (willChangeHint ? 2 : 0);
_addPicture(engineLayer, offset.dx, offset.dy, picture, hints, oldLayer?._nativeLayer);
return PictureEngineLayer._(engineLayer);
_addPicture(offset.dx, offset.dy, picture, hints);
}

void _addPicture(
EngineLayer outEngineLayer,
double dx,
double dy,
Picture picture,
int hints,
EngineLayer? oldLayer) native 'SceneBuilder_addPicture';
void _addPicture(double dx, double dy, Picture picture, int hints)
native 'SceneBuilder_addPicture';

/// Adds a backend texture to the scene.
///
Expand Down
14 changes: 4 additions & 10 deletions lib/ui/compositing/scene_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -272,20 +272,14 @@ void SceneBuilder::pop() {
PopLayer();
}

void SceneBuilder::addPicture(Dart_Handle layer_handle,
double dx,
void SceneBuilder::addPicture(double dx,
double dy,
Picture* picture,
int hints,
fml::RefPtr<EngineLayer> oldLayer) {
auto layer = std::make_shared<flutter::PictureLayer>(
int hints) {
auto layer = std::make_unique<flutter::PictureLayer>(
SkPoint::Make(dx, dy), UIDartState::CreateGPUObject(picture->picture()),
!!(hints & 1), !!(hints & 2));
AddLayer(layer);
EngineLayer::MakeRetained(layer_handle, layer);
if (oldLayer && oldLayer->Layer()) {
layer->AssignOldLayer(oldLayer->Layer().get());
}
AddLayer(std::move(layer));
}

void SceneBuilder::addTexture(double dx,
Expand Down
7 changes: 1 addition & 6 deletions lib/ui/compositing/scene_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,7 @@ class SceneBuilder : public RefCountedDartWrappable<SceneBuilder> {
double top,
double bottom);

void addPicture(Dart_Handle layer_handle,
double dx,
double dy,
Picture* picture,
int hints,
fml::RefPtr<EngineLayer> oldLayer);
void addPicture(double dx, double dy, Picture* picture, int hints);

void addTexture(double dx,
double dy,
Expand Down
5 changes: 2 additions & 3 deletions lib/ui/painting/engine_layer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

#include "flutter/lib/ui/painting/engine_layer.h"

#include "flutter/lib/ui/ui_dart_state.h"
#include "third_party/tonic/converter/dart_converter.h"
#include "third_party/tonic/dart_args.h"
#include "third_party/tonic/dart_binding_macros.h"
Expand All @@ -14,14 +13,14 @@ using tonic::ToDart;

namespace flutter {

EngineLayer::EngineLayer(std::shared_ptr<flutter::Layer> layer)
EngineLayer::EngineLayer(std::shared_ptr<flutter::ContainerLayer> layer)
: layer_(layer) {}

EngineLayer::~EngineLayer() = default;

size_t EngineLayer::GetAllocationSize() const {
// Provide an approximation of the total memory impact of this object to the
// Dart GC. The Layer may hold references to a tree of other layers,
// Dart GC. The ContainerLayer may hold references to a tree of other layers,
// which in turn may contain Skia objects.
return 3000;
};
Expand Down
14 changes: 6 additions & 8 deletions lib/ui/painting/engine_layer.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,19 @@ class EngineLayer : public RefCountedDartWrappable<EngineLayer> {

size_t GetAllocationSize() const override;

static fml::RefPtr<EngineLayer> MakeRetained(
Dart_Handle dart_handle,
std::shared_ptr<flutter::Layer> layer) {
auto engine_layer = fml::MakeRefCounted<EngineLayer>(std::move(layer));
static void MakeRetained(Dart_Handle dart_handle,
std::shared_ptr<flutter::ContainerLayer> layer) {
auto engine_layer = fml::MakeRefCounted<EngineLayer>(layer);
engine_layer->AssociateWithDartWrapper(dart_handle);
return engine_layer;
}

static void RegisterNatives(tonic::DartLibraryNatives* natives);

std::shared_ptr<flutter::Layer> Layer() const { return layer_; }
std::shared_ptr<flutter::ContainerLayer> Layer() const { return layer_; }

private:
explicit EngineLayer(std::shared_ptr<flutter::Layer> layer);
std::shared_ptr<flutter::Layer> layer_;
explicit EngineLayer(std::shared_ptr<flutter::ContainerLayer> layer);
std::shared_ptr<flutter::ContainerLayer> layer_;

FML_FRIEND_MAKE_REF_COUNTED(EngineLayer);
};
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/engine/canvaskit/layer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ class ShaderMaskEngineLayer extends ContainerLayer implements ui.ShaderMaskEngin
}

/// A layer containing a [Picture].
class PictureLayer extends Layer implements ui.PictureEngineLayer {
class PictureLayer extends Layer {
/// The picture to paint into the canvas.
final CkPicture picture;

Expand Down
13 changes: 3 additions & 10 deletions lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -45,21 +45,14 @@ class LayerSceneBuilder implements ui.SceneBuilder {
}

@override
ui.PictureEngineLayer addPicture(
void addPicture(
ui.Offset offset,
ui.Picture picture, {
bool isComplexHint = false,
bool willChangeHint = false,
ui.PictureEngineLayer? oldLayer,
}) {
final PictureLayer layer = PictureLayer(
picture as CkPicture,
offset,
isComplexHint,
willChangeHint,
);
currentLayer.add(layer);
return layer;
currentLayer.add(PictureLayer(
picture as CkPicture, offset, isComplexHint, willChangeHint));
}

@override
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/engine/html/picture.dart
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ void _recycleCanvas(EngineCanvas? canvas) {

/// A surface that uses a combination of `<canvas>`, `<div>` and `<p>` elements
/// to draw shapes and text.
class PersistedPicture extends PersistedLeafSurface implements ui.PictureEngineLayer {
class PersistedPicture extends PersistedLeafSurface {
PersistedPicture(this.dx, this.dy, this.picture, this.hints)
: localPaintBounds = picture.recordingCanvas!.pictureBounds;

Expand Down
13 changes: 3 additions & 10 deletions lib/web_ui/lib/src/engine/html/scene_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -359,12 +359,11 @@ class SurfaceSceneBuilder implements ui.SceneBuilder {
///
/// The picture is rasterized at the given offset.
@override
ui.PictureEngineLayer addPicture(
void addPicture(
ui.Offset offset,
ui.Picture picture, {
bool isComplexHint = false,
bool willChangeHint = false,
ui.PictureEngineLayer? oldLayer,
}) {
int hints = 0;
if (isComplexHint) {
Expand All @@ -373,14 +372,8 @@ class SurfaceSceneBuilder implements ui.SceneBuilder {
if (willChangeHint) {
hints |= 2;
}
final PersistedPicture layer = PersistedPicture(
offset.dx,
offset.dy,
picture as EnginePicture,
hints,
);
_addSurface(layer);
return layer;
_addSurface(PersistedPicture(
offset.dx, offset.dy, picture as EnginePicture, hints));
}

/// Adds a backend texture to the scene.
Expand Down
7 changes: 1 addition & 6 deletions lib/web_ui/lib/src/ui/compositing.dart
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ abstract class ShaderMaskEngineLayer implements EngineLayer {}

abstract class PhysicalShapeEngineLayer implements EngineLayer {}

// TODO(yjbanov): Incorporate into DOM diffing algorithm.
// https://github.com/flutter/flutter/issues/82287
abstract class PictureEngineLayer implements EngineLayer {}

abstract class SceneBuilder {
factory SceneBuilder() {
if (engine.useCanvasKit) {
Expand Down Expand Up @@ -103,12 +99,11 @@ abstract class SceneBuilder {
void addRetained(EngineLayer retainedLayer);
void pop();
void addPerformanceOverlay(int enabledOptions, Rect bounds);
PictureEngineLayer addPicture(
void addPicture(
Offset offset,
Picture picture, {
bool isComplexHint = false,
bool willChangeHint = false,
PictureEngineLayer? oldLayer,
});
void addTexture(
int textureId, {
Expand Down
21 changes: 4 additions & 17 deletions testing/dart/compositing_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -279,18 +279,16 @@ void main() {
builder2.build();
}

void testNoSharing(_TestNoSharingFunction pushFunction, {bool isLeafLayer = false}) {
void testNoSharing(_TestNoSharingFunction pushFunction) {
testPushThenIllegalRetain(pushFunction);
testAddRetainedThenIllegalPush(pushFunction);
testDoubleAddRetained(pushFunction);
testPushOldLayerTwice(pushFunction);
testPushChildLayerOfRetainedLayer(pushFunction);
testRetainParentLayerOfPushedChild(pushFunction);
testRetainOldLayer(pushFunction);
testPushOldLayer(pushFunction);
if (!isLeafLayer) {
testPushChildLayerOfRetainedLayer(pushFunction);
testRetainParentLayerOfPushedChild(pushFunction);
testRetainsParentOfOldLayer(pushFunction);
}
testRetainsParentOfOldLayer(pushFunction);
}

test('SceneBuilder does not share a layer between addRetained and push*', () {
Expand Down Expand Up @@ -379,17 +377,6 @@ void main() {
oldLayer: oldLayer as ImageFilterEngineLayer,
);
});
testNoSharing((SceneBuilder builder, EngineLayer oldLayer) {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.drawPaint(Paint());
final Picture picture = recorder.endRecording();
return builder.addPicture(
Offset.zero,
picture,
oldLayer: oldLayer as PictureEngineLayer,
);
}, isLeafLayer: true);
});
}

Expand Down

0 comments on commit 7e34f1f

Please sign in to comment.