-
Notifications
You must be signed in to change notification settings - Fork 510
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
Animated webp support #411
Conversation
🚫 CI failed with log |
🚫 CI failed with log |
e89b440
to
ef0f5c9
Compare
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
Ooph, should have updated webp in a different diff. Easiest way to review is search for |
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
the same NSData. | ||
|
||
PINAnimatedImage's are also decoded chunks at a time, writing each chunk to a separate file. This allows callback | ||
PINGAnimatedImage's are also decoded chunks at a time, writing each chunk to a separate file. This allows callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: PINMemMapAnimatedImage
perhaps?
{ | ||
if (_demux) { | ||
WebPDemuxDelete(_demux); | ||
} if (_durations) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: new line.
Generated by 🚫 Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -21,7 +21,7 @@ | |||
static const Float32 maxFileDuration = 1; //max duration of a file in seconds | |||
static const NSUInteger kCleanupAfterStartupDelay = 10; //clean up files after 10 seconds if it hasn't been done. | |||
|
|||
typedef void(^PINAnimatedImageInfoProcessed)(PINImage *coverImage, NSUUID *UUID, Float32 *durations, CFTimeInterval totalDuration, size_t loopCount, size_t frameCount, size_t width, size_t height, size_t bitsPerPixel, UInt32 bitmapInfo); | |||
typedef void(^PINAnimatedImageInfoProcessed)(PINImage *coverImage, NSUUID *UUID, Float32 *durations, CFTimeInterval totalDuration, size_t loopCount, size_t frameCount, UInt32 width, UInt32 height, size_t bitsPerPixel, UInt32 bitmapInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we don't use uint32_t
in here for width and height too? I saw we cast CGImageSourceGetCount to UInt32
, any reason we don't cast that to uint32_t
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly this class is now unused and likely is going to go away so I didn't clean it up. I'm keeping it for now because some of the code might be useful if we decide to make a hybrid memory / disk cache solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me, minor feedback.
|
||
NSString *kPINAnimatedImageErrorDomain = @"kPINAnimatedImageErrorDomain"; | ||
|
||
const NSTimeInterval kPINAnimatedImageDisplayRefreshRate = 60.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value isn't true on some newer devices (the new iPad Pros). Could this instead be gleaned from UIDevice
somehow?
return MAX(self.minimumFrameInterval * kPINAnimatedImageDisplayRefreshRate, 1); | ||
} | ||
|
||
//Credit to FLAnimatedImage ( https://github.com/Flipboard/FLAnimatedImage ) for display link interval calculations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary at the moment, but might be good to unit test this piece. Always seemed a bit risky to me!
return (scaledGCD / kGreatestCommonDivisorPrecision); | ||
} | ||
|
||
//Credit to FLAnimatedImage ( https://github.com/Flipboard/FLAnimatedImage ) for display link interval calculations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, perhaps the unit tests should be here actually.
} | ||
|
||
@property (atomic, strong) NSDate *lastMemoryWarning; | ||
@property (atomic, assign) BOOL weAreTheProblem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dig the name, but might be worth a comment explaining the meaning of this property ;)
|
||
PINLog(@"Frames left: %ld", (long)_playbackReady); | ||
|
||
dispatch_block_t notify = nil; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but this could be moved up a couple levels in scope. It's only referenced in one of the inner ifs
here.
} else { | ||
// No caching :( | ||
framesToCache = 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comments. Might be good in the future to add awareness of multiple live instances of PINCachedAnimatedImage
in the future. If you have many live instances in the first bucket above we could still drive the system to run out of memory. You could have each live instance get a memory quota that grows/shrinks with the number of live instances.
(CFDictionaryRef)@{(__bridge NSString *)kCGImageSourceShouldCache: | ||
(__bridge NSNumber *)kCFBooleanFalse}); | ||
_width = (uint32_t)[(NSNumber *)imageProperties[(__bridge NSString *)kCGImagePropertyPixelWidth] unsignedIntegerValue]; | ||
_height = (uint32_t)[(NSNumber *)imageProperties[(__bridge NSString *)kCGImagePropertyPixelHeight] unsignedIntegerValue]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker at the moment, but I've seen loading GIF loopCount and frame durations become a bottleneck on larger GIFs. I was working on loading loopCount and the durations asynchronously in timonus/FLAnimatedImage@bddff94 and timonus/FLAnimatedImage@0198b74 respectively, but never finished. Could be a nice thing to have here if we ever notice performance issues during initialization of GIFs. playbackReady
could be adjusted to only become true once those are loaded.
- (BOOL)pin_isAnimatedWebP | ||
{ | ||
WebPBitstreamFeatures features; | ||
if (WebPGetFeatures([self bytes], [self length], &features) == VP8_STATUS_OK) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to call this without checking -pin_isWebP
first?
This PR switches our technique for GIF handling to keeping in memory frames instead of trying to decode the entire GIF and memory mapping it. That technique was fraught with peril for devices without disk space left and had longer 'startup' costs. In addition there was a pesky race condition I never tracked down.
While I think the former solution has a place, it'll be best used in a hybrid scenario along with the solution introduced here.
Oh, this also adds animated webP support.