A couple performance tweaks for animated images #trivial #634
Conversation
|
🚫 CI failed with log |
Source/ASImageNode+AnimatedImage.mm
Outdated
| @@ -53,6 +53,14 @@ - (void)_locked_setAnimatedImage:(id <ASAnimatedImageProtocol>)animatedImage | |||
| if (ASObjectIsEqual(_animatedImage, animatedImage)) { | |||
| return; | |||
| } | |||
|
|
|||
| if (animatedImage == nil) { | |||
There was a problem hiding this comment.
I don't understand this check. Shouldn't we do this whenever _animatedImage != nil?
There was a problem hiding this comment.
Yeeeeees, good catch.
Source/ASImageNode+AnimatedImage.mm
Outdated
| if (animatedImage == nil) { | ||
| // Animated image can take while to dealloc, do it off the main queue | ||
| __block id <ASAnimatedImageProtocol>deallocImage = _animatedImage; | ||
| ASPerformBlockOnBackgroundThread(^{ |
There was a problem hiding this comment.
For consistency, let's use ASPerformBackgroundDeallocation(). Also, let's use previousAnimatedImage declared a few lines below.
There was a problem hiding this comment.
Lastly, I think it's easier to reason if we do this at the end of this method.
| if (ASObjectIsEqual(strongSelf->_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) { | ||
| return; | ||
| } | ||
| ASPerformBlockOnBackgroundThread(^{ |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
@garrettmoon There are conflicts that I'm not confident in fixing. Please resolve them and merge away. |
Source/ASNetworkImageNode.mm
Outdated
|
|
||
| if (imageContainer != nil) { | ||
| [strongSelf _locked_setCurrentImageQuality:1.0]; | ||
| if ([imageContainer asdk_animatedImageData] && strongSelf->_downloaderFlags.downloaderImplementsAnimatedImage) { |
There was a problem hiding this comment.
You could move up calling asdk_animatedImageData and assign it to a variable so you don't have to call it twice if this evaluates to true. Don't know how heavy asdk_animatedImageData is though.
|
@garrettmoon Ping :) |
|
@nguyenhuy Ah! Will resolve and land today, thank you all for the review! |
garrettmoon
force-pushed
the
animatedImagePerformanceTweaks
branch
from
9d82d03
to
4d020db
Compare
|
🚫 CI failed with log |
garrettmoon
changed the title
Source/ASImageNode+AnimatedImage.mm
Outdated
| if (previousAnimatedImage != nil) { | ||
| ASPerformBackgroundDeallocation(&previousAnimatedImage); | ||
| } | ||
|
|
||
| [self animatedImageSet:_animatedImage previousAnimatedImage:previousAnimatedImage]; |
There was a problem hiding this comment.
previousAnimatedImage will be nilled out by this function. Call this after calling -animatedImageSet:previousAnimatedImage:
…p#634) * A couple performance tweaks for animated images * @nguyenhuy's comments * Avoid calling animatedImageData twice. Thanks @maicki. * Fix call to background deallocation * Good catch by @Adlai-Holler
No description provided.