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

Add snapshot tests for dot lottie animations, and other cleanup #1794

Merged
merged 7 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Lottie.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -2658,7 +2658,7 @@
CODE_SIGN_STYLE = Automatic;
CURRENT_PROJECT_VERSION = 1;
GENERATE_INFOPLIST_FILE = YES;
IPHONEOS_DEPLOYMENT_TARGET = 11.0;
IPHONEOS_DEPLOYMENT_TARGET = 15.0;
Copy link
Member Author

Choose a reason for hiding this comment

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

Bumped the test target to 15.0 so we can use async/await in the tests

MACOSX_DEPLOYMENT_TARGET = 10.15;
MARKETING_VERSION = 1.0;
PRODUCT_BUNDLE_IDENTIFIER = com.airbnb.LottieTests;
Expand All @@ -2677,7 +2677,7 @@
CODE_SIGN_STYLE = Automatic;
CURRENT_PROJECT_VERSION = 1;
GENERATE_INFOPLIST_FILE = YES;
IPHONEOS_DEPLOYMENT_TARGET = 11.0;
IPHONEOS_DEPLOYMENT_TARGET = 15.0;
MACOSX_DEPLOYMENT_TARGET = 10.15;
MARKETING_VERSION = 1.0;
PRODUCT_BUNDLE_IDENTIFIER = com.airbnb.LottieTests;
Expand Down
10 changes: 5 additions & 5 deletions Sources/Private/Model/DotLottie/DotLottieImageProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import AppKit

// MARK: - DotLottieImageProvider

