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

transparent pixels aren't elided from output sixel #149

Open
dankamongmen opened this issue Mar 17, 2021 · 17 comments · May be fixed by libsixel/libsixel#23 or WSLUser/libsixel#2
Open

transparent pixels aren't elided from output sixel #149

dankamongmen opened this issue Mar 17, 2021 · 17 comments · May be fixed by libsixel/libsixel#23 or WSLUser/libsixel#2

Comments

@dankamongmen
Copy link

Hello there! I'm adding libsixel support to Notcurses. Everything's working pretty well, except that transparent pixels (A=255 in RGBA8888) aren't being elided. For the attached image, you can see from oiiotool that we've got alphas of either 0 or 255:

...
    Pixel (37, 98): 112 72 24 255 (0.439216 0.282353 0.0941177 1)
    Pixel (38, 98): 112 72 24 255 (0.439216 0.282353 0.0941177 1)
    Pixel (39, 98): 112 72 24 255 (0.439216 0.282353 0.0941177 1)
    Pixel (40, 98): 112 72 24 255 (0.439216 0.282353 0.0941177 1)
    Pixel (41, 98): 112 72 24 255 (0.439216 0.282353 0.0941177 1)
    Pixel (42, 98): 0 0 0 0 (0 0 0 0)
    Pixel (43, 98): 0 0 0 0 (0 0 0 0)
    Pixel (44, 98): 0 0 0 0 (0 0 0 0)
    Pixel (45, 98): 0 0 0 0 (0 0 0 0)

Here's my native decoder:

2021-03-17-030356_334x165_scrot

Here's libsixel:

2021-03-17-030435_401x233_scrot

All the [0 0 0 0] RGBA values have somehow become red, when ideally they wouldn't be included at all.

Some decision function taking the 8 bit alpha input needs yield a 1 bit output of "include" or "elide". It makes sense to me to make this an attribute of the sixel_dither_t that the user can set (just a threshold, not a callback).

I'm happy to write this up, but wanted to ask first whether I'm missing something, or whether this is a good idea.

@dankamongmen
Copy link
Author

I think this might actually hinge on emission of Set Raster Attributes (the sequence following the initial \ePq, containing geometry). If I include "1;1;x;y" in my Sixel, i get this background. Without it, I do not. Is the SRA sequence necessary?

@dankamongmen
Copy link
Author

I think this might actually hinge on emission of Set Raster Attributes (the sequence following the initial \ePq, containing geometry). If I include "1;1;x;y" in my Sixel, i get this background. Without it, I do not. Is the SRA sequence necessary?

nevermind, that's false. it's just that color #0 that we pick up -- 44,0,0 for this image, a color nothing like anything from the actual image source.

@db48x
Copy link

db48x commented May 28, 2021

@dankamongmen, have you made a start on fixing this?

@dankamongmen
Copy link
Author

@dankamongmen, have you made a start on fixing this?

