-
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
Implement Sixel encoder/decoder support #381
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Co-authored-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Should be enabled by default and name should reflect that.
d3ee822
to
db2c579
Compare
github.com/rivo/uniseg v0.4.7 | ||
github.com/soniakeys/quant v1.0.0 // indirect; go:build benchthis |
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.
not github.com/aymanbagabas/quant?
this is just for the bench?
) | ||
|
||
require github.com/aymanbagabas/quant v0.0.0-20250220224823-9dea6ec382b5 |
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.
would be good to move to the first require no?
@@ -18,3 +18,6 @@ | |||
|
|||
testdata | |||
*.png | |||
|
|||
# Allow graphics used for bench test | |||
!ansi/fixtures/graphics/*.png |
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 added ansi/fixtures/graphics/JigokudaniMonkeyPark.png
originally but i have a concern if it would come along when someone would install x/ansi, i assume not right?
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 think they wont as long as we use git-lfs
Only writes Sixel images for now
} | ||
} | ||
|
||
func writeSixel(w io.Writer, img image.Image) (int, 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.
fancy! loved you added an example :D
"github.com/charmbracelet/x/ansi/sixel" | ||
) | ||
|
||
func main() { |
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.
nit: is up to you, but would be nice to add a comment somewhere to a quick example (could be even using the png image we have in the source code)
@@ -0,0 +1,8 @@ | |||
package sixel | |||
|
|||
func max(a, b int) int { |
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.
this function seems quite simple do we need this? (doesn't exist in std?)
and I wonder if actually make sense to have in util.go if isn't used in more than 1 file
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.
it does in go1.20+ I think
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.
Go 1.21, actually. I know it seems silly but this was standard practice prior to that release (and generally not a big deal).
// make adaptive palette using median cut alogrithm | ||
q := e.Quantizer | ||
if q == nil { | ||
q = median.Quantizer(nc) |
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.
cool!
@aymanbagabas tested locally and this branch had a regression in performance compared to #380 and #352 ansi/sixel feat-sixel-support-pp - go test -bench=. -benchmem -count 10
goos: darwin
goarch: arm64
pkg: github.com/charmbracelet/x/ansi/sixel
cpu: Apple M4 Pro
BenchmarkEncodingGoSixel-14 4 295443552 ns/op 11894708 B/op 1035163 allocs/op
BenchmarkEncodingGoSixel-14 4 293885792 ns/op 11896036 B/op 1035164 allocs/op
BenchmarkEncodingGoSixel-14 4 304176646 ns/op 11894780 B/op 1035164 allocs/op
BenchmarkEncodingGoSixel-14 4 301280604 ns/op 11894680 B/op 1035163 allocs/op
BenchmarkEncodingGoSixel-14 4 299805969 ns/op 11894680 B/op 1035163 allocs/op
BenchmarkEncodingGoSixel-14 4 301399333 ns/op 11894688 B/op 1035163 allocs/op
BenchmarkEncodingGoSixel-14 4 300609573 ns/op 11894736 B/op 1035163 allocs/op
BenchmarkEncodingGoSixel-14 4 300067042 ns/op 11894688 B/op 1035163 allocs/op
BenchmarkEncodingGoSixel-14 4 300933344 ns/op 11894696 B/op 1035163 allocs/op
BenchmarkEncodingGoSixel-14 4 302264438 ns/op 11894728 B/op 1035163 allocs/op
BenchmarkEncodingXSixel-14 4 297760062 ns/op 43927074 B/op 636054 allocs/op
BenchmarkEncodingXSixel-14 4 298991354 ns/op 43927074 B/op 636054 allocs/op
BenchmarkEncodingXSixel-14 4 298944302 ns/op 43927074 B/op 636054 allocs/op
BenchmarkEncodingXSixel-14 4 301061729 ns/op 43927102 B/op 636054 allocs/op
BenchmarkEncodingXSixel-14 4 299697396 ns/op 43927106 B/op 636054 allocs/op
BenchmarkEncodingXSixel-14 4 302270990 ns/op 43927070 B/op 636054 allocs/op
BenchmarkEncodingXSixel-14 4 299696969 ns/op 43926950 B/op 636052 allocs/op
BenchmarkEncodingXSixel-14 4 302977177 ns/op 43926986 B/op 636053 allocs/op
BenchmarkEncodingXSixel-14 4 301184490 ns/op 43927074 B/op 636054 allocs/op
BenchmarkEncodingXSixel-14 4 300504781 ns/op 43927050 B/op 636054 allocs/op
PASS
ok github.com/charmbracelet/x/ansi/sixel 48.628s |
PR originally implemented by @CannibalVox
Ref: #352
Most of this PR changes are just updates on the implementation per requests
Supersedes: #380