warrenm / GLTFKit2

A glTF 2.0 asset loader and exporter for Objective-C and Swift.
MIT License
156 stars 27 forks source link

Fix CGImageRef leak when loading image from buffer view #1

Closed jmousseau closed 3 years ago

jmousseau commented 3 years ago

Two modifications were made locally to create an environment appropriate for leak testing. First, the GLTFDocument's asset property was removed to eliminate one reference. Second, the SCNView's scene was cleared along with the asset four seconds after the view loaded:

Timer.scheduledTimer(withTimeInterval: 4.0, repeats: false) { _ in
    self.scnView.scene = nil
    self.asset = nil
}

Here's the before and after in Instruments when loading DamagedHelmet.glb:

allocations-before allocations-after

I considered an alternative approach where the createCGImage helper method returned an NSUIImage so the caller doesn't need to clean up with CFRelease. However, it looks like CGImageRef may be preferred as a cross-platform image type for this code base. If that isn't the case, happy to resubmit with NSUIImage implementation.

jmousseau commented 3 years ago

I forgot to say the CFDataRef also leaks, but the fix requires a copy.

+ CFDataRef sourceData = CFDataCreate(NULL, imageBytes, self.bufferView.length);
- CFDataRef sourceData = CFDataCreateWithBytesNoCopy(NULL, imageBytes, self.bufferView.length, NULL);
  imageSource = CGImageSourceCreateWithData(sourceData, NULL);
+ CFRelease(sourceData);

I was hesitant to make the change as I noticed the SCNScene sometimes pointed to GLTFAsset buffers not managed directly by ARC, such as the CFDataCreateWithBytesNoCopy call with bufferView above. As a result, calling CFRelease without performing a copy would cause a crash.