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

fix(zoom) multiplication overflow with zoom on 16-bit platforms #2536

Merged
merged 1 commit into from
Sep 7, 2021
Merged

fix(zoom) multiplication overflow with zoom on 16-bit platforms #2536

merged 1 commit into from
Sep 7, 2021

Conversation

kevinpt
Copy link
Contributor

@kevinpt kevinpt commented Sep 6, 2021

Description of the feature or fix

lv_coord_t is typically an int16_t. On 32-bit and larger platforms this will be promoted to int (32-bits wide)
before any operations are performed and then there is enough precision to prevent any numeric range
issues. However, on a 16-bit platform int16_t is (generally) the same rank as int and no promotion will
happen. This creates an overflow risk with multiplications. Zoom factors are encoded as a fixed point
number so the risk is even greater.

Consider the case where both a style transform zoom property is set and an image zoom is set. If both
are 2.0 they will be multiplied for a 4.0 overall zoom factor. In Q8.8 fixed point this is:
(512 * 512) >> 8 = (262144) >> 8 = 1024 == 4.0
On a 16-bit platform the intermediate multiply overflows before the final shift adjustment. Even if one
property is left at the default LV_IMG_ZOOM_NONE (256 == 1.0) an overflow will still happen.

This affects both images and the lv_chart widget. I cleaned up all the muls I could find related to zoom
by casting to int32_t. For lv_chart the expressions are dropping the fractional part and a round up by
adding 0.5 would be more correct but probably won't make any visual improvement so I left it as is.

Checkpoints

Copy link
Member

@embeddedt embeddedt left a comment

Choose a reason for hiding this comment

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

We really need a way of running CI tests on a 16-bit architecture.

@embeddedt embeddedt merged commit ec9b41a into lvgl:master Sep 7, 2021
@kisvegabor
Copy link
Member

We really need a way of running CI tests on a 16-bit architecture.

It'd be really great. Maybe do you already have some ideas? 🙂

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.

None yet

3 participants