Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] DlCanvas implementation wrapping Aiks canvas #44248

Merged
auto-submit merged 18 commits into from
Aug 4, 2023
Merged

[Impeller] DlCanvas implementation wrapping Aiks canvas #44248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f20fee7
Create Impeller strucutres on the UI thread, avoid re-dispatching on …
dnfield Aug 2, 2023
d107a5d
missing gn dep
dnfield Aug 2, 2023
150e5c0
Fuchsia rules, lints
dnfield Aug 2, 2023
468bbb3
Re-enable transparent layer culling
dnfield Aug 2, 2023
d6de746
licenses, unnecessary headers
dnfield Aug 2, 2023
f64c2ef
guard include
dnfield Aug 2, 2023
887f134
fix condition
dnfield Aug 2, 2023
23ebd1c
tidy
dnfield Aug 2, 2023
4f8f2fa
license issue, make Fuchsia build, commented out code
dnfield Aug 2, 2023
a6f26e9
more linting and Fuchsia
dnfield Aug 2, 2023
371933c
[Impeller] Fail loudly if --enable-software-rendering is used.
dnfield Aug 3, 2023
4b42deb
Merge remote-tracking branch 'upstream/main' into software
dnfield Aug 3, 2023
8a9fa32
Fix test
dnfield Aug 3, 2023
9b51312
Merge branch 'software' into impdl
dnfield Aug 3, 2023
9a3dc6c
Skip fuchsia
dnfield Aug 3, 2023
1135df9
Merge branch 'software' into impdl
dnfield Aug 3, 2023
d34957a
Merge branch 'main' into impdl
dnfield Aug 4, 2023
3b52376
Jonah review
dnfield Aug 4, 2023
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
Prev Previous commit
Next Next commit
license issue, make Fuchsia build, commented out code
  • Loading branch information
@dnfield
dnfield committed Aug 2, 2023
commit 4f8f2fa9b9c120a90a24d236f796698ec4e9def5
4 changes: 2 additions & 2 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3437,6 +3437,8 @@ FILE: ../../../flutter/display_list/dl_op_receiver.cc
FILE: ../../../flutter/display_list/dl_op_receiver.h
FILE: ../../../flutter/display_list/dl_op_records.cc
FILE: ../../../flutter/display_list/dl_op_records.h
FILE: ../../../flutter/display_list/dl_op_spy.cc
FILE: ../../../flutter/display_list/dl_op_spy.h
FILE: ../../../flutter/display_list/dl_paint.cc
FILE: ../../../flutter/display_list/dl_paint.h
FILE: ../../../flutter/display_list/dl_sampling_options.h
Expand Down Expand Up @@ -4920,8 +4922,6 @@ FILE: ../../../flutter/shell/common/display.cc
FILE: ../../../flutter/shell/common/display.h
FILE: ../../../flutter/shell/common/display_manager.cc
FILE: ../../../flutter/shell/common/display_manager.h
FILE: ../../../flutter/shell/common/dl_op_spy.cc
FILE: ../../../flutter/shell/common/dl_op_spy.h
FILE: ../../../flutter/shell/common/engine.cc
FILE: ../../../flutter/shell/common/engine.h
FILE: ../../../flutter/shell/common/pipeline.cc
Expand Down
5 changes: 0 additions & 5 deletions display_list/dl_canvas.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,6 @@ class DlCanvas {
virtual void DrawImpellerPicture(
const std::shared_ptr<const impeller::Picture>& picture,
SkScalar opacity = SK_Scalar1) = 0;
// {
// FML_LOG(ERROR) << "Attempted to draw an Impeller picture into a "
// "non-Impeller enabled canvas.";
// FML_DCHECK(false);
// }
virtual void DrawTextBlob(const sk_sp<SkTextBlob>& blob,
SkScalar x,
SkScalar y,
Expand Down
6 changes: 2 additions & 4 deletions flow/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ source_set("flow") {
"instrumentation.h",
"layer_snapshot_store.cc",
"layer_snapshot_store.h",
"layers/aiks_layer.cc",
"layers/aiks_layer.h",
"layers/backdrop_filter_layer.cc",
"layers/backdrop_filter_layer.h",
"layers/cacheable_layer.cc",
Expand Down Expand Up @@ -95,10 +97,6 @@ source_set("flow") {
deps = [ "//third_party/skia" ]

if (impeller_supports_rendering) {
sources += [
"layers/aiks_layer.cc",
"layers/aiks_layer.h",
]
deps += [ "//flutter/impeller" ]
}
}
Expand Down
2 changes: 2 additions & 0 deletions flow/embedded_views.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

namespace flutter {

#if IMPELLER_SUPPORTS_RENDERING
ImpellerEmbedderViewSlice::ImpellerEmbedderViewSlice(SkRect view_bounds) {
canvas_ = std::make_unique<impeller::DlAiksCanvas>(
/*bounds=*/view_bounds);
Expand Down Expand Up @@ -39,6 +40,7 @@ bool ImpellerEmbedderViewSlice::recording_ended() {
bool ImpellerEmbedderViewSlice::renders_anything() {
return !picture_->rtree->bounds().isEmpty();
}
#endif // IMPELLER_SUPPORTS_RENDERING

DisplayListEmbedderViewSlice::DisplayListEmbedderViewSlice(SkRect view_bounds) {
builder_ = std::make_unique<DisplayListBuilder>(
Expand Down
2 changes: 2 additions & 0 deletions flow/embedded_views.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ class EmbedderViewSlice {
virtual bool renders_anything() = 0;
};

#if IMPELLER_SUPPORTS_RENDERING
class ImpellerEmbedderViewSlice : public EmbedderViewSlice {
public:
explicit ImpellerEmbedderViewSlice(SkRect view_bounds);
Expand All @@ -361,6 +362,7 @@ class ImpellerEmbedderViewSlice : public EmbedderViewSlice {
std::unique_ptr<impeller::DlAiksCanvas> canvas_;
std::shared_ptr<const impeller::Picture> picture_;
};
#endif

class DisplayListEmbedderViewSlice : public EmbedderViewSlice {
public:
Expand Down
2 changes: 2 additions & 0 deletions flow/layers/aiks_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ AiksLayer::AiksLayer(const SkPoint& offset,
bool is_complex,
bool will_change)
: offset_(offset), picture_(picture) {
#if IMPELLER_SUPPORTS_RENDERING
if (picture_) {
auto bounds = picture_->pass->GetElementsCoverage(std::nullopt)
.value_or(impeller::Rect());
bounds_ = SkRect::MakeXYWH(bounds.origin.x + offset_.x(),
bounds.origin.y + offset.y(), bounds.size.width,
bounds.size.height);
}
#endif
}
Copy link
Contributor

@flar flar Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There is no override for IsReplacing. I forget the details, but sometimes a new Layer is constructed with the same ui.Picture as was used in the previous frame and so we need to check if the pictures are the same, not just the layer, otherwise we lose a lot of partial repaint reduction.

Copy link
Member

@knopp knopp Aug 4, 2023

Choose a reason for hiding this comment

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

Indeed. The way it is now partial repaint will not work correctly with AiksLayer. With DisplayListLayer (and similarly AiksLayer) the diffing is moved one level up - to ContainerLayer. The DisplayListLayer::Diff basically just reports the layer paint region.

The reason for this is that we want to be able to detect added / removed display list layers similar to how the framework diffs element children. (See ContainerLayer::DiffChildren).

Copy link
Member

@knopp knopp Aug 4, 2023

Choose a reason for hiding this comment

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

The reason why picture layers are different than other layers in this regard is that picture layers don't exit in framework in a similar way other layers exist - so we don't get the old layer to link with the new layer when adding picture like we would get with a backdrop filter or a clip rect layer for example. So we relay on comparing the contents.


void AiksLayer::Diff(DiffContext* context, const Layer* old_layer) {
Expand Down
9 changes: 8 additions & 1 deletion flow/layers/aiks_layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,14 @@
#include <memory>

#include "flutter/flow/layers/layer.h"
#include "impeller/aiks/picture.h"

#if IMPELLER_SUPPORTS_RENDERING
#include "impeller/aiks/picture.h" // nogncheck
#else // IMPELLER_SUPPORTS_RENDERING
namespace impeller {
struct Picture;
} // namespace impeller
#endif // !IMPELLER_SUPPORTS_RENDERING

namespace flutter {

Expand Down
4 changes: 4 additions & 0 deletions flow/layers/layer_tree.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ void LayerTree::Paint(CompositorContext::ScopedFrame& frame,
std::shared_ptr<const impeller::Picture> LayerTree::FlattenToImpellerPicture(
const SkRect& bounds,
const std::shared_ptr<TextureRegistry>& texture_registry) {
#if IMPELLER_SUPPORTS_RENDERING
TRACE_EVENT0("flutter", "LayerTree::FlattenToImpellerPicture");

impeller::DlAiksCanvas canvas(bounds);
Expand Down Expand Up @@ -212,6 +213,9 @@ std::shared_ptr<const impeller::Picture> LayerTree::FlattenToImpellerPicture(

return std::make_shared<const impeller::Picture>(
canvas.EndRecordingAsPicture());
#else // IMPELLER_SUPPORTS_RENDERING
return nullptr;
#endif // !IMPELLER_SUPPORTS_RENDERING
}

sk_sp<DisplayList> LayerTree::Flatten(
Expand Down
9 changes: 9 additions & 0 deletions flow/surface_frame.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,13 @@ SurfaceFrame::SurfaceFrame(sk_sp<SkSurface> surface,
canvas_ = &adapter_;
} else if (display_list_fallback) {
FML_DCHECK(!frame_size.isEmpty());
#if IMPELLER_SUPPORTS_RENDERING
Copy link
Contributor

@flar flar Aug 4, 2023

Choose a reason for hiding this comment

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

We should perhaps rename this flag if it really means "rendering_to_impeller" otherwise you are creating an impeller picture for a flag that is named "please create a display list"...

Copy link
Contributor

@flar flar Aug 4, 2023

Choose a reason for hiding this comment

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

This is also an odd combination of tests that implies that a given call can contain either or both of a surface and/or the flag. If it's "one or the other", then delete the flag and just assume it if there is no surface. If it is both, we aren't implementing it very well. If there are cases when neither is provided - why is that?

aiks_canvas_ =
std::make_shared<impeller::DlAiksCanvas>(SkRect::Make(frame_size));
canvas_ = aiks_canvas_.get();
#else
FML_DCHECK(false);
#endif // IMPELLER_SUPPORTS_RENDERING
}
}

Expand Down Expand Up @@ -74,8 +78,13 @@ bool SurfaceFrame::PerformSubmit() {
}

std::shared_ptr<const impeller::Picture> SurfaceFrame::GetImpellerPicture() {
#if IMPELLER_SUPPORTS_RENDERING
return std::make_shared<impeller::Picture>(
aiks_canvas_->EndRecordingAsPicture());
#else
FML_DCHECK(false);
return nullptr;
#endif // IMPELLER_SUPPORTS_RENDERING
}

} // namespace flutter
8 changes: 5 additions & 3 deletions impeller/tools/impeller.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ declare_args() {
flutter_runtime_mode == "debug" || flutter_runtime_mode == "profile"

# Whether the Metal backend is enabled.
impeller_enable_metal = is_mac || is_ios
impeller_enable_metal = (is_mac || is_ios) && target_os != "fuchsia"

# Whether the OpenGLES backend is enabled.
impeller_enable_opengles = is_linux || is_win || is_android
impeller_enable_opengles =
(is_linux || is_win || is_android) && target_os != "fuchsia"

# Whether the Vulkan backend is enabled.
impeller_enable_vulkan = is_linux || is_win || is_android
impeller_enable_vulkan =
(is_linux || is_win || is_android) && target_os != "fuchsia"

Comment on lines +15 to 24
Copy link
Contributor

@jonahwilliams jonahwilliams Aug 4, 2023

Choose a reason for hiding this comment

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

Was this necessary? I don't remember where we landed on the embedder.h issue with fuchsia

Copy link
Contributor Author

@dnfield dnfield Aug 4, 2023

Choose a reason for hiding this comment

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

If I don't do this, there are host artifacts built for linux or mac when building Fuchsia where the target_os is Fuchsia, and enable_impeller ends up being true in places where it really shouldn't be.

I'd be happy to separate this into another patch if it helps.

Copy link
Contributor

@jonahwilliams jonahwilliams Aug 4, 2023

Choose a reason for hiding this comment

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

Nah thats fine.

# Whether to use a prebuilt impellerc.
# If this is the empty string, impellerc will be built.
Expand Down
3 changes: 1 addition & 2 deletions lib/ui/compositing/scene_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "flutter/lib/ui/compositing/scene_builder.h"

#include "flutter/flow/layers/aiks_layer.h"
#include "flutter/flow/layers/backdrop_filter_layer.h"
#include "flutter/flow/layers/clip_path_layer.h"
#include "flutter/flow/layers/clip_rect_layer.h"
Expand All @@ -29,8 +30,6 @@
#include "third_party/tonic/dart_binding_macros.h"
#include "third_party/tonic/dart_library_natives.h"

#include "flutter/flow/layers/aiks_layer.h"

namespace flutter {

IMPLEMENT_WRAPPERTYPEINFO(ui, SceneBuilder);
Expand Down
9 changes: 8 additions & 1 deletion lib/ui/painting/picture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@
#include "flutter/lib/ui/dart_wrapper.h"
#include "flutter/lib/ui/painting/image.h"
#include "flutter/lib/ui/ui_dart_state.h"
#include "impeller/aiks/picture.h" // nogncheck
#include "third_party/skia/include/core/SkPicture.h"

#if IMPELLER_SUPPORTS_RENDERING
#include "impeller/aiks/picture.h" // nogncheck
#else // IMPELLER_SUPPORTS_RENDERING
namespace impeller {
struct Picture;
} // namespace impeller
#endif // !IMPELLER_SUPPORTS_RENDERING

namespace flutter {
class Canvas;

Expand Down
7 changes: 7 additions & 0 deletions lib/ui/painting/picture_recorder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@

#include "flutter/display_list/dl_builder.h"
#include "flutter/lib/ui/dart_wrapper.h"

#if IMPELLER_SUPPORTS_RENDERING
#include "impeller/display_list/dl_aiks_canvas.h" // nogncheck
#else // IMPELLER_SUPPORTS_RENDERING
namespace impeller {
class DlAiksCanvas;
} // namespace impeller
#endif // !IMPELLER_SUPPORTS_RENDERING

namespace flutter {
class Canvas;
Expand Down
4 changes: 4 additions & 0 deletions shell/platform/embedder/embedder_external_view.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,17 @@ EmbedderExternalView::EmbedderExternalView(
surface_transformation_(surface_transformation),
view_identifier_(view_identifier),
embedded_view_params_(std::move(params)) {
#if IMPELLER_SUPPORTS_RENDERING
if (enable_impeller) {
slice_ =
std::make_unique<ImpellerEmbedderViewSlice>(SkRect::Make(frame_size));
} else {
#endif // IMPELLER_SUPPORTS_RENDERING
slice_ = std::make_unique<DisplayListEmbedderViewSlice>(
SkRect::Make(frame_size));
#if IMPELLER_SUPPORTS_RENDERING
}
#endif // IMPELLER_SUPPORTS_RENDERING
}

EmbedderExternalView::~EmbedderExternalView() = default;
Expand Down