I did a bunch of testing of various ansi escape codes (including
italics) with all three char atlas implementations (none, static,
dynamic), with and without `drawBoldTextInBrightColors`, and with and
without `enableBold` to test all of the edge cases that I could.
Drawing dim colors sets the globalAlpha to 0.5, so we need to
save/restore the ctx state. Otherwise, every time we draw some dim
content, our display will get progressively dimmer.
I tested this by running `echo '\u001b[2mfoo'` a few times in zsh.
Fixes#1424
Adjacent cells on the same row sharing the same background color will be
drawn using the same `fillCells` call. This should make drawing an
application with a solid or mostly solid background color (e.g. vim)
faster.
I set vim to draw the background color for a file, and started scrolling
through it. Before, _drawBackground was taking around 6ms per frame.
After this commit, it was taking around 0.5ms per frame.
Based on prior experience, I'd expect these results to be more drastic
for larger terminal windows. The demo's window is pretty small.
I'm primarily interested in doing this because it'll allow me to
optimize the background rendering in later diffs, but this may also make
it possible to fix some minor rendering issues.
Right now, if we draw a single-width character that overflows its cell's
bounds, and the character to the right of it has a background, that
background may cover up the first character's foreground. By drawing the
background in a separate pass, we can avoid those cases.
There's still some issues with how dirty regions are computed that makes
rendering stuff like that flaky, but this at least gets us closer to
"correct" rendering.
Other terminal emulators (e.g. alacritty) render the foreground and
background in separate passes:
https://github.com/jwilm/alacritty/blob/1b7ffea/src/renderer/mod.rs#L766
It turns out that `glyph.char.charCodeAt(0)` isn't equivalent to the
character's actual code because of utf-16 garbage, so this adds
`glyph.code` to the `glyph` object so that we can use the `code` value,
and don't need to regenerate it.
- We need to make a clone of this array, because ColorManager mutates
it.
- We only want to compare the first 16 colors instead of all 256, since
only the first 16 will ever change. In DynamicCharAtlas, we can pull
the additional colors from DEFAULT_ANSI_COLORS.
I tested this out by loading the demo and changing the theme a few
times.
This limits the amount of damage a pathological program can cause in
thrashing the LRU cache.
I ran the script described here:
https://github.com/xtermjs/xterm.js/pull/1327#issuecomment-374003960
Without this patch, the entire output would get drawn in one frame that
took about 140 ms to draw. With this patch, it takes about 60 ms. It's
still nowhere as good as the static or none atlas implementations, but
it's not terrible, like it was.
This should have minimal impact on real-world applications: it'll just
take a little longer for the atlas to warm up.
The implementation is a little hacky, since it involves throwing some
code into TextRenderLayer that violates the separation of concerns, but
we can clean this up later.
Pros:
- The code is simpler.
- AA rendering is consistent between cached and uncached characters.
- No long requires parsing color values to determine the alpha, which
means that we no longer depend on #rrggbbaa color value support.
Cons:
- We're drawing without SPAA in some cases where we could be.
Addresses this comment:
https://github.com/xtermjs/xterm.js/pull/1327#discussion_r176109332
Drawing to a ctx with `{alpha: true}` causes the canvas API (on chrome
at least) to switch to grayscale AA, instead of RGB subpixel AA.
This fixes it by drawing to a ctx that's using `{alpha: false}` when
posisble, and switching to a ctx that's using `{alpha: true}` only when
we have to.
There were two bugs:
- We need to clear the background of _tmpCtx instead of just drawing on
top if the background color is transparent.
- We should set the background to fully transparent if it's partially
transparent, to avoid drawing the transparent background twice.
Nothing actually cares about the return value, so we can avoid depending
on `Promise` by not returning a promise.
This should make this work for IE11 (though I haven't tested that).
I tried this in Chrome and Firefox to test both codepaths (ImageBitmap
and HTMLCanvasElement).
LRUMap implements the parts of an ordered map that we need to
efficiently implement DynamicCharAtlas. It's more code, but there's a
few advantages of this approach:
- Map isn't available on some older browsers, so this removes the need
for a polyfill.
- Moving an item to the end of the map's iteration order now only
requires unlinking and linking a linked-list node, whereas before we
had to delete and re-insert our value.
- Peeking at the oldest entry in the map no longer requires allocating
and destroying an iterator.
- We can preallocate the linked-list nodes we want to improve cache
locality. Similarly, we can recycle linked-list nodes to reduce
allocations and the GC pauses those allocations may cause.
- LRUMap seems to give slightly better results in Chrome's profiler than
Map did. We now spend about 5% of our time on map operations instead
of about 10%.
- In my (limited) testing, it doesn't look like LRUMap is slowing down
over time. Map appeared to get slightly slower the longer I ran the
terminal for, either due to memory fragmentation or some sort of leak.
I still need to write some tests for LRUMap, but I've been using this
implementation for the last hour without problems.
When we remove the background from a glyph, we can keep track of if we
made all the pixels fully transparent. If we did, we can just skip
future calls to _drawFromCache, since it would be a noop anyways.
This adds a very minimal implementation of DynamicCharAtlas using a LRU
cache.
I've got some optimizations that I'll add on top of this, but this
proves the concept.
This adds the logic needed to switch between multiple char atlas
implementations, and an example second implementation to test it out
with.
This also offers an escape hatch if something goes wrong with the char
atlas implementation(s).
The API defined by BaseCharAtlas will let us support multiple char atlas
implementations at once.
I tested this by running the demo with Chrome's profiler, which shows
that calls are going to StaticCharAtlas/drawImage instead of
_drawUncachedChar. Applications using 256 colors also still work fine,
via _drawUncachedChar.
This mostly worked before, but clearColor made the assumption that the
colors were always in #RRBBGG format. See the discussion here:
https://github.com/xtermjs/xterm.js/pull/1327#issuecomment-374812842
This changes the internal representation of a color so that we hold onto
into an object containing the original css string along with an RGBA
value encoded as a 32-bit uint. clearColor can then use that RGBA
representation.
Additionally, this deduplicates some code between ColorManager and
Terminal. Terminal was generating the 256 ansi colors the same way
ColorManager was, so this just makes Terminal use the same constant.
ICharAtlasRequest and ICharAtlasConfig need almost exactly the same set
of information, so it's simpler if we just merge the two types.
As an added bonus, this also adds devicePixelRatio to the config, which
helps guarantee that we won't ever accidentally end up with an atlas
using a different pixel ratio than we need.
- Ensures that everything is only defined in one place, and everything
imports from that place.
- Moves CHAR_ATLAS_CELL_SPACING into a subdirectory of shared/ so that
CharAtlasGenerator can pull from it.
This addresses the comments on
https://github.com/xtermjs/xterm.js/pull/1307/files/454fd4bfb1ab5c9ece
The only thing `didCharSizeChange` was used for was to figure out if we
need to refresh the character atlas or not. However, `acquireCharAtlas`
already inspects the atlas owned by this terminal, and if it matches, it
avoids generating a new atlas.
So if `didCharSizeChange` is true and it's not needed, it quickly bails
out as a no-op. But if `didCharSizeChange` is false, and it was needed
(possible given the complexity of testing all of these edge cases), it
could introduce a bug.
I think it makes sense to just get rid of `didCharSizeChange`, and just
always call `acquireCharAtlas`.
I'm starting to pull some changes off of my large WIP branch/commit that
adds an alternative dynamic character atlas. We're going to need to
support multiple CharAtlas implementations, and this should make that
easier.
Benjamin Woodruff