Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
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
40 changes: 10 additions & 30 deletions shell/gpu/gpu_surface_metal.mm
Original file line number Diff line number Diff line change
Expand Up @@ -39,32 +39,25 @@
}

// |Surface|
std::unique_ptr<SurfaceFrame> GPUSurfaceMetal::AcquireFrame(const SkISize& size) {
std::unique_ptr<SurfaceFrame> GPUSurfaceMetal::AcquireFrame(const SkISize& frame_size) {
if (!IsValid()) {
FML_LOG(ERROR) << "Metal surface was invalid.";
return nullptr;
}

if (size.isEmpty()) {
if (frame_size.isEmpty()) {
FML_LOG(ERROR) << "Metal surface was asked for an empty frame.";
return nullptr;
}

const auto bounds = layer_.get().bounds.size;
const auto scale = layer_.get().contentsScale;
if (bounds.width <= 0.0 || bounds.height <= 0.0 || scale <= 0.0) {
FML_LOG(ERROR) << "Metal layer bounds were invalid.";
return nullptr;
}
Comment on lines -53 to -58
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not needed anymore? Was it just wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was redundant. The frame_size already accounts for these calculation. There is no point in doing them again here.

const auto drawable_size = CGSizeMake(frame_size.width(), frame_size.height());

const auto scaled_bounds = CGSizeMake(bounds.width * scale, bounds.height * scale);
if (!CGSizeEqualToSize(drawable_size, layer_.get().drawableSize)) {
layer_.get().drawableSize = drawable_size;
}

ReleaseUnusedDrawableIfNecessary();

if (!CGSizeEqualToSize(scaled_bounds, layer_.get().drawableSize)) {
layer_.get().drawableSize = scaled_bounds;
}

auto surface = SkSurface::MakeFromCAMetalLayer(context_.get(), // context
layer_.get(), // layer
kTopLeft_GrSurfaceOrigin, // origin
Expand All @@ -89,30 +82,17 @@
return false;
}

const auto has_external_view_embedder = delegate_->GetExternalViewEmbedder() != nullptr;

auto command_buffer =
fml::scoped_nsprotocol<id<MTLCommandBuffer>>([[command_queue_.get() commandBuffer] retain]);

fml::scoped_nsprotocol<id<CAMetalDrawable>> drawable(
reinterpret_cast<id<CAMetalDrawable>>(next_drawable_));
next_drawable_ = nullptr;

// External views need to present with transaction. When presenting with
// transaction, we have to block, otherwise we risk presenting the drawable
// after the CATransaction has completed.
// See:
// https://developer.apple.com/documentation/quartzcore/cametallayer/1478157-presentswithtransaction
// TODO(dnfield): only do this if transactions are actually being used.
// https://github.com/flutter/flutter/issues/24133
if (!has_external_view_embedder) {
[command_buffer.get() presentDrawable:drawable.get()];
[command_buffer.get() commit];
} else {
[command_buffer.get() commit];
[command_buffer.get() waitUntilScheduled];
[drawable.get() present];
}
[command_buffer.get() commit];
[command_buffer.get() waitUntilScheduled];
[drawable.get() present];
Comment on lines +92 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this will break platform views - have you tested this with a platform view? e.g. run the webview example on a local engine built with metal support.

Copy link
Member Author

Choose a reason for hiding this comment

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

If anything, this brings the behavior more in line with the condition where there are platform views. When there is a platform view, has_external_view_embedder in the previous code path will be true. Now, both paths do the same thing where they wait for the GPU commands to be flushed in the current transaction.

Copy link
Member Author

Choose a reason for hiding this comment

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

And yes. I did test this locally with platform views enabled (specifically the web view example).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh duh I misread the old code.


return true;
};

Expand Down