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

Application synchronized updates / DECRQM #1582

Closed
dankamongmen opened this issue Apr 22, 2021 · 38 comments · Fixed by #1813
Closed

Application synchronized updates / DECRQM #1582

dankamongmen opened this issue Apr 22, 2021 · 38 comments · Fixed by #1813
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Milestone

Comments

@dankamongmen
Copy link
Owner

https://gitlab.freedesktop.org/terminal-wg/specifications/-/merge_requests/2 (seen in the foot man page). hey @dnkl if you could invite me into that terminal-wg, i'd appreciate it.

@dankamongmen dankamongmen added documentation Improvements or additions to documentation enhancement New feature or request question/research Further information is requested labels Apr 22, 2021
@dankamongmen dankamongmen added this to the 2.3.0 milestone Apr 22, 2021
@dankamongmen dankamongmen self-assigned this Apr 22, 2021
@dankamongmen dankamongmen removed this from the 2.3.0 milestone Apr 22, 2021
@dnkl
Copy link
Contributor

dnkl commented Apr 22, 2021

hey @dnkl if you could invite me into that terminal-wg, i'd appreciate it.

Would if I could, but I'm not a member...

@j4james
Copy link

j4james commented Apr 25, 2021

I don't know how much of the thread on terminal-wg you've read, but note that the DCS sequence that was originally proposed is somewhat problematic, in that it produces garbage output on some terminals that don't recognise DCS, so it's not really safe to use by default.

As a result it was generally agreed that a DECSET private mode sequence would be preferable. That way you can use the sequence indiscriminately, and worst case it will be ignored on terminals that don't support it.

The proposed mode sequences are as follows:

CSI ?2026h indicates the start of a synchronized update
CSI ?2026l indicates the end of the update

@dankamongmen
Copy link
Owner Author

thanks for the tip, @j4james !

@dnkl
Copy link
Contributor

dnkl commented Apr 25, 2021

Foot issue: https://codeberg.org/dnkl/foot/issues/459, PR: https://codeberg.org/dnkl/foot/pulls/461

@christianparpart
Copy link
Contributor

I just came here as I wanted to see if notcurses does or will support that. I am currently trying to gather a list of terminals / toolkits / apps that do support it. And I hoped to be able to add you soon, too. Just to let you know, @dankamongmen :)

@dankamongmen
Copy link
Owner Author

yeah @christianparpart i'm intending to support it. reading over the spec more closely now (i'd just seen a summary, and liked the idea). oooh, i don't like this timeout business one bit =[

@dankamongmen
Copy link
Owner Author

most interesting to me would be using this to ensure graphic+text updates are performed at the same time (i would use it around each frame i draw, but the main benefit would be eliminating flicker in graphic+text cases). i don't see anything regarding graphics at https://gitlab.com/gnachman/iterm2/-/wikis/synchronized-updates-spec. what's the story there?

@dankamongmen
Copy link
Owner Author

so i'm taking a look at this spec, and it's not...necessarily what i would have done.

a few thoughts:

  • is support discoverable? i'm not delighted about the idea of adding a header and footer to every frame that's just going to be thrown away. i see no method for discovery.
    • similarly, an incomplete VTxxx state machine might end up throwing away my input, it seems
  • that timeout is anathema. for that alone, i'd not want to use this most of the time. people on wireless have multisecond hiccups all the time. that output is just going to be thrown away? that's furthermore going to force a user-initiated refresh (ctrl-L), because my idea of what's currently drawn will be invalid

i'd hope that when a terminal reads a frame in toto, the display is effectively automatic, at least most of the time. so i'm thinking i maybe only want to use this on frames larger than some size, with that size ideally being "the number of bytes that the terminal is prepared to display automatically" (which of course might not be defined).

i mean, plenty of apps may very well be changing a single cell and redrawing. that would currently map to a 2-byte frame in Notcurses. if i throw this around every frame, that's 700% overhead for a totally common case. erp!

@dankamongmen
Copy link
Owner Author

i mean i guess this is already implemented in a number of things, but here's what i would have liked:

  • no timeout ideally, but whatever
  • i send a query and find out if you support this
  • if you do, great! i send an escape enabling it
  • once enabled, nothing is displayed until receiving an escape flushing the output

that at least cuts the overhead. oh well. maybe this wouldn't work for reasons i don't understand.

@dankamongmen
Copy link
Owner Author

maybe i'll use this around any rasterization that involves a graphic. that's gonna be big enough that the overhead is a wash, and that's where i need it, anyway.

@dankamongmen dankamongmen added this to the 3.0.0 milestone Jun 17, 2021
@dankamongmen dankamongmen removed the question/research Further information is requested label Jun 17, 2021
@christianparpart
Copy link
Contributor

