Bug 5589

I can reproduce this bug with cairo-1.0.2 (dapper packages). I've tried with
current cairo git and now loading the file is fast (less than a second).

Thanks for the interesting news. I guess I'll wait for the release of the stable
version.

Luca, I have tried it with cairo-1.0.4 and it is as slow as it was with
cairo-1.0.2. Could you confirm this? (Include the date you downloaded cairo git,
if 1.0.4 doesn't fix the bug.)

The difference that matters here is probably the version of xserver you are using.

I don't think so
(http://mces.blogspot.com/2006/03/new-cairo-stable-release.html#114274706364115581),
Jeff. (Correct me, if I'm wrong.)

Thanks, anyway.

Opening speed is now fine with cairo-1.1.2, but bitmaps aren't antialiased and
it doesn't look fine (changing summary).

Would it be possible to have all bitmaps rendered by poppler-cairo antialiased?

I'm afraid that poppler 0.5.2 and cairo 1.1.2 are not able to display the bitmap
font.

This is really a cairo bug. Cairo doesn't do minification filtering so scaling down images doesn't look very 
good. Until cairo gets minification filtering I don't think there is really anything that can be done...

Also, the poppler 0.5.2 problem is now fixed in cvs.

Created attachment 8041 [details] [review]
pre-scale images instead of having cairo scale them.

A very rough and only briefly tested patch that should fix this problem.

Created attachment 8048 [details] [review]
pre-scale images instead of having cairo scale them ver 2

A slightly better version.

*** Bug 9149 has been marked as a duplicate of this bug. ***

*** Bug 8982 has been marked as a duplicate of this bug. ***

*** Bug 8844 has been marked as a duplicate of this bug. ***

*** Bug 4420 has been marked as a duplicate of this bug. ***

*** Bug 10302 has been marked as a duplicate of this bug. ***

Created attachment 9278 [details] [review]
Latest patch.

Really ugly code but it should work. It should even do a better job than splash.

Any progress on this Jeff? I think this bug is a blocker for poppler 0.6, it would be really nice to have this fixed before, even though it's not fixed in cairo. 

It would be good to hear how the current patch works for people. I probably wont have any time to look at this before 28th of July or so.

Created attachment 10826 [details]
bitmap font text that poppler doesn't scale

Jeff, bitmap font texts are displayed antialiased (or simply better) appliying your patch.

But at least with text, it seems that bitmap text does not scale at all.

The bug is not introduced by your patch, because I had the bug after moving to poppler-0.5.9.

I guess this should be fixed before the final 0.6 release.

Created attachment 10827 [details]
bitmap font right scaled by poppler-0.5.4

As announced before, a bug might have been introduced in poppler-0.5.9 that scales bitmap font texts wrong. poppler-0.5.4 scales the image right.

Is this bug related to #11421? with current cvs head this document: 

http://dmi.uib.es/~loren/docencia/propostaproj2005.pdf

is rendered fine, but document attached to bug #11421 is still wrong rendered. 

Yeah, they are related. My guess is that the pdf in 11421 uses drawImage() instead drawImageMask() and so was affected by the fix that I committed. 

Jeff, is it possible then to fix bug #11421 by using the same code, or at least the same approach?

btw, there are some documents that are crashing now in this assert() in CairoOutputDev::drawImageMaskPrescaled():

if (n != 0) {
       printf("n = %d (%d %d %d)\n", n, head_pad_size, pix_size, tail_pad_size);
      assert(n == 0);
}

like the one reported here:

http://bugzilla.gnome.org/show_bug.cgi?id=468667

(In reply to comment #23)
> Jeff, is it possible then to fix bug #11421 by using the same code, or at least
> the same approach?

Yes it is possible to fix it using the same approach. Though it would take a little bit of work.

> 
> btw, there are some documents that are crashing now in this assert() in
> CairoOutputDev::drawImageMaskPrescaled():
> 
> if (n != 0) {
>        printf("n = %d (%d %d %d)\n", n, head_pad_size, pix_size,
> tail_pad_size);
>       assert(n == 0);
> }
> 
> like the one reported here:
> 
> http://bugzilla.gnome.org/show_bug.cgi?id=468667

I've just fixed this in CVS.

-Jeff

*** Bug 12270 has been marked as a duplicate of this bug. ***

Bug #9860 is related to this bug.

*** Bug 11421 has been marked as a duplicate of this bug. ***

I was unable to apply the patch to poppler-0.10.7, may i ask what the current situation regarding this bug is?

Created attachment 31130 [details]
jstor pdf. Drag the background to a new evince and it looks fine.

Using poppler 0.12.0
It is hard to read jstor pdfs because of this bug.
If I open the pdf I see a badly rendered picture.
However, if I drag the background (drag from a blank area) to a new evince window, it will load the background image nicely rendered.
So it is possible to render the image much nicer, it just doesn't.

But maybe the change in image quality is because evince doesn't use poppler for displaying image files, so when I drag the background image to a new evince it doesn't use poppler?

(In reply to comment #29)
> But maybe the change in image quality is because evince doesn't use poppler for
> displaying image files, so when I drag the background image to a new evince it
> doesn't use poppler?
> 

No, the background is not a pdf document, but an image which is rendered with GdkPixbuf. 

(In reply to comment #30)
> (In reply to comment #29)
> > But maybe the change in image quality is because evince doesn't use poppler for
> > displaying image files, so when I drag the background image to a new evince it
> > doesn't use poppler?
> > 
> 
> No, the background is not a pdf document, but an image which is rendered with
> GdkPixbuf. 
> 

So my issue is fixed if either cairo is fixed and thus scales the image nicely, or worked around by prescaling the image before handing it to cairo?

Reassigning bug because it seems jeff is not working on this anymore (no reply since feb. 2008 and no reply to comment #28 aug. 2009).

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > But maybe the change in image quality is because evince doesn't use poppler for
> > > displaying image files, so when I drag the background image to a new evince it
> > > doesn't use poppler?
> > > 
> > 
> > No, the background is not a pdf document, but an image which is rendered with
> > GdkPixbuf. 
> > 
> 
> So my issue is fixed if either cairo is fixed and thus scales the image nicely,
> or worked around by prescaling the image before handing it to cairo?

I hope it's fixed in cairo/pixman soon.

> Reassigning bug because it seems jeff is not working on this anymore (no reply
> since feb. 2008 and no reply to comment #28 aug. 2009).
> 

Well, maybe Jeff is not working on this bug anymore, but he is still working on this issue in pixman/cairo side, see:

http://lists.cairographics.org/archives/cairo/2009-July/017867.html

Created attachment 31242 [details] [review]
prescale patch

Here is my patch that adds prescale capability

Created attachment 31245 [details]
updated patch

Thank you very much, your patch seems to work great.

(In reply to comment #34)
> Created an attachment (id=31245) [details]

Some comments first on the presentation of the patch:

 * just send the patch by itself, there's no need to include the modified file as well - the patch is much easier to read.

 * generate the patch using "diff -urNp old new" when doing so manually. However, git will generate easy-to-read patches with either "git diff", or preferably "git format-patch -1". Note the latter will require you to commit your changes and so also will include a changelog entry and a record of authorship. (And means we can very simply apply the patch to our git trees.)

 * Please try to conform to the coding style of the file you are modifying.

My opinion is that this is vital to get this into pixman as soon as possible - however, the core poppler developers want to get a workaround into poppler now. In terms of implementation, I see nothing fundamentally wrong with the patch (it implements a basic 2D box filter, that would be worth a comment or a better function name!) - though the patches proposed for pixman should achieve better performance.

(In reply to comment #36)
Thanks for your comments

-Regarding presentation: you're probably right. I'm pretty much clueless here. Please let someone in the know (yourself?) fix this to get this patch accepted. For someone who has done this already this shouldn't take much time. I would have to install git, read the man pages etc, probably only to realize nobody is interested in this patch.

-I think this or any other fix should be implemented asap since it renders a lot of pdf readers useless when scanned documents must be read.(e.g. all older scientific papers). I had to install acrobat reader on Ubuntu. This is ludicrous.

-As for the pixman implementation. This might be better, but performance wise I wouldn't expect too much from it because this filtering just doesn't take very long compared to all the other processes. If you are interested in speed it might be worthwhile to write a separate one bit drawimage function. The majority of the the scanned documents are one bit (b/w). In the current implementation the one bit stream is converted to a byte stream then to a ARGB image and then prescaled which is a roundabout way of doing this.

*** Bug 25244 has been marked as a duplicate of this bug. ***

Note that while this patch does wonders for screen output, it horribly degrades printed output in Evince -- it very much looks like the same downscaled image is used for printing as well.

Created attachment 31667 [details] [review]
patch with printer exception

Added printing exception. Now only prescaling when not printing.

My version of patch says the first hunk of the most recent patch is malformed. It looks like you're missing half a line or so including initializing a couple variables. I've uploaded the version of the patch that seems to work on my systems. I haven't looked into this in any real depth. Is this right? Or am I just confused?

Created attachment 31683 [details] [review]
fixed version of huug's latest patch (?)

(In reply to comment #41)
> Is this right?

Yes. So was his previous patch -- I fixed it by hand and then forgot to upload a fixed version, sorry about that.

(In reply to comment #43)
Weird. My diff output seems to be missing parts! (a bug?)
Does the patch work? I don't have a printer.

(In reply to comment #44)
> Does the patch work?

Yes, thanks.

Created attachment 31924 [details] [review]
fixed version of huug's latest patch with added comment

What still needs to be done to get this workaround patch into poppler?
I added the comment before the function that Chris Wilson suggested.

We are working on merge current pixman patches into poppler, because it seems to be a better implementation and more correct fix rather than workaround. Your work is very appreciated anyway, and I'll use your patch if pixman approach doesn't work. 

Thanks. 

(In reply to comment #47)
> We are working on merge current pixman patches into poppler, because it seems
> to be a better implementation and more correct fix rather than workaround.

Is the target 0.12.3 or 0.14.0?

0.14, the next major release. 

(In reply to comment #49)
> 0.14, the next major release. 

Wouldn't it make sense to apply this patch (only) to 0.12 branch, then? 

*** Bug 25596 has been marked as a duplicate of this bug. ***

*** Bug 26026 has been marked as a duplicate of this bug. ***

when compiling latest version of the patch, I see two warnings :

CairoOutputDev.cc:2021: warning: 'buffer' may be used uninitialized in this function
CairoOutputDev.cc:2022: warning: 'stride' may be used uninitialized in this function

as suggested by Vaclav, I might be worth adding this patch in 0.12 branch. I'll push it in Mandriva cooker package, for more tests for now.

I just tried this patch against the Karmic poppler 0.12.0 (via apt-get source), and it does not fix the problem.

BTW this patch does not apply cleanly, it claims it is malformed on line 71 (?!?!)

Also, the original PDF in the URL? field above, which is supposed to show this problem has vanished.  Can someone upload a PDF which is supposed to have the problem fixed by this patch?

Here's a rebased patch I used (against 0.12.3),
http://cvs.fedoraproject.org/viewvc/devel/poppler/poppler-0.12.3-fdo%235589.patch

The fedora patch also does not fix this problem.  Could the differences between 0.12.0 and 0.12.3 be significant enough to cause this patch not to work?

(In reply to comment #57)
> Could the differences between
> 0.12.0 and 0.12.3 be significant enough to cause this patch not to work?

No, it works with 0.12.3. Unfortunately, I can't find any previously problematic document that I would be free to upload. Stupid question, but did you patch the right package, if you did it that way? E.g. on Gentoo, I had to add the patch to "poppler-glib", not "poppler".

Reporter in downstream report,
https://bugzilla.redhat.com/548826

Has screenshots showing improvments with the patch.

(fyi, poppler-glib comes from the same poppler tarball)

On ubuntu, the 'poppler' source package generates all the poppler libraries, including poppler-glib.  I just checked with ldd also, and evince is loading my newly-compiled libraries.

For me this bug crops up with older papers from most journals.  Someone above mentioned JSTOR and I do see papers from there having this bug.  Of course I'm not free to upload them either... :(  For those with access to journals, this is the paper I was looking at: http://rmp.aps.org/abstract/RMP/v34/i4/p694_1

> Can someone upload a PDF which is supposed to have the problem
> fixed by this patch?

Bug 25596 has an attached PDF.

Created attachment 32750 [details] [review]
Patch based on pixman patches

I finally have a patch based on the patches that Jeff proposed for pixman. Could you confirm this patch works for your test cases, please?

I confirm latest "pixman" patch fixes rendering with file from bug #26026

(In reply to comment #62)
> Created an attachment (id=32750) [details]
> Patch based on pixman patches
> 
> I finally have a patch based on the patches that Jeff proposed for pixman.
> Could you confirm this patch works for your test cases, please?
> 

I tested a few files and it seems to work fine. However rendering speed is reduced considerably compared to my patched setup. :-(
Did you test printing? I don't have one.

(In reply to comment #64)
> (In reply to comment #62)
> > Created an attachment (id=32750) [details] [details]
> > Patch based on pixman patches
> > 
> > I finally have a patch based on the patches that Jeff proposed for pixman.
> > Could you confirm this patch works for your test cases, please?
> > 
> 
> I tested a few files and it seems to work fine. However rendering speed is
> reduced considerably compared to my patched setup. :-(

Yes, but theoretically it gives better results. There are three algorithms in Jeff's patches: integer-box, box and lanczos. The first one is the fastest but it gives the worst quality, lanczos provides the highest quality but it's slower than the others. Jeff suggested me to use box since it provides quite good quality with a reasonable performance. 

> Did you test printing? I don't have one.
> 

The problem is only in the image backend, so we don't use it when printing. 

I've just applied the patch to git master, so that it'll be included in the next unstable release. If performance becomes a problem, we can consider using integer-box instead of box and even trying to improve the way we use the rescaler from poppler. 

*** Bug 16173 has been marked as a duplicate of this bug. ***

*** Bug 24701 has been marked as a duplicate of this bug. ***

This bug appears to still be present in the xournal and epdfview programs.  It has been suggested that it is because they use poppler_page_render_to_pixbuf rather than the Cairo backend (which is where this bug was fixed).

Can a poppler expert comment on this?

See: http://sourceforge.net/mailarchive/message.php?msg_name=20100312205839.GA13311@mcelrath.org and follow-up messages for some details.

(In reply to comment #69)
> This bug appears to still be present in the xournal and epdfview programs.  It
> has been suggested that it is because they use poppler_page_render_to_pixbuf
> rather than the Cairo backend (which is where this bug was fixed).

Not exactly, because when rendering to a pixbuf cairo backend is used too. 

> Can a poppler expert comment on this?

I've just found the problem. It's a bug in the glib frontend. The problem is that image prescaling is disabled when rendering for printing, and printing is initialized to True in CairoOutputDev. We are calling CairoOutputDev::setPrinting() in poppler_page_render but it's missing in poppler_page_render_to_pixbuf(). Now, that glib frontend depends on cairo, I'm going to change render_to_pixbuf to call poppler_page_render directly and the problem will be fixed. 

> See:
> http://sourceforge.net/mailarchive/message.php?msg_name=20100312205839.GA13311@mcelrath.org
> and follow-up messages for some details.

Please, note that poppler_page_render_to_pixbuf() and the other GDK functions are going to be deprecated in a near future, since we are just rendering to a cairo surface and then converting to a gdk pixbuf. 

Thanks for reporting.