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 using canvas #938

Merged
merged 108 commits into from
Sep 8, 2017
Merged

Render using canvas #938

merged 108 commits into from
Sep 8, 2017

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Sep 4, 2017

@parisk @mofux I don't expect the code to change much now, it's mainly just tests, documentation and any bugs that pop up at this point. Please review, I know it's big but most of the changes are in new files inside src/renderer 😄

Resolves #935
Fixes #936
Fixes #681
Fixes #730
Fixes #720
Fixes #475
Fixes #937
Fixes #920
Fixes #919


New Stuff!

  • Ascii characters with their foreground, 8 color and 8 bright/bold colors are pre-rendered and stored in an ImageBitmap. This is used as a texture atlas to draw to canvas really fast.

    image

  • Multiple canvases are used to layer different things onto the terminal, this method is fast and allows for a nice separation of logic.

    image

  • Render layers keep track of what they have drawn (when it makes sense) and as such will diff the new contents and only draw when necessary. This turns single character changes in lines to typically take < 0.25ms, down from ~6ms.

    image

  • Better support for line height, font size and font family. As they're all managed within Terminal now, just setOption and run the fit addon if necessary.

    image

  • There is now a very clean separation from the model and everything rendering, pretty mcuh everything that renders is within ./src/renderer (viewport being the only exception, which needs a background color set). We should move towards doing similar with the model and the DOM in Terminal to simplify and make testing easier.

  • Link handlers are getting reworked quite a bit as previously the validationCallback exposes the HTMLElement of the link to allow tooltips to be attached. There is now a hoverCallback which achieves a similar goal.

  • This fixes a bunch of bugs (see the link of "Fixes #x" above), a notable bug fixes is line characters being misaligned in vtop vtop features misaligned lines #937.

API Changes

See complete list of changes in the xterm.d.ts diff.

Limitations & regressions

Deferred

Benchmarks

This table contains the average time it takes to render when running various actions or programs.

Render time 2.9.2 canvas_render Improvement
Typing a single letter 4-5ms 0.4-0.5ms 10x faster
vtop idle 22-73ms* 0.8-1.6ms 27-46x faster
ls -lR large_folder 17-25ms* 2.3-4.6ms 5-7x faster
yes 17-30ms* 1-1.3ms 17-23x faster

* Rendering took multiple frames

Setup

  • Measured on Chrome 60, Macbook Pro.
  • Compared cc7c310 (master, 2.9.2) and Tyriar@2d1a1bf
  • The times are all rendering-related chunks exposed in devtools added up (render script, rendering, paint, composite layers).
  • I tried to take the smallest and largest samples for the range.

In pictures

Here's one of my favorites; comparing vtop's performance 🔥.

Before

screen shot 2017-09-03 at 9 52 10 am

After

screen shot 2017-09-03 at 9 53 53 am


TODO List

  • API: Move setting of theme into Terminal.setOption
  • Better emoji support
  • Add selection color to ITheme
  • Resolve/accept TODO's in code
  • Document important classes/methods
  • Test
    • Add unit tests
    • Chrome
      • macOS
      • Linux
      • Windows
    • Safari/macOS - Does not support ImageBitmap so falls back to HTMLCanvas
    • Firefox - Buggy and bad performance drawing from ImageBitmap, explicit fallback to HTMLCanvas
      • Linux
      • macOS
      • Windows
    • Edge/Windows - seems just as good as Chrome
    • IE11/Windows - 💣
  • Redraw terminal when devicePixelRatio changes (causes blurry text)
  • Support floating point devicePixelRatio (causes jagged text)
  • Fix right padding getting larger when resizing
  • Investigate support for ctrl only clicks in the linkifier (a render layer in an addon?)

const isAscii = code < 256;
const isBasicColor = (colorIndex > 1 && fg < 16);
const isDefaultColor = fg >= 256;
if (isAscii && (isBasicColor || isDefaultColor)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still seem to have a problem with at least the _ character that is only 1px high (whereas I would expect it to be 2px on a retina display, or equal the height of one of the = strokes). See the __proto__ here:

link

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This problem might actually be related to the 15px font size in the demo. Using 14px or any of the font sizes that are supported in Terminal.app gives good looking results. I am wondering if that's the reason why Terminal.app restricts the font-sizes to 9,10,11,12,13,14,18,24,... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should include this in this PR but one was we could fix this is by have 2 render layers/canvases for the foreground, one for odd rows, one for even. That would allow characters like _ to overlap cells while still being able to clear them without interfering with other cells.

@mofux
Copy link
Contributor

mofux commented Sep 8, 2017

@Tyriar Support for ligatures seems to be broken (works in v3 and master). I think we should check if there is a way to support it with canvas - I'm afraid there are lots of people that use it (and some like powerline users can hardly live without it).

@Tyriar
Copy link
Member Author

Tyriar commented Sep 8, 2017

Support for ligatures seems to be broken (works in v3 and master)

I didn't think powerline used ligatures? powerline/fonts#42 Do you have a screenshot of the problem?

@mofux
Copy link
Contributor

mofux commented Sep 8, 2017

@Tyriar Just double-checked, I thought some of the glyphs that are used by powerline are coming from the ligatures, but I was wrong, sorry 😓

@Tyriar Tyriar merged commit 948eedb into xtermjs:v3 Sep 8, 2017
@Tyriar Tyriar deleted the canvas_render branch September 8, 2017 19:05
@parisk
Copy link
Contributor

parisk commented Sep 10, 2017

🎉

@@ -63,6 +64,7 @@ export class CompositionHelper {
* @param {CompositionEvent} ev The event.
*/
public compositionupdate(ev: CompositionEvent): void {
console.log('compositionupdate');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a logger here I think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch 😅 #983

@arcanis
Copy link
Contributor

arcanis commented Oct 10, 2017

I'm curious, does this renderer supports copy/pasting?

@weihanglo
Copy link

weihanglo commented Oct 10, 2017

Hi @arcanis.
I am not from the Xterm.js team. But after reading the sources, I think the selection of canvas renderer was built almost as same as the custom selection logic shown in #670. There is a separate render layer to make selections work. You can also run the demo and experience its performance directly. All copy/pasting stuff works well in my MacBook.

@Tyriar
Copy link
Member Author

Tyriar commented Oct 10, 2017

@arcanis copy and paste was barely touched at all, only the drawing of the selection and the events that copy/paste listened to were tweaked.

@Tyriar Tyriar mentioned this pull request Oct 13, 2017
13 tasks
this._ctx.scale(window.devicePixelRatio, window.devicePixelRatio);
return result;
}

Copy link

@weihanglo weihanglo Oct 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. I am curious about why we chose createImageBitmap over passing canvas out directly. I thought ImageBitmap is for worker thread or image decoding on background thread. Does this gain any performance improvement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comment that explains this:

// ImageBitmap's draw about twice as fast as from a canvas

This is when I initially measured it. I also found that ImageBitmap was cripplingly slow on Firefox and wasn't supported on Safari, these fall back to drawing from HTMLCanvas.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.ImageBitmap is for fast accessing and drawing. Thanks for the reply!

@tbodt
Copy link

tbodt commented Nov 5, 2017

Will selection and copy/paste still work on iOS?

@Tyriar
Copy link
Member Author

Tyriar commented Nov 6, 2017

@tbodt probably not, I'm thinking we should probably bring back the old renderer anyway for environment edge cases. Some computers take much longer to render using canvas which I think it something to do with GPU rendering being disabled or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Breaks API and requires a major version bump reference A closed issue/pr that is expected to be useful later as a reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants