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.

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

Fix a race condition during progressive animation load in SDAnimatedImageView #2435

Merged

Conversation

dreampiggy
Copy link
Contributor

@dreampiggy dreampiggy commented Aug 10, 2018

When the coder was updated, currentData may not be the same instance as previousdData. We should check the that current data is appended by previous data

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

This merge request fixes / reffers to the following issues: ...

Pull Request Description

This PR fix a race condition, for that 5.0 SDAnimatedImageView progressive animated image rendering. (See Animatied Image - Rendering)

Because of current implementation detail of SDAnimatedImage architecture, each SDAnimatedImage hold a decoder, the SDAnimatedImage.animatedImageData is just a proxy to call decoder's data). For progressive animation support, each progressive image during same request is created with the request-level decoder for performance. Here is a race condition that during progressive download, the previousData may be not the same as currentData instance. Because they share one coder, the coder can be updated just at the two retrive function call. So it will cause we reset the progressive decoding status.

And current implementation is hard-coded, to assume that SDAnimatedImage always return the same data, it's not correct. The correct judgement, in natural language, is that The new data should be appened from the old data with new bytes. For code, we should check that new data contains hole subdata of old data, and they start with the same begining bytes.

This bug can actually be reproduced with our 5.x demo. You can limit your network speed to 512KB, go push and pop the Image#2 several times (Clear cache each time it loads finished). You'll have a rare chance to see that the current playing animation is reset to the 0 frame.

…mageView.

When the coder was updated, currentData may not be the same instance as previousdData. We should check the that current data is appended by previous data
@dreampiggy dreampiggy force-pushed the bugfix_progressive_race_condition branch from 59b4aed to 6e79ef5 Compare August 10, 2018 05:19
@codecov-io
Copy link

codecov-io commented Aug 10, 2018

Codecov Report

Merging #2435 into 5.x will decrease coverage by 0.18%.
The diff coverage is 27.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##              5.x   #2435      +/-   ##
=========================================
- Coverage   67.69%   67.5%   -0.19%     
=========================================
  Files          47      47              
  Lines        6714    6721       +7     
=========================================
- Hits         4545    4537       -8     
- Misses       2169    2184      +15
Flag Coverage Δ
#iOS 68.44% <27.77%> (-0.02%) ⬇️
#macOS 67.2% <27.77%> (-0.41%) ⬇️
Impacted Files Coverage Δ
SDWebImage/SDAnimatedImageView.m 67.95% <27.77%> (-2.72%) ⬇️
SDWebImage/SDImageLoader.m 81% <0%> (-5%) ⬇️
SDWebImage/SDImageGIFCoder.m 78.43% <0%> (-1.57%) ⬇️
SDWebImage/SDImageAPNGCoder.m 41.97% <0%> (-0.83%) ⬇️
SDWebImage/UIView+WebCache.m 58.95% <0%> (-0.44%) ⬇️
SDWebImage/SDWebImageDownloaderOperation.m 87.54% <0%> (+3.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 802e19b...6e79ef5. Read the comment docs.

Copy link
Member

@bpoplauschi bpoplauschi left a comment

Choose a reason for hiding this comment

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

Looks good to me

@bpoplauschi bpoplauschi merged commit 13207dd into SDWebImage:5.x Aug 10, 2018
@bpoplauschi bpoplauschi deleted the bugfix_progressive_race_condition branch August 10, 2018 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants