Bug 5589 - image scaling with cairo is poor quality
Summary: image scaling with cairo is poor quality
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: cairo backend (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: high major
Assignee: poppler-bugs
QA Contact:
URL: http://dmi.uib.es/~loren/docencia/pro...
Whiteboard:
Keywords:
: 4420 8844 8982 9149 10302 11421 12270 16173 24701 25244 25596 26026 (view as bug list)
Depends on: 11142
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-13 19:20 UTC by Pablo Rodríguez
Modified: 2010-04-12 08:13 UTC (History)
21 users (show)

See Also:
i915 platform:
i915 features:


Attachments
pre-scale images instead of having cairo scale them. (5.34 KB, patch)
2006-12-09 11:19 UTC, Jeff Muizelaar
Details | Splinter Review
pre-scale images instead of having cairo scale them ver 2 (5.64 KB, patch)
2006-12-09 18:25 UTC, Jeff Muizelaar
Details | Splinter Review
Latest patch. (8.50 KB, patch)
2007-03-24 13:07 UTC, Jeff Muizelaar
Details | Splinter Review
bitmap font text that poppler doesn't scale (37.66 KB, image/png)
2007-07-21 02:22 UTC, Pablo Rodríguez
Details
bitmap font right scaled by poppler-0.5.4 (40.87 KB, image/png)
2007-07-21 02:36 UTC, Pablo Rodríguez
Details
jstor pdf. Drag the background to a new evince and it looks fine. (206.75 KB, image/png)
2009-11-12 03:46 UTC, Rune Schjellerup Philosof
Details
prescale patch (15.65 KB, patch)
2009-11-16 13:19 UTC, Huug
Details | Splinter Review
updated patch (15.68 KB, application/x-gzip)
2009-11-16 15:46 UTC, Huug
Details
patch with printer exception (4.51 KB, patch)
2009-12-02 10:10 UTC, Huug
Details | Splinter Review
fixed version of huug's latest patch (?) (4.32 KB, patch)
2009-12-02 13:12 UTC, Benjamin Mako Hill
Details | Splinter Review
fixed version of huug's latest patch with added comment (4.34 KB, patch)
2009-12-10 02:48 UTC, Rune Schjellerup Philosof
Details | Splinter Review
Patch based on pixman patches (16.46 KB, patch)
2010-01-21 04:11 UTC, Carlos Garcia Campos
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pablo Rodríguez 2006-01-13 19:20:07 UTC
lt-gtk-cairo-test needs more than 330 seconds at 100% processor use to open the
following file: http://dmi.uib.es/~loren/docencia/propostaproj2005.pdf.

Probably this slowness is due to the fact that the file only contains bitmapped
fonts. And gtk-cairo displays bitmaps aliased.

lt-gtk-splash-test and test-poppler-qt need less than a second to open the same
file (on the same computer) and display it antialiased.
Comment 1 Luca Ognibene 2006-03-06 03:52:19 UTC
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).
Comment 2 Pablo Rodríguez 2006-03-06 18:55:42 UTC
Thanks for the interesting news. I guess I'll wait for the release of the stable
version.
Comment 3 Pablo Rodríguez 2006-03-16 23:20:18 UTC
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.)
Comment 4 Jeff Muizelaar 2006-03-23 16:18:28 UTC
The difference that matters here is probably the version of xserver you are using.
Comment 5 Pablo Rodríguez 2006-03-24 07:54:54 UTC
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.
Comment 6 Pablo Rodríguez 2006-04-29 21:27:53 UTC
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?
Comment 7 Pablo Rodríguez 2006-05-25 21:46:05 UTC
I'm afraid that poppler 0.5.2 and cairo 1.1.2 are not able to display the bitmap
font.
Comment 8 Jeff Muizelaar 2006-05-30 04:38:50 UTC
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.
Comment 9 Jeff Muizelaar 2006-12-09 11:19:52 UTC
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.
Comment 10 Jeff Muizelaar 2006-12-09 18:25:58 UTC
Created attachment 8048 [details] [review]
pre-scale images instead of having cairo scale them ver 2

A slightly better version.
Comment 11 Jeff Muizelaar 2006-12-11 21:26:47 UTC
*** Bug 9149 has been marked as a duplicate of this bug. ***
Comment 12 Jeff Muizelaar 2006-12-11 21:29:18 UTC
*** Bug 8982 has been marked as a duplicate of this bug. ***
Comment 13 Jeff Muizelaar 2006-12-11 21:30:55 UTC
*** Bug 8844 has been marked as a duplicate of this bug. ***
Comment 14 Jeff Muizelaar 2006-12-11 21:39:30 UTC
*** Bug 4420 has been marked as a duplicate of this bug. ***
Comment 15 Sebastien Bacher 2007-03-20 03:45:26 UTC
*** Bug 10302 has been marked as a duplicate of this bug. ***
Comment 16 Jeff Muizelaar 2007-03-24 13:07:40 UTC
Created attachment 9278 [details] [review]
Latest patch.

Really ugly code but it should work. It should even do a better job than splash.
Comment 17 Carlos Garcia Campos 2007-07-15 11:50:07 UTC
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. 
Comment 18 Jeff Muizelaar 2007-07-16 20:48:32 UTC
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.
Comment 19 Pablo Rodríguez 2007-07-21 02:22:52 UTC
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.
Comment 20 Pablo Rodríguez 2007-07-21 02:36:58 UTC
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.
Comment 21 Carlos Garcia Campos 2007-07-31 05:20:51 UTC
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. 
Comment 22 Jeff Muizelaar 2007-07-31 07:50:39 UTC
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. 
Comment 23 Carlos Garcia Campos 2007-08-22 07:37:02 UTC
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
Comment 24 Jeff Muizelaar 2007-08-22 12:32:10 UTC
(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
Comment 25 Chris Wilson 2007-09-03 08:34:54 UTC
*** Bug 12270 has been marked as a duplicate of this bug. ***
Comment 26 Jeff Muizelaar 2008-01-26 08:49:35 UTC
Bug #9860 is related to this bug.
Comment 27 Jeff Muizelaar 2008-02-21 20:08:14 UTC
*** Bug 11421 has been marked as a duplicate of this bug. ***
Comment 28 sascha heid 2009-08-02 14:45:56 UTC
I was unable to apply the patch to poppler-0.10.7, may i ask what the current situation regarding this bug is?

Comment 29 Rune Schjellerup Philosof 2009-11-12 03:46:05 UTC
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?
Comment 30 Carlos Garcia Campos 2009-11-12 03:51:08 UTC
(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. 
Comment 31 Rune Schjellerup Philosof 2009-11-12 04:21:38 UTC
(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).
Comment 32 Carlos Garcia Campos 2009-11-12 08:17:17 UTC
(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
Comment 33 Huug 2009-11-16 13:19:12 UTC
Created attachment 31242 [details] [review]
prescale patch

Here is my patch that adds prescale capability
Comment 34 Huug 2009-11-16 15:46:51 UTC
Created attachment 31245 [details]
updated patch
Comment 35 sascha heid 2009-11-16 18:21:19 UTC
Thank you very much, your patch seems to work great.
Comment 36 Chris Wilson 2009-11-17 02:26:46 UTC
(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.

Comment 37 Huug 2009-11-17 13:24:34 UTC
(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.
Comment 38 Carlos Garcia Campos 2009-11-25 03:11:02 UTC
*** Bug 25244 has been marked as a duplicate of this bug. ***
Comment 39 Vaclav Slavik 2009-12-02 03:08:42 UTC
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.
Comment 40 Huug 2009-12-02 10:10:12 UTC
Created attachment 31667 [details] [review]
patch with printer exception

Added printing exception. Now only prescaling when not printing.
Comment 41 Benjamin Mako Hill 2009-12-02 13:11:20 UTC
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?
Comment 42 Benjamin Mako Hill 2009-12-02 13:12:22 UTC
Created attachment 31683 [details] [review]
fixed version of huug's latest patch (?)
Comment 43 Vaclav Slavik 2009-12-02 23:59:53 UTC
(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.
Comment 44 Huug 2009-12-03 12:14:48 UTC
(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.
Comment 45 Vaclav Slavik 2009-12-03 12:18:28 UTC
(In reply to comment #44)
> Does the patch work?

Yes, thanks.
Comment 46 Rune Schjellerup Philosof 2009-12-10 02:48:53 UTC
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.
Comment 47 Carlos Garcia Campos 2009-12-10 02:58:14 UTC
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. 
Comment 48 Rune Schjellerup Philosof 2009-12-10 03:10:48 UTC
(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?
Comment 49 Carlos Garcia Campos 2009-12-10 03:21:50 UTC
0.14, the next major release. 
Comment 50 Vaclav Slavik 2009-12-10 04:05:09 UTC
(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? 
Comment 51 Carlos Garcia Campos 2009-12-12 01:37:00 UTC
*** Bug 25596 has been marked as a duplicate of this bug. ***
Comment 52 Frederic Crozat 2010-01-15 06:32:34 UTC
*** Bug 26026 has been marked as a duplicate of this bug. ***
Comment 53 Frederic Crozat 2010-01-15 06:39:19 UTC
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.
Comment 54 Bob McElrath 2010-01-20 05:53:14 UTC
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 (?!?!)
Comment 55 Bob McElrath 2010-01-20 05:54:57 UTC
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?
Comment 56 Rex Dieter 2010-01-20 06:06:49 UTC
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
Comment 57 Bob McElrath 2010-01-20 06:37:27 UTC
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?
Comment 58 Vaclav Slavik 2010-01-20 06:43:16 UTC
(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".
Comment 59 Rex Dieter 2010-01-20 06:45:00 UTC
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)
Comment 60 Bob McElrath 2010-01-20 06:55:24 UTC
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
Comment 61 Mikhail Glushenkov 2010-01-20 07:53:31 UTC
> Can someone upload a PDF which is supposed to have the problem
> fixed by this patch?

Bug 25596 has an attached PDF.
Comment 62 Carlos Garcia Campos 2010-01-21 04:11:00 UTC
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?
Comment 63 Frederic Crozat 2010-01-22 07:28:14 UTC
I confirm latest "pixman" patch fixes rendering with file from bug #26026
Comment 64 Huug 2010-01-23 02:20:26 UTC
(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.
Comment 65 Carlos Garcia Campos 2010-01-23 02:37:25 UTC
(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. 
Comment 66 Carlos Garcia Campos 2010-01-24 04:18:05 UTC
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. 
Comment 67 Carlos Garcia Campos 2010-01-24 10:30:32 UTC
*** Bug 16173 has been marked as a duplicate of this bug. ***
Comment 68 Carlos Garcia Campos 2010-01-26 10:39:50 UTC
*** Bug 24701 has been marked as a duplicate of this bug. ***
Comment 69 Bob McElrath 2010-04-11 14:06:32 UTC
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.
Comment 70 Carlos Garcia Campos 2010-04-12 08:13:46 UTC
(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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.