Re: [Xournal-devel] poppler for xournal

[Denis A. <au...@ma...>]

Hi again,

1. The patch applies semi-cleanly to the standard 0.4.2.1 version (with 
some offsets) -- I checked and "patch" definitely does what it's supposed 
to do. There are some offsets, probably because your patch is against 
a previously patched version and not the stock 0.4.2.1 -- which might be 
the problem if some of the necessary changes to 0.4.2.1 are not included 
in the patch. For your info:

auroux > patch -p1 < CONTRIB/poppler-pdf-rendering-0.0.5.patch 
patching file configure
Hunk #1 succeeded at 5719 (offset 91 lines).
patching file configure.in
patching file src/main.c
patching file src/xo-callbacks.c
Hunk #1 succeeded at 93 (offset 2 lines).
patching file src/xo-file.c
patching file src/xo-file.h
patching file src/xo-misc.c
Hunk #5 succeeded at 1183 (offset -8 lines).
Hunk #7 succeeded at 1619 with fuzz 1 (offset -22 lines).
patching file src/xo-print.c
patching file src/xournal.h

2.
> > (minor)
> > - scrolling through a long document is slow (even when returning to pages
> > that have already been generated -- shouldn't they be already in memory,
> > or are they destroyed as they become invisible ? In the latter case, I'll
> > work on adding a smarter cache feature if/when I get to it.)
> 
> They should not be slow when returning to pages already visited (as currently I
> don't free the pixmaps once rendered).  If I scroll to the end, afterward I can
> scroll as fast as it can paint...  I can't see any speed difference between
> 0.4.2.1. 

Ah, I see. You have "progressive backgrounds" disabled, while I have it
enabled. Scrolling past a page boundary calls do_switch_page(), which
when "progressive backgrounds" is enabled calls rescale_bg_pixmaps().
In the first for() loop in rescale_bg_pixmaps() you removed a line

-      if (pg->bg->pixbuf_scale == ui.zoom) continue;

which prevented background pixmaps that were already at the correct zoom 
level from being destroyed and later regenerated. Ok, not a big deal.

> Notice how evince does it: when you scroll fast it will display a blank
> "Loading..." page which still lets you flip pages while it waits for the pixmap.
> Something like that for xournal would be ideal, and shouldn't be too hard.

Yes, I agree entirely.

> The question remains: how many pages to keep in memory?  All?  Last N?  
> 2*#visible?

Ideally, some combination, with an upper bound on how much RAM one is
allowed to use for page backgrounds (configurable in the config file).
But this is not urgent for the time being. In fact, now that I see it
run at the intended speed (with progressive backgrounds disabled there's
many fewer calls to poppler) I'm not sure how much caching one needs to do
after all.

3.
> > (major)
> > - if I zoom in, the background disappears, it says 5 times
> > "Request for page 1 at scale 2.000000", but nothing appears and I can't
> > draw. If I zoom back out then the background shows up again, but at 
> > scale 2 (instead of back at the default scale 1.3333).
> 
> I don't see this at all.  Zooming in and out works perfectly.
> Can you play around and see if you can figure out what's causing this?

Ah, again only an issue if "Progressive backgrounds" is enabled.
I'll add a comment to your tracker item to say one must disable
the "progressive backgrounds" option to use the patch. I have no
idea why things break so badly with "progressive backgrounds" enabled.

4. regarding your comment about fuzziness of backgrounds in 0.4.2.1: 
yes, I see it, especially at certain zoom levels (the default zoom 
level is normally set up so usual page sizes end up being near-integers),
and in fact the fastest fix for it is just to change

     pg->bg->canvas_item = gnome_canvas_item_new(pg->group, 
         gnome_canvas_pixbuf_get_type(), 
         "pixbuf", pg->bg->pixbuf,
-        "width", pg->width, "height", pg->height, 
-        "width-set", TRUE, "height-set", TRUE, 
+        "width-in-pixels", 1, "height-in-pixels", 1,
         NULL);

as you did in a couple of places. (One should probably also do something
near the beginning of update_page_stuff() to make sure that the values of
pg->voffset get rounded to integers (line 1180 of xo-misc.c in 0.4.2.1)
so the bitmap not only doesn't get stretched by a factor like 1.0001 
(fixed by the change you made) but also doesn't get translated by a
half-pixel).

5. As you mentioned, things crash badly when one tries to open another
pdf file within a same xournal session. I think one should have kept a
cleanup function (shutdown_bgpdf() or equivalent) to restore things to
a sane state (delete the temporary copy of the pdf file, clean up data
structures, etc.).

6. Small but easy-to-fix bug: the background for page 7 of the xoj file
might be page 4 of the pdf file, e.g. if I inserted blank pages before it.
So when requesting a background, you should not call render_pdf_page()
with a page number within the journal, but rather with pg->bg->file_page_seq
(in rescale_bg_pixmaps() and update_page_stuff()).


	Denis
-- 
Denis Auroux                    au...@ma...
MIT Room 2-248                  phone: (617) 253 4993
77 Massachusetts Ave.           fax:   (617) 253 4358
Cambridge MA 02139