Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions internal/book/book.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"image"
"image/png"
"io"
"os"
"path/filepath"
Expand All @@ -15,7 +16,6 @@ import (

_ "image/gif"
_ "image/jpeg"
_ "image/png"

_ "golang.org/x/image/bmp"
_ "golang.org/x/image/webp"
Expand Down Expand Up @@ -277,7 +277,8 @@ func (b *Book) lockPage(ctx context.Context, i int) (func(), error) {
}
}

// PageBytes returns a copy of the cached encoded bytes of page i (for HTTP serving).
// PageBytes returns a copy of the cached encoded bytes of page i (for HTTP
// serving). Converter pages sit in the cache as PPM and leave as PNG.
func (b *Book) PageBytes(i int) ([]byte, string, error) {
if i < 0 || i >= len(b.pages) {
return nil, "", fmt.Errorf("page %d out of range", i)
Expand All @@ -289,16 +290,30 @@ func (b *Book) PageBytes(i int) ([]byte, string, error) {
}
cached, ok := b.encoded[i]
b.mu.Unlock()
if ok {
return bytes.Clone(cached.data), cached.mime, nil
data, mime := cached.data, cached.mime
if !ok {
unlock, err := b.lockPage(context.Background(), i)
if err != nil {
return nil, "", err
}
data, mime, err = b.pageBytes(context.Background(), i)
unlock()
if err != nil {
return nil, "", err
}
}
unlock, err := b.lockPage(context.Background(), i)
if mime != ppmMIME {
return bytes.Clone(data), mime, nil
}
img, _, err := image.Decode(bytes.NewReader(data))
if err != nil {
return nil, "", fmt.Errorf("decode page %d: %w", i+1, err)
}
var out bytes.Buffer
if err := (&png.Encoder{CompressionLevel: png.BestSpeed}).Encode(&out, img); err != nil {
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
return nil, "", err
}
defer unlock()
data, mime, err := b.pageBytes(context.Background(), i)
return bytes.Clone(data), mime, err
return out.Bytes(), "image/png", nil
}

// pageBytes requires lockPage. The cache holds at most eight pages and maxPageBytes.
Expand Down
31 changes: 18 additions & 13 deletions internal/book/pdftext.go

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium book/pdftext.go:15

When pdftotext fails, pdfTextLayer returns nil while pdftohtml keeps running for up to toolTimeout, consuming a converter process whose result is no longer used. Cancel a child context and wait for the pdftohtml goroutine before returning from this error path.

	n := strconv.Itoa(page)
+	toolCtx, cancel := context.WithCancel(ctx)
+	defer cancel()
 	// pdftohtml parses the document again; let it run alongside pdftotext.
 	linkXML := make(chan []byte, 1)
 	go func() {
-		out, _ := runToolContext(ctx, "pdftohtml", "-xml", "-i", "-q", "-zoom", "1", "-f", n, "-l", n, "-stdout", path)
+		out, _ := runToolContext(toolCtx, "pdftohtml", "-xml", "-i", "-q", "-zoom", "1", "-f", n, "-l", n, "-stdout", path)
 		linkXML <- out
 	}()
-	out, err := runToolContext(ctx, "pdftotext", "-cropbox", "-bbox-layout", "-f", n, "-l", n, path, "-")
+	out, err := runToolContext(toolCtx, "pdftotext", "-cropbox", "-bbox-layout", "-f", n, "-l", n, path, "-")
 	if err != nil {
+		cancel()
+		<-linkXML
 		return nil
 	}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/book/pdftext.go around lines 15-25:

When `pdftotext` fails, `pdfTextLayer` returns `nil` while `pdftohtml` keeps running for up to `toolTimeout`, consuming a converter process whose result is no longer used. Cancel a child context and wait for the `pdftohtml` goroutine before returning from this error path.

Evidence trail:
`internal/book/pdftext.go:16-36` (d00be63); `internal/book/tool.go:16-21,38-75` (d00be63); `internal/book/select.go:108-159` (d00be63); `internal/book/book.go:216-236` (d00be63).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 673b29d. pdfTextLayer now derives a child context and cancels it on return, so an early exit kills pdftohtml instead of letting it run to completion or timeout.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The child context cancels pdftohtml, but early returns still do not wait for its goroutine to exit. Please receive from linkXML after cancellation before returning, as proposed, so the converter cleanup completes before pdfTextLayer returns.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the early returns as they are. The goroutine's only effect is a send into a buffered channel; cancellation kills pdftohtml and runToolLimited's Wait reaps it a few milliseconds later. Nothing reads the result or watches for the goroutine to finish, so blocking the caller on that reap would only delay PrepareText storing the empty layer. On the normal path the receive before pdfLinks already joins the two.

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ import (
// pdftotext is a paragraph; its lines carry a trailing space until the last.
func pdfTextLayer(ctx context.Context, path string, page int) []textLine {
n := strconv.Itoa(page)
// pdftohtml parses the document again; let it run alongside pdftotext,
// and stop it when the words do not come.
ctx, cancel := context.WithCancel(ctx)
defer cancel()
linkXML := make(chan []byte, 1)
go func() {
out, _ := runToolContext(ctx, "pdftohtml", "-xml", "-i", "-q", "-zoom", "1", "-f", n, "-l", n, "-stdout", path)
linkXML <- out
}()
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
out, err := runToolContext(ctx, "pdftotext", "-cropbox", "-bbox-layout", "-f", n, "-l", n, path, "-")
if err != nil {
return nil
Expand All @@ -27,7 +36,7 @@ func pdfTextLayer(ctx context.Context, path string, page int) []textLine {
if w <= 0 || h <= 0 {
return nil
}
links := pdfLinks(ctx, path, page, w, h)
links := pdfLinks(ctx, <-linkXML, path, page, w, h)
var lines []textLine
for _, flow := range pg.children {
for _, block := range flow.children {
Expand Down Expand Up @@ -132,23 +141,19 @@ func pdfLinkTarget(href string) (target string, internal, ok bool) {
}
return "", false, false
}
if _, page, found := strings.Cut(href, "#"); found {
if _, err := strconv.Atoi(page); err == nil {
return "#" + page, true, true
// The document name precedes the page and may itself contain '#'.
if i := strings.LastIndexByte(href, '#'); i >= 0 {
if _, err := strconv.Atoi(href[i+1:]); err == nil {
return href[i:], true, true
}
}
return "", false, false
}

// pdfLinks returns the link rectangles of a page as crop box fractions.
// pdftohtml works in the media box, so pdfinfo supplies the offset when the
// boxes differ in size.
func pdfLinks(ctx context.Context, path string, page int, cropW, cropH float64) []pdfLink {
n := strconv.Itoa(page)
out, err := runToolContext(ctx, "pdftohtml", "-xml", "-i", "-q", "-zoom", "1", "-f", n, "-l", n, "-stdout", path)
if err != nil {
return nil
}
// pdfLinks returns the link rectangles of a page as crop box fractions, read
// from the pdftohtml XML of that page. pdftohtml works in the media box, so
// pdfinfo supplies the offset when the boxes differ in size.
func pdfLinks(ctx context.Context, out []byte, path string, page int, cropW, cropH float64) []pdfLink {
doc, err := parseXML(out, maxDocumentTokens)
if err != nil {
return nil
Expand Down
53 changes: 53 additions & 0 deletions internal/book/ppm.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package book

import (
"bufio"
"errors"
"fmt"
"image"
"image/color"
"io"
)

// ppmMIME is the type the page pipeline sees for converter output. Browsers
// do not display it; PageBytes converts it to PNG.
const ppmMIME = "image/x-portable-pixmap"

// pdftoppm and ddjvu write binary PPM, which decodes in one pass. PNG would
// cost a compression round trip on every page turn.
func init() { image.RegisterFormat("x-portable-pixmap", "P6", decodePPM, decodePPMConfig) }

func ppmHeader(r *bufio.Reader) (w, h int, err error) {
var magic string
var depth int
if _, err := fmt.Fscan(r, &magic, &w, &h, &depth); err != nil || magic != "P6" || depth != 255 || w < 1 || h < 1 || w > maxPagePixels/h {
return 0, 0, errors.New("unsupported PPM page")
}
_, err = r.ReadByte() // one whitespace byte precedes the pixels
return w, h, err
}

func decodePPMConfig(r io.Reader) (image.Config, error) {
w, h, err := ppmHeader(bufio.NewReader(r))
return image.Config{ColorModel: color.RGBAModel, Width: w, Height: h}, err
}

func decodePPM(r io.Reader) (image.Image, error) {
br := bufio.NewReader(r)
w, h, err := ppmHeader(br)
if err != nil {
return nil, err
}
img := image.NewRGBA(image.Rect(0, 0, w, h))
row := make([]byte, 3*w)
for y := range h {
if _, err := io.ReadFull(br, row); err != nil {
return nil, errors.New("truncated PPM page")
}
pix := img.Pix[y*img.Stride : (y+1)*img.Stride]
for x := range w {
pix[4*x], pix[4*x+1], pix[4*x+2], pix[4*x+3] = row[3*x], row[3*x+1], row[3*x+2], 255
}
}
return img, nil
}
45 changes: 6 additions & 39 deletions internal/book/tool.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
package book

import (
"bufio"
"bytes"
"context"
"errors"
"fmt"
"image"
"image/png"
"io"
"os/exec"
"path/filepath"
Expand Down Expand Up @@ -85,24 +82,21 @@ type toolPage struct {
djvu bool
}

func (p toolPage) Name() string { return "page-" + strconv.Itoa(p.page) + ".png" }
func (p toolPage) Name() string { return "page-" + strconv.Itoa(p.page) + ".ppm" }

func (p toolPage) Open() (io.ReadCloser, error) {
return p.open(context.Background())
}

// open renders the page as binary PPM, which the registered decoder reads in
// one pass; see ppm.go.
func (p toolPage) open(ctx context.Context) (io.ReadCloser, error) {
n := strconv.Itoa(p.page)
var data []byte
var err error
args := []string{"pdftoppm", "-cropbox", "-scale-to", renderedPageSize, "-f", n, "-l", n, "-singlefile", p.path}
if p.djvu {
data, err = runToolContext(ctx, "ddjvu", "-format=ppm", "-size="+renderedPageSize+"x"+renderedPageSize, "-page="+n, p.path)
if err == nil {
data, err = ppmToPNG(data)
}
} else {
data, err = runToolContext(ctx, "pdftoppm", "-png", "-cropbox", "-scale-to", renderedPageSize, "-f", n, "-l", n, "-singlefile", p.path)
args = []string{"ddjvu", "-format=ppm", "-size=" + renderedPageSize + "x" + renderedPageSize, "-page=" + n, p.path}
}
data, err := runToolContext(ctx, args[0], args[1:]...)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -139,33 +133,6 @@ func openRendered(path string, djvu bool) (*Book, error) {
return b, nil
}

// ppmToPNG converts the binary PPM ddjvu writes into PNG for the page pipeline.
func ppmToPNG(data []byte) ([]byte, error) {
r := bufio.NewReader(bytes.NewReader(data))
var magic string
var w, h, depth int
if _, err := fmt.Fscan(r, &magic, &w, &h, &depth); err != nil || magic != "P6" || depth != 255 || w < 1 || h < 1 || w > maxPagePixels/h {
return nil, errors.New("ddjvu wrote no usable PPM page")
}
if _, err := r.ReadByte(); err != nil { // one whitespace byte precedes the pixels
return nil, err
}
pix := make([]byte, 3*w*h)
if _, err := io.ReadFull(r, pix); err != nil {
return nil, errors.New("ddjvu wrote a truncated PPM page")
}
img := image.NewRGBA(image.Rect(0, 0, w, h))
for i := range w * h {
copy(img.Pix[4*i:], pix[3*i:3*i+3])
img.Pix[4*i+3] = 255
}
var out bytes.Buffer
if err := (&png.Encoder{CompressionLevel: png.BestSpeed}).Encode(&out, img); err != nil {
return nil, err
}
return out.Bytes(), nil
}

// openDOC keeps the headings in antiword's DocBook output.
func openDOC(path string) (*Book, error) {
out, err := runTool("antiword", "-x", "db", path)
Expand Down
24 changes: 18 additions & 6 deletions internal/book/tool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ func requireTools(t *testing.T, tools ...string) {
}
}

func TestPPMToPNG(t *testing.T) {
data, err := ppmToPNG([]byte("P6\n2 1\n255\n\xff\x00\x00\x00\x00\xff"))
if err != nil {
t.Fatal(err)
func TestDecodePPM(t *testing.T) {
ppm := "P6\n2 1\n255\n\xff\x00\x00\x00\x00\xff"
config, format, err := image.DecodeConfig(strings.NewReader(ppm))
if err != nil || "image/"+format != ppmMIME || config.Width != 2 || config.Height != 1 {
t.Fatalf("config: %+v %s, %v", config, format, err)
}
img, err := png.Decode(bytes.NewReader(data))
img, _, err := image.Decode(strings.NewReader(ppm))
if err != nil || img.Bounds() != image.Rect(0, 0, 2, 1) {
t.Fatalf("decoded page: %v", err)
}
Expand All @@ -37,10 +38,18 @@ func TestPPMToPNG(t *testing.T) {
t.Fatal("pixel colors changed")
}
for _, bad := range []string{"P5\n2 1\n255\n\xff\xff", "P6\n2 1\n255\n\xff", "P6\n0 1\n255\n", "P6\n99999 99999\n255\n"} {
if _, err := ppmToPNG([]byte(bad)); err == nil {
if _, _, err := image.Decode(strings.NewReader(bad)); err == nil {
t.Fatalf("accepted %q", bad)
}
}
b2 := &Book{pages: []entry{memEntry{"page.ppm", []byte(ppm)}}}
data, mime, err := b2.PageBytes(0)
if err != nil || mime != "image/png" {
t.Fatalf("served PPM as %s, %v", mime, err)
}
if served, err := png.Decode(bytes.NewReader(data)); err != nil || served.Bounds() != image.Rect(0, 0, 2, 1) {
t.Fatalf("served PNG: %v", err)
}
}

func TestOpenPDF(t *testing.T) {
Expand Down Expand Up @@ -292,6 +301,9 @@ func TestPDFTextLayerSelectionAndLinks(t *testing.T) {
if _, internal, ok := pdfLinkTarget("Tricky name.html#12"); !internal || !ok {
t.Fatal("page reference with spaces")
}
if target, internal, ok := pdfLinkTarget("issue#2.html#12"); target != "#12" || !internal || !ok {
t.Fatalf("page reference behind a '#' in the document name: %q", target)
}
if _, _, ok := pdfLinkTarget("file:///etc/passwd"); ok {
t.Fatal("file link survived")
}
Expand Down
13 changes: 8 additions & 5 deletions internal/ui/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -398,12 +398,12 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
if m.webtoon && p.webScroll != 0 {
rerender = true
}
ready := frameReadyAfterPaint(p, msg.pane, msg.slot, msg.gen, rerender)
step := 1
if m.spreadActive() {
step = 2
cmds := []tea.Cmd{frameReadyAfterPaint(p, msg.pane, msg.slot, msg.gen, rerender)}
// Warm the whole next page or spread.
for pg := msg.page + m.slotCount(); pg < msg.page+2*m.slotCount(); pg++ {
cmds = append(cmds, m.prefetch(msg.pane, pg))
}
return m, tea.Batch(ready, m.prefetch(msg.pane, msg.page+step))
return m, tea.Batch(cmds...)

case frameReadyMsg:
p := m.panes[msg.pane]
Expand Down Expand Up @@ -845,6 +845,9 @@ func (m Model) updateMenu(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
if it.path != "" && (p.book == nil || p.book.Path != it.path) {
mm, cmd := m.openBook(m.active, it.path, nil)
mdl := mm.(Model)
if b := mdl.panes[mdl.active].book; b == nil || b.Path != it.path {
return mdl, cmd // the open failed; keep the current page
}
mdl2, cmd2 := mdl.goTo(mdl.active, it.page)
return mdl2, tea.Batch(cmd, cmd2)
}
Expand Down
Loading