sdl: Fix display list damage tracking - #20
Closed
TomHoenderdos wants to merge 3 commits into
Closed
TomHoenderdos wants to merge 3 commits into
TomHoenderdos wants to merge 3 commits into
Conversation
Move the display list comparison and damage rectangle helpers from display.c to damage.c, unchanged apart from the names of the two functions display.c calls, now damage_diff() and damage_clip(). They don't use SDL, so they can be built and tested on their own. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Tom Hoenderdos <tomhoenderdos@gmail.com>
The SDL display only redraws the areas where the new display list
differs from the previous one, but two bugs made that comparison
wrong.
cmp_display_item() compared scaled_cropped_image items through
data.image_data.pix. That union member overlaps the width and height
of data.image_data_with_size, not its pixel pointer, so two items
showing different images of the same size compared equal and the
change was never drawn. The items are now compared by their own
pixel pointer, width and height.
damage_diff() compared each new item with orig[j] without checking j,
so a new list longer than the previous one read past the end of the
previous item array.
Add tests/damage, a host test of damage.c under ASan and UBSan. Without
this fix it reports no damage for the changed image and ASan aborts on
the read past the previous list. Build and run with:
cmake -S tests/damage -B build/damage \
-DLIBATOMVM_INCLUDE_PATH=<AtomVM>/src/libAtomVM
cmake --build build/damage && build/damage/test_damage
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Tom Hoenderdos <tomhoenderdos@gmail.com>
TomHoenderdos
force-pushed
the
sdl-damage-fix
branch
from
October 1, 2026 18:17
1b0200c to
99771cb
Compare
Even with the comparison fixed, the damage area could miss pixels that had to be redrawn: - When a new item matched an old one further down the previous list, the skipped old items were damaged from index k - j instead of j, so removing the first item of a list damaged nothing. - Old items after the last match were never damaged, so removing the last item or moving an item left its old pixels on screen. - update_damaged_area() and damage_clip() overwrote x and y before computing the right and bottom edges from them. Merging a damage rectangle left of or above the current one shrank the area, and clipping one that started off screen kept its full size. display.c still redraws the whole screen after computing the damage, so none of this was visible yet. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Tom Hoenderdos <tomhoenderdos@gmail.com>
Collaborator
|
If you don't mind I would rather close this PR and merge instead #23 that goes into the direction of handling partial updates. The rest of your open PR should be unaffected. |
bettio
added a commit
that referenced
this pull request
Oct 3, 2026
The comparison between the previous and the new display list had several bugs, and a workaround forced a full redraw anyway. Its only effect left was to skip an update whose list looked unchanged, which the bugs made unreliable. Remove it and always redraw; a damage tracking module shared by all drivers follows in a separate series. See also: #20 Signed-off-by: Davide Bettio <davide@uninstall.it>
bettio
added a commit
that referenced
this pull request
Oct 3, 2026
sdl: Fix plugin build and remove broken damage tracking The SDL display plugin no longer compiled on main and carried a display list comparison whose result was discarded. This series makes it build again and removes that code: - ufont_manager_register() takes an owned buffer since the font unloading change, so register_font did not compile. The font was also parsed straight from the message binary, which is destroyed once the call is answered. Copy the binary and hand it over, as the ESP32 display task does. - subscribe_input read element 2 of its arity-2 tuple. - The comparison between the previous and the new display list had several bugs: an out-of-bounds read when the new list is longer, the wrong union member compared for scaled images, removed and moved items never damaged, and a damage rectangle that shrank when merged or clipped. A workaround forced a full redraw anyway, so its only effect left was to skip an update whose list looked unchanged, which the bugs made unreliable. Remove it and always redraw the whole screen. Partial updates come back with a damage tracking module shared by all drivers, in a separate series. See also: #20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #18.
The SDL display only redraws where the new display list differs from the previous one. The code computing that damage area had several bugs:
cmp_display_item()comparedscaled_cropped_imageitems throughdata.image_data.pix, which overlaps the width and height ofdata.image_data_with_size, not its pixel pointer. Two items showing different images of the same size compared equal and the change was never drawn.orig[j]without checkingj, so a new list longer than the previous one read past the end of the previous item array.k - jinstead ofj, so removing the first item of a list damaged nothing.update_damaged_area()anddamage_clip()overwrote x and y before computing the right and bottom edges from them, so merging a rectangle left of or above the current damage shrank the area, and clipping one that started off screen kept its full size.display.cstill redraws the whole screen after computing the damage (BUG: damage area is not correct), so only the out-of-bounds read had a visible effect. That workaround stays until the SDL plugin builds again and the damage area can be checked on screen.Commits
sdl: Move display list damage tracking to its own file: moves the comparison and damage rectangle helpers, which don't use SDL, fromdisplay.ctodamage.cso they can be tested on their own. Unchanged apart from the two functionsdisplay.ccalls, nowdamage_diff()anddamage_clip().sdl: Fix display list damage comparison: the first two bugs, plustests/damage, a host test under ASan and UBSan.sdl: Fix damage areas of removed and moved items: the other three, with tests.Testing
tests/damageagainst AtomVM release-0.7, each commit's tests run against the code before it:The SDL plugin itself isn't built: on
mainit already fails to compile against SDL2 and the currentufont_manager_register().display.chas the same compiler errors before and after this PR, anddamage.cbuilds without warnings.🤖 Generated with Claude Code