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

#1951: Fix animated WebP decoding issue, including canvas size, the support for dispose method and the duration per frame #1952

Merged
merged 8 commits into from Jul 31, 2017

Conversation

Projects
None yet
4 participants
@dreampiggy
Contributor

dreampiggy commented Jun 30, 2017

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 - add an new test for ensure animated image array share the same size

  • I have updated the documentation (if necessary) - added sd_webpLoopCount(UIImage+WebP.h)

  • 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

SDWebImage 4.0 add support for animated WebP images. And the UIImage+WebP category use +[UIImage animatedImageWithImages:duration:] to create animated image.

Fixed Bugs:

  1. While decoding animated WebP image, for each frame, you need to draw the frame image on the canvas. The older version just use sd_rawWepImageWithData to draw, but the config.input.width/height for this frame does not refer the total image size, it refer to this frame's canvas size(can be smaller than image size). So finally this will lead the final UIImage array (which is used to build animated image) have different size, cause UIImage render strange.

  2. CGBitmapInfo should not use 0 for non-alpha channel, you should explicit assign it to kCGImageAlphaNoneSkipLast (with the byte-order bit kCGBitmapByteOrder32Big)

  3. The older version animated WeP decoding does not concentrate any diposal method. Which will cause animated WebP images render issue for many images. To fix this, I add a CGBitmapContext before the loop and share it and draw the frames. For each frame, it will clear the frame rect in context only when the background diposal method.

  4. +[UIImage animatedImageWithImages:duration:] just use the average duration for each frame, which is not the animated WebP standard. Animated WebP can has different duration per frame. Therefore, I divide the total duration and add image item multiple times to solve this issue.

Known Issues:

  1. Currently all the animated WebP showing animation infinity. To solve this, we can get the loop count and set UIImageView's animationRepeatCount property. But UIImageView need use this property with both animationImages and animationDuration. Only set image with an animated UIImage and animationDuration will not work. However sometimes auto-set this property may break user code. So I just add an new property called sd_webpLoopCount at UIImage+WebP.h to get the loop count from WebP data. For nil or static WebP, the default value is 0, otherwise it should become animated WebP loop count.

dreampiggy added some commits Jun 29, 2017

Fixed animated WebP image frame size wrong
Fix animated WebP image decoder bug causing rendering canvas in current canvas size context but not image size context for multiple frame.
Fixed CGContextBitmapCreate bitmapInfo and fix typo
Fixed CGContextBitmapCreate with wrong bitmapInfo if has no alpha channel
Rename sd_rawWepImageWithData to sd_rawWebpImageWithData
Add support to animted WebP dispose method
dispose method is widely used in animated WebP images and should add support
@codecov-io

This comment has been minimized.

codecov-io commented Jun 30, 2017

Codecov Report

Merging #1952 into master will increase coverage by 0.82%.
The diff coverage is 79.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1952      +/-   ##
==========================================
+ Coverage    76.6%   77.42%   +0.82%     
==========================================
  Files          27       27              
  Lines        2513     2631     +118     
==========================================
+ Hits         1925     2037     +112     
- Misses        588      594       +6
Impacted Files Coverage Δ
Tests/Tests/UIImageMultiFormatTests.m 100% <100%> (ø) ⬆️
SDWebImage/UIImage+WebP.m 75% <72.89%> (+31.7%) ⬆️
SDWebImage/SDWebImageCompat.m 88.23% <90%> (+0.23%) ⬆️
SDWebImage/SDImageCache.m 70.63% <0%> (-3.97%) ⬇️

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 041842b...e3ed82f. Read the comment docs.

@dreampiggy dreampiggy changed the title from Fix animated WebP decoding issue, including canvas size and the support for dispose method to #1951 Fix animated WebP decoding issue, including canvas size and the support for dispose method Jun 30, 2017

Fixed animated webp decoder issue when image has different duration p…
…er frame

Animated webp can set duration per frame but the UIImage animate array just use the average duration for each frame

@dreampiggy dreampiggy force-pushed the dreampiggy:master branch 2 times, most recently from a04e947 to cc82cc4 Jul 2, 2017

Add test for animated WebP image array
The image array used for +[UIImage animatedImageWithImages:] should share the same size.

@dreampiggy dreampiggy force-pushed the dreampiggy:master branch 5 times, most recently from e9e2353 to 2853e78 Jul 2, 2017

Add animated WebP loop count and fix duration zero issue
Animated WebP image support loop count. Set this value to UIImage decoded with WebP data.
Frame duration set to 100ms if is zero

@dreampiggy dreampiggy force-pushed the dreampiggy:master branch from 2853e78 to e3ed82f Jul 3, 2017

@dreampiggy dreampiggy changed the title from #1951 Fix animated WebP decoding issue, including canvas size and the support for dispose method to #1951 Fix animated WebP decoding issue, including canvas size, the support for dispose method and the duration per frame Jul 4, 2017

@dreampiggy dreampiggy changed the title from #1951 Fix animated WebP decoding issue, including canvas size, the support for dispose method and the duration per frame to #1951: Fix animated WebP decoding issue, including canvas size, the support for dispose method and the duration per frame Jul 4, 2017

@bpoplauschi bpoplauschi added this to the 4.1.0 milestone Jul 24, 2017

@worldofnick

This comment has been minimized.

worldofnick commented Jul 26, 2017

Awesome, how long until this is in?

@dreampiggy dreampiggy force-pushed the dreampiggy:master branch 4 times, most recently from e49c899 to 00c6f7c Jul 27, 2017

Improve Animated WebP decoder robustness and fix warning
Only decode the first frame on macOS
Fix CGBitmapContextCreate failed issue
Duration set to 100ms if it’s lower or equal than 10ms for compatibility
Fix unused variable warning for macOS

@dreampiggy dreampiggy force-pushed the dreampiggy:master branch from 00c6f7c to bd78863 Jul 29, 2017

Merge branch 'master' of https://github.com/rs/SDWebImage
# Conflicts:
#	SDWebImage/SDWebImageCompat.m
@bpoplauschi

This comment has been minimized.

Member

bpoplauschi commented Jul 31, 2017

I have to trust you @dreampiggy with this PR, this logic is a bit too complicated for me. But let's bring it it.

@bpoplauschi bpoplauschi merged commit f6cca30 into SDWebImage:master Jul 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dreampiggy

This comment has been minimized.

Contributor

dreampiggy commented Jul 31, 2017

@bpoplauschi Maybe I can use that Demo project(https://github.com/dreampiggy/AnimatedWebPDemo) with the lastest SD repo and add more WebP images to test.

This fix logic about 1. 2. 3. is nearly the same as #1694.

The last one(5. support duration per frame), it use the simple gcd number of each frame duration and add current frame for multiple times because that UIKit suck at +[UIImage animatedImageWithImages:duration:]. I search the GitHub and found one use the same method for GIF decoding(https://github.com/mayoff/uiimage-from-animated-gif/blob/master/uiimage-from-animated-gif/UIImage%2BanimatedGIF.m). But i'd check it more carefully.

@bpoplauschi

This comment has been minimized.

Member

bpoplauschi commented Jul 31, 2017

@dreampiggy I've run the builds, tests and CI, that's why we have them. Seems to be ok, so I merged it. We will found out soon if there are any problems :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment