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.

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

Don't retain the MTLTexture or MTLDevice in TestMetalContext #30832

Merged
merged 3 commits into from
Jan 13, 2022
Merged

Don't retain the MTLTexture or MTLDevice in TestMetalContext #30832

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21adc3b
Don't retain the MTLTexture or MTLTextureDescriptor
Jan 13, 2022
4f673c2
Don't double release
Jan 13, 2022
7629cd7
MTLTextureDescriptor is already in the autorelease pool, so the retai…
Jan 13, 2022
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
18 changes: 10 additions & 8 deletions testing/test_metal_context.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace flutter {

TestMetalContext::TestMetalContext() {
auto device = fml::scoped_nsprotocol{[MTLCreateSystemDefaultDevice() retain]};
auto device = fml::scoped_nsprotocol{MTLCreateSystemDefaultDevice()};
if (!device) {
FML_LOG(ERROR) << "Could not acquire Metal device.";
return;
Expand Down Expand Up @@ -42,6 +42,9 @@

TestMetalContext::~TestMetalContext() {
std::scoped_lock lock(textures_mutex);
for (auto& [_, texture] : textures_) {
[(id)texture.get() release];
}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary, those textures are stored in sk_cfp which calls release in its deconstructor. If you see this being necessary you probably have a double retain. It looks correct when you are making it to me, the apis are a bit confusing though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right - I missed that we were calling the sk_cfp copy ctor when assigning the MTLTexture into the textures_ map (and that the textures_ map holds the sk_cfp and not a raw pointer). This means that we don't need to release manually in this dtor, but I believe we still need to remove the extra retain call, right?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, I believe we should have the retain now. [device newTextureWithDescriptor:texture_descriptor.get()] should return an object that is on the autorelease pool. Which means it will get released when this event is done, so we'd need a retain to keep it alive in the smart pointer.

I'm not 100% certain because the semantics for an objc method called new* that return an objc object are a bit unclear to me. I think technically anything that isn't named create should be on the autorelease pool, but that isn't the case for -[NSObject new].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm misunderstanding the guide at https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html, it says that any ObjC method with "new" in the name returns an object that the caller owns outright. That implies to me that it won't be in the autorelease pool.

That being said, I think that the MTLTextureDescriptor does fall under that category perhaps, as the ObjC method name does not include the word "new", "create" etc, so I'd expect that to be in the autorelease pool?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yep, you're right, anything that begins with "new" means the caller owns it. No need to retain it again. You don't run into new methods often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, the unit test that was failing was because the MTLTextureDescriptor is indeed returned already in the autorelease pool, so that retain still needs to be there.

With that fixed, all the unit tests are now passing locally.

textures_.clear();
if (device_) {
[(__bridge id)device_ release];
Expand All @@ -65,11 +68,11 @@

TestMetalContext::TextureInfo TestMetalContext::CreateMetalTexture(const SkISize& size) {
std::scoped_lock lock(textures_mutex);
auto texture_descriptor = fml::scoped_nsobject{
[[MTLTextureDescriptor texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm
width:size.width()
height:size.height()
mipmapped:NO] retain]};
auto texture_descriptor = fml::scoped_nsobject{[MTLTextureDescriptor
texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm
width:size.width()
height:size.height()
mipmapped:NO]};

// The most pessimistic option and disables all optimizations but allows tests
// the most flexible access to the surface. They may read and write to the
Expand All @@ -82,8 +85,7 @@
}

id<MTLDevice> device = (__bridge id<MTLDevice>)GetMetalDevice();
sk_cfp<void*> texture =
sk_cfp<void*>{[[device newTextureWithDescriptor:texture_descriptor.get()] retain]};
sk_cfp<void*> texture = sk_cfp<void*>{[device newTextureWithDescriptor:texture_descriptor.get()]};

if (!texture) {
FML_CHECK(false) << "Could not create texture from texture descriptor.";
Expand Down