NSRare / NSGIF

🔮 iOS Library for converting videos to animated GIFs.
MIT License
924 stars 190 forks source link

Gif Conversion creates memory leak. #9

Closed cassidyclawson closed 7 years ago

cassidyclawson commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

khappucino commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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:

ksenia-lyagusha commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Hello Gang,

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

any ideas ?

smhjsw commented 8 years ago

iOS 10 fix this bug.

sebyddd commented 7 years ago

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.