search

tdewolff / canvas

Cairo in Go: vector to raster, SVG, PDF, EPS, WASM, OpenGL, Gio, etc.
MIT License
1.48k stars 99 forks source link

Switch from benoitkugler/textlayout to go-text/typesetting #227

Closed farhaven closed 1 year ago

farhaven commented 1 year ago

Hi,

github.com/benoitkugler/textlayout has been superseeded by github.com/go-text/typesetting. It might be a good idea to switch to that.

For us, it'd solve a bit of an issue that (luckily) only crops up in tests but that I'm a bit uncomfortable with:

The loop started in https://github.com/benoitkugler/textlayout/blob/main/harfbuzz/ot_tag.go#L316 contains a bug if the script is something like utf-8: It'll peek a character too far beyond the end of the string if it's looking at the 8:

panic: runtime error: index out of range [5] with length 5 [recovered]
    panic: runtime error: index out of range [5] with length 5

goroutine 100 [running]:
github.com/benoitkugler/textlayout/harfbuzz.NewOTTagsFromScriptAndLanguage(0x7a797979, {0xc000eb405b, 0x5})
    external/com_github_benoitkugler_textlayout/harfbuzz/ot_tag.go:317 +0x558
github.com/benoitkugler/textlayout/harfbuzz.newOtMapBuilder(0xc00093ac80, {{0xc000eb405b, 0x5}, 0x7a797979, 0x4})
    external/com_github_benoitkugler_textlayout/harfbuzz/ot_map.go:85 +0xd0
github.com/benoitkugler/textlayout/harfbuzz.newOtShapePlanner(0xc00093ac80, {{0xc000eb405b, 0x5}, 0x7a797979, 0x4})
    external/com_github_benoitkugler_textlayout/harfbuzz/ot_shaper.go:43 +0x1a4
github.com/benoitkugler/textlayout/harfbuzz.(*otShapePlan).init0(0xc000e44248, 0xc00093ac80, {{0xc000eb405b, 0x5}, 0x7a797979, 0x4}, {0x0, 0x0, 0x0}, {0xffffffffffffffff, ...})
    external/com_github_benoitkugler_textlayout/harfbuzz/ot_shaper.go:174 +0x88
github.com/benoitkugler/textlayout/harfbuzz.(*shaperOpentype).compile(0xc000e44240, {{0xc000eb405b, 0x5}, 0x7a797979, 0x4}, {0x0, 0x0, 0x0})
    external/com_github_benoitkugler_textlayout/harfbuzz/ot_shaper.go:762 +0x118
github.com/benoitkugler/textlayout/harfbuzz.newShapePlan(0xc000408000, {{0xc000eb405b, 0x5}, 0x7a797979, 0x4}, {0x0, 0x0, 0x0}, {0x0, 0x0, ...})
    external/com_github_benoitkugler_textlayout/harfbuzz/shape.go:134 +0x1c0
github.com/benoitkugler/textlayout/harfbuzz.newShapePlanCached(0xc000408000, {{0xc000eb405b, 0x5}, 0x7a797979, 0x4}, {0x0, 0x0, 0x0}, {0x0, 0x0, ...})
    external/com_github_benoitkugler_textlayout/harfbuzz/shape.go:179 +0x45c
github.com/benoitkugler/textlayout/harfbuzz.(*Buffer).Shape(0xc000e9da00, 0xc000408000, {0x0, 0x0, 0x0})
    external/com_github_benoitkugler_textlayout/harfbuzz/shape.go:29 +0xf0
github.com/tdewolff/canvas/text.Shaper.Shape({0xc000408000}, {0xc000ea5ff9, 0x4}, 0xd, 0x0, 0x7a797979, {0x0, 0x0}, {0x0, 0x0}, ...)
    external/com_github_tdewolff_canvas/text/harfbuzz.go:51 +0x210
github.com/tdewolff/canvas.(*RichText).ToText(0xc0003fed20, 0x4024000000000000, 0x4024000000000000, 0x2, 0x2, 0x0, 0x0)
    external/com_github_tdewolff_canvas/text.go:567 +0xf6c
github.com/tdewolff/canvas.NewTextBox(0xc00020d970, {0xc000ea5f7c, 0x4}, 0x4024000000000000, 0x4024000000000000, 0x2, 0x2, 0x0, 0x0)
    external/com_github_tdewolff_canvas/text.go:299 +0xdc
go.vaira.app/foo/bar.draw(/* ... */)
    foo/bar/draw.go:275 +0x684c

/* rest of stack trace elided */

The slightly weird paths come from our build system (Bazel), but shouldn't matter too much. I've truncated the stack trace at the first line of our own code and censored that a bit, but I don't think it's too relevant anyway.

The harfbuzz implementation in github.com/go-text/typesetting does not contain the problematic loop, it looks like the relevant code has been rewritten some time ago.

Would it be possible to switch from Benoit's old implementation of harfbuzz to the new one at go-text/typesetting? If you're OK with that, I'd prepare a PR for it so that you don't have to do too much work on our behalf.

tdewolff commented 1 year ago

Thanks for the heads-up. I've switched to using the go-text/typesetting library, let me know if it fixes your issue. There still is an indirect dependency on benoitkugler/textlayout that I don't know where it comes from though.

I see that we can't remove the benoitkugle/textprocessing dependency yet, as that has a Fribidi implementation they are hesitant to move to go-text due to the LGPL license.

farhaven commented 1 year ago

let me know if it fixes your issue.

It does indeed! Thanks for the quick fix, it is much appreciated :)