Fix Recycling of Animated Images#49850
Closed
NickGerleman wants to merge 1 commit into
Closed
Conversation
facebook-github-bot
added
CLA Signed
NickGerleman
force-pushed
the
export-D70668516
branch
4 times, most recently
from
d99245c to
cfd0eaf
Compare
Summary: `RCTUIImageViewAnimated` has some bugs around reuse. 1. Recycling will set `image` to null, which will no-op on comparison to `self.image`, but `self.image` is always null when the image is animated, because `RCTUIImageViewAnimated` handles rendering the frames itself. This means we don't properly do things like invalidating the DisplayLink when the image is first recycled. 2. If we ever set superclass image to nil, we make some change to the underlying CALayer, which causes the content to remain black, even though we customize our own `displayLayer`. Diffing layer descriptions, we seem to afterward have a `contentsMultiplyColor` and `contentsSwizzle` on the layer that aren't public. The solution I have in this diff is to, instead of drawing layers ourselves, update backing UIImage image to the frame. I think this would fix some other bugs as well, like tintColor not applying to animated images. My guess is that this shouldn't add too much extra work, since `UIImageView` should just be propagating the `UIImage` to the layer in a same way that we were before. This same bug may have also been possible before when switching between animated and non-animated image sources I think. Changelog: [iOS][Fixed] - Fix Recycling of Animated Images Reviewed By: cipolleschi, joevilches Differential Revision: D70668516
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D70668516 |
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D70668516 |
NickGerleman
force-pushed
the
export-D70668516
branch
from
cfd0eaf to
6c79a25
Compare
Contributor
|
This pull request has been merged in 1a9adfb. |
Collaborator
|
This pull request was successfully merged by @NickGerleman in 1a9adfb When will my fix make it into a release? | How to file a pick request? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
RCTUIImageViewAnimatedhas some bugs around reuse.Recycling will set
imageto null, which will no-op on comparison toself.image, butself.imageis always null when the image is animated, becauseRCTUIImageViewAnimatedhandles rendering the frames itself. This means we don't properly do things like invalidating the DisplayLink when the image is first recycled.If we ever set superclass image to nil, we make some change to the underlying CALayer, which causes the content to remain black, even though we customize our own
displayLayer. Diffing layer descriptions, we seem to afterward have acontentsMultiplyColorandcontentsSwizzleon the layer that aren't public.The solution I have in this diff is to, instead of drawing layers ourselves, update backing UIImage image to the frame. I think this would fix some other bugs as well, like tintColor not applying to animated images. My guess is that this shouldn't add too much extra work, since
UIImageViewshould just be propagating theUIImageto the layer in a same way that we were before. This same bug may have also been possible before when switching between animated and non-animated image sources I think.Changelog:
[iOS][Fixed] - Fix Recycling of Animated Images
Differential Revision: D70668516