From 4340f8a3b4c5149825d8f1786cfeb7a3af9c0e51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Thu, 22 Sep 2022 18:34:41 +0200 Subject: [PATCH] render: fix application of old scroll damage when double buffering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On compositors that forces us to double buffer, we need to re-apply the last frame’s damage to the current frame (which uses the buffer from the next-to-last frame). General cell updates are handled by simply copying from the last frame’s pixman buffer to the current frame’s. In an attempt to improve performance, scroll damage were up until now handled by re-playing the last frame’s scroll damage (on the current frame’s buffer). This does not work, and resulted in glitches when scrolling in the scrollback. This patch does the following: * grid_render_scroll{,_reverse}() now update the buffer’s "dirty" region. This means the generic copy-old-frames-buffer handles the scroll damage (albeit in, potentially, a less efficient way). * Tracking of, and re-applying old scroll damage is completely removed. Closes #1173 --- CHANGELOG.md | 5 ++ render.c | 132 +++++++++++++++++++++++++++------------------------ shm.c | 3 -- shm.h | 2 - 4 files changed, 74 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffbdd08b..17430354 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,11 @@ * Crash in `foot --server` on key press, after another `footclient` has terminated very early (for example, by trying to launch a non-existing shell/client). +* Glitchy rendering when scrolling in the scrollback, on compositors + that does not allow Wayland buffer re-use (e.g. KDE/plasma) + ([#1173][1173]) + +[1173]: https://codeberg.org/dnkl/foot/issues/1173 ### Security diff --git a/render.c b/render.c index 8775171e..7f883220 100644 --- a/render.c +++ b/render.c @@ -1014,6 +1014,13 @@ grid_render_scroll(struct terminal *term, struct buffer *buf, wl_surface_damage_buffer( term->window->surface, term->margins.left, dst_y, term->width - term->margins.left - term->margins.right, height); + + /* + * TODO: remove this if re-enabling scroll damage when re-applying + * last frame’s damage (see reapply_old_damage() + */ + pixman_region32_union_rect( + &buf->dirty, &buf->dirty, 0, dst_y, buf->width, height); } static void @@ -1079,6 +1086,13 @@ grid_render_scroll_reverse(struct terminal *term, struct buffer *buf, wl_surface_damage_buffer( term->window->surface, term->margins.left, dst_y, term->width - term->margins.left - term->margins.right, height); + + /* + * TODO: remove this if re-enabling scroll damage when re-applying + * last frame’s damage (see reapply_old_damage() + */ + pixman_region32_union_rect( + &buf->dirty, &buf->dirty, 0, dst_y, buf->width, height); } static void @@ -2538,22 +2552,27 @@ reapply_old_damage(struct terminal *term, struct buffer *new, struct buffer *old return; } - /* - * TODO: remove this frame’s damage from the region we copy from - * the old frame. - * - * - this frame’s dirty region is only valid *after* we’ve applied - * its scroll damage. - * - last frame’s dirty region is only valid *before* we’ve - * applied this frame’s scroll damage. - * - * Can we transform one of the regions? It’s not trivial, since - * scroll damage isn’t just about counting lines; there may be - * multiple damage records, each with different scrolling regions. - */ pixman_region32_t dirty; pixman_region32_init(&dirty); + /* + * Figure out current frame’s damage region + * + * If current frame doesn’t have any scroll damage, we can simply + * subtract this frame’s damage from the last frame’s damage. That + * way, we don’t have to copy areas from the old frame that’ll + * just get overwritten by current frame. + * + * Note that this is row based. A “half damaged” row is not + * excluded. I.e. the entire row will be copied from the old frame + * to the new, and then when actually rendering the new frame, the + * updated cells will overwrite parts of the copied row. + * + * Since we’re scanning the entire viewport anyway, we also track + * whether *all* cells are to be updated. In this case, just force + * a full re-rendering, and don’t copy anything from the old + * frame. + */ bool full_repaint_needed = true; for (int r = 0; r < term->rows; r++) { @@ -2583,35 +2602,31 @@ reapply_old_damage(struct terminal *term, struct buffer *new, struct buffer *old return; } - for (size_t i = 0; i < old->scroll_damage_count; i++) { - const struct damage *dmg = &old->scroll_damage[i]; - - switch (dmg->type) { - case DAMAGE_SCROLL: - if (term->grid->view == term->grid->offset) - grid_render_scroll(term, new, dmg); - break; - - case DAMAGE_SCROLL_REVERSE: - if (term->grid->view == term->grid->offset) - grid_render_scroll_reverse(term, new, dmg); - break; - - case DAMAGE_SCROLL_IN_VIEW: - grid_render_scroll(term, new, dmg); - break; - - case DAMAGE_SCROLL_REVERSE_IN_VIEW: - grid_render_scroll_reverse(term, new, dmg); - break; - } - } + /* + * TODO: re-apply last frame’s scroll damage + * + * We used to do this, but it turned out to be buggy. If we decide + * to re-add it, this is where to do it. Note that we’d also have + * to remove the updates to buf->dirty from grid_render_scroll() + * and grid_render_scroll_reverse(). + */ if (tll_length(term->grid->scroll_damage) == 0) { + /* + * We can only subtract current frame’s damage from the old + * frame’s if we don’t have any scroll damage. + * + * If we do have scroll damage, the damage region we + * calculated above is not (yet) valid - we need to apply the + * current frame’s scroll damage *first*. This is done later, + * when rendering the frame. + */ pixman_region32_subtract(&dirty, &old->dirty, &dirty); pixman_image_set_clip_region32(new->pix[0], &dirty); - } else + } else { + /* Copy *all* of last frame’s damaged areas */ pixman_image_set_clip_region32(new->pix[0], &old->dirty); + } pixman_image_composite32( PIXMAN_OP_SRC, old->pix[0], NULL, new->pix[0], @@ -2702,38 +2717,29 @@ grid_render(struct terminal *term) shm_addref(buf); buf->age = 0; - free(term->render.last_buf->scroll_damage); - buf->scroll_damage_count = tll_length(term->grid->scroll_damage); - buf->scroll_damage = xmalloc( - buf->scroll_damage_count * sizeof(buf->scroll_damage[0])); - { - size_t i = 0; - tll_foreach(term->grid->scroll_damage, it) { - buf->scroll_damage[i++] = it->item; - - switch (it->item.type) { - case DAMAGE_SCROLL: - if (term->grid->view == term->grid->offset) - grid_render_scroll(term, buf, &it->item); - break; - - case DAMAGE_SCROLL_REVERSE: - if (term->grid->view == term->grid->offset) - grid_render_scroll_reverse(term, buf, &it->item); - break; - - case DAMAGE_SCROLL_IN_VIEW: + tll_foreach(term->grid->scroll_damage, it) { + switch (it->item.type) { + case DAMAGE_SCROLL: + if (term->grid->view == term->grid->offset) grid_render_scroll(term, buf, &it->item); - break; + break; - case DAMAGE_SCROLL_REVERSE_IN_VIEW: + case DAMAGE_SCROLL_REVERSE: + if (term->grid->view == term->grid->offset) grid_render_scroll_reverse(term, buf, &it->item); - break; - } + break; - tll_remove(term->grid->scroll_damage, it); + case DAMAGE_SCROLL_IN_VIEW: + grid_render_scroll(term, buf, &it->item); + break; + + case DAMAGE_SCROLL_REVERSE_IN_VIEW: + grid_render_scroll_reverse(term, buf, &it->item); + break; } + + tll_remove(term->grid->scroll_damage, it); } /* diff --git a/shm.c b/shm.c index 4c342ddf..4394dbe9 100644 --- a/shm.c +++ b/shm.c @@ -151,7 +151,6 @@ buffer_destroy(struct buffer_private *buf) pool_unref(buf->pool); buf->pool = NULL; - free(buf->public.scroll_damage); pixman_region32_fini(&buf->public.dirty); free(buf); } @@ -581,8 +580,6 @@ shm_get_buffer(struct buffer_chain *chain, int width, int height) LOG_DBG("re-using buffer %p from cache", (void *)cached); cached->busy = true; pixman_region32_clear(&cached->public.dirty); - free(cached->public.scroll_damage); - cached->public.scroll_damage = NULL; xassert(cached->public.pix_instances == chain->pix_instances); return &cached->public; } diff --git a/shm.h b/shm.h index bed4285c..440cfa1d 100644 --- a/shm.h +++ b/shm.h @@ -24,8 +24,6 @@ struct buffer { unsigned age; - struct damage *scroll_damage; - size_t scroll_damage_count; pixman_region32_t dirty; };