Skip to content

Conversation

@andyf-canva
Copy link
Owner

@andyf-canva andyf-canva commented Feb 27, 2024

Changes

This PR is the final PR in this refactor. After having introduced simpler internal types and migrating the Lottie type to use them, we can now introduce the LottieRenderer type to further refactor the bloated Lottie type.

This moves all rendering logic out of the Lottie type and into its own dedicated space. On top of this, engine management has also been separated into its own Engine type.

Splitting rendering logic into its own type

The original API for Lottie rendering included rendering inside the Lottie type. This was available via the renderFrame(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:

  • Separation of concerns: Internally we have a Canvas which handles the buffer and sizing details. Yet we have separate objects (Animation/Picture) for handling setting the animation frame index and the picture crop details. This means that is becomes tricky for the Lottie type 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.
  • Function parameter bloating: As you can see, the more functionality we want to add to our rendering calls means we need to add more parameters to this function. If we separate out the constants to this function (buffer, stride, size), we can significantly reduce how many parameters we have to pass in to our render call.
  • Using the same buffer for each render pass: There was actually a bug in this code that meant that the first buffer you passed to the renderFrame function was the only buffer targeted to the internal Canvas. So if you used a different buffer on subsequent renderFrame calls, 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 LottieRenderer type. The LottieRenderer is responsible for two things:

  1. Setting up a Canvas of a desired size that points to a buffer.
  2. Provide the ability to render Lottie frames into that buffer.
/// Shorthand alias for the buffer type, representing image pixel data in a mutable pointer to UInt32.
public typealias Buffer = UnsafeMutablePointer<UInt32>

/// Object used to load and render Lottie frames.
public class Lottie {
    ...
}

/// Object responsible for rendering a Lottie animation using ThorVG.
public class LottieRenderer {

    /// 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: thorvg_swift.Lottie, engine: thorvg_swift.Engine = .default, size: CGSize, buffer: thorvg_swift.Buffer, stride: Int)

    /// Renders a specific frame of the Lottie animation, with cropping and 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 `LottieRenderer` size.
    ///   - rotation: Rotation angle in degrees to apply to the renderered frame.
    public func render(frameIndex: Int, contentRect: CGRect, rotation: Double = 0.0) throws
}

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 Engine type that provides a default singleton instance, running on a single thread. This default instance is used as the default parameter to the LottieRenderer initialiser.

/// 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`: thorvg_swift.Engine
}

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 🚂

  1. Refactoring Swift Wrapper: Introduce internal Swift types #9
  2. Refactoring Swift Wrapper: Refactor Lottie type #10
  3. Refactoring Swift Wrapper: Introduce LottieRenderer #11 <- YOU ARE HERE

@andyf-canva andyf-canva changed the title Refactoring Swift Wrapper: Introduce Lottie Renderer Refactoring Swift Wrapper: Introduce LottieRenderer Feb 27, 2024
@andyf-canva andyf-canva force-pushed the andyf-refactor-lottie-type branch from ac94d2c to e6da3c1 Compare February 29, 2024 07:22
@andyf-canva andyf-canva force-pushed the andyf-introduce-lottie-renderer branch from 2590368 to 85dd7c0 Compare February 29, 2024 08:20
Comment on lines 14 to 31
/// 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)
}
Copy link
Owner Author

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.

Copy link
Collaborator

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?

Copy link
Owner Author

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 👍

Comment on lines +33 to +42
/// 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 {
Copy link
Owner Author

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.

Comment on lines +110 to +111
/// This behaviour is akin to a "stretch-to-fit" resizing as the picture's original aspect ratio is not preserved.
func stretchToFit(_ rect: CGRect) {
Copy link
Owner Author

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.

Copy link
Collaborator

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, that's right 😄

Copy link
Collaborator

@rlaguilar rlaguilar left a 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 = {
Copy link
Collaborator

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.

Copy link
Owner Author

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()
Copy link
Collaborator

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?

Copy link
Owner Author

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.

Comment on lines 14 to 31
/// 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)
}
Copy link
Collaborator

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?

Comment on lines +110 to +111
/// This behaviour is akin to a "stretch-to-fit" resizing as the picture's original aspect ratio is not preserved.
func stretchToFit(_ rect: CGRect) {
Copy link
Collaborator

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 = {
Copy link
Collaborator

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()

@andyf-canva andyf-canva changed the base branch from andyf-refactor-lottie-type to main March 5, 2024 01:17
@andyf-canva andyf-canva requested a review from ray-canva March 5, 2024 01:48
Copy link
Collaborator

@ray-canva ray-canva left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

@andyf-canva andyf-canva merged commit 5a91849 into main Mar 5, 2024
@andyf-canva andyf-canva deleted the andyf-introduce-lottie-renderer branch March 8, 2024 07:09
andyf-canva added a commit that referenced this pull request Apr 29, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants