Async image loading from url inside a UITableView cell - image changes to wrong image while scrolling

Question

I've written two ways to async load pictures inside my UITableView cell. In both cases the image will load fine but when I'll scroll the table the images will change a few times until the scroll will end and the image will go back to the right image. I have no idea why this is happening.

#define kBgQueue dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)

- (void)viewDidLoad
{
    [super viewDidLoad];
    dispatch_async(kBgQueue, ^{
        NSData* data = [NSData dataWithContentsOfURL: [NSURL URLWithString:
                                                       @"http://myurl.com/getMovies.php"]];
        [self performSelectorOnMainThread:@selector(fetchedData:)
                               withObject:data waitUntilDone:YES];
    });
}

-(void)fetchedData:(NSData *)data
{
    NSError* error;
    myJson = [NSJSONSerialization
              JSONObjectWithData:data
              options:kNilOptions
              error:&error];
    [_myTableView reloadData];
}    

- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView {
    // Return the number of sections.
    return 1;
}

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section{
    // Return the number of rows in the section.
    // Usually the number of items in your array (the one that holds your list)
    NSLog(@"myJson count: %d",[myJson count]);
    return [myJson count];
}
    - (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath{

        myCell *cell = [tableView dequeueReusableCellWithIdentifier:@"cell"];
        if (cell == nil) {
            cell = [[myCell alloc] initWithStyle:UITableViewCellStyleDefault reuseIdentifier:@"cell"];
        }

        dispatch_async(kBgQueue, ^{
        NSData *imgData = [NSData dataWithContentsOfURL:[NSURL URLWithString:[NSString stringWithFormat:@"http://myurl.com/%@.jpg",[[myJson objectAtIndex:indexPath.row] objectForKey:@"movieId"]]]];

            dispatch_async(dispatch_get_main_queue(), ^{
        cell.poster.image = [UIImage imageWithData:imgData];
            });
        });
         return cell;
}

... ...

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath{

            myCell *cell = [tableView dequeueReusableCellWithIdentifier:@"cell"];
            if (cell == nil) {
                cell = [[myCell alloc] initWithStyle:UITableViewCellStyleDefault reuseIdentifier:@"cell"];
            }
    NSURL* url = [NSURL URLWithString:[NSString stringWithFormat:@"http://myurl.com/%@.jpg",[[myJson objectAtIndex:indexPath.row] objectForKey:@"movieId"]]];
    NSURLRequest* request = [NSURLRequest requestWithURL:url];


    [NSURLConnection sendAsynchronousRequest:request
                                       queue:[NSOperationQueue mainQueue]
                           completionHandler:^(NSURLResponse * response,
                                               NSData * data,
                                               NSError * error) {
                               if (!error){
                                   cell.poster.image = [UIImage imageWithData:data];
                                   // do whatever you want with image
                               }

                           }];
     return cell;
}

You're trying to store information in the actual cells. This is bad, very bad. You should store information in n array (or something similar) and then display it in the cells. The information in this case is the actual UIImage. Yes load it asynchronously but load it into an array. — Fogmeister
@Fogmeister Are you referring to poster? That's presumably an imageview in his custom cell, so what EXEC_BAD_ACCESS is doing is perfectly right. You are correct that you should not use the cell as the repository for model data, but I don't think that's what he's doing. He's just giving the custom cell what it needs to present itself. Furthermore, and this is a more subtle issue, I would be wary about storing an image, itself, in your model array backing your tableview. It's better to use a image caching mechanism and your model object should retrieve from that cache. — Rob
Yes, exactly my point. Looking at the request (which is shown in full) he is downloading the image asynchronously and putting it directly into the imageView in the cell. (Thus using the cell to store the data, i.e. the image). What he should be doing is referencing an object and requesting the image from that object (contained in an array or somewhere). If the object doesn't yet have the image it should return a placeholder and download the image. Then when the image is downloaded and ready to display let the table know so it can update the cell (if it's visible). — Fogmeister
What he is doing will force the download every single time he scrolls to that cell in the table. Whether the images are stored persistently is up to him, but at least store them for the life time of the tableview. — Fogmeister
Exactly :D That way you only need to fetch the image from the URL once. You will see this on things like Facebook Friend Picker. When you start it all the avatars are grey placeholders. Then as you scroll they all fill in as it moves along. But then when you scroll back to a cell previously shown it will instantly show the already downloaded image. — Fogmeister

