dankamongmen / notcurses

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

ncsixel_as_rgba fails when sixel image height is greater than 13 #2784

Closed waveplate closed 4 weeks ago

waveplate commented 4 weeks ago

ncsixel_as_rgba fails when the height of the sixel image exceeds 13 pixels. as far as i can tell, the width is irrelevant -- i tested sixel images over 1000 pixels wide without issue.

the sixel images were mostly generated with img2sixel, but i also tested with random sixel images i found online, thinking maybe img2sixel was doing something outside of the sixel spec.

but it always fails when the height exceeds 13 pixels.

with loglevel set to trace, these are the relevant errors:

ncsixel_as_rgba:59:expected octothorpe, got 63
ncvisual_from_sixel:803:failed converting sixel to rgba
ncvisual_from_file failed

and here is an example sixel image that triggers the bug (22x14)

alien14.txt

thanks!


waveplate commented 4 weeks ago

i think i found the issue:

}else if(*sx == '$'){
  x = 0;
  state = STATE_WANT_HASH;

the state should instead be set to STATE_WANT_DATA after the carriage return command. this fixed the issue for me, and in the case that a hash is encounter after this, sx pointer will be decremented and the state set to STATE_WANT_HASH, so i don't think there's any risk of this breaking anything

dankamongmen commented 4 weeks ago

interesting; i have src/tests/sixels.cpp which ought be testing this. let me check it out...that's part of notcurses-tester and ought be running hrmm.

dankamongmen commented 4 weeks ago

test SixelRoundTrip for instance works with worldmap.png, which is definitely more than 13 rows. let me look at your comment+example more closely...

dankamongmen commented 4 weeks ago

ok, the only test i have of ncvisual_from_sixel() etc. is LoadSixel, which just uses

"\x1bP0;1;0q\"1;1;1;6#0;2;66;18;80#0@-\x1b\\"

let's see what happens if i plug your file in there....

dankamongmen commented 4 weeks ago

looks confirmed, new test LoadBigSixel fails. good find, thanks @waveplate !


  SUBCASE("LoadBigSixel") {
    auto ncv = ncvisual_from_sixel("\x1bPq\"1;1;22;14#0;2;9;13;13#1;2;9;16;19#2;2;16;22;25#3;2;53;56;56#4;2;66;63;60#5;2;72;75;69#6;2;69;72;66#7;2;60;63;63#8;2;35;41;44#9;2;28;35;35#10;2;25;28;28#11;2;19;22;25#12;2;0;0;0#13;2;16;19;22#14;2;31;38;38#15;2;63;66;66#16;2;19;25;28#17;2;3;3;6#18;2;47;50;50#19;2;69;72;72#20;2;25;31;31#21;2;6;9;13#22;2;47;47;47#23;2;6;13;19#24;2;47;53;50#25;2;19;19;19#26;2;6;6;13#27;2;44;41;38#28;2;56;53;53#29;2;50;47;47#30;2;19;16;16#31;2;47;50;47#32;2;63;60;56#33;2;47;47;44#34;2;25;25;28#35;2;35;31;31#36;2;53;50;50#37;2;6;3;6#1eLB#14A!7?_K???GO#12@^~~$#0H#13qK#18C???_!6?_#11BDC#0G#26_$#23O#20?_!9?_OC#2?A#13AO$#16??O#3G@??O_?U#22G???_O#25__$#7???O!6?H#8V?_WG#1?@#17A$#4???__@#15o?]M#24_!4?O#21??C$#2???@#5[[FB?o#9??RN#10@C_G$#6!4?Aa??@@#16!4?A$#19!6?GK-#0KO!4?G!6?oG?`X???C$#1@B#10_??_?G???GwCA!4?C#12_`$#21o#13K???G#30oo!5?GCA??F`#1K$#23A#17_#22O?CC??@??A?A#17_G!4?PQ$#27??@!4?CgG#3A#20?@!5?_O$#28??KH??@A?_#8??A@@#11DC_#25GA$#31??A#4_?A#35C?O??o#37??OO#21W???AG$#32???U#14O!6?@C!6?G$#9!4?G!6?C#26???_#34AAO$#29!4?`@#33A?CCG#13!6?C$#5!4?A#25O#24?@#18A?D$#36!9?Oo$#7!9?B-#0B?@#8@A#36@#27@!4?A#10A??AA@#17A@$?BA#1A#32@A??@AA#35@#34@#11BB#13@@A@#12ABB$#28!6?AA?@@$#29!7?@#4A\x1b\\", 1, 1);
    REQUIRE(ncv);
    uint32_t p;
    ncvisual_at_yx(ncv, 0, 0, &p);
    /*CHECK(0xff == ncpixel_a(p));
    CHECK(0xa8 == ncpixel_r(p));
    CHECK(0x2d == ncpixel_g(p));
    CHECK(0xcc == ncpixel_b(p));*/
    ncvisual_destroy(ncv);
  }
dankamongmen commented 4 weeks ago

yeah your fix seems reasonable; thanks!

dankamongmen commented 4 weeks ago

hrmmm, even with that change, we're running into two possible problems...with dimensions of 0, 0 we get:

ncsixel_as_rgba:164:too many rows 0 + 6 > 0
ncvisual_from_sixel:805:failed converting sixel to rgba

and with 1, 1 we get:

ncsixel_as_rgba:168:invalid rle 1 + 1 > 1
ncvisual_from_sixel:805:failed converting sixel to rgba

with 100, 100 things work. let me investigate this...

dankamongmen commented 4 weeks ago

i get these same errors with these dimensions using the old code, but now 100, 100 also breaks, with:

ncsixel_as_rgba:59:expected octothorpe, got 63

ncvisual_from_sixel:805:failed converting sixel to rgba
dankamongmen commented 4 weeks ago

ok, ncsixel_as_rgba() clearly oughtn't be accepting 0 as either argument, at least as currently written:

  uint32_t* rgba = (uint32_t*)calloc(leny * lenx, sizeof(*rgba));                                                                   
  if(rgba == NULL){                                                                                                                 
    return NULL;                                                                                                                    
  }        
dankamongmen commented 4 weeks ago

i've added code to reject these invalid geometries

dankamongmen commented 4 weeks ago

this particular sixel appears to require at least 18, 22 geometry

dankamongmen commented 4 weeks ago

Things ought now work for you, @waveplate ; thanks for the report and fix! I've added a unit test for this case.