render: Allow cells to bleed into their neighbor #592

This patch adds a confined flag to each cell to track if the last
rendered glyph bled into it's right neighbor. To keep things simple,
bleeding into any other neighbor cell than the immediate right one is
not allowed. This should cover most use cases.

Before rendering a row we now do a prepass and mark all cells unclean
that are affected by a bleeding neighbor. If there are consecutive
bleeding cells, the whole group must be re-rendered even if only a
single cell has changed.

Remaining issues:

• Glyphs are still cut-off for very large font sizes
• (削除) Last cell of a row bleeds into right margin area (削除ここまで) Not able to reproduce, probably occured due to a dirty build by myself

Followup to #502

I gave this another shot. The patch still claims one bit from the cell attributes, because we need to store the bleeding/overflow information somewhere until the next frame. I could create another bitmask in the grid struct, but on the other hand this can still be done if there are no more bits left in the cell attributes?

I changed the bit from overflow to confined because occasionally the information was lost (sgr_reset maybe?). If the attributes are reset, we must assume that the cell did bleed into it's neighbor, because it might have.

This still needs to be gated by a tweak option. I will add one if this get's a green light to merge.

I changed the bit from overflow to confined because occasionally the information was lost (sgr_reset maybe?). If the attributes are reset, we must assume that the cell did bleed into it's neighbor, because it might have.

I haven't looked at the patch yet, but my guess would be its lost when a (new) character is printed to the grid. When this happens, all cell attributes are replaced with the "current" ANSI attributes, from term->vt.attrs.

FWIW, the selected bits (set in selection.c when you select/deselect text with the mouse) have exactly the same problem; if a selected cell is updated by the application (example: you have selected text in top, which then refreshes the screen content), its "selected" state is lost. This is worked around in grid_render(), by calling selection_dirty_cells().

What we need is to split the ANSI state from "other" cell state. Right now, I'm counting 6 bits that can be moved out from the ANSI state. This means we can't just add an uint8_t bitfield outside the attrs struct. But perhaps the attrs struct can be re-arranged? Hmm... I'll have a look.

As for the rest of the patch. Really excited about it. Will try to find some time to go through it, and test it, as soon as possible.

Really happy you keep trying :D

But perhaps the attrs struct can be re-arranged? Hmm... I'll have a look.

Nope. Not without either increasing the size of the cell, or slowing down term_print() (which is in the hot path, so no).

The attr struct is currently 8 bytes. Meaning it can be copied with a single mov. Anything else will result either multiple byte moves, or masking.

Sorry for hijacking your PR :D

I'll see how adding an uint8_t to the cell struct affects performance. It will of course increase the run-time memory usage, but hopefully it plays nicely with the cache.

If that works out, we'll move the confined bit, along with selected and url(?) there.

I'll see how adding an uint8_t to the cell struct affects performance.

#593

Sorry for hijacking your PR :D

Glad it got attention!

One more thing I noticed that should be looked at before this gets merged: If the font size is very large, some characters are still cut off. Might be a bug in my font (Go Mono), but haven't seen this in other terminal emulators. Will investigate later.

FYI, I haven't forgotten about this, but I need to spend some time instrumenting, and benchmarking this (in addition to verifying that it actually works...).

Also trying to decide whether or not to add those extra state bytes. Right now, I don't think we'll add them. But if we do, that would change how we want this PR to work, and I don't want to spend time benchmarking something that may have to be re-written...

Thanks for understanding :)

That's perfectly fine, I'm still dogfooding this. There is another issue, where the last cell in a row might bleed into the right margin area. I will update the PR description to keep track of open issues.

It would be interesting if you could share what exactly you are benchmarking, so I can evaluate performance degradation to some degree by myself. Should I just use benchmark.py to run the default vtebench benchmarks with a full PGO build?

I will probably do a couple of things:

