-
Notifications
You must be signed in to change notification settings - Fork 22
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
Sixel Graphics Implementation #352
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work here!
Initial review here with few comments to abstract and generalize the implementation. I wonder if we need a sixel.Options
here, instead, we can place encoder/decoder customizations and options directly in the structure definitions.
"strconv" | ||
"strings" | ||
|
||
"github.com/bits-and-blooms/bitset" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway we can get rid of this dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way I see it working is writing our own bitset implementation- is that something you want to pursue?
// sixelColor is a flat struct that contains a single color: all channels are 0-100 | ||
// instead of anything sensible | ||
type sixelColor struct { | ||
Red uint32 | ||
Green uint32 | ||
Blue uint32 | ||
Alpha uint32 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// sixelColor is a flat struct that contains a single color: all channels are 0-100 | |
// instead of anything sensible | |
type sixelColor struct { | |
Red uint32 | |
Green uint32 | |
Blue uint32 | |
Alpha uint32 | |
} | |
// sixelColor is a flat struct that contains a single color: all channels are 0-100 | |
// instead of anything sensible | |
type RGBAColor color.RGBA | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we good with the fact that RGBA colors sourced from outside this package will not work properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RGBA colors that are not sixel.RGBAColor
can be converted using sixelConvertColor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more about this, I think it makes more sense if we have type Color struct { Pc, Pu uint8; Px, Py Pz int }
and functions the same as the other types, func (Color) Encode(io.Writer) error
, func (Color) String() string
, and func ParseColor([]byte) (Color, error)
// - If a single color sits on a cut line, all pixels of that color are assigned to one of the subcubes | ||
// rather than try to split them up between the subcubes. This allows us to use a slice of unique colors | ||
// and a map of pixel counts rather than try to represent each pixel individually. | ||
type sixelPalette struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we expose this and be able to pass a custom Palette
in *Options
?
ansi/sixel/encoder.go
Outdated
func (e *Encoder) encodePaletteColor(w io.Writer, paletteIndex int, c sixelColor) { | ||
// Initializing palette entries | ||
// #<a>;<b>;<c>;<d>;<e> | ||
// a = palette index | ||
// b = color type, 2 is RGB | ||
// c = R | ||
// d = G | ||
// e = B | ||
|
||
w.Write([]byte{sixelUseColor}) //nolint:errcheck | ||
io.WriteString(w, strconv.Itoa(paletteIndex)) //nolint:errcheck | ||
io.WriteString(w, ";2;") | ||
io.WriteString(w, strconv.Itoa(int(c.Red))) //nolint:errcheck | ||
w.Write([]byte{';'}) //nolint:errcheck | ||
io.WriteString(w, strconv.Itoa(int(c.Green))) //nolint:errcheck | ||
w.Write([]byte{';'}) | ||
io.WriteString(w, strconv.Itoa(int(c.Blue))) //nolint:errcheck | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's define sixel.BasicColor
, sixel.RGBAColor
, sixel.HLSColor
types that implement color.Color
and fmt.Stringer
. We can also have a sixel.ParseColor([]byte) (color.Color, error)
to decode a color from bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a basiccolor in this case, is it just getting a palette index with no color information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
type Decoder struct { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make the palette customizable?
type Decoder struct { | |
} | |
type Decoder struct { | |
Palette *Palette | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this just cover default colors, or should we also ignore color definitions when a palette is provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use default colors if Palette
is nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but do the passed in palette colors get overwritten by defined colors, or do they only replace the default colors?
Tests are failing because the |
Co-authored-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Co-authored-by: Ayman Bagabas <ayman.bagabas@gmail.com>
I think I should probably mention: the encoder previously used a single stringbuilder in order to minimize allocations. These Raster/Repeat/Color/etc. methods will each add an allocation every time they're called, which will impact performance. |
Fair point. Let's change these methods to be writers |
@@ -288,6 +288,11 @@ func ParseRaster(data io.Reader) (pan, pad, ph, pv int, err error) { | |||
return | |||
} | |||
|
|||
func ParseRepeat(data io.Reader) (count int, r byte, err error) { | |||
_, err = fmt.Fscanf(data, "%d%b", &count, &r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would take a []byte
over a reader for simplicity and so that it works with raw bytes
var buf bytes.Buffer | ||
|
||
buf.WriteString("\x1bP") | ||
buf.WriteString(strconv.Itoa(p1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buf.WriteString(strconv.Itoa(p1)) | |
if p1 >= 0 { | |
buf.WriteString(strconv.Itoa(p1)) | |
} |
buf.WriteString("\x1bP") | ||
buf.WriteString(strconv.Itoa(p1)) | ||
buf.WriteByte(';') | ||
buf.WriteString(strconv.Itoa(p2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buf.WriteString(strconv.Itoa(p2)) | |
if p2 >= 0 { | |
buf.WriteString(strconv.Itoa(p2)) | |
} |
buf.WriteByte(';') | ||
buf.WriteString(strconv.Itoa(p2)) | ||
buf.WriteByte(';') | ||
buf.WriteString(strconv.Itoa(p3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buf.WriteString(strconv.Itoa(p3)) | |
if p3 >= 0 { | |
buf.WriteString(strconv.Itoa(p3)) | |
} |
func Raster(pan, pad, ph, pv int) string { | ||
return fmt.Sprintf("%s%d;%d;%d;%d", string(RasterAttribute), pan, pad, ph, pv) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can have a type Raster struct { Pan, Pad, Ph, Pv int}
with func (Raster) Encode(io.Writer) error
, func (Raster) String() string
that uses r.Encode
, and func ParseRaster([]byte) (Raster, error)
func Repeat(count int, repeatRune rune) string { | ||
var sb strings.Builder | ||
sb.WriteByte(RepeatIntroducer) | ||
sb.WriteString(strconv.Itoa(count)) | ||
sb.WriteRune(repeatRune) | ||
return sb.String() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, a type Repeat struct { Count int; Char byte }
with func (Repeat) Encode(io.Writer) error
, func (Encode) String() string
, and func ParseRepeat([]byte) (Repeat, error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these types reflecting the sixel graphics protocol, we could drop the sixel.Options
type and embed these types in sixel.Decoder
and sixel.Encoder
// sixelColor is a flat struct that contains a single color: all channels are 0-100 | ||
// instead of anything sensible | ||
type sixelColor struct { | ||
Red uint32 | ||
Green uint32 | ||
Blue uint32 | ||
Alpha uint32 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more about this, I think it makes more sense if we have type Color struct { Pc, Pu uint8; Px, Py Pz int }
and functions the same as the other types, func (Color) Encode(io.Writer) error
, func (Color) String() string
, and func ParseColor([]byte) (Color, error)
// p3 = Horizontal grid size parameter. Everyone ignores this and uses a fixed grid | ||
// size, as far as I can tell. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI, this p3 parameter was only used by Sixel printers. It never applied to Sixel terminals because their screen pixels were obviously in a fixed position, so there was no way to change the horizontal spacing.
fyi this implementation perf comparisson with go-sixel
comparing encoding only: package abs
import (
"os"
"bytes"
"fmt"
"testing"
"image"
"image/png"
"github.com/mattn/go-sixel"
"github.com/charmbracelet/x/ansi"
sixxel "github.com/charmbracelet/x/ansi/sixel"
)
func BenchmarkGoSixel(b *testing.B) {
for b.Loop() {
raw, err := loadImage("./sixel.png")
if err != nil {
os.Exit(1)
}
var b = bytes.NewBuffer(nil)
enc := sixel.NewEncoder(b)
if err := enc.Encode(raw); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
// fmt.Println(b)
}
}
func BenchmarkXSixel(b *testing.B) {
for b.Loop() {
raw, err := loadImage("./sixel.png")
if err != nil {
os.Exit(1)
}
var b = bytes.NewBuffer(nil)
if err := ansi.WriteSixelGraphics(b, raw, &sixxel.Options{}); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
}
}
func loadImage(path string) (image.Image, error) {
f, err := os.Open(path)
if err != nil {
return nil, err
}
return png.Decode(f)
} |
Memory, although it was tested with 154kb image.
|
With 10.9mb png image (encoding only)
|
Hey @CannibalVox! We’ve pulled your changes into #380 and built on them a bit 🙏 Thank you very much for the contribution @CannibalVox and making Sixel possible in x ! |
This PR adds support for sixels to x/ansi. Sixels are a protocol for writing images to the terminal by writing a large blob of ANSI-escaped data. They function by encoding columns of 6 pixels into a single character (in much the same way base64 encodes data 6 bits at a time). Sixel images are paletted, with a palette established at the beginning of the image blob and pixels identifying palette entries by index while writing the pixel data.
Sixels are written one 6-pixel-tall band at a time, one color at a time. For each band, a single color's pixels are written, then a carriage return is written to bring the "cursor" back to the beginning of a band where a new color is selected and pixels written. This continues until the entire band has been drawn, at which time a line break is written to begin the next band.
Sixel writing and reading take the form of the sixel.Encoder and sixel.Decoder types, in order to match the existing kitty graphics implementation as closely as we reasonably can. The only issue is that because there is no way to know the size of a sixel blob in advance (unlike the kitty graphics protocol, we can't determine the size of the payload from the width and height), we can only determine the boundaries of the payload by detecting the ST sequence (or BEL or whatever we're using to close the payload). As a result, the sixel decoder accepts a byte slice that contains only the payload and relies on the caller to determine the boundaries.
I included an options type in the parameters for WriteSixelGraphics, but there are not yet any options. I'd like to include a dithering implementation in the future, but I wanted to get the actual sixel implementation checked in first.