Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delay metal drawable acquisition till frame submission. #13367

Merged
merged 2 commits into from
Oct 28, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 shell/gpu/gpu_surface_metal.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class GPUSurfaceMetal : public Surface {
fml::scoped_nsobject<CAMetalLayer> layer_;
sk_sp<GrContext> context_;
fml::scoped_nsprotocol<id<MTLCommandQueue>> command_queue_;
GrMTLHandle next_drawable_ = nullptr;

// |Surface|
bool IsValid() override;
Expand Down
73 changes: 31 additions & 42 deletions shell/gpu/gpu_surface_metal.mm
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#include "third_party/skia/include/gpu/GrBackendSurface.h"
#include "third_party/skia/include/ports/SkCFObject.h"

static_assert(!__has_feature(objc_arc), "ARC must be disabled.");

namespace flutter {

GPUSurfaceMetal::GPUSurfaceMetal(GPUSurfaceDelegate* delegate,
Expand Down Expand Up @@ -93,65 +95,52 @@
return nullptr;
}

const auto scale = layer_.get().contentsScale;

auto next_drawable = fml::scoped_nsprotocol<id<CAMetalDrawable>>([[layer_ nextDrawable] retain]);
if (!next_drawable) {
FML_LOG(ERROR) << "Could not acquire next metal drawable.";
return nullptr;
// Get ready for the next frame no matter what.
if (next_drawable_ != nullptr) {
CFRelease(next_drawable_);
next_drawable_ = nullptr;
}

auto metal_texture = fml::scoped_nsprotocol<id<MTLTexture>>([next_drawable.get().texture retain]);
if (!metal_texture) {
FML_LOG(ERROR) << "Could not acquire metal texture from drawable.";
return nullptr;
}

GrMtlTextureInfo metal_texture_info;
metal_texture_info.fTexture.reset(SkCFSafeRetain(metal_texture.get()));

GrBackendRenderTarget metal_render_target(bounds.width * scale, // width
bounds.height * scale, // height
1, // sample count
metal_texture_info // metal texture info
auto surface = SkSurface::MakeFromCAMetalLayer(context_.get(), // context
Copy link
Contributor

Choose a reason for hiding this comment

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

Does next_drawable_ get set synchronously here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. This will defer the next drawable acquisition till much later. In fact, it is almost guaranteed to not be set after this call. I would have captured it in one of the scoped wrappers otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the next_drawable_ isn't set until after we check if it's null in the callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

The next GPUSurfaceMetal::AcquireFrame will collect it if there is a next frame. GPUSurfaceMetal::~GPUSurfaceMetal will collect it via ReleaseUnusedDrawableIfNecessary if there isn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should mention that either case is extremely unlikely because the surface frame itself is collected immediately after submission.

layer_.get(), // layer
kTopLeft_GrSurfaceOrigin, // origin
1, // sample count
kBGRA_8888_SkColorType, // color type
nullptr, // colorspace
nullptr, // surface properties
&next_drawable_ // drawable (transfer out)
);

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

SkSurface::RenderTargetReleaseProc release_proc = [](SkSurface::ReleaseContext context) {
[reinterpret_cast<id>(context) release];
};

auto surface =
SkSurface::MakeFromBackendRenderTarget(context_.get(), // context
metal_render_target, // backend render target
kTopLeft_GrSurfaceOrigin, // origin
kBGRA_8888_SkColorType, // color type
nullptr, // colorspace
nullptr, // surface properties
release_proc, // release proc
metal_texture.release() // release context (texture)
);

if (!surface) {
FML_LOG(ERROR) << "Could not create the SkSurface from the metal texture.";
return nullptr;
}

bool hasExternalViewEmbedder = delegate_->GetExternalViewEmbedder() != 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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe we should move this comment down to line 143 now, since we're no longer getting whether it's an external view embedder here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

auto submit_callback = [drawable = next_drawable, command_buffer, hasExternalViewEmbedder](
const SurfaceFrame& surface_frame, SkCanvas* canvas) -> bool {
auto submit_callback = [this](const SurfaceFrame& surface_frame, SkCanvas* canvas) -> bool {
canvas->flush();
if (!hasExternalViewEmbedder) {

if (next_drawable_ == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that this callback gets invoked between lines 101 and 104 above?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. These are on the same thread and tasks are posted serially.

FML_DLOG(ERROR) << "Could not acquire next Metal drawable from the SkSurface.";
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;

if (!has_external_view_embedder) {
[command_buffer.get() presentDrawable:drawable.get()];
[command_buffer.get() commit];
} else {
Expand Down