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

[Impeller] iOS/macOS: Only wait for command scheduling prior to present #40781

Merged
merged 3 commits into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
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
21 changes: 0 additions & 21 deletions impeller/renderer/backend/metal/command_buffer_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -167,27 +167,6 @@ static bool LogMTLCommandBufferErrorIfPresent(id<MTLCommandBuffer> buffer) {

[buffer_ commit];

#if (FML_OS_MACOSX || FML_OS_IOS_SIMULATOR)
// We're using waitUntilScheduled on macOS and iOS simulator to force a hard
// barrier between the execution of different command buffers. This forces all
// renderable texture access to be synchronous (i.e. a write from a previous
// command buffer will not get scheduled to happen at the same time as a read
// in a future command buffer).
//
// Metal hazard tracks shared memory resources by default, and we don't need
// to do any additional work to synchronize access to MTLTextures and
// MTLBuffers on iOS devices with UMA. However, shared textures are disallowed
// on macOS according to the documentation:
// https://developer.apple.com/documentation/metal/mtlstoragemode/shared
// And so this is a stopgap solution that has been present in Impeller since
// multi-pass rendering/SaveLayer support was first set up.
//
// TODO(bdero): Remove this for all targets once a solution for resource
// tracking that works everywhere is established:
// https://github.com/flutter/flutter/issues/120406
[buffer_ waitUntilScheduled];
#endif

buffer_ = nil;
return true;
}
Expand Down
2 changes: 2 additions & 0 deletions impeller/renderer/backend/metal/context_mtl.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ class ContextMTL final : public Context,
// |Context|
bool UpdateOffscreenLayerPixelFormat(PixelFormat format) override;

id<MTLCommandBuffer> CreateMTLCommandBuffer() const;

private:
id<MTLDevice> device_ = nullptr;
id<MTLCommandQueue> command_queue_ = nullptr;
Expand Down
4 changes: 4 additions & 0 deletions impeller/renderer/backend/metal/context_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -290,4 +290,8 @@ static bool DeviceSupportsComputeSubgroups(id<MTLDevice> device) {
return true;
}

id<MTLCommandBuffer> ContextMTL::CreateMTLCommandBuffer() const {
return [command_queue_ commandBuffer];
}

} // namespace impeller
5 changes: 4 additions & 1 deletion impeller/renderer/backend/metal/surface_mtl.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,12 @@ class SurfaceMTL final : public Surface {
id<MTLDrawable> drawable() const { return drawable_; }

private:
std::weak_ptr<Context> context_;
id<MTLDrawable> drawable_ = nil;

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

// |Surface|
bool Present() const override;
Expand Down
22 changes: 17 additions & 5 deletions impeller/renderer/backend/metal/surface_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "flutter/fml/trace_event.h"
#include "impeller/base/validation.h"
#include "impeller/renderer/backend/metal/context_mtl.h"
#include "impeller/renderer/backend/metal/formats_mtl.h"
#include "impeller/renderer/backend/metal/texture_mtl.h"
#include "impeller/renderer/render_target.h"
Expand Down Expand Up @@ -111,12 +112,14 @@
render_target_desc.SetStencilAttachment(stencil0);

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

SurfaceMTL::SurfaceMTL(const RenderTarget& target, id<MTLDrawable> drawable)
: Surface(target), drawable_(drawable) {}
SurfaceMTL::SurfaceMTL(const std::weak_ptr<Context>& context,
const RenderTarget& target,
id<MTLDrawable> drawable)
: Surface(target), context_(context), drawable_(drawable) {}

// |Surface|
SurfaceMTL::~SurfaceMTL() = default;
Expand All @@ -127,7 +130,16 @@
return false;
}

[drawable_ present];
auto context = context_.lock();
if (!context) {
return false;
}

id<MTLCommandBuffer> command_buffer =
Copy link
Member

Choose a reason for hiding this comment

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

The lock can fail in case of context loss. We need to check and return false from this method.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, derp. You got rid of the weak ptr. I think the weak ptr does make more sense though. We just need to be able to survive context loss.

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 just switched to hanging onto a shared_ptr instead.

ContextMTL::Cast(context.get())->CreateMTLCommandBuffer();
[command_buffer presentDrawable:drawable_];
[command_buffer commit];

return true;
}
#pragma GCC diagnostic pop
Expand Down