fix(draw/opengles): cache compare image src by content#10020
Conversation
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/draw/opengles/lv_draw_opengles.c">
<violation number="1" location="src/draw/opengles/lv_draw_opengles.c:283">
P2: Cache comparator uses relational ordering on unrelated pointers, making key ordering non-portable/undefined in C.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The OpenGL ES texture cache compared draw descriptors with lv_memcmp, which compared the bytes of the `src` pointer instead of the file path content. Because lv_image_set_src() lv_strdup()s the path per widget, two widgets pointing at the same file produced different src pointers, causing a cache miss on every screen recreation. Reported in lvgl#9903. - Add `task_type` to cache_data_t; partition the cache by it before dispatching the compare, so image-dsc compares are isolated and the RB tree ordering stays symmetric. - Replace lv_memcmp for IMAGE tasks with image_dsc_compare, which better resolves lv_draw_image_dsc_t fields to avoid comparing raw pointers. - lv_strdup the path on insert and lv_free it on eviction so the cache owns the dereferenced memory; otherwise lv_strcmp would trade the cache miss for a use-after-free when the widget frees its src buffer. - Move `colorkey` to pointer compare, consistent with the existing `bitmap_mask_src` policy: fields the cache doesn't own are never dereferenced. Closes lvgl#9903 Signed-off-by: Borgeuzz <mmam2407@gmail.com>
9d6ee1a to
8bf4c2f
Compare
|
Hi 👋, thank you for your PR! We've run benchmarks in an emulated environment. Here are the results: ARM Emulated 32b - lv_conf_perf32b
Detailed Results Per Scene
ARM Emulated 64b - lv_conf_perf64b
Detailed Results Per Scene
Disclaimer: These benchmarks were run in an emulated environment using QEMU with instruction counting mode. 🤖 This comment was automatically generated by a bot. |
There was a problem hiding this comment.
Pull request overview
Fixes OpenGL ES texture cache misses for file/symbol-backed images by comparing image sources by content (path/symbol string) rather than by pointer value, addressing #9903.
Changes:
- Added
task_typeto cache keys and used it to partition comparisons so different task descriptors are never compared with mismatched logic. - Implemented an image-specific descriptor comparator that compares
srcby string content forFILE/SYMBOLand by pointer forVARIABLE, while keeping other task types on the existinglv_memcmppath. - Made the texture cache own duplicated
FILE/SYMBOLsrcstrings (strdup on insert, free on eviction) to avoid use-after-free.
| lv_draw_image_dsc_t * img_dsc = (lv_draw_image_dsc_t *)cache_data->draw_dsc; | ||
| if(img_dsc->src != NULL) { | ||
| lv_image_src_t src_type = lv_image_src_get_type(img_dsc->src); | ||
| if(src_type == LV_IMAGE_SRC_FILE || src_type == LV_IMAGE_SRC_SYMBOL) { | ||
| img_dsc->src = lv_strdup((const char *)img_dsc->src); | ||
| } |
There was a problem hiding this comment.
addressed in commit 00509d
|
Hi. Thank you for the PR Please address Copilot's remarks |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
Fixes #9903.
The OpenGL ES texture cache used
lv_memcmpon the entirelv_draw_image_dsc_t, which compared the bytes of thesrcpointer instead of the file path it points at. Becauselv_image_set_src()lv_strdups the path per widget, two widgets pointing at the same file ended up with different pointers and produced a cache miss on every screen recreation,even though the underlying texture was identical.
Changes (
src/draw/opengles/lv_draw_opengles.c)task_typetocache_data_t; the compare callback now partitions the RB tree by it before dispatching, soimage_dsc_compareonly ever runs on two image descriptors andthe ordering stays symmetric.
image_dsc_comparedoes a field-by-field comparison forLV_DRAW_TASK_TYPE_IMAGE.srcis resolved vialv_image_src_get_type:lv_strcmpforFILE/SYMBOL, pointercompare for
VARIABLE. All other task types keep the existinglv_memcmppath.lv_strdupon insert (draw_to_texture),lv_freeon eviction (opengles_texture_cache_free_cb). Without this,lv_strcmpwoulddereference a path the widget had already freed — swapping the cache miss for a use-after-free.
colorkeyfrom content compare to pointer compare. The cache doesn't own the colorkey struct, so dereferencing it would create the same UAF class assrc. Same policyalready used for
bitmap_mask_src. Comments aligned.supis intentionally skipped (decoder-derived, would always miss);bitmap_mask_srckeeps pointer compare. Behaviour unchanged.