A potential problem with the proposed image cache/pool policy.

[ismail-yilmaz]

Hello,

According to page 6:

When leaving the alternate screen, its image storage pool gets cleared too.

I am not sure if this is really a viable strategy. Not that it is inherently problematic, aside from the fact that it makes cache management slightly more complex, but AFAIK, some ncurses based applications, such as nano exits the alternate screen if you try to resize the screen. This is IIRC its resize policy. Now, ofc nano is a text editor, but it is a real-life example of such behaviour. Besides, an application can use both primary and alternate screens, alternatively.

[christianparpart]

That is a good point I did not yet think of. I think this statement should then be removed. I will adapt the draft source tomorrow morning. :)

[ismail-yilmaz]

I'll try to come up with some suggestions especially regarding the cache management, in the following days.
(Thanks for this open discussion repo. Hopefully this discussions and the draft will brew into something actually useful.)

[jerch]

Eww, yes can repro that with nano. To me this seems rather awkward, maybe it is a workaround for some older TE bugs? Following default buffer switch behavior I would not expect that images on the alternate will be preserved, while images on normal buffer will. But to me the differences of DECSET 47 | 1047 | 1049 are not so clear (I think we dont handle them correctly in xterm.js)

Currently I have implemented image eviction this way in xterm.js:

• one pool for all images
• eviction by image data pressure following FIFO (with customizable limit)
• images have buffer adherence (cannot be referred on other buffer)
• buffer switch always clear alternate images
• in place eviction on tile basis (delete image on tile_counter = 0)

[ismail-yilmaz]

Eww, yes can repro that with nano. To me this seems rather awkward, maybe it is a workaround for some older TE bugs?

Possibly. But I remember another such app, (But I forgot what it was...)

Following default buffer switch behavior I would not expect that images on the alternate will be preserved, while images on > normal buffer will. But to me the differences of DECSET 47 | 1047 | 1049 are not so clear (I think we dont handle them correctly in xterm.js)

No, they aren't clear. Not in practice, at least. IME, terminals vary in their implementation.

Currently I have implemented image eviction this way in xterm.js:

* one pool for all images

* eviction by image data pressure following FIFO (with customizable limit)

* images have buffer adherence (cannot be referred on other buffer)

* buffer switch always clear alternate images

* in place eviction on tile basis (delete image on tile_counter = 0)

Similar to mine. Single - shared - pool + LRU + discarding images a certain points (but no ref count.) This works well with the existing forms of image protocols and apps. One advantage, IMO, of LRU-cache is that it prioritizes what's displayed/refreshed/requested most.

As I mentioned above, I am going to open a separate discussion about the cache management policy of the draft proposal. I think it has some scalability problems. As if it implicitly assumes a single terminal app with normal and alt buffers. It doesn't seem to account for multiple terminal widgets in a single terminal app. Hence disregards the shared cache implementations. In my case our vte is a widget that can be used to build a terminal app. Why would I have to discard an already existing image If I can use it in multiple vtes (in the same app)?

[jerch]

As I mentioned above, I am going to open a separate discussion about the cache management policy of the draft proposal.

Sure, just one thing - I am not sure, if cache policy should be part of the image protocol in the end beside some basic guarantees. But lets see that after discussing it.

[ismail-yilmaz]

@jerch

I am not sure, if cache policy should be part of the image protocol in the end beside some basic guarantees.

That's one of the points I wanted to bring up in that discussion. Pool/cache management should be up to vtes. But the recommended interaction in the draft, between apps and vtes in this regards, seems to pose some problems too. Or maybe I have misunderstood something. Either way, we'll hopefully clarify some things up. :)

[christianparpart]

Hey guys.

I'd like to share some of my thoughts when I was formalizing the text. (I am totally open to change of course).

• I first thought about doing the image pool cleaning when entering alt buffer, but that's basically the same, but sooner (so resources are getting freed earlier)
• I decided to require a "alt-buffer" image pool cleaning on the alt buffer because of SM ? 1049 doing the same on the screen buffer and cursor position, too. I am absolutely fine in dropping that though, and even go with one single image pool.
• Cache Eviction: I think requiring the VTE implementer to use LRU or similar protocol is way over the top. I chose reference counting because that is the most deterministic, and can be very well defined in the spec as well.
• And instead of forcing any kind of out-of-bounds cache eviction due to low resource situation or whatever, I highly recommend to go the other way around (I basically stole the idea from OpenGL specs). the protocol spec instead should provide minimum guarantees that are required to be supported. Above that, a VTE is of course free to raise these values in their implementation, but definitely not belowing.
• With that, it may be useful to also be query for the actual resource limit, just in case there might be some image-heavy application that's not simply cat-like printing or a imaginary vim or Emacs being able to display nifty image files.

p.s.: I've to refresh my mind a bit on that topic also as my last months have been quite occupying due to personal healthcare reasons. I'll be more available therefore now and can also allocate more time on fun stuff like these :)

[christianparpart]

FYI: I've updated the spec to only have one shared image storage pool with no forced eviction upon screen switch between primary/alternate.

The only way to get images out is by erasing them from the screen, and having no named reference on them - or if the storage pool becomes full and a new image is added, then FIFO must be applied for eviction with either rendering a placeholder instead on the screen for images that still referenced that, or just an empty cell (I think the latter is better than a placeholder, and also simpler from the dev-perspective).

[christianparpart]

@ismail-yilmaz / @jerch would you mind having a look at it again, and tell me if you're fine with the updated text? I'd highly appreciate that (no hurry though). :-)

[ismail-yilmaz]

@christianparpart Will do. But I've been very busy lately, I'll have the time to look at it and comment on it next weekend. :)
Thank you for your hard work and updates!

[jerch]

@christianparpart Sounds good to me. 👍

Imho the placeholder thingy is helpful for peeps to get at least a visual feedback, why there is some room in the scrollback, that it was meant to hold an image thats already gone. I dont think it should be mandatory in the spec, maybe just outlined as an idea, how things can be implemented with basic UX in mind.

Same with the caching strategy - imho it should not be a fixed part of the spec, but outlined as an implementation idea. I'd leave those details always to TEs, maybe someone got a really nice idea later on and others want to adopt that.

(Imho the worst we could do is very strict spec about implementation details. 😸)

[christianparpart]

I agree with both. Will update the text accordingly.

EDIT: Done, and pushed to master. It's part of the latest prerelease PDF/md too.