Rob Rob · Accepted Answer · 2013-05-21T06:47:50

Assuming you're looking for a quick tactical fix, what you need to do is make sure the cell image is initialized and also that the cell's row is still visible, e.g:

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {
    MyCell *cell = [tableView dequeueReusableCellWithIdentifier:@"cell" forIndexPath:indexPath];

    cell.poster.image = nil; // or cell.poster.image = [UIImage imageNamed:@"placeholder.png"];

    NSURL *url = [NSURL URLWithString:[NSString stringWithFormat:@"http://myurl.com/%@.jpg", self.myJson[indexPath.row][@"movieId"]]];

    NSURLSessionTask *task = [[NSURLSession sharedSession] dataTaskWithURL:url completionHandler:^(NSData * _Nullable data, NSURLResponse * _Nullable response, NSError * _Nullable error) {
        if (data) {
            UIImage *image = [UIImage imageWithData:data];
            if (image) {
                dispatch_async(dispatch_get_main_queue(), ^{
                    MyCell *updateCell = (id)[tableView cellForRowAtIndexPath:indexPath];
                    if (updateCell)
                        updateCell.poster.image = image;
                });
            }
        }
    }];
    [task resume];

    return cell;
}

The above code addresses a few problems stemming from the fact that the cell is reused:

You're not initializing the cell image before initiating the background request (meaning that the last image for the dequeued cell will still be visible while the new image is downloading). Make sure to nil the image property of any image views or else you'll see the flickering of images.
A more subtle issue is that on a really slow network, your asynchronous request might not finish before the cell scrolls off the screen. You can use the UITableView method cellForRowAtIndexPath: (not to be confused with the similarly named UITableViewDataSource method tableView:cellForRowAtIndexPath:) to see if the cell for that row is still visible. This method will return nil if the cell is not visible.

The issue is that the cell has scrolled off by the time your async method has completed, and, worse, the cell has been reused for another row of the table. By checking to see if the row is still visible, you'll ensure that you don't accidentally update the image with the image for a row that has since scrolled off the screen.
Somewhat unrelated to the question at hand, I still felt compelled to update this to leverage modern conventions and API, notably:
- Use NSURLSession rather than dispatching -[NSData contentsOfURL:] to a background queue;
- Use dequeueReusableCellWithIdentifier:forIndexPath: rather than dequeueReusableCellWithIdentifier: (but make sure to use cell prototype or register class or NIB for that identifier); and
- I used a class name that conforms to Cocoa naming conventions (i.e. start with the uppercase letter).

Even with these corrections, there are issues:

The above code is not caching the downloaded images. That means that if you scroll an image off screen and back on screen, the app may try to retrieve the image again. Perhaps you'll be lucky enough that your server response headers will permit the fairly transparent caching offered by NSURLSession and NSURLCache, but if not, you'll be making unnecessary server requests and offering a much slower UX.
We're not canceling requests for cells that scroll off screen. Thus, if you rapidly scroll to the 100th row, the image for that row could be backlogged behind requests for the previous 99 rows that aren't even visible anymore. You always want to make sure you prioritize requests for visible cells for the best UX.

The simplest fix that addresses these issues is to use a UIImageView category, such as is provided with SDWebImage or AFNetworking. If you want, you can write your own code to deal with the above issues, but it's a lot of work, and the above UIImageView categories have already done this for you.

Async image loading from url inside a UITableView cell - image changes to wrong image while scrolling

13 Answers

Swift 3

ImageCacheLoader.swift

Usage example