Skip to content

fix(ffmpeg_player): increase align to 32 #9166

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 5, 2025
Merged

fix(ffmpeg_player): increase align to 32 #9166

merged 4 commits into from
Nov 5, 2025
+8 −5

Conversation

@cnjhb
Copy link
Contributor

@cnjhb cnjhb commented Oct 30, 2025

On my x86 computer, AV images aligned to 4 bytes cause memory corruption during sws_scale. This manifests as sigsegv errors, disrupting SDL2 functionality and other operations.
However, 32-byte alignment works without issues. I suspect this may be related to SIMD.
32-byte alignment has also been tested on my MIPS board and functions correctly.

Notes

@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025 •
edited
Loading

Hi 👋, thank you for your PR!

We've run benchmarks in an emulated environment. Here are the results:

ARM Emulated 32b - lv_conf_perf32b

Scene Name Avg CPU (%) Avg FPS Avg Time (ms) Render Time (ms) Flush Time (ms)
All scenes avg. 28 37 (-1) 7 7 0
Detailed Results Per Scene
Scene Name Avg CPU (%) Avg FPS Avg Time (ms) Render Time (ms) Flush Time (ms)
Empty screen 11 33 0 0 0
Moving wallpaper 2 33 1 1 0
Single rectangle 0 50 0 0 0
Multiple rectangles 0 35 (-3) 0 0 0
Multiple RGB images 0 39 0 0 0
Multiple ARGB images 14 (+5) 39 (-4) 0 0 0
Rotated ARGB images 59 (+5) 44 15 15 0
Multiple labels 5 (-1) 33 (-2) 0 0 0
Screen sized text 95 (-2) 47 20 20 0
Multiple arcs 31 (-2) 33 7 7 0
Containers 4 37 0 0 0
Containers with overlay 99 (+2) 21 44 44 0
Containers with opa 16 (+6) 37 0 (-1) 0 (-1) 0
Containers with opa_layer 16 (-2) 33 (-1) 5 5 0
Containers with scrolling 45 46 (-1) 12 12 0
Widgets demo 66 (-1) 40 16 16 0
All scenes avg. 28 37 (-1) 7 7 0

ARM Emulated 64b - lv_conf_perf64b

Scene Name Avg CPU (%) Avg FPS Avg Time (ms) Render Time (ms) Flush Time (ms)
All scenes avg. 23 (-1) 38 6 6 0
Detailed Results Per Scene
Scene Name Avg CPU (%) Avg FPS Avg Time (ms) Render Time (ms) Flush Time (ms)
Empty screen 11 33 0 0 0
Moving wallpaper 1 33 0 0 0
Single rectangle 0 50 0 0 0
Multiple rectangles 0 46 0 0 0
Multiple RGB images 0 39 0 0 0
Multiple ARGB images 1 38 0 0 0
Rotated ARGB images 29 34 9 9 0
Multiple labels 3 38 (-4) 0 0 0
Screen sized text 82 45 (-1) 17 17 0
Multiple arcs 32 (-2) 34 (+1) 6 6 0
Containers 4 39 (+2) 0 0 0
Containers with overlay 87 (-2) 23 41 41 0
Containers with opa 16 (+1) 38 (+1) 0 (-1) 0 (-1) 0
Containers with opa_layer 7 36 (-2) 1 1 0
Containers with scrolling 45 (+1) 46 11 11 0
Widgets demo 65 (-2) 42 15 15 0
All scenes avg. 23 (-1) 38 6 6 0

Disclaimer: These benchmarks were run in an emulated environment using QEMU with instruction counting mode.
The timing values represent relative performance metrics within this specific virtualized setup and should
not be interpreted as absolute real-world performance measurements. Values are deterministic and useful for
comparing different LVGL features and configurations, but may not correlate directly with performance on
physical hardware. The measurements are intended for comparative analysis only.

🤖 This comment was automatically generated by a bot.

cubic-dev-ai[bot]
cubic-dev-ai bot reviewed Oct 30, 2025
View reviewed changes
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

kisvegabor
kisvegabor previously approved these changes Oct 30, 2025
View reviewed changes
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

Thank you for this and the many other great ffmpeg updates!

@kisvegabor
Copy link
Member

kisvegabor commented Oct 30, 2025

Upps, I just noticed that a test is failing.

This is the incorrect screenshot: screenshot-errors-amd64 (2).zip

The new alignment messes up with the image decoder part.

@kisvegabor kisvegabor dismissed their stale review October 30, 2025 17:00

Tests are failing

@cnjhb
Copy link
Contributor Author

cnjhb commented Oct 31, 2025

Upps, I just noticed that a test is failing.

This is the incorrect screenshot: screenshot-errors-amd64 (2).zip

The new alignment messes up with the image decoder part.

You're right, I mainly used it for decoding videos before.
I just tested PNG decoding too, and this issue does exist. It seems image decoding requires different byte alignment.

@cnjhb cnjhb changed the title fix(ffmpeg): increase align to 32 fix(ffmpeg_player): increase align to 32 Oct 31, 2025
@FASTSHIFT FASTSHIFT requested a review from Copilot November 2, 2025 11:33
FASTSHIFT
FASTSHIFT previously approved these changes Nov 2, 2025
View reviewed changes
Copilot AI reviewed Nov 2, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the ffmpeg_image_allocate function to accept a configurable alignment parameter instead of using a hardcoded value of 4. The alignment value is now specified at the call site based on the use case.

  • Added an align parameter to ffmpeg_image_allocate function
  • Player code uses alignment of 32 for video playback
  • Decoder code uses alignment of 4 for static image decoding

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cnjhb cnjhb requested review from FASTSHIFT and kisvegabor November 3, 2025 01:20
kisvegabor
kisvegabor approved these changes Nov 4, 2025
View reviewed changes
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@FASTSHIFT FASTSHIFT merged commit f95b994 into lvgl:master Nov 5, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@cubic-dev-ai cubic-dev-ai[bot] cubic-dev-ai[bot] left review comments

@kisvegabor kisvegabor kisvegabor approved these changes

@FASTSHIFT FASTSHIFT FASTSHIFT approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cnjhb @kisvegabor @FASTSHIFT