Skip to content

Fix Metal command buffer ownership in command lists #96

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.

Sign up for GitHub

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

Closed
cdlewis wants to merge 3 commits into from
Closed

Fix Metal command buffer ownership in command lists #96

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd89b9e
Fix Metal command buffer ownership in command lists
cdlewis May 22, 2026
371250a
Retain Metal blit encoder stored on command list
cdlewis May 22, 2026
555ed19
TextureDescriptor was not owned so it does not need to be released
cdlewis May 22, 2026
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
8 changes: 5 additions & 3 deletions plume_metal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,8 +1196,6 @@ namespace plume {
// Create texture with configured descriptor and alignment
MTL::TextureDescriptor *descriptor = MTL::TextureDescriptor::textureBufferDescriptor(pixelFormat, width, options, usage);
this->texture = buffer->mtl->newTexture(descriptor, 0, bytesPerRow);

descriptor->release();
Copy link
Copy Markdown
Author

@cdlewis cdlewis May 22, 2026

Choose a reason for hiding this comment

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

This is the opposite problem, we did not own TextureDescriptor and the autorelease pool (?) should take care of it

Copy link
Copy Markdown
Contributor

@squidbus squidbus May 22, 2026

Choose a reason for hiding this comment

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

Need to add an autoreleasepool to this function then.

}

MetalBufferFormattedView::~MetalBufferFormattedView() {
Expand Down Expand Up @@ -2238,7 +2236,9 @@ namespace plume {
}

MetalCommandList::~MetalCommandList() {
mtl->release();
if (mtl != nullptr) {
Comment thread
DarioSamo marked this conversation as resolved.
mtl->release();
}

for (auto& fenceSet : fences) {
for (auto* fence : fenceSet) {
Expand All @@ -2253,6 +2253,7 @@ namespace plume {
assert(mtl == nullptr);
startedEncoding = false;
mtl = queue->mtl->commandBufferWithUnretainedReferences();
mtl->retain();
Copy link
Copy Markdown
Contributor

@squidbus squidbus May 22, 2026

Choose a reason for hiding this comment

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

Adding a retain here is right but also needs an autorelease pool around this function.

mtl->setLabel(MTLSTR("RT64 Command List"));

// Reset fence waits and updates for new command list.
Expand Down Expand Up @@ -3551,6 +3552,7 @@ namespace plume {
if (activeBlitEncoder == nullptr) {
activeBlitEncoder = mtl->blitCommandEncoder(device->sharedBlitDescriptor);
activeBlitEncoder->setLabel(MTLSTR("Copy Blit Encoder"));
activeBlitEncoder->retain();
Copy link
Copy Markdown
Contributor

@DarioSamo DarioSamo May 22, 2026

Choose a reason for hiding this comment

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

A brief review of all "commandEncoder" command tells me not every one of them are using retain() and some are using release() despite that. CC @squidbus should all command encoders need to manually increase and decrease the refcount when created and destroyed?

Copy link
Copy Markdown
Author

@cdlewis cdlewis May 22, 2026

Choose a reason for hiding this comment

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

+1

I was hesitant to make more sweeping changes than were necessary to stop crashes. My read is that pointers that can escape the function should be retained/released but I'm far from an expert at this stuff.

Copy link
Copy Markdown
Contributor

@squidbus squidbus May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Again, retain here is right but needs an autoreleasepool around this. These were also addressed in #95, there's a missing pair for checkActiveResolveTextureComputeEncoder as well.

Copy link
Copy Markdown
Contributor

@squidbus squidbus May 22, 2026

Choose a reason for hiding this comment

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

should all command encoders need to manually increase and decrease the refcount when created and destroyed?

Only if it's kept outside the scope of the function, otherwise an autoreleasepool will free it. Although these missing retains did not have an autoreleasepool so they were living longer.

Copy link
Copy Markdown
Author

@cdlewis cdlewis May 22, 2026

Choose a reason for hiding this comment

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

is it worth just waiting for #95 to merge then?

Copy link
Copy Markdown
Contributor

@DarioSamo DarioSamo May 22, 2026

Choose a reason for hiding this comment

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

It'd be best to clean up the logic of auto-release pools in that PR for sure, and then we can look at retain()/release() logic to be done where it's necessary.


startedEncoding = true;

Expand Down