• instrument the code to count how many cells we're re-rendering due to overflowing glyphs (or a simplified version where we just count the number of rendered cells per frame, and manually check how much more we're rendering with this PR).
• Check frame rendering times, comparing with master. This can be done by enabling enableing tweak.render-timer=[osd|log|both] in foot.ini.
• Run vtebench benchmarks. vtebench was recently (well, it's been a while now, I guess) re-written, and no longer produces raw output files that you manually cat (which is basically what benchmark.py does), but instead loads, primes and runs benchmarks on its own.

I'd definitely run all benchmarks in PGO builds, but probably on regular release builds as well.

Also, I'd like to see benchmarks of best vs. worst case scenarios. I.e. are there any regressions in the best case scenario, where there aren't any overflowing glyphs?

And how much slower are we on this branch, compared to master, when all glyphs on the screen are overflowing?

I've done some testing to make sure everything works as expected and while that's the case, I noticed many glyphs marked as bleeding with small font sizes because of anti-aliasing. It might be worth consideration to add a threshold of one pixel to the bleeding logic, but that resulted in some visible cutoffs for me, especially with emojis.

Regarding performance, assuming all glyphs of a font are bleeding: If a cell changes, i.e. marked as not clean, this will cause all cells left and right to be re-rendered until the next whitespace.

I noticed many glyphs marked as bleeding with small font sizes because of anti-aliasing

This is actually the very reason we added cell clipping to begin with :)

Do we need a cut-off? I mean, visually, it'll look better if we allow them to bleed. And the option to enable bleeding will have a fat warning about possible performance impacts.

Regarding performance, assuming all glyphs of a font are bleeding: If a cell changes, i.e. marked as not clean, this will cause all cells left and right to be re-rendered until the next whitespace.

Yeah, and that's what we need to focus on, when benchmarking. Running the vtebench benchmarks won't tell us much actually since all cells will have changed in every frame anyway. I.e. we'll always be doing a full screen refresh. The only thing we'll be measuring is some of the overhead added by render_cell_prepass().

I think what needs to be done is create a benchmark that overflows roughly half the cells, and then measure render times only, not wall clock time.

I'd like to use this as my daily driver for a couple of days, with tweak.allow-overflowing-double-width-glyphs=no.

Can you please rebase onto latest master? No need to add a conf option yet (but do feel free to do so, if you have the time).

Sure, rebase is done. Conf option is still on my list, will add sometime this week.

Minor fixes after the rebase:

diff --git a/render.c b/render.c
index dd5dab1..b3c5391 100644
--- a/render.c
+++ b/render.c
@@ -658,11 +658,11 @@ render_cell(struct terminal *term, pixman_image_t *pix,
 * the next frame.
 */
 int render_width;
- if (glyph != NULL &&
- glyph->x + glyph->width > cell_cols * term->cell_width &&
+ if (glyph_count == 1 && glyphs[0] != NULL &&
+ glyphs[0]->x + glyphs[0]->width > cell_cols * term->cell_width &&
 col < term->cols - 1)
 {
- render_width = min(glyph->x + glyph->width,
+ render_width = min(glyphs[0]->x + glyphs[0]->width,
 (cell_cols + 1) * term->cell_width);
 for (int i = 0; i < cell_cols; i++)
 row->cells[col + i].attrs.confined = false;

Sorry, I've been in a hurry. Fixed that.

I've read a bit through #100 and as I understood it, to do this correctly, for glyph_count > 1 we would need to find the widest glyph, as they are just rendered on top of each other?

Yes, I think that would be the correct way to do it. Remember to include the offsets as well; the glyphs may have been positioned next to each other by harfbuzz.

I have deprecated the other options for overflowing glyphs as discussed. The wiki will need an update if this gets released.

What I have not tested yet are graphemes and sixels. Any suggestions for a font with overflowing graphemes?

Any suggestions for a font with overflowing graphemes?

Depends a bit what you're looking for. If we by "grapheme" mean "a cell with multiple codepoints", then those fall into two categories:

• Emoji sequences, which typically resolve to a single glyph
• Combining characters, like COMBINING ACUTE ACCENT. These are much more likely to produce multiple glyphs.

So, if you want to test how multiple glyphs work, just try things like echo -e 'q\u0301'. But be careful - foot will normalize the codepoints to a single codepoint if possible. For example, echo -e 'e\u0301' will produce the single codepoint LATIN SMALL LETTER E WITH ACUTE.

require the glyphs to bleed more than 1-2px?

If you know it, you'll see it. I wouldn't do it, but your decision! Or make overflowing-glyphs an integer option, which specifies the threshold.

What I have not tested yet are graphemes and sixels.

Murphys law was proven correct. Both were broken 😅

If you know it, you'll see it. I wouldn't do it, but your decision!

Let's allow them. At least to begin with. Might want to mention it explicitly in the man page.

Murphys law was proven correct. Both were broken 😅

dnkl's law is: if it hasn't been tested, it's broken :)

I've been running this for ~1 week now, and haven't noticed any issues.

one thing though: I'd like to invert the meaning of the confined bit. I.e. I would like 0 to mean it's confined. We'd need to make sure term->vt.attrs.confined is initialized, and kept to, 1, to keep the current semantics, where a printed to, but not yet rendered cell is considered to bleed.

The reason is that erased cells are currently treated as bleeding (erasing a cell is basically a memset() on the entire cell struct, including its attributes). By having 0 mean "confined", erased cells (which are guaranteed to be empty, and thus not bleeding) would be treated as confined instead of bleeding.

By changing the value of term->vt.attrs.confined, printed cells would now have confined=1 before having been rendered.

And it would probably make sense to rename it to bleeding, or overflowing again ;)

one thing though: I'd like to invert the meaning of the confined bit

I haven't tested this, but I'm not sure this will work as you intend. The confined flag is unset, when the last render did overflow. Erased cells may be guaranteed to be not bleeding, but in the next render will still need to know if the previous glyph did overflow. So if a cell is erased, we either need to restore the confined flag or just assume the last render did overflow.

FWIW this shouldn't have a big impact on erasing a large chunks of cells, because they will all be set to unclean anyway.

I haven't tested this, but I'm not sure this will work as you intend. The confined flag is unset, when the last render did overflow. Erased cells may be guaranteed to be not bleeding, but in the next render will still need to know if the previous glyph did overflow.

🤦 of course... I do know how it works. Promise! Just need to be reminded from time to time...

Let's allow them. At least to begin with. Might want to mention it explicitly in the man page.

I meant "allow them" as in, let's not add a threshold - let all glyphs overflow. But mention the fact that small font sizes may have this problem, and that it may affect performance.

Sorry, I should have been more clear about what I meant...

I meant "allow them" as in, let's not add a threshold - let all glyphs overflow. But mention the fact that small font sizes may have this problem, and that it may affect performance.

No prob. I have removed the 1px threshold again, although using it the last two days I was somewhat okay with it. But still, I think we should remove it, because it highly depends on colors, font, hinting, etc.

By the way I just realized Gitea won't show you a history of rebases. I've rebased some fixups instead of adding new commits, hope that wasn't confusing.

Worst case comparison:

https://codeberg.org/attachments/333a090d-96ad-477c-bf50-721ea3772b04.

A fullscreen refresh with this content takes:
https://codeberg.org/attachments/3311bc57-c56a-44c9-9490-b3232b9e2109

Whereas a fullscreen refresh with no bleeding cells take:
https://codeberg.org/attachments/5125a8ee-67e4-433f-b5ae-e0f5d9f6b6c0

The take away is that yes, this slows down rendering, especially when bleeding glyphs are "chained".

It's kind of expected though, I think. A fullscreen refresh with this content will actually render each cell twice, if I'm not mistaken?

The only time I believe this could be an issue is with small fonts and overflowing subpixels. I still think we should just leave things as they are (i.e lets not introduce a threshold just yet).

Once you've taken care of the issues pointed out above, can you also rebase on latest master, and squash the commits?

I think it's time this saw a wider audience (as in, it's time for a 1.8.2 release!) :)

It's kind of expected though, I think. A fullscreen refresh with this content will actually render each cell twice, if I'm not mistaken?

render_cell() will be called only once per cell, but if the glyph is the full width of two cells, it does render two cells. In that sense every cell is rendered twice, yes. You might get better results with glyphs that overflow less, e.g. only by one pixel.

The worst case here is actually changing a single cell in each row, which will cause all rows to be fully re-rendered. Whereas without overflowing-glyphs, only a single glyph per row will be re-rendered.

The worst case here is actually changing a single cell in each row, which will cause all rows to be fully re-rendered.

True.

The way I see this going forward, is we start out with a tweak option, enabled by default, to be able to quickly (within a patch release cycle) change how the option works. I.e. if it becomes apparent that we do need a threshold, then we can add it very quickly.