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.

Sign up for GitHub

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

Jump to bottom

A couple performance tweaks for animated images #trivial #634

Merged
merged 5 commits into from
Dec 4, 2017
Merged

A couple performance tweaks for animated images #trivial #634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ebfa855
A couple performance tweaks for animated images
garrettmoon Oct 23, 2017
1fcd529
@nguyenhuy's comments
garrettmoon Nov 17, 2017
4d020db
Avoid calling animatedImageData twice. Thanks @maicki.
garrettmoon Dec 4, 2017
1f300ad
Fix call to background deallocation
garrettmoon Dec 4, 2017
bdf684c
Good catch by @Adlai-Holler
garrettmoon Dec 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Source/ASImageNode+AnimatedImage.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ - (void)_locked_setAnimatedImage:(id <ASAnimatedImageProtocol>)animatedImage
}

id <ASAnimatedImageProtocol> previousAnimatedImage = _animatedImage;

_animatedImage = animatedImage;

if (animatedImage != nil) {
Expand All @@ -80,6 +81,11 @@ - (void)_locked_setAnimatedImage:(id <ASAnimatedImageProtocol>)animatedImage
}

[self animatedImageSet:_animatedImage previousAnimatedImage:previousAnimatedImage];

// Animated image can take while to dealloc, do it off the main queue
if (previousAnimatedImage != nil) {
ASPerformBackgroundDeallocation(&previousAnimatedImage);
}
}

- (void)animatedImageSet:(id <ASAnimatedImageProtocol>)newAnimatedImage previousAnimatedImage:(id <ASAnimatedImageProtocol>)previousAnimatedImage
Expand Down
104 changes: 58 additions & 46 deletions Source/ASNetworkImageNode.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,55 +713,67 @@ - (void)_lazilyLoadImageIfNecessary
} else {
__weak __typeof__(self) weakSelf = self;
auto finished = ^(id <ASImageContainerProtocol>imageContainer, NSError *error, id downloadIdentifier, ASNetworkImageSource imageSource) {

__typeof__(self) strongSelf = weakSelf;
if (strongSelf == nil) {
return;
}

as_log_verbose(ASImageLoadingLog(), "Downloaded image for %@ img: %@ urls: %@", self, [imageContainer asdk_image], URLs);

// Grab the lock for the rest of the block
ASDN::MutexLocker l(strongSelf->__instanceLock__);

//Getting a result back for a different download identifier, download must not have been successfully canceled
if (ASObjectIsEqual(strongSelf->_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) {
return;
}
ASPerformBlockOnBackgroundThread(^{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, the only change here is that the (previous) finished block is now executed on a background thread? (it's a bit hard to read)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly. Everything before was happening on main. Now it's being dispatched to the background and once it's done the work, the delegate callbacks are dispatched to main.

__typeof__(self) strongSelf = weakSelf;
if (strongSelf == nil) {
return;
}

//No longer in preload range, no point in setting the results (they won't be cleared in exit preload range)
if (ASInterfaceStateIncludesPreload(self->_interfaceState) == NO) {
return;
}

if (imageContainer != nil) {
[strongSelf _locked_setCurrentImageQuality:1.0];
if ([imageContainer asdk_animatedImageData] && strongSelf->_downloaderFlags.downloaderImplementsAnimatedImage) {
id animatedImage = [strongSelf->_downloader animatedImageWithData:[imageContainer asdk_animatedImageData]];
[strongSelf _locked_setAnimatedImage:animatedImage];
} else {
[strongSelf _locked__setImage:[imageContainer asdk_image]];
as_log_verbose(ASImageLoadingLog(), "Downloaded image for %@ img: %@ urls: %@", self, [imageContainer asdk_image], URLs);

// Grab the lock for the rest of the block
ASDN::MutexLocker l(strongSelf->__instanceLock__);

//Getting a result back for a different download identifier, download must not have been successfully canceled
if (ASObjectIsEqual(strongSelf->_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) {
return;
}
strongSelf->_imageLoaded = YES;
}

strongSelf->_downloadIdentifier = nil;
strongSelf->_cacheUUID = nil;

if (imageContainer != nil) {
if (strongSelf->_delegateFlags.delegateDidLoadImageWithInfo) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
ASNetworkImageNodeDidLoadInfo info = {};
info.imageSource = imageSource;
[delegate imageNode:strongSelf didLoadImage:strongSelf.image info:info];
} else if (strongSelf->_delegateFlags.delegateDidLoadImage) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
[delegate imageNode:strongSelf didLoadImage:strongSelf.image];

//No longer in preload range, no point in setting the results (they won't be cleared in exit preload range)
if (ASInterfaceStateIncludesPreload(self->_interfaceState) == NO) {
return;
}
} else if (error && strongSelf->_delegateFlags.delegateDidFailWithError) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
[delegate imageNode:strongSelf didFailWithError:error];
}

if (imageContainer != nil) {
[strongSelf _locked_setCurrentImageQuality:1.0];
NSData *animatedImageData = [imageContainer asdk_animatedImageData];
if (animatedImageData && strongSelf->_downloaderFlags.downloaderImplementsAnimatedImage) {
id animatedImage = [strongSelf->_downloader animatedImageWithData:animatedImageData];
[strongSelf _locked_setAnimatedImage:animatedImage];
} else {
[strongSelf _locked__setImage:[imageContainer asdk_image]];
}
strongSelf->_imageLoaded = YES;
}

strongSelf->_downloadIdentifier = nil;
strongSelf->_cacheUUID = nil;

ASPerformBlockOnMainThread(^{
__typeof__(self) strongSelf = weakSelf;
if (strongSelf == nil) {
return;
}

// Grab the lock for the rest of the block
ASDN::MutexLocker l(strongSelf->__instanceLock__);

if (imageContainer != nil) {
if (strongSelf->_delegateFlags.delegateDidLoadImageWithInfo) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
ASNetworkImageNodeDidLoadInfo info = {};
info.imageSource = imageSource;
[delegate imageNode:strongSelf didLoadImage:strongSelf.image info:info];
} else if (strongSelf->_delegateFlags.delegateDidLoadImage) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
[delegate imageNode:strongSelf didLoadImage:strongSelf.image];
}
} else if (error && strongSelf->_delegateFlags.delegateDidFailWithError) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
[delegate imageNode:strongSelf didFailWithError:error];
}
});
});
};

// As the _cache and _downloader is only set once in the intializer we don't have to use a
Expand Down