Hey, i answer one question/concern for now and at that time of the day: fear not the discoverability. Use DECRQM to do that and the reply (https://vt100.net/docs/vt510-rm/DECRPM.html) will tell you also if it is supported.

On the latency: i would recommend using this not for one byte updates. It is rather intended for full screen / bigger updates to protect against tearing.

On timeout: the TE should have one, should not be configurable, should be reasonable (i e. not too big). I don't see why other people liked to talk that to death, well yeah. :)

If a TE has a too short timeout (or the line was too slow) the worst that can happen is that the flush (draw) happens earlier, and that's not worse than without, in the worst case.

Only visual changes are affected by this. So soft/hard reset etc can still have a direct effect. I like to compare that to GPU programming where you swap the buffer to render a new frame. This is similar: the rendering is deferred to a tactically better fitting later point in order to avoid tearing.

I hope all these arguments where convincing :)

I like to start counting at negative 3 instead 1.

@christianparpart
Copy link
Contributor

I should mention DECRQM/DECRPM in the linked document. Thanks for pointing that out.

@dankamongmen
Copy link
Owner Author

i've gone ahead and created the dankamongmen/app-sync-updates branch to test this functionality out. thanks for the response @christianparpart ; that makes things much more palatable.

@dnkl
Copy link
Contributor

dnkl commented Jun 18, 2021

I'd just like to take the opportunity to chime in with everything @christianparpart said. Not that I have seen much tearing with notcurses anyway - testimony of nicely done optimizations!

@dankamongmen
Copy link
Owner Author

I'd just like to take the opportunity to chime in with everything @christianparpart said. Not that I have seen much tearing with notcurses anyway - testimony of nicely done optimizations!

it is true that i track emitted bytes and spent seconds with some fanaticism.

with that said, it's easy to see problems when you're mixing overlapping text and graphics over an ssh session.

@dankamongmen
Copy link
Owner Author

dankamongmen commented Jun 19, 2021

Let's add a stat for this -- at the very least, I want to see how many times we used it across a given context's lifetime.

I think for a first try, I'm going to use this whenever we have an output burst of 8KB or more (maybe use BUFSIZ? or check the kernel buffer size? maybe even feed back from partial write()s, and keep a dynamic threshold?). At 8192, 14B is ~0.17% overhead, not so bad.

we of course won't know the output size until after putting together the output, so this will cost us one extra write() call for rasterization (we can tuck the footer into the actual memstream). that's fine. i've experimented before with replacing such paired write()s with a single writev(), and suffered generally poor results; fuck that noise.

a Kitty notcurses-demo at 107x70@10x20 has 2.89GiB (6B min, 56.83KiB avg, 7.42MiB max)
an Xterm notcurses-demo at 80x61@11x23 has 306.96MiB (6B min, 5.95KiB avg, 1.06MiB max)

so those kitty frames are surely being dragged high by the gigantic RGBA frames. 8192 looks decent.

@christianparpart
Copy link
Contributor

Everything larger that kernel page size i would recommend to frame using this technique.

i've experimented before with replacing such paired write()s with a single writev(), and suffered generally poor results; fuck that noise.

write() exists to reduce the context switches of too many consecutive write() calls. Context switches on Linux aren't that expensive anymore since many years though. So it may make sense to do the buffer stitching client side if the individual chunks are not too large (> kernel page size)

@dankamongmen
Copy link
Owner Author

write() exists to reduce the context switches of too many consecutive write() calls. Context switches on Linux aren't that expensive anymore since many years though. So it may make sense to do the buffer stitching client side if the individual chunks are not too large (> kernel page size)

yep, my conclusion as well. i expect this initial implementation to go in tonight.

@dankamongmen dankamongmen changed the title Look at doing application synchronized updates Application synchronized updates / DECRQM Jun 20, 2021
@dankamongmen
Copy link
Owner Author

@christianparpart i cannot for the life of me get contour to reply to any DECRQMs. kitty does just fine:

[schwarzgerat](0) $ echo -e '\e[?2017$p\e[?1049$p\e[?2026$p'

[schwarzgerat](0) $ 2017;0$y1049;2$y2026;0$y

but contour never seems to reply:

[schwarzgerat](0) $  echo -e '\e[?2017$p\e[?1049$p\e[?2026$p'

[schwarzgerat](0) $

any idea what i'm missing here?

@christianparpart
Copy link
Contributor

@dankamongmen i found my culprit. I stubbed the impl of DECRQM. I will implement that tonight. Expect it on master asap (and sorry for being disappointed ☹️)

@dankamongmen
Copy link
Owner Author

@dankamongmen i found my culprit. I stubbed the impl of DECRQM. I will implement that tonight. Expect it on master asap (and sorry for being disappointed )

i contemplated a header off my terrace for some hours, i assure you

@dankamongmen
Copy link
Owner Author

