Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render glitches (blacked out content) #4480

Closed
willmcgugan opened this issue Apr 18, 2023 · 15 comments · Fixed by #4533
Closed

Render glitches (blacked out content) #4480

willmcgugan opened this issue Apr 18, 2023 · 15 comments · Fixed by #4533
Labels
area/addon/webgl type/bug Something is misbehaving
Milestone

Comments

@willmcgugan
Copy link

I'm working on a TUI framework for Python, which does tend to push the terminal more than a typical app.

On certain apps, after a few seconds of updates the terminal starts glitching heavily. The more animation, the quicker this seems to happen.

It's probably best to show in a video:

Screen.Recording.2023-04-18.at.14.17.53.mov

This doesn't occur on other terminals. And I'm fairly sure (although not 100%) that this is not an error in Textual itself.

The video above is with an embedded xterm.js, but this issue also occurs in VSCode.

I haven't addressed flow control yet. But from experimentation, it only takes a a few hundred K before this issue occurs.

Details

  • Browser and browser version: Chrome. Version 112.0.5615.49 (Official Build) (arm64)
  • OS version: MacOS 13.3.1 (22E261)
  • xterm.js version: 5.1.0"

Steps to reproduce

Install Textual:

pip install textual[dev]

Then in VS code integrated terminal:

textual easing
@willmcgugan
Copy link
Author

Possibly part of the same issue, but if I keep clicking the app in this glitched state, then a different glitch occurs.

Screen.Recording.2023-04-18.at.14.35.00_1.mp4

Which looks like some kind of texture cache has become corrupt?

@jerch
Copy link
Member

jerch commented Apr 18, 2023

To me both looks like texture cache issues, but at different levels - 1. looks like no invalidate/erase going on, as if nothing gets into textures anymore (works fine in the beginning). 2. looks like a cache key collision, placing stuff into wrong place. (Quickly tested in xterm.js demo under linux/chrome and firefox...)

One question regarding your screen updates - from terminal logs it seems that you do a full screen updates for every animation step. Wouldn't a diff buffer approach save a lot of bandwidth and TE work here?

@willmcgugan
Copy link
Author

One question regarding your screen updates - from terminal logs it seems that you do a full screen updates for every animation step. Wouldn't a diff buffer approach save a lot of bandwidth and TE work here?

It does do diffs at the widget level, but not at the character level. In this particular example, the changed widgets cover most of the screen so the updates are large.

@willmcgugan
Copy link
Author

After some investigation, I've found that it is nothing to do with the speed or volume of updates. It seems to be triggered when Textual writes a large number of different colors.

If I disable the blending effect in the video above, then I can't reproduce the issue at all.

I can reproduce this outside of Textual. I've documented this in #4484

@willmcgugan
Copy link
Author

I've found that calling clearTextureAtlas after every write essentially fixes this issue. I'm assuming this is sub-optimal, and it does seem a little slower to update.

@jerch
Copy link
Member

jerch commented Apr 23, 2023

I've found that calling clearTextureAtlas after every write essentially fixes this issue. I'm assuming this is sub-optimal, and it does seem a little slower to update.

This kinda confirms that there is an issue with eviction in the cache, like it is ever-growing until an upper bound is hit (thats also what I see in the demo, the atlas pages grow really big there from your easing example).
Since I have not dealt with the atlas implementation myself this prolly has to wait until @Tyriar finds some time to look into it.

@Tyriar
Copy link
Member

Tyriar commented May 2, 2023

I'm assuming this is sub-optimal, and it does seem a little slower to update.

Very, especially if you use a lot of unique glyphs. You don't seem to use that much other than the fading of the yellow square.

#4351 is the only issue I was aware of with the texture atlas currently, it's possibly the same.

@willmcgugan what renderer is that using?

@willmcgugan
Copy link
Author

It was the GL renderer.

@Tyriar
Copy link
Member

Tyriar commented May 20, 2023

Maybe related? #4485

@Tyriar Tyriar added type/bug Something is misbehaving area/addon/webgl and removed needs more info labels May 20, 2023
@jerch
Copy link
Member

jerch commented May 21, 2023

@Tyriar It still repros with #4527, which fixes #4485. (Chrome)

Edit: Also repros in Firefox with the same weird blacked out behavior. (#4485 never happened on Firefox)

Edit2: Maybe a hint - if I return early from the microtask here skipping all code, I cannot provoke the blackout anymore:

if (TextureAtlas.maxAtlasPages && this._pages.length >= Math.max(4, TextureAtlas.maxAtlasPages / 2)) {
queueMicrotask(() => {
// Find the set of the largest 4 images, below the maximum size, with the highest
// percentages used
const pagesBySize = this._pages.filter(e => {
return e.canvas.width * 2 <= (TextureAtlas.maxTextureSize || Constants.FORCED_MAX_TEXTURE_SIZE);
}).sort((a, b) => {
if (b.canvas.width !== a.canvas.width) {
return b.canvas.width - a.canvas.width;

Yes, just tracked the invocation of the microtask - the issue occurs as soon as the page merging happened.

@jerch
Copy link
Member

jerch commented May 21, 2023

@Tyriar Did a bit more digging.

For webgl the issue gets resolved by this change:

diff --git a/addons/xterm-addon-webgl/src/WebglRenderer.ts b/addons/xterm-addon-webgl/src/WebglRenderer.ts
index fcb0dd80..2b96c7d6 100644
--- a/addons/xterm-addon-webgl/src/WebglRenderer.ts
+++ b/addons/xterm-addon-webgl/src/WebglRenderer.ts
@@ -338,10 +338,13 @@ export class WebglRenderer extends Disposable implements IRenderer {
     // Tell renderer the frame is beginning
     if (this._glyphRenderer.beginFrame()) {
       this._clearModel(true);
+      this._updateModel(0, this._terminal.rows);
+    } else {
+      this._updateModel(start, end);
     }
 
     // Update model to reflect what's drawn
-    this._updateModel(start, end);
+    //this._updateModel(start, end);
 
     // Render
     this._rectangleRenderer?.render();

which forces the model into a full refresh upon a page merge in the atlas.

For canvas the issue goes away, if comment out the bitmap usage here:

diff --git a/addons/xterm-addon-canvas/src/BaseRenderLayer.ts b/addons/xterm-addon-canvas/src/BaseRenderLayer.ts
index 88d929f1..06185528 100644
--- a/addons/xterm-addon-canvas/src/BaseRenderLayer.ts
+++ b/addons/xterm-addon-canvas/src/BaseRenderLayer.ts
@@ -388,7 +388,8 @@ export abstract class BaseRenderLayer extends Disposable implements IRenderLayer
       this._bitmapGenerator[glyph.texturePage]!.version = this._charAtlas.pages[glyph.texturePage].version;
     }
     this._ctx.drawImage(
-      this._bitmapGenerator[glyph.texturePage]?.bitmap || this._charAtlas!.pages[glyph.texturePage].canvas,
+      // this._bitmapGenerator[glyph.texturePage]?.bitmap || this._charAtlas!.pages[glyph.texturePage].canvas,
+      this._charAtlas!.pages[glyph.texturePage].canvas,
       glyph.texturePosition.x,
       glyph.texturePosition.y,
       glyph.size.x,

It looks like some issue with the bitmap used after a page merge. Also bitmap.close() is never called in the bitmap lifecycling (not sure if JS will fully cleanup bitmaps by GC, still wondering why there is an explicit ImageBitmap.close).

Since I am not sure, if these are the right fixes (not familiar with the renderer code), I'd leave it up to you.

@Tyriar
Copy link
Member

Tyriar commented May 21, 2023

@jerch the first one is definitely the first fix as when beginFrame returns true the atlas is cleared/merged so any pointers they have to textures become invalidated:

https://github.com/Tyriar/xterm.js/blob/eb56356133c91df39fbdab2e416d30a79d678acd/src/browser/renderer/shared/TextureAtlas.ts#L192-L194

You can PR that fix if you want, I'll look into the second one now as we don't want to lose the bitmap optimization

@jerch
Copy link
Member

jerch commented May 21, 2023

Ah ok, gonna do the webgl PR then.

@jerch
Copy link
Member

jerch commented May 21, 2023

@Tyriar Still not fully fixed - still seeing the glitch as shown here #4480 (comment)

To me this also looks like a different issue 👉 #4534

@willmcgugan
Copy link
Author

I can still reproduce this isssue. Tried with the latest xterm and xterm-addon-webgl, and the beta versions.

Seems the same in VSCode:

Screenshot 2023-07-15 at 17 48 55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/addon/webgl type/bug Something is misbehaving
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants