dankamongmen / notcurses

blingful character graphics/TUI library. definitely not curses.
https://nick-black.com/dankwiki/index.php/Notcurses
Other
3.61k stars 114 forks source link

sixel actively clears too often, leading to flicker #1493

Closed dankamongmen closed 3 years ago

dankamongmen commented 3 years ago

As noted in #1487, the work in PR #1491 correctly actively clears moved or destroyed sixel graphics. Unfortunately, this is happening too frequently, leading to flicker of the same sort we saw prior to introducing the TAM level 2. This is due to the active clearing cycle. We need to eliminate that on a reuse, either by reusing the sprixel in toto or some other means.

dankamongmen commented 3 years ago

So just as we changed the TAM to be associated with the ncplane rather than the sprixel, I think the sprixel should be associated with the ncvisual. That allows us to reuse it both for multiframe media and for things like yield where we're directly modifying the ncvisual in-place. Just as a sprixel can (briefly) outlive the associated ncplane (the ncplane is deleted immediately; the sprixel is deleted upon the next rasterization of the pile), the sprixel must be able to outlive the associated ncvisual, perhaps substantially. That's fine -- the sprixel never needs reach into the ncvisual, only the other way. So just detach it upon ncvisual_delete().

dankamongmen commented 3 years ago

So to do this, let's add a sprixel* to ncvisual. When we get to the end of ncvisual_render_pixels(), we currently just get a new sprixel ID. Instead, break up sprixel_create() into sprixel_create() and sprixel_load(). The former allocs the sprixel and gets a new sprixel ID. The latter loads it with a glyph and marks it as SPRIXEL_INVALIDATED. Call sprixel_create() iff our sprixel* is NULL.

dankamongmen commented 3 years ago

This does indeed eliminate the flicker in sixel...but brings it back in kitty. Remember, kitty does require the delete-before-reload to eliminate flicker, for whatever reason. We've got to have two paths here, but we mustn't do it before rasterization time. Let's see if we can't sneak it into sprixel_draw() for kitty...?

dankamongmen commented 3 years ago

ok this makes sixel run perfectly, everything is golden, good FPS rates. i want it. get rid of the flicker in kitty while holding this down, and we're perfect. DO IT, NICHOLAS.

dankamongmen commented 3 years ago

no, it's not just deletion in kitty; i can put a delete right in front of the fwrite(), and we still flicker there. hrmm. let's capture output and compare the branch to master, where kitty is operating flicker-free.

dankamongmen commented 3 years ago
[schwarzgerat](2) $ ls -l err-branch err-master branch-flicker master 
-rw-r--r-- 1 dank dank 789587758 2021-04-01 20:05 branch-flicker
-rw-r--r-- 1 dank dank       443 2021-04-01 20:05 err-branch
-rw-r--r-- 1 dank dank       442 2021-04-01 20:06 err-master
-rw-r--r-- 1 dank dank 695109510 2021-04-01 20:06 master
[schwarzgerat](0) $ 

wait...kitty on master appears to be afflicted with flicker(?!)... =[ what

dankamongmen commented 3 years ago

oh fuck i checked this into master lol

dankamongmen commented 3 years ago

oh fuck i checked this into master lol

reverted, working in dankamongmen/sixelsmooth now

dankamongmen commented 3 years ago

ok, confirmed real master does not flicker on kitty, good. now to do the differential analysis.

dankamongmen commented 3 years ago
[schwarzgerat](0) $ ls -l master-yield branch-flicker 
-rw-r--r-- 1 dank dank 789587758 2021-04-01 20:05 branch-flicker
-rw-r--r-- 1 dank dank 755848440 2021-04-01 20:15 master-yield
[schwarzgerat](0) $ 

just from the file size differences, i'm thinking master is not scrubbing a bunch of cells that the branch is, and which we need scrubbed for sixel, but must not scrub for kitty.

dankamongmen commented 3 years ago

hrmmm, the only obvious change is the existence of the delete prior to the write, but i thought we tried that, and it definitely failed?

branch:

wAAAP8AAAD/AAAA/wAAAP8AAAD/AAAA/wAAAP8AAAD/AAAA/wAAAP8AAAD/^[\^[[2;5H^[(B^[[m^[[38;2;        
95;175;132m^[[48;2;0;0;1m8^[[C ^[[21G8^[[C ^[[35G8^[[C ^[[48G8^[[C ^[[76G8^[[C ^[[91G8^[[C ^[
[3;5H^[[38;2;103;183;140m"^[[C ^[[21G8^[[C ^[[35G8^[[C ^[[48G8^[[C ^[[76G8^[[C ^[[91G"^[[C ^[
[4;21H^[[38;2;111;191;148m8^[[C ^[[35G8^[[C ^[[48G8^[[C ^[[76G8^[[C ^[[5;1H^[[38;2;119;199   
;156m, ^[[5G8^[[C ^[[10G,adP^[[CYba, ^[[C8^[[C ^[[26G,d8 ^[[35G8^[[C,dP^[[CYba, ^[[48G8^[[C  
^[[C,adP^[[CY^[[Cba, ^[[65G,adP^[[CYba, ^[[C8^[[C ^[[81G,d8 ^[[91G8^[[C ^[[6;1H^[[38;2;127;2 
07;164m8a ^[[C8^[[C ^[[Ca8" ^[[17G"^[[C ^[[C8^[[C ,a8" ^[[35G8^[[CP' ^[[43G"8a ^[[C8^[[C ^[[C
"^[[C ^[[59G`Y8 ^[[Ca8" ^[[72G"^[[C ^[[C8^[[C ,a8" ^[[91G8^[[C ^[[7;1H^[[38;2;135;215;172m8  
^[[C ^[[C8^[[C ^[[C8b ^[[21G8^[[25G[ ^[[35G8^[[C ^[[44Gd8 ^[[C8^[[C ^[[C,adP^[[60G8^[[C ^[[C8
^[[76G8^[[80G[ ^[[91G8^[[C ^[[8;1H^[[38;2;143;223;180m8^[[C ^[[C8^[[C ^[[C"8a, ^[[16G,a^[[C  
^[[C8^[[C`"Yba, ^[[35G8^[[Cb, ^[[42G,a8" ^[[C8^[[C ^[[C8^[[C, ^[[59G,8^[[C ^[[C"8a, ^[[71G,a^
[C ^[[C8^[[C`"Yba, ^[[91G8^[[C ^[[9;1H^[[38;2;151;231;188m8^[[C ^[[C8^[[C ^[[10G`"Yb^[[Cd8"  
' ^[[C8^[[C ^[[26G`Y8a ^[[35G8Y"Yb^[[Cd8"' ^[[48G8^[[C ^[[C`"8b^[[CdP"Y8 ^[[65G`"Yb^[[Cd8"'  
 ^[[C8^[[C ^[[81G`Y8a ^[[91G8^[[C ^[[10;90H^[[38;2;159;239;196m,8^[[C ^[[11;88H^[[38;2;167   
;247;204m8^[[91GP ^[[65;10H^[(B^[[0;1m^[[3m^[[38;2;128;255;128m^[[48;2;16;8;16m6^[[66;12     
H^[[38;2;128;230;153m6^[[67;12H^[[38;2;128;204;179m5^[[15G^[[23m^[[38;2;128;208;255m^[[      
48;2;53;48;53m6^[[23G17^[[68;12H^[[3m^[[38;2;128;179;204m^[[48;2;16;8;16m9^[[69;12H^[[3      
8;2;128;153;230m9^[[70;12H^[[38;2;128;128;255m7^[[22;1H^[_Gf=32,s=930,v=523,i=1,a=T,      

master:

AAD/^[\^[_Ga=d,d=i,i=19^[\^[[2;11H^[(B^[[m^[[38;2;95;175;132m^[[48;2;0;0;1m8^[[C ^[[27G8^[[  
C ^[[41G8^[[C ^[[54G8^[[C ^[[82G8^[[C ^[[3;11H^[[38;2;103;183;140m"^[[C ^[[27G8^[[C ^[[41G8^[
[C ^[[54G8^[[C ^[[82G8^[[C ^[[4;27H^[[38;2;111;191;148m8^[[C ^[[41G8^[[C ^[[54G8^[[C ^[[82G8 
^[[C ^[[5;1H^[[38;2;119;199;156mdP^[[CYba, ^[[11G8^[[C ^[[16G,adP^[[CYba, ^[[C8^[[C ^[[32G,  
d8 ^[[41G8^[[C,dP^[[CYba, ^[[54G8^[[C ^[[C,adP^[[CY^[[Cba, ^[[71G,adP^[[CYba, ^[[C8^[[C ^[[87
G,d8 ^[[6;1H^[[38;2;127;207;164m' ^[[5G`"8a ^[[C8^[[C ^[[Ca8" ^[[23G"^[[C ^[[C8^[[C ,a8" ^[  
[41G8^[[CP' ^[[49G"8a ^[[C8^[[C ^[[C"^[[C ^[[65G`Y8 ^[[Ca8" ^[[78G"^[[C ^[[C8^[[C ,a8" ^[[7;7
H^[[38;2;135;215;172m8^[[C ^[[C8^[[C ^[[C8b ^[[27G8^[[31G[ ^[[41G8^[[C ^[[50Gd8 ^[[C8^[[C ^[[
C,adP^[[66G8^[[C ^[[C8b ^[[82G8^[[86G[ ^[[8;7H^[[38;2;143;223;180m8^[[C ^[[C8^[[C ^[[C"8a,   
^[[22G,a^[[C ^[[C8^[[C`"Yba, ^[[41G8^[[Cb, ^[[48G,a8" ^[[C8^[[C ^[[C8^[[C, ^[[65G,8^[[C ^[[C"
a, ^[[77G,a^[[C ^[[C8^[[C`"Yba, ^[[9;7H^[[38;2;151;231;188m8^[[C ^[[C8^[[C ^[[16G`"Yb^[[Cd8  
"' ^[[C8^[[C ^[[32G`Y8a ^[[41G8Y"Yb^[[Cd8"' ^[[54G8^[[C ^[[C`"8b^[[CdP"Y8 ^[[71G`"Yb^[[Cd8"  
' ^[[C8^[[C ^[[87G`Y8a ^[[65;9H^[(B^[[0;1m^[[3m^[[38;2;128;255;128m^[[48;2;16;8;16m20^[[66   
;12H^[[38;2;128;230;153m6^[[67;10H^[[38;2;128;204;179m7^[[C1^[[C^[[23m^[[38;2;128;208;2      
55m^[[48;2;53;48;53m20^[[23G31^[[68;12H^[[3m^[[38;2;128;179;204m^[[48;2;16;8;16m3^[[22;      
1H^[_Gf=32,s=930,v=523,i=21,a=T,m=1;AAAA/wAAAP8AAAD/AAAA/wAAAP8AAAD/AAAA/wAAAP8AA          
dankamongmen commented 3 years ago

hrmm, we're writing new indexes on master. could it possibly come down to that?

dankamongmen commented 3 years ago

hrmm, we're writing new indexes on master. could it possibly come down to that?

or possibly that all the writing text came between the two?

dankamongmen commented 3 years ago

YEAH BITCHES THAT GOT IT

dankamongmen commented 3 years ago

it's uggggggggly as hell, need to clean it up, but this gets us flicker-free in both, finally:

[schwarzgerat](0) $ git diff
diff --git src/lib/internal.h src/lib/internal.h
index 53cd80f94..c0bf1acf5 100644
--- src/lib/internal.h
+++ src/lib/internal.h
@@ -812,6 +812,7 @@ int kitty_draw(const notcurses* n, const ncpile *p, sprixel* s, FILE* out);
 int sixel_draw(const notcurses* n, const ncpile *p, sprixel* s, FILE* out);
 // dimy and dimx are cell geometry, not pixel. takes ownership of s on success.
 sprixel* sprixel_alloc(ncplane* n, int dimy, int dimx);
+sprixel* sprixel_recycle(ncplane* n);
 int sprixel_load(sprixel* spx, char* s, int bytes, int placey, int placex,
                  int pixy, int pixx, int parse_start);
 int sprite_wipe_cell(const notcurses* nc, sprixel* s, int y, int x);
diff --git src/lib/render.c src/lib/render.c
index 86c3b69f3..e08a27de3 100644
--- src/lib/render.c
+++ src/lib/render.c
@@ -903,7 +903,7 @@ clean_sprixels(notcurses* nc, const ncpile* p, FILE* out){
   int ret = 0;
   while( (s = *parent) ){
     if(s->invalidated == SPRIXEL_HIDE){
-//fprintf(stderr, "OUGHT HIDE %d [%dx%d @ %d/%d] %p\n", s->id, s->dimy, s->dimx, s->y, s->x, s);
+fprintf(stderr, "OUGHT HIDE %d [%dx%d @ %d/%d] %p\n", s->id, s->dimy, s->dimx, s->y, s->x, s);
       if(sprite_destroy(nc, p, out, s) == 0){
         *parent = s->next;
         sprixel_free(s);
diff --git src/lib/sprite.c src/lib/sprite.c
index 3a0225523..0d8b6a8d9 100644
--- src/lib/sprite.c
+++ src/lib/sprite.c
@@ -10,6 +10,17 @@ void sprixel_free(sprixel* s){
   }
 }

+sprixel* sprixel_recycle(ncplane* n){
+  const notcurses* nc = ncplane_notcurses(n);
+  if(nc->tcache.pixel_destroy == sprite_kitty_annihilate){
+    int dimy = n->sprite->dimy;
+    int dimx = n->sprite->dimx;
+    sprixel_hide(n->sprite);
+    return sprixel_alloc(n, dimy, dimx);
+  }
+  return n->sprite;
+}
+
 // store the original (absolute) coordinates from which we moved, so that
 // we can invalidate them in sprite_draw().
 void sprixel_movefrom(sprixel* s, int y, int x){
diff --git src/lib/visual.c src/lib/visual.c
index 43d835f98..4dca586fb 100644
--- src/lib/visual.c
+++ src/lib/visual.c
@@ -608,6 +608,8 @@ ncplane* ncvisual_render_pixels(notcurses* nc, ncvisual* ncv, const struct blits
     if((ncv->spx = sprixel_alloc(n, rows, cols)) == NULL){
       goto err;
     }
+  }else{
+    ncv->spx = sprixel_recycle(n);
   }
   bargs.u.pixel.spx = ncv->spx;
   if(ncvisual_blit(ncv, disprows, dispcols, n, bset, disprows, dispcols, &bargs)){
[schwarzgerat](0) $