-
Notifications
You must be signed in to change notification settings - Fork 88
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
transparent pixels aren't elided from output sixel #149
Comments
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. |
@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). |
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. |
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 |
This library doesn't support transparency, at least via the Line 801 in 6a5be8b
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 |
By the way, issue #28 also says this, meaning I think it should be a solvable problem:
Meaning:
|
Thanks for pointing out #28; I hadn’t noticed it. It will be worth taking a look at. |
@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. :-) |
On Wed, Jun 09, 2021 at 07:48:47AM -0700, Fredrick Brennan wrote:
@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. :-)
Yeah most definitely dont want kitty depending on an unmaintained library, that's just asking for
security issues.
|
@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. |
Cool, sounds good to me. |
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). |
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:
But…«単色» can also have the meaning "monochromatic", as in 単色光 (tanshokkou), a monochromatic light. 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. |
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. :) |
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.
@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. |
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:Here's my native decoder:
Here's libsixel:
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.
The text was updated successfully, but these errors were encountered: