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.

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

Gif Conversion creates memory leak. #9

Closed
cassidyclawson opened this issue Feb 20, 2016 · 15 comments
Closed

Gif Conversion creates memory leak. #9

cassidyclawson opened this issue Feb 20, 2016 · 15 comments
Labels

Comments

@cassidyclawson
Copy link

This small Objective-C library converts videos to animated Gifs. Unfortunately, the library contains a memory leak which fails to release approximately the same amount of memory as the final size of the gif. Eventually this leads to a crash after multiple sequential conversions.

(This is a duplicate of the previous open issue. I have reposted to make a more concise explanation of the problem for the bugbounty)

@cassidyclawson
Copy link
Author

I posted a $100 bounty for a fix to this bug, available to anyone (including the original author).
https://www.bountysource.com/issues/31041391-gif-conversion-creates-memory-leak

@khappucino
Copy link
Contributor

So I believe that in NSGIF.m if you replace this line

CGImageDestinationRef destination = CGImageDestinationCreateWithURL((__bridge CFURLRef)fileURL, kUTTypeGIF , frameCount, NULL);

if (fileURL == nil)
return nil;

with....

if (fileURL == nil)
return nil;

CGImageDestinationRef destination = CGImageDestinationCreateWithURL((__bridge CFURLRef)fileURL, kUTTypeGIF , frameCount, NULL);

you will return without ever allocating destination, running Analyzer complained about this line and two other points.

One is during a failure to finalize the file, we return without releasing destination
if (!CGImageDestinationFinalize(destination)) {
NSLog(@"Failed to finalize GIF destination: %@", error);
perhaps add this guy ---> CFRelease(destination);
return nil;
}

and then apparently ImageWithScale should be named CopyImageWithScale or CreateImageWithScale (I don't know if this really matters but xcode complains that the naming convention of methods that return an owned +1 retain count instance should have the word Create or Copy in the method name).

@cassidyclawson
Copy link
Author

Awesome work, @khappucino! If you prepare your suggested changes as a pull request, and I can confirm the memory leak is resolved, I will be happy to award the bounty!

@khappucino
Copy link
Contributor

Ok the pull request is in flight. Let us know if that actually fixes anything. Those Analyzer findings are primarily suggestions but hopefully it works.

@cassidyclawson
Copy link
Author

Thanks much @khappucino. I will check out your pull request shortly.

@sebyddd: are you going to also review the pull request and confirm it solves the problem? If so, I can defer to your judgement or I can make a determination myself and award the bounty if its indeed fixed.

Thanks all!

@sebyddd
Copy link
Member

sebyddd commented Feb 21, 2016

@cassidyclawson: Yes, I will definitely check the pull request first thing tomorrow and update you.

@khappucino
Copy link
Contributor

I'll waive the bounty even if it works. I am going to use this for one of my apps and I'm cool without the bounty if it helps this component.

@cassidyclawson
Copy link
Author

Just tested, and unfortunately, in my implementation, the code still produces a memory leak of the same size as the gif.

I read the pull request pretty carefully, and I had a hard time understanding how any of the changes would make a meaningful impact on this issue, though they were indeed improvements in syntax and edge cases. Thanks for the work so far, khappucino.

Any other ideas, gang?

@khappucino
Copy link
Contributor

So I just ran the allocation profiler in my use case and there is a transient allocation of large buffers but after the gif is written to the camera roll (which is my use case) there are no 'created and persistant' allocations that are anywhere near the size of the created GIF. (I am not seeing a memory leak in the below usage scenario, the generated GIF was approximately 9-10 megs)

Here is the scenario that I am using is effectively this. With this usage

NSGIF.createGIFfromURL(url, withFrameCount: 30, delayTime: 0, loopCount: 0,
completion: {
[weak self](url : NSURL!) -> Void in
PHPhotoLibrary.sharedPhotoLibrary().performChanges({ weak self -> Void in
PHAssetChangeRequest.creationRequestForAssetFromImageAtFileURL(url)
},
completionHandler: nil })

For this use style here are the Analyzer traces, one shows the Created and Destroyed elements, and then for the same time period there is a trace for Created and Persistant. The trace window takes place right before NSGIF creates and then ends after the image is written to the camera roll.

screen shot 2016-02-21 at 8 46 47 pm
screen shot 2016-02-21 at 8 46 56 pm

@cassidyclawson
Copy link
Author

Well, thanks to the good work of khappucino, I should acknowledge that me and the other bug reporter are probably mistaken.

I believe that my previously okay camera code is now suffering from this iOS 9 camera bug: https://forums.developer.apple.com/thread/17142

I will be able to confirm later this weekend when working with an AV Foundation expert. I will close the bug bounty if we confirm the bug is outside this library. However, in appreciation for your hard work @khappucino, I will make a donation to a cause of our mutual choosing. I suggest:

  • Bernie Sanders US Presidential Campaign
  • Electronic Frontier Foundation
  • Wildlife Conservation Network

@ksenia-lyagusha
Copy link

I've changed the code as @khappucino wrote above but memory warning appears on iPhone 6+ when I set framesPerSecond = 10. On the other devices are ok.

What's a solution?
Thanks!

@zenyuhao
Copy link

hi all

I found the memory leak only happend on gif scaled in IOS device

and I add the line to fix the memory leak

#pragma mark - Helpers

CGImageRef ImageWithScale(CGImageRef imageRef, float scale) {

    #if TARGET_OS_IPHONE || TARGET_IPHONE_SIMULATOR
    CGSize newSize = CGSizeMake(CGImageGetWidth(imageRef)*scale, CGImageGetHeight(imageRef)*scale);
    CGRect newRect = CGRectIntegral(CGRectMake(0, 0, newSize.width, newSize.height));

    UIGraphicsBeginImageContextWithOptions(newSize, NO, 0);
    CGContextRef context = UIGraphicsGetCurrentContext();
    if (!context) {
        return nil;
    }

    // Set the quality level to use when rescaling
    CGContextSetInterpolationQuality(context, kCGInterpolationHigh);
    CGAffineTransform flipVertical = CGAffineTransformMake(1, 0, 0, -1, 0, newSize.height);

    CGContextConcatCTM(context, flipVertical);
    // Draw into the context; this scales the image

    CGContextDrawImage(context, newRect, imageRef);

    // release last image
    CGImageRelease(imageRef); //<-------- this image ref is copy from generator; should be released before next step creating;

    // Get the resized image from the context and a UIImage
    imageRef = CGBitmapContextCreateImage(context);

    UIGraphicsEndImageContext();
    #endif

    return imageRef;
}

@abrez
Copy link

abrez commented Sep 15, 2016

Hello Gang,

I am receiving memory leak on 4S in NSGIF at following line createGIFforTimePoints
if (!CGImageDestinationFinalize(destination))

any ideas ?

@smhjsw
Copy link
Contributor

smhjsw commented Sep 20, 2016

iOS 10 fix this bug.

@sebyddd
Copy link
Member

sebyddd commented Oct 7, 2016

I was aware of an actual memory leak in AVFoundation that caused the converting to fail in some cases. So far it looks like iOS 10 fixes this issue and conversions are working seamlessly.
Despite this, I will still keep the library >= iOS 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants