-
Notifications
You must be signed in to change notification settings - Fork 23
Add Lottie animation playback support #152
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
base: main
Are you sure you want to change the base?
Conversation
Integrate Samsung's rlottie library to enable Lottie animation playback in the Twin window system. This implementation follows the existing TinyVG handler pattern for architectural consistency. Features: - Support pure JSON Lottie files (.json) with automatic format detection - On-demand frame rendering with single-buffer design for memory efficiency - Playback controls: play/pause, loop toggle via mado_lottie_* API - Seamless integration with existing apps_animation_start() function - New apps_lottie_start() providing dedicated player with control buttons Implementation details: - Add mado_lottie_image_t structure holding rlottie handle, viewport dimensions, frame tracking, and reusable ARGB8888 render buffer - Implement BGRA to ARGB pixel format conversion for Mado compatibility - Extend twin_animation_t to support Lottie via magic number identification - Add Lottie dispatch in twin_animation_advance_frame() and twin_animation_destroy() for proper frame rendering and cleanup New files: - src/image-lottie.c: Core Lottie loader and rendering implementation - apps/lottie.c: Lottie player application with playback controls - apps/apps_lottie.h: Public API declarations Modified files: - src/image.c: Add Lottie format detection in image type dispatcher - src/animation.c: Add Lottie-specific dispatch for frame advance/destroy Dependencies: - librlottie-dev: Samsung rlottie library for Lottie parsing and rendering Signed-off-by: Chungyi Chi <[email protected]>
Signed-off-by: Chungyi Chi <[email protected]>
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.
8 issues found across 13 files
Prompt for AI agents (all 8 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/image-lottie.c">
<violation number="1" location="src/image-lottie.c:78">
P1: Command injection vulnerability: `system()` with shell command is dangerous. If `path` contains shell metacharacters (e.g., `'`), arbitrary commands could be executed. Use POSIX APIs like `nftw()` with `unlink()`/`rmdir()` or `remove()` instead of shell commands.</violation>
<violation number="2" location="src/image-lottie.c:122">
P1: Return value of `zip_fread()` is not checked. If the read fails or is incomplete, the buffer may contain uninitialized data, leading to undefined behavior when parsing.</violation>
<violation number="3" location="src/image-lottie.c:284">
P1: Return value of `fread()` is not checked. If the read fails or is incomplete, the JSON buffer may contain uninitialized data.</violation>
<violation number="4" location="src/image-lottie.c:388">
P1: Potential buffer overflow: The function writes `lot->width * lot->height` pixels to `pix` without verifying the pixmap's dimensions match. If `pix` is smaller, this causes out-of-bounds writes.</violation>
</file>
<file name="include/twin.h">
<violation number="1" location="include/twin.h:1554">
P2: Duplicate function declaration of `twin_animation_is_lottie`. This function is already declared in the "Animation integration" section above. Remove this redundant declaration.</violation>
</file>
<file name="src/image.c">
<violation number="1" location="src/image.c:103">
P1: Using `strstr()` on a non-null-terminated buffer causes undefined behavior. The `header` array is only 8 bytes and not null-terminated, so `strstr()` will read past the buffer boundary. Consider using `memmem()` (POSIX) which takes a length parameter, or ensure the buffer is null-terminated before searching.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:72">
P2: The macOS installation instructions (line 71) should also be updated to include `rlottie` and `libzip` for consistency with the Ubuntu/Debian changes. Without these packages, macOS users won't have the dependencies for Lottie animation support.</violation>
</file>
<file name="apps/lottie.c">
<violation number="1" location="apps/lottie.c:180">
P2: Resource leak: when `display` creation fails, the toplevel `top` is not destroyed. Consider also destroying the toplevel before returning.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| return NULL; | ||
| } | ||
|
|
||
| fread(json, 1, sz, f); |
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.
P1: Return value of fread() is not checked. If the read fails or is incomplete, the JSON buffer may contain uninitialized data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/image-lottie.c, line 284:
<comment>Return value of `fread()` is not checked. If the read fails or is incomplete, the JSON buffer may contain uninitialized data.</comment>
<file context>
@@ -0,0 +1,634 @@
+ return NULL;
+ }
+
+ fread(json, 1, sz, f);
+ json[sz] = '\0';
+ fclose(f);
</file context>
| return NULL; | ||
| } | ||
|
|
||
| zip_fread(zf, data, st.size); |
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.
P1: Return value of zip_fread() is not checked. If the read fails or is incomplete, the buffer may contain uninitialized data, leading to undefined behavior when parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/image-lottie.c, line 122:
<comment>Return value of `zip_fread()` is not checked. If the read fails or is incomplete, the buffer may contain uninitialized data, leading to undefined behavior when parsing.</comment>
<file context>
@@ -0,0 +1,634 @@
+ return NULL;
+ }
+
+ zip_fread(zf, data, st.size);
+ data[st.size] = '\0';
+ zip_fclose(zf);
</file context>
|
|
||
| /* BGRA -> ARGB */ | ||
| twin_pointer_t p = twin_pixmap_pointer(pix, 0, 0); | ||
| size_t n = lot->width * lot->height; |
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.
P1: Potential buffer overflow: The function writes lot->width * lot->height pixels to pix without verifying the pixmap's dimensions match. If pix is smaller, this causes out-of-bounds writes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/image-lottie.c, line 388:
<comment>Potential buffer overflow: The function writes `lot->width * lot->height` pixels to `pix` without verifying the pixmap's dimensions match. If `pix` is smaller, this causes out-of-bounds writes.</comment>
<file context>
@@ -0,0 +1,634 @@
+
+ /* BGRA -> ARGB */
+ twin_pointer_t p = twin_pixmap_pointer(pix, 0, 0);
+ size_t n = lot->width * lot->height;
+ for (size_t i = 0; i < n; i++) {
+ uint32_t px = lot->buffer[i];
</file context>
| if (!path) | ||
| return; | ||
| char cmd[DOTLOTTIE_MAX_PATH + 16]; | ||
| snprintf(cmd, sizeof(cmd), "rm -rf '%s'", path); |
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.
P1: Command injection vulnerability: system() with shell command is dangerous. If path contains shell metacharacters (e.g., '), arbitrary commands could be executed. Use POSIX APIs like nftw() with unlink()/rmdir() or remove() instead of shell commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/image-lottie.c, line 78:
<comment>Command injection vulnerability: `system()` with shell command is dangerous. If `path` contains shell metacharacters (e.g., `'`), arbitrary commands could be executed. Use POSIX APIs like `nftw()` with `unlink()`/`rmdir()` or `remove()` instead of shell commands.</comment>
<file context>
@@ -0,0 +1,634 @@
+ if (!path)
+ return;
+ char cmd[DOTLOTTIE_MAX_PATH + 16];
+ snprintf(cmd, sizeof(cmd), "rm -rf '%s'", path);
+ int r = system(cmd);
+ (void)r;
</file context>
| void mado_lottie_set_loop(mado_lottie_image_t *lottie, bool loop); | ||
|
|
||
| /* Animation integration */ | ||
| bool twin_animation_is_lottie(const twin_animation_t *anim); |
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.
P2: Duplicate function declaration of twin_animation_is_lottie. This function is already declared in the "Animation integration" section above. Remove this redundant declaration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At include/twin.h, line 1554:
<comment>Duplicate function declaration of `twin_animation_is_lottie`. This function is already declared in the "Animation integration" section above. Remove this redundant declaration.</comment>
<file context>
@@ -1507,6 +1507,65 @@ void twin_custom_widget_queue_paint(twin_custom_widget_t *custom);
+void mado_lottie_set_loop(mado_lottie_image_t *lottie, bool loop);
+
+/* Animation integration */
+bool twin_animation_is_lottie(const twin_animation_t *anim);
+mado_lottie_image_t *twin_animation_get_lottie(twin_animation_t *anim);
+
</file context>
| return false; | ||
|
|
||
| /* Look for "v" key which indicates Lottie version */ | ||
| const char *str = (const char *)header; |
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.
P1: Using strstr() on a non-null-terminated buffer causes undefined behavior. The header array is only 8 bytes and not null-terminated, so strstr() will read past the buffer boundary. Consider using memmem() (POSIX) which takes a length parameter, or ensure the buffer is null-terminated before searching.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/image.c, line 103:
<comment>Using `strstr()` on a non-null-terminated buffer causes undefined behavior. The `header` array is only 8 bytes and not null-terminated, so `strstr()` will read past the buffer boundary. Consider using `memmem()` (POSIX) which takes a length parameter, or ensure the buffer is null-terminated before searching.</comment>
<file context>
@@ -63,13 +70,43 @@ typedef enum {
+ return false;
+
+ /* Look for "v" key which indicates Lottie version */
+ const char *str = (const char *)header;
+ if (strstr(str, "\"v\"") || strstr(str, "\"v\":"))
+ return true;
</file context>
| packages, including [libjpeg](https://www.ijg.org/) and [libpng](https://github.com/pnggroup/libpng). | ||
| * macOS: `brew install jpeg libpng` | ||
| * Ubuntu Linux / Debian: `sudo apt install libjpeg-dev libpng-dev` | ||
| * Ubuntu Linux / Debian: `sudo apt install libjpeg-dev libpng-dev librlottie-dev libzip-dev` |
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.
P2: The macOS installation instructions (line 71) should also be updated to include rlottie and libzip for consistency with the Ubuntu/Debian changes. Without these packages, macOS users won't have the dependencies for Lottie animation support.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 72:
<comment>The macOS installation instructions (line 71) should also be updated to include `rlottie` and `libzip` for consistency with the Ubuntu/Debian changes. Without these packages, macOS users won't have the dependencies for Lottie animation support.</comment>
<file context>
@@ -69,7 +69,7 @@ relies on certain third-party packages for full functionality and access to all
packages, including [libjpeg](https://www.ijg.org/) and [libpng](https://github.com/pnggroup/libpng).
* macOS: `brew install jpeg libpng`
-* Ubuntu Linux / Debian: `sudo apt install libjpeg-dev libpng-dev`
+* Ubuntu Linux / Debian: `sudo apt install libjpeg-dev libpng-dev librlottie-dev libzip-dev`
The renderer implementation can either use the built-in pixel manipulation or be based on [Pixman](https://pixman.org/).
</file context>
| &top->box, 0xffe0e0e0, anim->width, anim->height, 1, 10, | ||
| _lottie_dispatch, sizeof(lottie_app_t)); | ||
|
|
||
| if (!display) { |
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.
P2: Resource leak: when display creation fails, the toplevel top is not destroyed. Consider also destroying the toplevel before returning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/lottie.c, line 180:
<comment>Resource leak: when `display` creation fails, the toplevel `top` is not destroyed. Consider also destroying the toplevel before returning.</comment>
<file context>
@@ -0,0 +1,208 @@
+ &top->box, 0xffe0e0e0, anim->width, anim->height, 1, 10,
+ _lottie_dispatch, sizeof(lottie_app_t));
+
+ if (!display) {
+ twin_pixmap_destroy(pix);
+ return;
</file context>
jserv
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.
Follow the way how 'tools/build-neatvnc.sh' prepares Lottie library.
This PR integrates the rlottie library to provide comprehensive Lottie animation support, significantly enhancing UI animation capabilities.
Key Changes:
Core Integration: Implements the Lottie loader (src/image-lottie.c) which uses rlottie for rendering.
Dual File Support: Natively supports Lottie JSON (.json) and the optimized dotLottie (.lottie) formats.
libzip Integration: We integrate the libzip library to handle the necessary decompression of .lottie archives before they are passed to rlottie.
Configuration: Ensures that both rlottie and libzip CFLAGS and TARGET_LIBS are correctly added.
Demo: Adds a new Lottie demonstration application (CONFIG_DEMO_LOTTIE) for testing.
Summary by cubic
Add Lottie animation playback using rlottie with support for .json and .lottie files. Includes a simple demo player and integrates on-demand rendering into the existing animation system.
New Features
Dependencies
Written for commit 4f81c76. Summary will update automatically on new commits.