-
Notifications
You must be signed in to change notification settings - Fork 0
Refactoring Swift Wrapper: Introduce LottieRenderer #11
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
Conversation
ac94d2c to
e6da3c1
Compare
2590368 to
85dd7c0
Compare
| /// Initializes the LottieRenderer with a specific Lottie object, engine, size, buffer, and stride. | ||
| /// - Parameters: | ||
| /// - lottie: The `Lottie` object containing the animation to render. | ||
| /// - engine: An optional `Engine` object to use. If not provided, the default engine configuration is used. | ||
| /// - size: The size of the rendering canvas. This size determines the final size of the rendered Lottie content. | ||
| /// - buffer: A buffer to hold the rendered pixel data. | ||
| /// - stride: The number of bytes in a row of the buffer. | ||
| public init( | ||
| _ lottie: Lottie, | ||
| engine: Engine = .default, | ||
| size: CGSize, | ||
| buffer: Buffer, | ||
| stride: Int | ||
| ) { | ||
| self.lottie = lottie | ||
| self.engine = engine | ||
| self.canvas = Canvas(size: size, buffer: buffer, stride: stride) | ||
| } |
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.
Just a heads up: I've moved the Lottie instance out of the render function and into the LottieRenderer initialiser.
Previously, we actually only supported rendering a single Lottie anyways due to the fact that we only push to the Canvas once (further down - line 57), which was actually a bug of the old implementation.
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.
Just curious will it improve performance on multiple lottie running at same time with this change?
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.
We still have a 1:1 mapping of Lottie instance to LottieRenderer instance, so I don't think this change will immediately see performance increases across multiple Lotties. But I will check 👍
| /// Renders a specific frame of the Lottie animation using a specified area of the content, applying optional rotation. | ||
| /// - Parameters: | ||
| /// - frameIndex: Index of the frame in the animation. | ||
| /// - contentRect: Specifies the area of the content to be rendered. This rectangle defines the portion of the animation that should be visible in the final rendered frame, scaled to fit the canvas size. | ||
| /// - rotation: Rotation angle in degrees to apply to the renderered frame. | ||
| public func render( | ||
| frameIndex: Int, | ||
| contentRect: CGRect, | ||
| rotation: Double = 0.0 | ||
| ) throws { |
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.
Another note: I've renamed crop to contentRect as that aligns more with the behaviour we are providing.
| /// This behaviour is akin to a "stretch-to-fit" resizing as the picture's original aspect ratio is not preserved. | ||
| func stretchToFit(_ rect: CGRect) { |
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've renamed this to stretchToFit as we're not actually performing any clipping here, we are resizing the image to fit within the crop rectangle boundary.
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.
so it's scaleToFill not scaleAspectFill? like your comment there
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.
Yep, that's right 😄
rlaguilar
left a comment
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.
👏
|
|
||
| final class LottieRendererTests: XCTestCase { | ||
|
|
||
| let lottie: Lottie = { |
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.
would be nice to have lottie in func setUp() {}, so we know for sure that lottie usage is isolated and each test starts with a consistent setup.
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 idea. I'll change the code 👍
| /// A Swift wrapper for managing ThorVG's engine initialization and termination. | ||
| public class Engine { | ||
| /// A default instance of `Engine`, running on the main thread. | ||
| public static let `default` = Engine() |
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.
Now we made the Eninge a singleton which could be more efficient.
I also noticed you moved tvg_canvas_destroy and tvg_animation_del into Canvas and Animation respectively. However, we seem never to terminate the engine tvg_engine_term.
Just curious will this be a problem?
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.
It's a good point you raise here, because we are indeed no longer calling tvg_engine_term.
The reason why I've done this is because we're exposing the default instance of the Engine through a static variable, meaning that the deinit function will never be called.
I've tested this and haven't ran into any issues, as it looks like ThorVG cleans itself up at the end of the process.
| /// Initializes the LottieRenderer with a specific Lottie object, engine, size, buffer, and stride. | ||
| /// - Parameters: | ||
| /// - lottie: The `Lottie` object containing the animation to render. | ||
| /// - engine: An optional `Engine` object to use. If not provided, the default engine configuration is used. | ||
| /// - size: The size of the rendering canvas. This size determines the final size of the rendered Lottie content. | ||
| /// - buffer: A buffer to hold the rendered pixel data. | ||
| /// - stride: The number of bytes in a row of the buffer. | ||
| public init( | ||
| _ lottie: Lottie, | ||
| engine: Engine = .default, | ||
| size: CGSize, | ||
| buffer: Buffer, | ||
| stride: Int | ||
| ) { | ||
| self.lottie = lottie | ||
| self.engine = engine | ||
| self.canvas = Canvas(size: size, buffer: buffer, stride: stride) | ||
| } |
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.
Just curious will it improve performance on multiple lottie running at same time with this change?
| /// This behaviour is akin to a "stretch-to-fit" resizing as the picture's original aspect ratio is not preserved. | ||
| func stretchToFit(_ rect: CGRect) { |
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.
so it's scaleToFill not scaleAspectFill? like your comment there
| private let testLottieUrl: URL = Bundle.module.url(forResource: "test", withExtension: "json")! | ||
| private let testSize = CGSize(width: 1024, height: 1024) | ||
| class LottieSnapshotTests: XCTestCase { | ||
| let lottie: Lottie = { |
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.
ditto, if you want use func setup()
ray-canva
left a comment
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, nice work!
* Introduce internal Swift types * Remove Engine from this PR * Refactor Lottie to use internal types * Adopt changes from past PR * Introduce LottieRenderer * Remove optionals from LottieRenderer * Rename crop to stretchToFit * Update docs * Docs update * Address PR comments
Changes
This PR is the final PR in this refactor. After having introduced simpler internal types and migrating the
Lottietype to use them, we can now introduce theLottieRenderertype to further refactor the bloatedLottietype.This moves all rendering logic out of the
Lottietype and into its own dedicated space. On top of this, engine management has also been separated into its ownEnginetype.Splitting rendering logic into its own type
The original API for Lottie rendering included rendering inside the
Lottietype. This was available via therenderFrame(at index: Int, into buffer: Buffer, stride: Int, size: CGSize, crop: CGRect? = nil)function.There are some limitations for designing the API in this way:
Lottietype to handle the lifecycle of all these internal objects. Here we have a clear separation we can make to match this desired functional separation of the Canvas details versus animation/picture details. Hence,LottieRenderer.buffer,stride,size), we can significantly reduce how many parameters we have to pass in to ourrendercall.renderFramefunction was the only buffer targeted to the internal Canvas. So if you used a different buffer on subsequentrenderFramecalls, they wouldn't be filled with pixel data. This was also highlighted in a previous comment by @rlaguilar here.Thus, we have now created the new
LottieRenderertype. TheLottieRendereris responsible for two things:Engine management
A note on how we manage the ThorVG Engine. Previously, every time a Canvas was created, we were initialising the ThorVG engine. The ThorVG engine only needs to initialised once, and not doing so hinders performance.
Thus, we've introduced a new
Enginetype that provides adefaultsingleton instance, running on a single thread. Thisdefaultinstance is used as the default parameter to theLottieRendererinitialiser.Larger refactoring
As mentioned, this is part of a larger factor across the entire Swift wrapper. This is the final PR, introducing
LottieRenderer.Look at the PR train below for more details.
PR Train 🚂