/// Provides an image for a lottie animation from a provided Bundle.
/// An image provide that loads the images from a DotLottieFile into memory
calda marked this conversation as resolved.
Show resolved Hide resolved
class DotLottieImageProvider: AnimationImageProvider {

// MARK: Lifecycle
Expand All @@ -38,12 +38,12 @@ class DotLottieImageProvider: AnimationImageProvider {
let filepath: URL

func imageForAsset(asset: ImageAsset) -> CGImage? {
imageCache.object(forKey: asset.name as NSString)
images[asset.name]
}

// MARK: Private

private var imageCache: NSCache<NSString, CGImage> = .init()
private var images = [String: CGImage]()
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated this to use a dictionary instead of a cache, since this data isn't something that can be recomputed once evicted from a cache

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we store the filepath.url couldn't we recover from a memory pressure eviction? It seems risky to have a bunch of CGImages in memory and not be able to release them on memory pressure warning as it may lead to apps being jetsam'd

Copy link
Member Author

Choose a reason for hiding this comment

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

When loading a dot lottie file, we currently:

  1. decompress the zip archive to a temporary directory
  2. load the file contents into memory
  3. delete the files in the temporary directory

I think deleting the temporary unzipped files after loading them is a good idea because:

  1. I think we don't want to be leaving behind a large number of files in the host app's temporary directory. The OS cleans these up in theory but I'm not sure we should rely on that.
  2. Since the OS can clean up those files at any time in theory, I don't think its safe to assume that they will still exist at any point in the future

That means that we don't have a way to gracefully reload the images if they get removed from the NSCache, since the filepath URLs are no longer valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we could re-decompress the zip archive to reload the images if necessary after they're evicted from the cache, but that functionality doesn't current exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha, sounds good. Perhaps we could add a note about adding that behavior if needed?


private func loadImages() {
filepath.urls.forEach {
Expand All @@ -52,14 +52,14 @@ class DotLottieImageProvider: AnimationImageProvider {
let data = try? Data(contentsOf: $0),
let image = UIImage(data: data)?.cgImage
{
imageCache.setObject(image, forKey: $0.lastPathComponent as NSString)
images[$0.lastPathComponent] = image
}
#elseif os(macOS)
if
let data = try? Data(contentsOf: $0),
let image = NSImage(data: data)?.lottie_CGImage
{
imageCache.setObject(image, forKey: $0.lastPathComponent as NSString)
images[$0.lastPathComponent] = image
}
#endif
}
Expand Down
3 changes: 0 additions & 3 deletions Sources/Public/Animation/LottieAnimation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,4 @@ public final class LottieAnimation: Codable, DictionaryInitializable {
/// Markers
let markers: [Marker]?
let markerMap: [String: Marker]?

/// DotLottie configuration to setup the player
var dotLottieConfiguration: DotLottieConfiguration?
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this property in favor of a DotLottieFile.Animation type that includes a LottieAnimation and DotLottieConfiguration

}
22 changes: 12 additions & 10 deletions Sources/Public/Animation/LottieAnimationView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -126,16 +126,17 @@ final public class LottieAnimationView: LottieAnimationViewBase {
configuration: LottieConfiguration = .shared,
logger: LottieLogger = .shared)
{
animation = dotLottie?.animation(for: animationId)
let dotLottieAnimation = dotLottie?.animation(for: animationId)
animation = dotLottieAnimation?.animation
imageProvider = dotLottie?.imageProvider ?? BundleImageProvider(bundle: Bundle.main, searchPath: nil)
self.textProvider = textProvider
self.fontProvider = fontProvider
self.configuration = configuration
self.logger = logger
super.init(frame: .zero)
commonInit()
loopMode = animation?.dotLottieConfiguration?.loopMode ?? .playOnce
animationSpeed = CGFloat(animation?.dotLottieConfiguration?.speed ?? 1)
loopMode = dotLottieAnimation?.configuration.loopMode ?? .playOnce
animationSpeed = CGFloat(dotLottieAnimation?.configuration.speed ?? 1)
makeAnimationLayer(usingEngine: configuration.renderingEngine)
if let animation = animation {
frame = animation.bounds
Expand Down Expand Up @@ -440,15 +441,16 @@ final public class LottieAnimationView: LottieAnimationViewBase {
_ animationId: String? = nil,
from dotLottieFile: DotLottieFile)
{
guard let animation = dotLottieFile.animation(for: animationId) else { return }
if let configuration = animation.dotLottieConfiguration {
loopMode = configuration.loopMode
animationSpeed = CGFloat(configuration.speed)
}
if let imageProvider = dotLottieFile.imageProvider {
guard let dotLottieAnimation = dotLottieFile.animation(for: animationId) else { return }

loopMode = dotLottieAnimation.configuration.loopMode
animationSpeed = CGFloat(dotLottieAnimation.configuration.speed)

if let imageProvider = dotLottieAnimation.configuration.imageProvider {
self.imageProvider = imageProvider
}
self.animation = animation

animation = dotLottieAnimation.animation
}

/// Plays the animation from its current state to the end.
Expand Down
16 changes: 8 additions & 8 deletions Sources/Public/Animation/LottieAnimationViewInitializers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,10 @@ extension LottieAnimationView {
configuration: LottieConfiguration = .shared)
{
self.init(dotLottie: nil, animationId: animationId, configuration: configuration)
DotLottieFile.named(name, bundle: bundle, subdirectory: nil, closure: { result in
DotLottieFile.named(name, bundle: bundle, dotLottieCache: dotLottieCache) { result in
guard case Result.success(let lottie) = result else { return }
self.loadAnimation(animationId, from: lottie)
}, dotLottieCache: dotLottieCache)
}
}

/// Loads a Lottie from a .lottie file in a specific path on disk.
Expand All @@ -132,10 +132,10 @@ extension LottieAnimationView {
configuration: LottieConfiguration = .shared)
{
self.init(dotLottie: nil, animationId: animationId, configuration: configuration)
DotLottieFile.filepath(filePath, closure: { result in
DotLottieFile.loadedFrom(filepath: filePath, dotLottieCache: dotLottieCache) { result in
guard case Result.success(let lottie) = result else { return }
self.loadAnimation(animationId, from: lottie)
}, dotLottieCache: dotLottieCache)
}
}

/// Loads a Lottie file asynchronously from the URL
Expand All @@ -156,14 +156,14 @@ extension LottieAnimationView {
closure(nil)
} else {
self.init(dotLottie: nil, configuration: configuration)
DotLottieFile.loadedFrom(url: url, closure: { result in
DotLottieFile.loadedFrom(url: url, dotLottieCache: dotLottieCache) { result in
switch result {
case .success(let lottie):
self.loadAnimation(animationId, from: lottie)
case .failure(let error):
closure(error)
}
}, dotLottieCache: dotLottieCache)
}
}
}

Expand All @@ -181,10 +181,10 @@ extension LottieAnimationView {
configuration: LottieConfiguration = .shared)
{
self.init(dotLottie: nil, animationId: animationId, configuration: configuration)
DotLottieFile.asset(name, bundle: bundle, closure: { result in
DotLottieFile.asset(named: name, bundle: bundle, dotLottieCache: dotLottieCache) { result in
guard case Result.success(let lottie) = result else { return }
self.loadAnimation(animationId, from: lottie)
}, dotLottieCache: dotLottieCache)
}
}

// MARK: Public
Expand Down
47 changes: 38 additions & 9 deletions Sources/Public/DotLottie/DotLottieFile.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,17 @@ public final class DotLottieFile {

// MARK: Internal

/// Definition for a single animation within a `DotLottieFile`
struct Animation {
let animation: LottieAnimation
let configuration: DotLottieConfiguration
}

/// List of `LottieAnimation` in the file
var animations: [LottieAnimation] = []
private(set) var animations: [Animation] = []

/// Image provider for animations
var imageProvider: AnimationImageProvider?
private(set) var imageProvider: AnimationImageProvider?

/// Manifest.json file loading
lazy var manifest: DotLottieManifest? = {
Expand Down Expand Up @@ -62,6 +68,15 @@ public final class DotLottieFile {
FileManager.default.urls(for: imagesUrl) ?? []
}()

/// The `LottieAnimation` and `DotLottieConfiguration` for the given animation ID in this file
func animation(for id: String? = nil) -> DotLottieFile.Animation? {
if let id = id {
return animations.first(where: { $0.configuration.id == id })
} else {
return animations.first
}
}

// MARK: Private

private static let manifestFileName = "manifest.json"
Expand All @@ -70,6 +85,14 @@ public final class DotLottieFile {

private let fileUrl: URL

private var dotLottieAnimations: [DotLottieAnimation] {
manifest?.animations.map({
var animation = $0
animation.animationUrl = animationsUrl.appendingPathComponent("\($0.id).json")
return animation
}) ?? []
}

/// Decompresses .lottie file from `URL` and saves to local temp folder
///
/// - Parameters:
Expand Down Expand Up @@ -100,14 +123,20 @@ public final class DotLottieFile {
private func loadContent() {
imageProvider = DotLottieImageProvider(filepath: imagesUrl)

animations = dotLottieAnimations.compactMap {
let animation = try? $0.animation()
animation?.dotLottieConfiguration = DotLottieConfiguration(
id: $0.id,
animations = dotLottieAnimations.compactMap { dotLottieAnimation -> DotLottieFile.Animation? in
guard let animation = try? dotLottieAnimation.animation() else {
return nil
}

let configuration = DotLottieConfiguration(
id: dotLottieAnimation.id,
imageProvider: imageProvider,
loopMode: $0.loopMode,
speed: $0.animationSpeed)
return animation
loopMode: dotLottieAnimation.loopMode,
speed: dotLottieAnimation.animationSpeed)

return DotLottieFile.Animation(
animation: animation,
configuration: configuration)
}
}
}
Expand Down
Loading