no; i never heard anything back (is libsixel development dead? last commit was 2020-01), and wasn't interested in forking this or putting up a PR that wouldn't get merged. Notcurses is using its own encoder, which is not yet as fast nor as high-quality IMHO as libsixel, but ought rapidly improve when i put some more work into it (it's good enough for now).

@db48x
Copy link

db48x commented May 28, 2021

Yea, dead or very slow; it happens.

Another annoyance is that much of what libsixel does is there to directly support img2sixel. Things like processing environment variables, printing directly to stdout after encoding, the way options are handled, etc.

@dankamongmen
Copy link
Author

Another annoyance is that much of what libsixel does is there to directly support img2sixel. Things like processing environment variables, printing directly to stdout after encoding, the way options are handled, etc.

i hate to pump for my own project on libsixel's page, but try out the sprixel functionality in notcurses (you get kitty support for free, in addition). like i said, my quantizer is slower than libsixel's, and has banding artifacts that imgsixel floyd-steinbergs away, but i know how to fix that. feel free to mail me or post on our discussion board if you have any questions/suggestions/unsolicited bulk commercial mail etc

@ctrlcctrlv
Copy link

This library doesn't support transparency, at least via the sixel_decode_raw API. The comment here is not true:

unsigned char /* out */ **palette, /* ARGB palette */

In actual fact you receive an RGB (24-bit) palette in all cases. I do not know why "A" is promised but there is no "A".

This is actually known to @saitoha. Just, the issue is in Japanese. (I don't expect most people to know Japanese, so I mention this just to help out.) Cf. #28, «透過色出力対応». 透過色⇒transparent color, 出力⇒output, 対応⇒support. Since this is an open issue, it is known there is no support for transparent colors.

It's possible that sixel_helper_load_image_file can be used to get such support, but this isn't useful for most of us because it requires a filename, so it's a dead end. I see sixel_frame_get_transparent and sixel_frame_strip_alpha which seem to promise some support, but I can't see a why to use frame API when sixel data already in memory.

@ctrlcctrlv
Copy link

By the way, issue #28 also says this, meaning I think it should be a solvable problem:

libsixelの内部では単色カラーキーのインデックス番号を保持しているので、それにアクセスするAPIを公開するべき

Meaning:

libsixel internally holds an index to the transparent color. A way to access this index should be added.

@db48x
Copy link

db48x commented Jun 9, 2021

Thanks for pointing out #28; I hadn’t noticed it. It will be worth taking a look at.

@ctrlcctrlv
Copy link

@db48x I decided to take over as maintainer. (See #154 if you haven't already.)

Happy to collaborate on this. It's something I know I need for merger into Kitty. I doubt @kovidgoyal will accept a Sixel implementation that doesn't even support alpha channel, since it's in the spec and xterm handles it fine. I didn't think the Kitty patch would lead to me proofreading paragraphs of DeepL-produced Japanese text, but whatever, open source leads in strange directions. :-)

@kovidgoyal
Copy link

kovidgoyal commented Jun 9, 2021 via email

@ctrlcctrlv
Copy link

@kovidgoyal Yes, I already resolved one CVE (libsixel#8) and am waiting for more info on the other one as it's no longer reproducible under recent libpng (I think it's a libpng CVE that bubbled up). So I'd consider my fork secure.

For Kitty, we're going to version lock it. Last version of libsixel by @saitoha is 1.8.6. When I work out the other CVE I'm bumping to 1.9. When I work out this issue, transparency, I'm bumping to 2.0. Kitty will refuse to use any libsixel below 2.0.

@kovidgoyal
Copy link

Cool, sounds good to me.

@db48x
Copy link

db48x commented Jun 9, 2021

libsixel internally holds an index to the transparent color. A way to access this index should be added.

I think this is a misunderstanding of how sixel images work. The palette for sixel images does not have a transparent color. Instead, transparency happens whenever you avoid placing color in some spot in the image. It may be that @saitoha misunderstood the sixel spec when he was originally writing the code (he wouldn’t have been the only one).

@ctrlcctrlv
Copy link

I agree, but don't put endless faith in my Japanese translation either :-)

I had a hard time understanding what was meant by «単色カラーキーのインデックス» (tanshoku karā kī no indekkusu). I think that 単 is meant as in ひとえ (hitoe), which means "single". So…"index of a single color key" literally.

More literal:

libsixelの内部では単色カラーキーのインデックス番号を保持しているので、それにアクセスするAPIを公開するべき
libsixel no naibu de wa tanshoku karā kī no indekkusu bangō wo hoji shiteiru no de, sore ni akusesu-suru API wo koukai-suru beki
because libsixel's internals contain an index of a single color's key, an access API for it should be made public

But…«単色» can also have the meaning "monochromatic", as in 単色光 (tanshokkou), a monochromatic light.

image

So, "monochromatic color key". I don't think that makes sense. I think perhaps what he's trying to say is that internally we choose a single color in the palette and mark that as the transparent color. Then API consumers are supposed to consider that single color to be alpha 0.

You can imagine how confusing it must have been to implement the spec as a non-native speaker, which compounds the issue.

@db48x
Copy link

db48x commented Jun 9, 2021

I don’t know any Japanese at all, but it makes sense to me. Color keying or “green screen” is not exactly a new technique. :)

ctrlcctrlv added a commit to libsixel/libsixel that referenced this issue Jun 16, 2021
Closes #15.
Closes #16.
Closes saitoha#149.
Closes saitoha#28.
Closes saitoha#61.

This PR fixes both problems at once. It does so by adding a new decode
function, and using it by default in `img2sixel`.

This function returns to the user RGBA data instead of palette'd data.
This sidesteps the many issues I found when considering raising the
maximum size of a palette to 255*255*255*255. For one thing, I already
knew I needed at some point for Kitty project something like this so I
wouldn't need to allocate my own, which is needless effort when the
decoder can do it.

This is ready for review. @dankamongmen may have time. I'll leave it for
a while as I go back to working on Kitty. If no one has time to review,
I'll merge it in ~4 days or so.
@ctrlcctrlv
Copy link

ctrlcctrlv commented Jun 16, 2021

@db48x I believe I've fixed this. Consider trying the libsixel#23 branch. I am aware some tests are failing, will rectify tomorrow or in the days ahead. sixel2png now can decode transparent SIXEL data. It can't yet encode such data from transparent images, that will be a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants