Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
449fce2
[Impeller] partial repaint for Impeller/iOS.
Apr 6, 2023
ee5b155
Merge branch 'master' of github.com:flutter/engine into partial_repai…
Apr 8, 2023
c0c227b
use blit to preserve previous frame contents
Apr 11, 2023
18fc13e
Merge branch 'master' of github.com:flutter/engine into partial_repai…
Apr 17, 2023
4f96864
temp disable playground
Apr 17, 2023
5477505
++
Apr 18, 2023
503c7e1
++
Apr 18, 2023
3488b59
++
Apr 18, 2023
3732339
Merge branch 'master' of github.com:flutter/engine into partial_repai…
Apr 18, 2023
5081bb7
tidy
Apr 18, 2023
b48d948
Merge branch 'main' into partial_repaint_impeller_2
Apr 19, 2023
fc3ef29
remove debug feature
Apr 19, 2023
00dcdba
++
Apr 19, 2023
04c79db
Merge branch 'master' of github.com:flutter/engine into partial_repai…
Apr 19, 2023
a76d650
++
Apr 19, 2023
2bde543
++
Apr 19, 2023
bff3803
Merge branch 'master' of github.com:flutter/engine into partial_repai…
Apr 21, 2023
a1c4caa
remove separate clip rect field
Apr 21, 2023
ef33292
Merge branch 'master' of github.com:flutter/engine into partial_repai…
Apr 21, 2023
034036f
Merge branch 'master' of github.com:flutter/engine into partial_repai…
Apr 24, 2023
353bd2f
null check frame damage
Apr 24, 2023
38ef3db
Merge branch 'main' into partial_repaint_impeller_2
Apr 24, 2023
c701ac0
add comment on why frame damage is not reset
Apr 25, 2023
4055369
Merge branch 'partial_repaint_impeller_2' of github.com:jonahwilliams…
Apr 25, 2023
4fb6a98
Merge branch 'master' of github.com:flutter/engine into partial_repai…
Apr 26, 2023
4c84d0f
++
Apr 26, 2023
1a0de9a
++
Apr 26, 2023
b91f739
tidy
Apr 26, 2023
0fdbd2c
dnfield review
Apr 26, 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
76 changes: 66 additions & 10 deletions flow/compositor_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,8 @@ std::optional<SkRect> FrameDamage::ComputeClipRect(
context.ComputeDamage(additional_damage_, horizontal_clip_alignment_,
vertical_clip_alignment_);
return SkRect::Make(damage_->buffer_damage);
} else {
return std::nullopt;
}
return std::nullopt;
}

CompositorContext::CompositorContext()
Expand Down Expand Up @@ -125,10 +124,16 @@ RasterStatus CompositorContext::ScopedFrame::Raster(
FrameDamage* frame_damage) {
TRACE_EVENT0("flutter", "CompositorContext::ScopedFrame::Raster");

std::optional<SkRect> clip_rect =
frame_damage
? frame_damage->ComputeClipRect(layer_tree, !ignore_raster_cache)
: std::nullopt;
std::optional<SkRect> clip_rect;
if (frame_damage) {
clip_rect = frame_damage->ComputeClipRect(layer_tree, !ignore_raster_cache);

if (aiks_context_ &&
!ShouldPerformPartialRepaint(clip_rect, layer_tree.frame_size())) {
clip_rect = std::nullopt;
frame_damage->Reset();
}
}

bool root_needs_readback = layer_tree.Preroll(
*this, ignore_raster_cache, clip_rect ? *clip_rect : kGiantRect);
Expand All @@ -146,10 +151,22 @@ RasterStatus CompositorContext::ScopedFrame::Raster(
return RasterStatus::kSkipAndRetry;
}

if (aiks_context_) {
PaintLayerTreeImpeller(layer_tree, clip_rect, ignore_raster_cache);
} else {
PaintLayerTreeSkia(layer_tree, clip_rect, needs_save_layer,
ignore_raster_cache);
}
return RasterStatus::kSuccess;
}

void CompositorContext::ScopedFrame::PaintLayerTreeSkia(
flutter::LayerTree& layer_tree,
std::optional<SkRect> clip_rect,
bool needs_save_layer,
bool ignore_raster_cache) {
DlAutoCanvasRestore restore(canvas(), clip_rect.has_value());

// Clearing canvas after preroll reduces one render target switch when preroll
// paints some raster cache.
if (canvas()) {
if (clip_rect) {
canvas()->ClipRect(*clip_rect);
Expand All @@ -164,9 +181,48 @@ RasterStatus CompositorContext::ScopedFrame::Raster(
}
canvas()->Clear(DlColor::kTransparent());
}
layer_tree.Paint(*this, ignore_raster_cache);

// The canvas()->Restore() is taken care of by the DlAutoCanvasRestore
return RasterStatus::kSuccess;
layer_tree.Paint(*this, ignore_raster_cache);
}

void CompositorContext::ScopedFrame::PaintLayerTreeImpeller(
flutter::LayerTree& layer_tree,
std::optional<SkRect> clip_rect,
bool ignore_raster_cache) {
if (canvas() && clip_rect) {
canvas()->Translate(-clip_rect->x(), -clip_rect->y());
}

layer_tree.Paint(*this, ignore_raster_cache);
}

/// @brief The max ratio of pixel width or height to size that is dirty which
/// results in a partial repaint.
///
/// Performing a partial repaint has a small overhead - Impeller needs to
/// allocate a fairly large resolve texture for the root pass instead of
/// using the drawable texture, and a final blit must be performed. At a
/// minimum, if the damage rect is the entire buffer, we must not perform
/// a partial repaint. Beyond that, we could only experimentally
/// determine what this value should be. From looking at the Flutter
/// Gallery, we noticed that there are occassionally small partial
/// repaints which shave off trivial numbers of pixels.
constexpr float kImpellerRepaintRatio = 0.7f;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a comment about why this number and how to figure out if changing it is helping or hurting things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


bool CompositorContext::ShouldPerformPartialRepaint(
std::optional<SkRect> damage_rect,
SkISize layer_tree_size) {
if (!damage_rect.has_value()) {
return false;
}
if (damage_rect->width() >= layer_tree_size.width() &&
damage_rect->height() >= layer_tree_size.height()) {
return false;
}
auto rx = damage_rect->width() / layer_tree_size.width();
auto ry = damage_rect->height() / layer_tree_size.height();
return rx <= kImpellerRepaintRatio || ry <= kImpellerRepaintRatio;
}

void CompositorContext::OnGrContextCreated() {
Expand Down
26 changes: 25 additions & 1 deletion flow/compositor_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,24 @@ class FrameDamage {

// See Damage::buffer_damage.
std::optional<SkIRect> GetBufferDamage() {
return damage_ ? std::make_optional(damage_->buffer_damage) : std::nullopt;
return (damage_ && !ignore_damage_)
? std::make_optional(damage_->buffer_damage)
: std::nullopt;
}

// Remove reported buffer_damage to inform clients that a partial repaint
// should not be performed on this frame.
// frame_damage is required to correctly track accumulated damage for
// subsequent frames.
void Reset() { ignore_damage_ = true; }
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this could simply be void Reset() { damage_ = std::nullopt; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly if I do this rendering breaks. I wonder if the logic for computing damage requires all frames to have up to date damage calculations? Or if perhaps the std::nullopt signal is incorrect for frame damage and it should instead be treated as if the entire screen were dirty?

Copy link
Member

@knopp knopp Apr 25, 2023

Choose a reason for hiding this comment

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

I see. It breaks because in that case frame_damage is not properly reported in submit info which breaks tracking of accumulated damage in other two buffers. So the area subsequently repainted in back buffers is smaller than it should be, causing glitches. This is visible in the material progress indicator example when sliding in/out the menu:

glitch.mov

Possible solution might be something like knopp@30c3d61 . Except this doesn't work because it causes ShouldPerformPartialRepaint always return false in subsequent repaints effectively disabling partial repaint completely.

So I'd keep it as it is, maybe add a comment that we need to preserve frame_damage to correctly track accumulated damage for other buffers while at same time reset buffer_damage to disable partial repaint for current buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks for the investigation here. I added a comment, but I think it makes sense to keep frame damage because the damage info is still correct a subsequent frame could use this and decide to do a partial repaint.


private:
SkIRect additional_damage_ = SkIRect::MakeEmpty();
std::optional<Damage> damage_;
const LayerTree* prev_layer_tree_ = nullptr;
int vertical_clip_alignment_ = 1;
int horizontal_clip_alignment_ = 1;
bool ignore_damage_ = false;
};

class CompositorContext {
Expand Down Expand Up @@ -144,6 +153,15 @@ class CompositorContext {
FrameDamage* frame_damage);

private:
void PaintLayerTreeSkia(flutter::LayerTree& layer_tree,
std::optional<SkRect> clip_rect,
bool needs_save_layer,
bool ignore_raster_cache);

void PaintLayerTreeImpeller(flutter::LayerTree& layer_tree,
std::optional<SkRect> clip_rect,
bool ignore_raster_cache);

CompositorContext& context_;
GrDirectContext* gr_context_;
DlCanvas* canvas_;
Expand Down Expand Up @@ -205,6 +223,12 @@ class CompositorContext {

void EndFrame(ScopedFrame& frame, bool enable_instrumentation);

/// @brief Whether Impeller shouild attempt a partial repaint.
/// The Impeller backend requires an additional blit pass, which may
/// not be worthwhile if the damage region is large.
static bool ShouldPerformPartialRepaint(std::optional<SkRect> damage_rect,
SkISize layer_tree_size);

FML_DISALLOW_COPY_AND_ASSIGN(CompositorContext);
};

Expand Down
4 changes: 3 additions & 1 deletion impeller/playground/backend/metal/playground_impl_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@
data_->metal_layer.drawableSize =
CGSizeMake(layer_size.width * scale.x, layer_size.height * scale.y);

return SurfaceMTL::WrapCurrentMetalLayerDrawable(context, data_->metal_layer);
auto drawable =
SurfaceMTL::GetMetalDrawableAndValidate(context, data_->metal_layer);
return SurfaceMTL::WrapCurrentMetalLayerDrawable(context, drawable);
}

} // namespace impeller
26 changes: 20 additions & 6 deletions impeller/renderer/backend/metal/surface_mtl.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <QuartzCore/CAMetalLayer.h>

#include "flutter/fml/macros.h"
#include "impeller/geometry/rect.h"
#include "impeller/renderer/context.h"
#include "impeller/renderer/surface.h"

Expand All @@ -32,26 +33,39 @@ class SurfaceMTL final : public Surface {
///
/// @return A pointer to the wrapped surface or null.
///
static std::unique_ptr<SurfaceMTL> WrapCurrentMetalLayerDrawable(
static id<CAMetalDrawable> GetMetalDrawableAndValidate(
const std::shared_ptr<Context>& context,
CAMetalLayer* layer);

static std::unique_ptr<SurfaceMTL> WrapCurrentMetalLayerDrawable(
const std::shared_ptr<Context>& context,
id<CAMetalDrawable> drawable,
std::optional<IRect> clip_rect = std::nullopt);
#pragma GCC diagnostic pop

// |Surface|
~SurfaceMTL() override;

id<MTLDrawable> drawable() const { return drawable_; }

// |Surface|
bool Present() const override;

private:
std::weak_ptr<Context> context_;
id<MTLDrawable> drawable_ = nil;
std::shared_ptr<Texture> resolve_texture_;
id<CAMetalDrawable> drawable_ = nil;
bool requires_blit_ = false;
std::optional<IRect> clip_rect_;

static bool ShouldPerformPartialRepaint(std::optional<IRect> damage_rect);

SurfaceMTL(const std::weak_ptr<Context>& context,
const RenderTarget& target,
id<MTLDrawable> drawable);

// |Surface|
bool Present() const override;
std::shared_ptr<Texture> resolve_texture,
id<CAMetalDrawable> drawable,
bool requires_blit,
std::optional<IRect> clip_rect);

FML_DISALLOW_COPY_AND_ASSIGN(SurfaceMTL);
};
Expand Down
87 changes: 75 additions & 12 deletions impeller/renderer/backend/metal/surface_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "impeller/renderer/backend/metal/surface_mtl.h"

#include "flutter/fml/trace_event.h"
#include "flutter/impeller/renderer/command_buffer.h"
#include "impeller/base/validation.h"
#include "impeller/renderer/backend/metal/context_mtl.h"
#include "impeller/renderer/backend/metal/formats_mtl.h"
Expand All @@ -16,7 +17,7 @@
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunguarded-availability-new"

std::unique_ptr<SurfaceMTL> SurfaceMTL::WrapCurrentMetalLayerDrawable(
id<CAMetalDrawable> SurfaceMTL::GetMetalDrawableAndValidate(
const std::shared_ptr<Context>& context,
CAMetalLayer* layer) {
TRACE_EVENT0("impeller", "SurfaceMTL::WrapCurrentMetalLayerDrawable");
Expand All @@ -35,23 +36,37 @@
VALIDATION_LOG << "Could not acquire current drawable.";
return nullptr;
}
return current_drawable;
}

const auto color_format =
FromMTLPixelFormat(current_drawable.texture.pixelFormat);
std::unique_ptr<SurfaceMTL> SurfaceMTL::WrapCurrentMetalLayerDrawable(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split this and the above into two functions, so that we can ge the clip rect for DRM before we construct the render target, which is required for conditionally adjusting the strategy.

const std::shared_ptr<Context>& context,
id<CAMetalDrawable> drawable,
std::optional<IRect> clip_rect) {
bool requires_blit = ShouldPerformPartialRepaint(clip_rect);
const auto color_format = FromMTLPixelFormat(drawable.texture.pixelFormat);

if (color_format == PixelFormat::kUnknown) {
VALIDATION_LOG << "Unknown drawable color format.";
return nullptr;
}
// compositor_context.cc will offset the rendering by the clip origin. Here we
// shrink to the size of the clip. This has the same effect as clipping the
// rendering but also creates smaller intermediate passes.
ISize root_size;
if (requires_blit) {
root_size = ISize(clip_rect->size.width, clip_rect->size.height);
} else {
root_size = {static_cast<ISize::Type>(drawable.texture.width),
static_cast<ISize::Type>(drawable.texture.height)};
}

TextureDescriptor msaa_tex_desc;
msaa_tex_desc.storage_mode = StorageMode::kDeviceTransient;
msaa_tex_desc.type = TextureType::kTexture2DMultisample;
msaa_tex_desc.sample_count = SampleCount::kCount4;
msaa_tex_desc.format = color_format;
msaa_tex_desc.size = {
static_cast<ISize::Type>(current_drawable.texture.width),
static_cast<ISize::Type>(current_drawable.texture.height)};
msaa_tex_desc.size = root_size;
msaa_tex_desc.usage = static_cast<uint64_t>(TextureUsage::kRenderTarget);

auto msaa_tex = context->GetResourceAllocator()->CreateTexture(msaa_tex_desc);
Expand All @@ -68,8 +83,17 @@
resolve_tex_desc.sample_count = SampleCount::kCount1;
resolve_tex_desc.storage_mode = StorageMode::kDevicePrivate;

std::shared_ptr<Texture> resolve_tex =
std::make_shared<TextureMTL>(resolve_tex_desc, current_drawable.texture);
// Create color resolve texture.
std::shared_ptr<Texture> resolve_tex;
if (requires_blit) {
resolve_tex_desc.compression_type = CompressionType::kLossy;
resolve_tex =
context->GetResourceAllocator()->CreateTexture(resolve_tex_desc);
} else {
resolve_tex =
std::make_shared<TextureMTL>(resolve_tex_desc, drawable.texture);
}

if (!resolve_tex) {
VALIDATION_LOG << "Could not wrap resolve texture.";
return nullptr;
Expand Down Expand Up @@ -112,18 +136,42 @@
render_target_desc.SetStencilAttachment(stencil0);

// The constructor is private. So make_unique may not be used.
return std::unique_ptr<SurfaceMTL>(new SurfaceMTL(
context->weak_from_this(), render_target_desc, current_drawable));
return std::unique_ptr<SurfaceMTL>(
new SurfaceMTL(context->weak_from_this(), render_target_desc, resolve_tex,
drawable, requires_blit, clip_rect));
}

SurfaceMTL::SurfaceMTL(const std::weak_ptr<Context>& context,
const RenderTarget& target,
id<MTLDrawable> drawable)
: Surface(target), context_(context), drawable_(drawable) {}
std::shared_ptr<Texture> resolve_texture,
id<CAMetalDrawable> drawable,
bool requires_blit,
std::optional<IRect> clip_rect)
: Surface(target),
context_(context),
resolve_texture_(std::move(resolve_texture)),
drawable_(drawable),
requires_blit_(requires_blit),
clip_rect_(clip_rect) {}

// |Surface|
SurfaceMTL::~SurfaceMTL() = default;

bool SurfaceMTL::ShouldPerformPartialRepaint(std::optional<IRect> damage_rect) {
// compositor_context.cc will conditionally disable partial repaint if the
// damage region is large. If that happened, then a nullopt damage rect
// will be provided here.
if (!damage_rect.has_value()) {
return false;
}
// If the damage rect is 0 in at least one dimension, partial repaint isn't
// performed as we skip right to present.
if (damage_rect->size.width <= 0 || damage_rect->size.height <= 0) {
return false;
}
return true;
}

// |Surface|
bool SurfaceMTL::Present() const {
if (drawable_ == nil) {
Expand All @@ -135,6 +183,21 @@
return false;
}

if (requires_blit_) {
auto blit_command_buffer = context->CreateCommandBuffer();
if (!blit_command_buffer) {
return false;
}
auto blit_pass = blit_command_buffer->CreateBlitPass();
auto current = TextureMTL::Wrapper({}, drawable_.texture);
blit_pass->AddCopy(resolve_texture_, current, std::nullopt,
clip_rect_->origin);
blit_pass->EncodeCommands(context->GetResourceAllocator());
if (!blit_command_buffer->SubmitCommands()) {
return false;
}
}

// If a transaction is present, `presentDrawable` will present too early. And
// so we wait on an empty command buffer to get scheduled instead, which
// forces us to also wait for all of the previous command buffers in the queue
Expand Down
4 changes: 4 additions & 0 deletions shell/gpu/gpu_surface_metal_impeller.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ class SK_API_AVAILABLE_CA_METAL_LAYER GPUSurfaceMetalImpeller : public Surface {
std::shared_ptr<impeller::Renderer> impeller_renderer_;
std::shared_ptr<impeller::AiksContext> aiks_context_;
fml::scoped_nsprotocol<id<MTLDrawable>> last_drawable_;
bool disable_partial_repaint_ = false;
// Accumulated damage for each framebuffer; Key is address of underlying
// MTLTexture for each drawable
std::map<uintptr_t, SkIRect> damage_;

// |Surface|
std::unique_ptr<SurfaceFrame> AcquireFrame(const SkISize& size) override;
Expand Down
Loading