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

improve use of kitty pixel graphics protocol #1395

Closed
6 tasks done
dankamongmen opened this issue Mar 9, 2021 · 26 comments · Fixed by #1803
Closed
6 tasks done

improve use of kitty pixel graphics protocol #1395

dankamongmen opened this issue Mar 9, 2021 · 26 comments · Fixed by #1803
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Milestone

Comments

@dankamongmen
Copy link
Owner

dankamongmen commented Mar 9, 2021

We got the basics done as #1095. Kitty's protocol is much saner and more powerful than Sixel (and well speced out, to boot). We ought take advantage of more of its power. First off, we need to:

  • delete images explicitly when we're done using them; there's a control sequence for this
  • get transparency working at the same level of Sixel. there, we can simply leave pixels out via the band mechanism. no such luck here, where we go pixel-by-pixel rather than color-by-color. we might have to go full RGBA rather than RGB, and fold the 8 bits to a single effective bit. this would substantially blow up our control sequence size (33% to add the A, but then that's also base64-encoded -- note that this base64 encoded result no longer always cleanly divides the Kitty sequence length limit of 4096, and no longer maps 3->4 clean base64s)
  • explore use of deflate compression
  • detect with a terminal query rather than heuristics
  • implement plane moves with kitty presentation changes, to avoid flicker/reload time
  • sprixelnonce needs to be atomic

we then ought look into the z-axis etc. capabilites of kitty graphics, and see if we can't cleanly match them to our current design. with that said, we should probably aim for the same visible architecture state across all NCBLIT_PIXEL implementations.

@dankamongmen dankamongmen added documentation Improvements or additions to documentation enhancement New feature or request labels Mar 9, 2021
@dankamongmen dankamongmen added this to the 2.3.0 milestone Mar 9, 2021
@dankamongmen dankamongmen self-assigned this Mar 9, 2021
@WSLUser
Copy link

WSLUser commented Mar 10, 2021

Maybe it's premature, but once you've got this figured out for notcurses, perhaps you could help get Kitty's graphics protocol implemented in Windows Terminal (I expect/hope for Sixel support to come in the next month or two there, your experience in consolidating logic to support both from perspective of this project should prove very helpful from a terminal point of view).

@dankamongmen
Copy link
Owner Author

Maybe it's premature, but once you've got this figured out for notcurses, perhaps you could help get Kitty's graphics protocol implemented in Windows Terminal (I expect/hope for Sixel support to come in the next month or two there, your experience in consolidating logic to support both from perspective of this project should prove very helpful from a terminal point of view).

heh, i am a MSFT employee these days, so i'd be happy to help out =]

@dankamongmen
Copy link
Owner Author

so i'm almost thinking that we require some kind of special mode for pixel graphics. maybe NCBLIT_PIXEL requires a NULL value for n, so that it always creates a new plane...and when this plane is destroyed or moved, it sets an atomic flag in the pile. when the pile is rendered, if this flag is set, note it and clear it. by "note it", i mean "write a prefix that clears the active images, when in kitty mode." it's gruesome, but it ought work...

@dankamongmen
Copy link
Owner Author

this will have to integrate with whatever damage manipulation falls out of #1388 .

@dankamongmen
Copy link
Owner Author

Maybe it's premature, but once you've got this figured out for notcurses, perhaps you could help get Kitty's graphics protocol implemented in Windows Terminal (I expect/hope for Sixel support to come in the next month or two there, your experience in consolidating logic to support both from perspective of this project should prove very helpful from a terminal point of view).

btw #1388 is where the real "expose a single model implemented by sixel and kitty" is happening. the main difference, as i detail there, is that kitty is positional and sixel is temporal in terms of text drawn on graphics. in kitty, you can place a graphic on a z-axis, where all text effectively lives at "z=1". i don't believe it's possible in kitty to have a single graphic where you draw atop some text and below other text. sixel, on the other hand, always blows the cell out when you write to it, whether that cell has text or graphics. so with sixel you need redraw your graphic whenever you change something underneath it (in a cell where the sixel has some transparency), but with kitty you sometimes need to manually cut out a chunk of the graphic (by setting alpha to 0) so you can write text atop it (in the case where you have natural transparency in your graphic, and want text underneath it, but also want to draw on top of it). you lose the ability to draw text with a transparent background (i.e. text with graphic behind it) in this model, but you can't do that in sixel at all. that's what i've concluded, anyway. again, see #1388 for the full details.

@dankamongmen
Copy link
Owner Author

hey @WSLUser i came across you the other day while wondering why i couldn't use conntrack in WSL =] you're all over the place!

@dankamongmen
Copy link
Owner Author

When building a kitty sprixel, it ought be easy enough to determine whether the alpha channel is actually in use (we're already doing something very similar with sixel, to determine whether P2 ought be 0 or 1). If it's not being used, we ought just transmit the data as RGB rather than RGBA, thus saving about 25% of the bulk. The downside would be that if we wiped a cell on such an image, we'd need introduce an alpha channel, requiring a complete rebuild of the sprixel (since we would know all existing alphas to be 0, we wouldn't need the original data to do this). With that said, compression using deflate might be able to cut most of this as well (but perhaps not as well as hoped, given that base64 encoding takes place before deflation).

@kovidgoyal
Copy link

You would get the most dramatic performance improvements by checking if
the filesystem/shared memory is useable for transmitting image data.

@WSLUser
Copy link

WSLUser commented May 30, 2021

Yes I tried to make the optimal kernel for WSL2 and conntrack should be supported. The true last stretch would be adding snapd support but there are plenty of other alternatives out there. If my optimized kernel is helping you get notcurses working better on WSL2 I'm glad to of been of assistance. I actually installed it on Pengwin, which still makes use of the Debian repos where notcurses is an easily installed library while running in Windows Terminal. Haven't run into anything that uses it yet but also still waiting for Sixel to get implemented. The architecture to get it running is there now so it just a matter of someone implementing it.

@WSLUser
Copy link

WSLUser commented May 30, 2021

I would try using both approaches of using an alpha channel along with the compression if possible as that would likely optimize perf as much as possible. If the base64 encoding is impeding the compression benefit, can the encoding be done after the compression instead?

@dankamongmen
Copy link
Owner Author

Detection via terminal query is moving along with the completion of #1469. We now just need #1761.

I don't intend to use deflate. See #1695; if we want compression, we'll use PNG.

So all that's left here is movement via kitty moves, which will be most welcome. Let's get on that soon.

@dankamongmen
Copy link
Owner Author

i'm having some trouble using placement for flicker-free movement.

i understand that if i load an image and specify the image id and placement id with a=T,i=N,p=M, i can then later use a=p,i=N,p=M at a different location to move the previously displayed image.

i load image ID 13, placement 1, and get a confirmation:

[schwarzgerat](127) $ cat l
[image]
[schwarzgerat](0) $ Gi=13,p=1;OK
```

but if i then follow up with a request to move it, i'm told "non-existent image":

```
[schwarzgerat](0) $ echo -e '\e_Ga=p,i=13,p=1;\e\\'
[schwarzgerat](0) $ '\e_Ga=p,i=13,p=1;\e\\'Gi=13,p=1;ENOENT:Put command refers to non-existent image with id: 13 and number: 0
```

@kovidgoyal , what am i missing? i'm sure i'm doing something stupid.

@kovidgoyal
Copy link

Works for me with the following script:

import sys
from base64 import standard_b64encode


def serialize_gr_command(cmd, payload=None):
    cmd = ','.join('{}={}'.format(k, v) for k, v in cmd.items())
    ans = []
    w = ans.append
    w(b'\033_G'), w(cmd.encode('ascii'))
    if payload:
        w(b';')
        w(payload)
    w(b'\033\\')
    return b''.join(ans)


def write_chunked(cmd, data=b''):
    if data:
        data = standard_b64encode(data)
        while data:
            chunk, data = data[:4096], data[4096:]
            m = 1 if data else 0
            cmd['m'] = m
            sys.stdout.buffer.write(serialize_gr_command(cmd, chunk))
            sys.stdout.flush()
            cmd.clear()
    else:
        sys.stdout.buffer.write(serialize_gr_command(cmd))
        sys.stdout.flush()


with open(sys.argv[-1], 'rb') as f:
    write_chunked({'a': 'T', 'i': 13, 'p': 17, 'f': 100}, f.read())
    write_chunked({'a': 'p', 'i': 13, 'p': 17})

Call it is python /t/png.py image.png

The second write_chunked() causes the image to move to the right and
lower from where it would be displayed without it.

@dankamongmen
Copy link
Owner Author

thanks for the explanation! moving on this now, and expecting an end to flicker (and a nice reduction in time and bytes) in the intro demo.

@dankamongmen
Copy link
Owner Author

confirmed that your script moves a png-loaded graphic; i'll take it from there

@dankamongmen
Copy link
Owner Author

write(1, "\33_Ga=T,i=13,p=17,f=100,m=1;iVBOR"..., 4125) = 4125                          
write(1, "\33_Gm=1;RcvbVx/FHOi+ceDWszZZCbAN9"..., 4105) = 4105                          
write(1, "\33_Gm=1;Dq6iH7SJfQEV9ZOxuq/hquY+G"..., 4105) = 4105                          
write(1, "\33_Gm=0;oO2IJSBJ7TO8ODr4UG0q34GWo"..., 3341) = 3341                          
select(0, NULL, NULL, NULL, {tv_sec=3, tv_usec=0}) = 0 (Timeout)                        
write(1, "\33_Ga=p,i=13,p=17\33\\", 18) = 18      

@dankamongmen
Copy link
Owner Author

hrmmm, the same example i showed above is now working just fine. mysterious! alright, i'll keep an eye out for that (maybe something is disappearing from the image cache? or maybe i just made some stupid error), but go ahead and implement this as intended.

@dankamongmen
Copy link
Owner Author

alright, i'm now using this whenever we hit sprixel_cleanup() with state SPRIXEL_MOVED. it appears to be working well enough, but we're not using it as often as we should -- intro only sees it used on the last move, and we redraw all the others, presumably due to state SPRIXEL_INVALIDATED. so we'll need do some work on our state machine to integrate this better. also, when we do use it, we're seeing characters drawn atop, but that's surely our bug.

@dankamongmen
Copy link
Owner Author

awwwwwwwww shiiittttt, we've got it working now (had to modify kitty_wipe() not to put us in SPRIXEL_INVALIDATED when already in SPRIXEL_MOVED), and it's sweeeeet. no flicker whatsoever in intro. yay! this is looking big!

@dankamongmen
Copy link
Owner Author

yeah, this is all looking good, i think it's ready to go honestly.

@dankamongmen
Copy link
Owner Author

https://www.youtube.com/watch?v=k8JflBNovLE boom!

Sprixel emits:elides: 1654:837 (33.60%) 2.61GB

@dankamongmen
Copy link
Owner Author

intro was:
Sprixel emits:elides: 18:507 (96.57%) 16.06MB

intro is now:
Sprixel emits:elides: 18:508 (96.58%) 2.85MB

almost an 82% reduction in emitted sprixel bytes

https://www.youtube.com/watch?v=1b9n0Amr9RI YEAHHHHHHHHHHHH

@dankamongmen
Copy link
Owner Author

tested with margins, looks good, i'm putting up the PR

@kovidgoyal
Copy link

Looks cool and I dont know how you have your internal graphical data structures designed but if calculating deltas between frames is relatively cheap you could get even more savings using the animation protcol.

@dankamongmen
Copy link
Owner Author

hrmmm no not yet perfect...we pass the wipebitmap test, but in intro, we're on top of the bottom graph, i.e. it looks like we never redrew after true wiping, which we ought do exactly one upon first moving into that area. so hold off on the merge. but we're close!

@dankamongmen
Copy link
Owner Author

Looks cool and I dont know how you have your internal graphical data structures designed but if calculating deltas between frames is relatively cheap you could get even more savings using the animation protcol.

i literally just ran back from a piss to reopen that bug and say exactly this <3

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.

3 participants