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

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

Closed
waveplate opened this issue Jun 11, 2024 · 12 comments
Closed

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

waveplate opened this issue Jun 11, 2024 · 12 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@waveplate
Copy link

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!


  • export | egrep 'LANG|LC_CTYPE|TERM'
declare -x LANG="en_US.UTF-8"
declare -x TERM="xterm-256color"
declare -x XTERM_LOCALE="en_US.UTF-8"
declare -x XTERM_SHELL="/usr/bin/bash"
declare -x XTERM_VERSION="XTerm(392)"
  • notcurses version (available from notcurses-demo i)
    3.0.9
  • terminal name + version
    XTerm(392)
@waveplate waveplate added the bug Something isn't working label Jun 11, 2024
@waveplate
Copy link
Author

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 dankamongmen self-assigned this Jun 11, 2024
@dankamongmen dankamongmen added this to the 3.1.0 milestone Jun 11, 2024
@dankamongmen
Copy link
Owner

dankamongmen commented Jun 11, 2024

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
Copy link
Owner

dankamongmen commented Jun 11, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

yeah your fix seems reasonable; thanks!

@dankamongmen
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

i've added code to reject these invalid geometries

@dankamongmen
Copy link
Owner

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

@dankamongmen
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants