From 8a3620bafaa4119b9f6d3f74189c2dac78614d3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Fri, 23 Jun 2023 20:20:01 +0200 Subject: [PATCH] term: scroll: only record scroll damage when viewport is at the bottom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t need to record scroll damage if the viewport isn’t at the bottom, since in this case, the renderer ignores the scroll damage anyway. This fixes a performance corner case, when the viewport is at the top of the scrollback history. When application scrolls the terminal contents, and the scrollback history is full, and the viewport is at top of the history, then the viewport needs to be moved (the scrollback history is a circular buffer, and thus the top of the history “moves” when we’re scrolling in new contents). Moving the viewport typically results in another type of scroll damage (DAMAGE_SCROLL_IN_VIEW, instead of the “normal” DAMAGE_SCROLL). Thus, each application triggered scroll, will result in two scroll damage records: one DAMAGE_SCROLL, and one DAMAGE_SCROLL_IN_VIEW. These two are incompatible, meaning they can’t be merged. What’s worse, it also means the DAMAGE_SCROLL records from two application triggered scrolls cannot be merged (since there’s a DAMAGE_SCROLL_IN_VIEW in between). As a result, the renderer will not see one, or “a few” scroll damage events, but a *ton*. _Each_ one typically a single line, or so. And each one resulting in lots of traffic on the wayland socket, as we create and destroy new buffer pools, when doing “shm scrolling”. This eventually leads to the socket not being able to keep up, and the socket is closed on us, forcing us to exit. The fix is really simple: don’t record “normal” scroll damage when scrolling, _unless_ the viewport is at the bottom (and thus “follows” the application output). As soon as the user scrolls up in the history, we’ll stop emitting normal scroll damage records. This is just fine, since, as mentioned above, the renderer ignores them when the viewport isn’t at the bottom. What if the viewport is moved back down again, before the next frame has been rendered? Wont there be “missing” scroll damage records? No, because moving the viewport results in scroll damage records by itself. Closes #1380 --- CHANGELOG.md | 3 +++ terminal.c | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2eab13eb..658e0c1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,10 +89,13 @@ opaque and non-opaque at runtime (using OSC-11). * Regression: crash when resizing the window when `resize-delay-ms > 0` ([#1377][1377]). +* Crash when scrolling up while running something that generates a lot + of output (for example, `yes`) ([#1380][1380]). [1317]: https://codeberg.org/dnkl/foot/issues/1317 [1355]: https://codeberg.org/dnkl/foot/issues/1355 [1377]: https://codeberg.org/dnkl/foot/issues/1377 +[1380]: https://codeberg.org/dnkl/foot/issues/1380 ### Security diff --git a/terminal.c b/terminal.c index 3beffcf3..815dc2d4 100644 --- a/terminal.c +++ b/terminal.c @@ -2714,6 +2714,7 @@ term_scroll_partial(struct terminal *term, struct scroll_region region, int rows term->grid->offset &= term->grid->num_rows - 1; if (likely(view_follows)) { + term_damage_scroll(term, DAMAGE_SCROLL, region, rows); selection_view_down(term, term->grid->offset); term->grid->view = term->grid->offset; } else if (unlikely(rows > view_sb_start_distance)) { @@ -2737,13 +2738,12 @@ term_scroll_partial(struct terminal *term, struct scroll_region region, int rows erase_line(term, row); } + term->grid->cur_row = grid_row(term->grid, term->grid->cursor.point.row); + #if defined(_DEBUG) for (int r = 0; r < term->rows; r++) xassert(grid_row(term->grid, r) != NULL); #endif - - term_damage_scroll(term, DAMAGE_SCROLL, region, rows); - term->grid->cur_row = grid_row(term->grid, term->grid->cursor.point.row); } void @@ -2800,6 +2800,7 @@ term_scroll_reverse_partial(struct terminal *term, xassert(term->grid->offset < term->grid->num_rows); if (view_follows) { + term_damage_scroll(term, DAMAGE_SCROLL_REVERSE, region, rows); selection_view_up(term, term->grid->offset); term->grid->view = term->grid->offset; } @@ -2818,7 +2819,6 @@ term_scroll_reverse_partial(struct terminal *term, erase_line(term, row); } - term_damage_scroll(term, DAMAGE_SCROLL_REVERSE, region, rows); term->grid->cur_row = grid_row(term->grid, term->grid->cursor.point.row); #if defined(_DEBUG)