Skip to content

Commit

Permalink
[graphite] Fix some Viewer issues
Browse files Browse the repository at this point in the history
There are two issues fixed here:
* In Viewer we don't have a GraphiteTestContext, so check for that.

* When running in Xcode with validation enabled, sometimes we get
asserts that we're trying to delete an resource used by a
MTLCommandBuffer. However, that MTLCommandBuffer is the one we're
trying to clean up. Because MTLCommandBuffer objects are assigned
to an AutoreleasePool, there may be a second ref on the command buffer object, which won't be released until its pool is drained at some future time. The solution for that is to create a temporary pool at object creation, which will get drained immediately when it leaves scope, releasing that dangling ref. We also have to do this for the CommandEncoders, which apparently have refs to their CommandBuffer.
While this doesn't 100% fix the issue it does seem to help some.

Change-Id: Ic8684ce05a116389dd9cbadfd7d5128ce8023475
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/836116
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
  • Loading branch information
jvanverth authored and SkCQ committed Apr 4, 2024
1 parent 4a9e0d0 commit 4fa6c69
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 29 deletions.
8 changes: 6 additions & 2 deletions gm/asyncrescaleandread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,9 @@ class AsyncReadGMBase : public skiagm::GM {
graphiteContext->submit();
while (!asyncContext->fCalled) {
graphiteContext->checkAsyncWorkCompletion();
this->graphiteTestContext()->tick();
if (this->graphiteTestContext()) {
this->graphiteTestContext()->tick();
}
}
#endif
} else {
Expand Down Expand Up @@ -254,7 +256,9 @@ class AsyncReadGMBase : public skiagm::GM {
graphiteContext->submit();
while (!asyncContext.fCalled) {
graphiteContext->checkAsyncWorkCompletion();
this->graphiteTestContext()->tick();
if (this->graphiteTestContext()) {
this->graphiteTestContext()->tick();
}
}
#endif
} else {
Expand Down
12 changes: 7 additions & 5 deletions src/gpu/graphite/mtl/MtlBlitCommandEncoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ class MtlBlitCommandEncoder : public Resource {
public:
static sk_sp<MtlBlitCommandEncoder> Make(const SharedContext* sharedContext,
id<MTLCommandBuffer> commandBuffer) {
// Adding a retain here to keep our own ref separate from the autorelease pool
sk_cfp<id<MTLBlitCommandEncoder>> encoder =
sk_ret_cfp<id<MTLBlitCommandEncoder>>([commandBuffer blitCommandEncoder]);
return sk_sp<MtlBlitCommandEncoder>(new MtlBlitCommandEncoder(sharedContext,
std::move(encoder)));
@autoreleasepool {
// Adding a retain here to keep our own ref separate from the autorelease pool
sk_cfp<id<MTLBlitCommandEncoder>> encoder =
sk_ret_cfp<id<MTLBlitCommandEncoder>>([commandBuffer blitCommandEncoder]);
return sk_sp<MtlBlitCommandEncoder>(new MtlBlitCommandEncoder(sharedContext,
std::move(encoder)));
}
}

const char* getResourceType() const override { return "Metal Blit Command Encoder"; }
Expand Down
23 changes: 14 additions & 9 deletions src/gpu/graphite/mtl/MtlCommandBuffer.mm
Original file line number Diff line number Diff line change
Expand Up @@ -59,17 +59,22 @@

bool MtlCommandBuffer::createNewMTLCommandBuffer() {
SkASSERT(fCommandBuffer == nil);
if (@available(macOS 11.0, iOS 14.0, tvOS 14.0, *)) {
sk_cfp<MTLCommandBufferDescriptor*> desc([[MTLCommandBufferDescriptor alloc] init]);
(*desc).retainedReferences = NO;

// Inserting a pool here so the autorelease occurs when we return and the
// only remaining ref is the retain below.
@autoreleasepool {
if (@available(macOS 11.0, iOS 14.0, tvOS 14.0, *)) {
sk_cfp<MTLCommandBufferDescriptor*> desc([[MTLCommandBufferDescriptor alloc] init]);
(*desc).retainedReferences = NO;
#ifdef SK_ENABLE_MTL_DEBUG_INFO
(*desc).errorOptions = MTLCommandBufferErrorOptionEncoderExecutionStatus;
(*desc).errorOptions = MTLCommandBufferErrorOptionEncoderExecutionStatus;
#endif
// We add a retain here because the command buffer is set to autorelease (not alloc or copy)
fCommandBuffer.reset([[fQueue commandBufferWithDescriptor:desc.get()] retain]);
} else {
// We add a retain here because the command buffer is set to autorelease (not alloc or copy)
fCommandBuffer.reset([[fQueue commandBufferWithUnretainedReferences] retain]);
// We add a retain here because the command buffer is set to autorelease (not alloc or copy)
fCommandBuffer.reset([[fQueue commandBufferWithDescriptor:desc.get()] retain]);
} else {
// We add a retain here because the command buffer is set to autorelease (not alloc or copy)
fCommandBuffer.reset([[fQueue commandBufferWithUnretainedReferences] retain]);
}
}
return fCommandBuffer != nil;
}
Expand Down
20 changes: 12 additions & 8 deletions src/gpu/graphite/mtl/MtlComputeCommandEncoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,18 @@ class MtlComputeCommandEncoder : public Resource {
public:
static sk_sp<MtlComputeCommandEncoder> Make(const SharedContext* sharedContext,
id<MTLCommandBuffer> commandBuffer) {
// Adding a retain here to keep our own ref separate from the autorelease pool
sk_cfp<id<MTLComputeCommandEncoder>> encoder =
sk_ret_cfp([commandBuffer computeCommandEncoder]);

// TODO(armansito): Support concurrent dispatch of compute passes using
// MTLDispatchTypeConcurrent on macOS 10.14+ and iOS 12.0+.
return sk_sp<MtlComputeCommandEncoder>(
new MtlComputeCommandEncoder(sharedContext, std::move(encoder)));
// Inserting a pool here so the autorelease occurs when we return and the
// only remaining ref is the retain below.
@autoreleasepool {
// Adding a retain here to keep our own ref separate from the autorelease pool
sk_cfp<id<MTLComputeCommandEncoder>> encoder =
sk_ret_cfp([commandBuffer computeCommandEncoder]);

// TODO(armansito): Support concurrent dispatch of compute passes using
// MTLDispatchTypeConcurrent on macOS 10.14+ and iOS 12.0+.
return sk_sp<MtlComputeCommandEncoder>(
new MtlComputeCommandEncoder(sharedContext, std::move(encoder)));
}
}

const char* getResourceType() const override { return "Metal Compute Command Encoder"; }
Expand Down
14 changes: 9 additions & 5 deletions src/gpu/graphite/mtl/MtlRenderCommandEncoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,15 @@ class MtlRenderCommandEncoder : public Resource {
static sk_sp<MtlRenderCommandEncoder> Make(const SharedContext* sharedContext,
id<MTLCommandBuffer> commandBuffer,
MTLRenderPassDescriptor* descriptor) {
// Adding a retain here to keep our own ref separate from the autorelease pool
sk_cfp<id<MTLRenderCommandEncoder>> encoder =
sk_ret_cfp([commandBuffer renderCommandEncoderWithDescriptor:descriptor]);
return sk_sp<MtlRenderCommandEncoder>(new MtlRenderCommandEncoder(sharedContext,
std::move(encoder)));
// Inserting a pool here so the autorelease occurs when we return and the
// only remaining ref is the retain below.
@autoreleasepool {
// Adding a retain here to keep our own ref separate from the autorelease pool
sk_cfp<id<MTLRenderCommandEncoder>> encoder =
sk_ret_cfp([commandBuffer renderCommandEncoderWithDescriptor:descriptor]);
return sk_sp<MtlRenderCommandEncoder>(new MtlRenderCommandEncoder(sharedContext,
std::move(encoder)));
}
}

const char* getResourceType() const override { return "Metal Render Command Encoder"; }
Expand Down

0 comments on commit 4fa6c69

Please sign in to comment.