Xcode - iOS Memory leak that is driving me crazy I think it's NSNotificationCenter, but hoping fresh eyes can see what I can't
The code here is a modal view launched from the RootViewController and is to display a video with a thumbnail filmstrip below the movie and then timed instructions bound to the movie.
It all works, but there is a memory leak / lack of release that I just can't see for looking and having spent three days trying to fix it, the time has come to ask for help...
If I disable the NSNotificationCenter by commenting it out (highlighted in the .m) I don't have any issues regarding memory and keep the timed text. But I also don't have any thumbnails. I have tried inserting [[NSNotificationCenter alloc] removeObserver:self];
at numerous points to see if that will get rid of it for me. But alas, to no avail.
I have also tried releasing the 'backgroundTimer' but it's not overly impressed when I try to compile and run.
In essence, the first time I load the modal view, there are no issues whatsoever and all seems great - However, if I close it with the -(IBAction)close:(id)sender;
it seems something isn't releasing as the next time I launch the same page the memory usage increases by about 30% (roughly the amount that is used by the thumbnail generation) and increases by roughly the same amount each time I re-launch the modal view.
Please bare in mind I am a newbie to this and the error is likely to a bloody stupid one to those of you in the know. But in the interest of getting this project done, I'll gladly take any abuse you fancy throwing at me.
Here's the code:
.h
#import <UIKit/UIKit.h>
#import <MediaPlayer/MPMoviePlayerController.h>
#import "ImageViewWithTime.h"
#import "CommentView.h"
@interface SirloinVideoViewController_iPad : UIViewController {
UIView *landscapeView;
UIView *viewForMovie;
MPMoviePlayerController *player;
UILabel *onScreenDisplayLabel;
UIScrollView *myScrollView;
NSMutableArray *keyframeTimes;
NSArray *shoutOutTexts;
NSArray *shoutOutTimes;
NSTimer *backgroundTimer;
UIView *instructions;
}
-(IBAction)close:(id)sender;
-(IBAction)textInstructions:(id)sender;
@property (nonatomic, retain) IBOutlet UIView *instructions;
@property (nonatomic, retain) NSTimer *theTimer;
@property (nonatomic, retain) NSTimer *backgroundTimer;
@property (nonatomic, retain) IBOutlet UIView *viewForMovie;
@property (nonatomic, retain) MPMoviePlayerController *player;
@property (nonatomic, retain) IBOutlet UILabel *onScreenDisplayLabel;
@property (nonatomic, retain) IBOutlet UIScrollView *myScrollView;
@property (nonatomic, retain) NSMutableArray *keyframeTimes;
-(NSURL *)movieURL;
- (void) playerThumbnailImageRequestDidFinish:(NSNotification*)notification;
- (ImageViewWithTime *)makeThumbnailImageViewFromImage:(UIImage *)image andTimeCode:(NSNumber *)timecode;
- (void)handleTapFrom:(UITapGestureRecognizer *)recognizer;
@end
.m
#import "SirloinVideoViewController_iPad.h"
#import "SirloinTextViewController.h"
@implementation SirloinVideoViewController_iPad
@synthesize theTimer, backgroundTimer, viewForMovie, player,
onScreenDisplayLabel, myScrollView, keyframeTimes, instructions;
- (id)initWithNibName:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil
{
self = [super initWithNibName:nibNameOrNil bundle:nibBundleOrNil];
if (self) {
}
return self;
[nibNameOrNil release];
[nibBundleOrNil release];
}
- (IBAction)close:(id)sender{
[self.parentViewController dismissModalViewControllerAnimated:YES];
[player stop];
[player release];
[theTimer invalidate];
[theTimer release];
[backgroundTimer invalidate];
[SirloinVideoViewController_iPad release];
}
—
-(IBAction)textInstructions:(id)sender {
SirloinTextViewController *vController = [[SirloinTextViewController alloc] initWithNibName:nil bundle:nil];
[self presentModalViewController:vController animated:YES];
[vController release];
}
- (void)viewDidLoad {
[super viewDidLoad];
keyframeTimes = [[NSMutableArray alloc] init];
shoutOutTexts = [[NSArray
arrayWithObjects:
@"1. XXXXXXXXXXXX",
@"2. XXXXXXXXXXXX",
@"3. XXXXXXXXXXXX",
@"4. XXXXXXXXXXXX",
@"5. XXXXXXXXXXXX",
@"6. XXXXXXXXXXXX"
@"7. XXXXXXXXXXXX",
@"8. XXXXXXXXXXXX",
@"9. XXXXXXXXXXXX",
@"10. XXXXXXXXXXXX",
@"11. XXXXXXXXXXXX",
@"12. XXXXXXXXXXXX",
@"13. XXXXXXXXXXXX",
@"14. XXXXXXXXXXXX",
@"15. XXXXXXXXXXXX",
nil] retain];
shoutOutTimes = [[NSArray
arrayWithObjects:
[[NSNumber alloc] initWithInt: 1],
[[NSNumber alloc] initWithInt: 73],
[[NSNumber alloc] initWithInt: 109],
[[NSNumber alloc] initWithInt: 131],
[[NSNumber alloc] initWithInt: 205],
[[NSNumber alloc] initWithInt: 250],
[[NSNumber alloc] initWithInt: 337],
[[NSNumber alloc] initWithInt: 378],
[[NSNumber alloc] initWithInt: 402],
[[NSNumber alloc] initWithInt: 420],
[[NSNumber alloc] initWithInt: 448],
[[NSNumber alloc] initWithInt: 507],
[[NSNumber alloc] 开发者_如何学运维initWithInt: 531],
[[NSNumber alloc] initWithInt: 574],
nil] retain];
self.player = [[MPMoviePlayerController alloc] init];
self.player.contentURL = [self movieURL];
self.player.view.frame = self.viewForMovie.bounds;
self.player.view.autoresizingMask =
UIViewAutoresizingFlexibleWidth |
UIViewAutoresizingFlexibleHeight;
[self.viewForMovie addSubview:player.view];
backgroundTimer = [NSTimer scheduledTimerWithTimeInterval:0.5f target:self selector:@selector(timerAction:) userInfo:nil repeats:YES];
[self.view addSubview:self.myScrollView];
//I am pretty sure that this is the culprit - Just not sure why...
[[NSNotificationCenter defaultCenter]
addObserver:self
selector:@selector(movieDurationAvailable:)
name:MPMovieDurationAvailableNotification
object:theTimer];
//Could be wrong, but when commented out I don't have the memory issues
}
—
- (NSInteger)positionFromPlaybackTime:(NSTimeInterval)playbackTime
{
NSInteger position = 0;
for (NSNumber *startsAt in shoutOutTimes)
{
if (playbackTime > [startsAt floatValue])
{
++position;
}
}
return position;
}
-(NSURL *)movieURL
{
NSBundle *bundle = [NSBundle mainBundle];
NSString *moviePath =
[bundle
pathForResource:@"sirloin"
ofType:@"m4v"];
if (moviePath) {
return [NSURL fileURLWithPath:moviePath];
} else {
return nil;
}
}
NSTimeInterval lastCheckAt = 0.0;
- (void)timerAction: theTimer
{
int count = [shoutOutTimes count];
NSInteger position = [self positionFromPlaybackTime:self.player.currentPlaybackTime];
NSLog(@"position is at %d", position);
if (position > 0)
{
--position;
}
if (position < count)
{
NSNumber *timeObj = [shoutOutTimes objectAtIndex:position];
int time = [timeObj intValue];
NSLog(@"shout scheduled for %d", time);
NSLog(@"last check was at %g", lastCheckAt);
NSLog(@"current playback time is %g", self.player.currentPlaybackTime);
if (lastCheckAt < time && self.player.currentPlaybackTime >= time)
{
NSString *shoutString = [shoutOutTexts objectAtIndex:position];
NSLog(@"shouting: %@", shoutString);
CommentView *cview = [[CommentView alloc] initWithText:shoutString];
[self.instructions addSubview:cview];
[shoutString release];
}
}
lastCheckAt = self.player.currentPlaybackTime;
}
// Override to allow orientations other than the default portrait orientation.
- (BOOL)shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation {
return YES;
}
-(void)removeObserver:(NSObject *)observer forKeyPath:(NSString *)keyPath {
[[NSNotificationCenter defaultCenter] removeObserver:MPMovieDurationAvailableNotification];
[[NSNotificationCenter defaultCenter] removeObserver:MPMoviePlayerThumbnailImageRequestDidFinishNotification];
[keyPath release];
}
- (void) movieDurationAvailable:(NSNotification*)notification {
float duration = [self.player duration];
[[NSNotificationCenter defaultCenter]
addObserver:self
selector:@selector(playerThumbnailImageRequestDidFinish:)
name:MPMoviePlayerThumbnailImageRequestDidFinishNotification
object:nil];
NSMutableArray *times = [[NSMutableArray alloc] init];
for(int i = 0; i < 20; i++) {
float playbackTime = i * duration/20;
[times addObject:[NSNumber numberWithInt:playbackTime]];
}
[self.player
requestThumbnailImagesAtTimes:times
timeOption: MPMovieTimeOptionExact];
}
- (void) playerThumbnailImageRequestDidFinish:(NSNotification*)notification {
NSDictionary *userInfo = [notification userInfo];
NSNumber *timecode =
[userInfo objectForKey: MPMoviePlayerThumbnailTimeKey];
UIImage *image =
[userInfo objectForKey: MPMoviePlayerThumbnailImageKey];
ImageViewWithTime *imageView =
[self makeThumbnailImageViewFromImage:image andTimeCode:timecode];
[myScrollView addSubview:imageView];
UITapGestureRecognizer *tapRecognizer =
[[UITapGestureRecognizer alloc]
initWithTarget:self action:@selector(handleTapFrom:)];
[tapRecognizer setNumberOfTapsRequired:1];
[imageView addGestureRecognizer:tapRecognizer];
[tapRecognizer release];
[image release];
[imageView release];
}
- (void)handleTapFrom:(UITapGestureRecognizer *)recognizer {
ImageViewWithTime *imageView = (ImageViewWithTime *) recognizer.view;
self.player.currentPlaybackTime = [imageView.time floatValue];
}
- (ImageViewWithTime *)makeThumbnailImageViewFromImage:(UIImage *)image andTimeCode:(NSNumber *)timecode {
float timeslice = self.player.duration / 3.0;
int pos = [timecode intValue] / (int)timeslice;
float width = 75 *
((float)image.size.width / (float)image.size.height);
self.myScrollView.contentSize =
CGSizeMake((width + 2) * 13, 75);
ImageViewWithTime *imageView =
[[ImageViewWithTime alloc] initWithImage:image];
[imageView setUserInteractionEnabled:YES];
[imageView setFrame:CGRectMake(pos * width + 2, 0, width, 75.0f)];
imageView.time = [[NSNumber alloc] initWithFloat:(pos * timeslice)];
return imageView;
[myScrollView release];
}
- (void)dealloc {
[player release];
[viewForMovie release];
[onScreenDisplayLabel release];
[keyframeTimes release];
[instructions release];
[shoutOutTexts release];
[shoutOutTimes release];
[super dealloc];
}
@end
This app is already out there heavily using UIWebView (which just plain sux) so I am trying to things right and do it properly.
You do have a couple more issues than one leak.
The first one
is in your initWithNibName:bundle:
as you aren't doing anything useful there: get rid of it, entirely! (Besides: don't release arguments, that are passed to your methods! Luckily, you've placed those releases in lines that are unreachable, i.e. after the return statement...)
Next method, next problems
- Why are you sending
release
to a class object? Don't! That's wrong on many levels. - You have decided to create properties for your timers. That's nothing bad per se. But why then, are you using the ivars directly here? I'd strongly encourage you to implement
setTheTimer:
andsetBackgroundTimer:
to handle the invalidation and release properly and simply doself.theTimer = nil; self.backgroundTimer = nil;
here. That would fix the asymmetry in handling those things, as well. (By the way: theTimer isn't such a great name for an ivar...especially when there is another ivar that is a timer!)
textInstructions:
looks unsuspicious but...
viewDidLoad
has some more issues
- It leaks an MPMoviePlayerController:
The@property
will retain it, so you need to balance thealloc
here. backgroundTimer
has a corresponding@property
that is declared to be retaining: You are violating this API contract here, since you only assign the timer to the ivar. Useself.backgroundTimer = ...
instead.- From all the code you posted, it seems to me that passing
theTimer
as the last argument in your call to-[NSNotificationCenter addObserver:selector:name:object:]
is a fancy way of passing innil
as that parameter. Which is kind of good, because usuallyNSTimer
doesn't post too manyMPMovieDurationAvailableNotification
s. In fact, as I can't seetheTimer
being used except inclose:
: Could it be that this is just a useless remnant from before you introduced thebackgroundTimer
ivar/@property? (Well there is another occurrence of a variable of that name, but it should be accompanied by a big fat compiler warning...) - Do you, in any way, implement
viewDidUnload
? If so, does it:self.player = nil;
?[shoutOutTexts release], shoutOutTexts = nil;
?[shoutOutTimes release], shoutOutTimes = nil;
?self.keyframeTimes = nil;
?[[NSNotificationCenter defaultCenter] removeObserver:self name: MPMovieDurationAvailableNotification object:nil];
?self.backgroundTimer = nil;
? (Assuming,setBackgroundTimer:
releases and invalidates the old value)
- Update I've missed this one on the first go: You are leaking 15
NSNumber
s here. Use[NSNumber numberWithInt:]
instead of alloc/init in the setup ofshoutOutTimes
.
A minor remark on movieURL
, which you can turn into the following one-liner:
-(NSURL*)movieURL {
return [[NSBundle mainBundle] URLForResource:@"sirloin" withExtension:@"m4v"];
}
And then this
NSTimeInterval lastCheckAt = 0.0;
within the global scope. From your usage of it: ivar PLZ?!?one?
More issues later. I gotta get myself something to eat first.
Part Two
Now let's get into timerAction:
The first issue is not too grave, — especially in this particular context — but you should be aware that -[NSArray count]
returns an NSUInteger
and that the U is not a typo, but a designation that this value is unsigned. You certainly won't run into problems with the signedness in this App and rarely do on other occasions, but when you do, they make up for really funky bugs and you should be aware of the implications...
The real issue with this method is, however, that you are leaking one CommentView
per iteration while — at the same time — overreleasing one NSString
. The fact, that you were using string literals (which will never be dealloced) in the first place, (i.e. when you were initializing shoutOutTimes) totally saves your butt, here.
Next up: removeObserver:forKeyPath:
You really should get rid of that really bad habit of releasing parameters, that are passed to your methods!
That being said, get rid of the entirety of this method!
First and foremost removeObserver:forKeyPath:
is a method from the NSKeyValueObserving
informal protocol and plays a totally different role than what you are (ab-)using it to accomplish here.
Secondly, it is one of those methods where it is essential to call through to super
if — by any means — you really need to override it. (Well, except, when you were overriding addObserver:forKeyPath:options:context:
as well and-it-should-go-without-saying-that-you-shouldn't-do-that-unless-you-really-know-what-you-are-doing-if-you-ever-planned-on-using-KVO.)
movieDurationAvailable:
Like Evan said, you're leaking times
here. Go for his suggestion or — instead — make it NSMutableArray *times = [NSMutableArray array];
and you are done here.
playerThumbnailImageRequestDidFinish:
You don't own image
, so don't release it!
Personally, I would finish setting up the view (i.e. add the recognizer and do stuff like that) before adding it into the view-hierarchy, but that's completely a matter of taste...
makeThumbnailImageViewFromImage:andTimeCode:
...leaks an NSNumber
(use [NSNumber numberWithFloat:(pos * timeslice)]
instead of the alloc/initWithFloat:
-dance) and prevents you from crashing due to overreleasing myScrollView
by the unconditional return statement that directly precedes it (phew!).
While we're at it: rename this method to either newThumbnailImageView...
so that when you'll revisit this code in a year or so, you instantly know that [imageView release];
at the bottom of playerThumbnailImageRequestDidFinish:
really is necessary, without having to look at the implementation of this method.
Alternatively, you could rename it to thumbnailImageView...
and change the return statement to return [imageView autorelease];
. Bonus: one fewer line in playerThumbnailImageRequestDidFinish:
as the [imageView release];
there becomes obsolete then.
dealloc
Add [[NSNotificationCenter defaultCenter] removeObserver:self];
at the very top.
The rest of it looks okay. (Although I find it odd, that the ivar landscapeView
is never/nowhere mentioned apart from its declaration.)
Summary
Read the sections Memory Management Rules and Autorelease from Apple's "Memory Management Programming Guide" once again. They're pure gold!
You never release times
in movieDurationAvailable:
:
NSMutableArray *times = [[NSMutableArray alloc] init];
You should use autorelease
when you pass it to the method:
[self.player requestThumbnailImagesAtTimes:[times autorelease] timeOption: MPMovieTimeOptionExact];
精彩评论