i'm thinking this will be the last big thing to go into 2.3.5.

@dankamongmen
Copy link
Owner Author

@christianparpart maybe i'm missing something, but oughtn't you have a '?' in here following the CSI? here's what i see in response from contour at the moment:

state:  0 char:    27 1b
state:  1 char: [  91 5b 
state:  2 char: 2  50 32 <----------------- shouldn't there be a qmark here
state:  2 char: 0  48 30
state:  2 char: 2  50 32
state:  2 char: 6  54 36
state:  2 char: ;  59 3b
state:  2 char: 2  50 32
state:  2 char: $  36 24
state:  2 char: y 121 79

Reporting DEC Modes
CSI
9/11 ? <--------------------------------------------
3/15 Pd
3/n ;
3/11 Ps
3/n $
2/4 y
7/9

@christianparpart
Copy link
Contributor

christianparpart commented Jun 22, 2021 via email

@dankamongmen
Copy link
Owner Author

notcurses is now detecting App-Synchronized Update support in contour, using DECRQM $2026:

colors: 256 rgb: y ccc: y setaf: y setab: y app-sync: y

dankamongmen added a commit that referenced this issue Jun 22, 2021
@dankamongmen
Copy link
Owner Author

now we just need hook it up to the rasterizer. when is everyone expected to move to $2026?

@dankamongmen
Copy link
Owner Author

490 renders, 106.32ms (101.63µs min, 216.98µs avg, 368.54µs max)
490 rasters, 47.34ms (33.10µs min, 96.60µs avg, 186.54µs max)
490 writes, 20.57s (27.56µs min, 41.98ms avg, 162.66ms max)
40.16MiB (25B min, 83.92KiB avg, 198.32KiB max)
0 failed renders, 0 failed rasters, 0 refreshes
RGB emits:elides: def 1545:157191 fg 8559:161854 bg 1041:11946
Cell emits:elides: 171723:8044085 (97.91%) 99.03% 94.98% 91.98%
Sprixel emits:elides: 486:0 (0.00%) 41.38MB ASUs: 476

             runtime│ frames│output(B)│    FPS│%r│%a│%w│TheoFPS║
══╤════════╤════════╪═══════╪═════════╪═══════╪══╪══╪══╪═══════╣
 1│    xray│  32.21s│    486│  40.15Mi│   15.1│ 0│ 0│63│  23.45║
══╧════════╧════════╪═══════╪═════════╪═══════╧══╧══╧══╧═══════╝
              32.21s│    486│  40.15Mi│

we're now sending ASUs when support is detected, and frames are sufficiently large.

@dnkl
Copy link
Contributor

dnkl commented Jun 22, 2021

Tested the asu branch briefly, and appears to work as expected. Really happy to finally see app synced updated being used :)

@dankamongmen
Copy link
Owner Author

Tested the asu branch briefly, and appears to work as expected. Really happy to finally see app synced updated being used :)

was this with foot? because foot is (at least for me) replying with \e[?2026;0$y (1.7.2 as packaged in the AUR), and thus i am not regarding it as supporting ASU.

@christianparpart
Copy link
Contributor

Now that i fixed my freaking brain damagei think you have it working on contour as well, right?

now we just need hook it up to the rasterizer. when is everyone expected to move to $2026?

This is now not just a single TE spring 2026. So it does make sense for the rest to hop on and drop the DCS.

What about pinging libvte on that matter?

@dankamongmen
Copy link
Owner Author

Now that i fixed my freaking brain damagei think you have it working on contour as well, right?

yes, it is working in contour. it is not working in foot because foot is replying with a ;0 rather than a ;2

@dnkl
Copy link
Contributor

dnkl commented Jun 23, 2021

it is not working in foot because foot is replying with a ;0 rather than a ;2

Foot has supported app sync:ed updates since forever. But, 2026 support was added ~2 months ago, and hasn't yet been released. So you need foot from git (there's a foot-git AUR package for Arch!).

(and yes, there'll be a 1.8 release very soon)

@dankamongmen
Copy link
Owner Author

it is not working in foot because foot is replying with a ;0 rather than a ;2
Foot has supported app sync:ed updates since forever. But, 2026 support was added ~2 months ago, and hasn't yet been released. So you need foot from git (there's a foot-git AUR package for Arch!).
(and yes, there'll be a 1.8 release very soon)

cool, i'll leave this as it stands, then. i thought i tested with current git as well. maybe not! i don't intend to support the old query, so i'll drive ASUs with foot wherever 2026 is supported.

@dankamongmen
Copy link
Owner Author

spun up weston today with the freshest foot available and wow that's definitely the cleanest intro i've seen since the orca showed up at the party. looks fantastic. nice work, @dnkl .

@dankamongmen
Copy link
Owner Author

oh man this is like playing on easy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants