Last Comment Bug 422179 - Implement Bug 381661 (bilinear filtering of upscaled images) for Linux
: Implement Bug 381661 (bilinear filtering of upscaled images) for Linux
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All Linux
: -- normal with 59 votes (vote)
: mozilla2.0b7
Assigned To: Takeshi Kurosawa
:
:
Mentors:
: 431990 455495 459508 465494 500191 509292 521866 557438 571853 (view as bug list)
Depends on:
Blocks: 426930 593569
  Show dependency treegraph
 
Reported: 2008-03-11 11:23 PDT by Norbert
Modified: 2014-04-26 02:24 PDT (History)
80 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
?


Attachments
Screenshots of Firefox zoom and Compiz zoom (184.04 KB, image/png)
2009-06-24 09:27 PDT, Dotan Cohen
no flags Details
fix (2.07 KB, patch)
2009-06-24 15:50 PDT, Tom Jaeger
no flags Details | Diff | Splinter Review
fixed indentation (2.12 KB, patch)
2009-06-30 08:39 PDT, Tom Jaeger
jmuizelaar: review-
Details | Diff | Splinter Review
comment updates (2.22 KB, patch)
2009-07-15 05:43 PDT, Tom Jaeger
jmuizelaar: review-
Details | Diff | Splinter Review
cosmetic changes (2.22 KB, patch)
2009-07-15 08:50 PDT, Tom Jaeger
jmuizelaar: review+
Details | Diff | Splinter Review
updated patch (2.11 KB, patch)
2009-08-11 19:13 PDT, Tom Jaeger
no flags Details | Diff | Splinter Review
change to using extend_pad against on cairo 1.9.2 (518 bytes, patch)
2009-12-24 01:56 PST, Janboe Ye
no flags Details | Diff | Splinter Review
screenshot of chrome, firefox w/wo extend_pad (284.71 KB, image/png)
2009-12-24 02:14 PST, Janboe Ye
no flags Details
Screenshot of a valid web page, with an artefact on FF under Linux (23.54 KB, image/png)
2010-08-13 09:21 PDT, Jean-Noel Rivasseau
no flags Details
Updated version of Tom's patch (5.24 KB, patch)
2010-08-25 22:30 PDT, Takeshi Kurosawa
jmuizelaar: review+
Details | Diff | Splinter Review
honor cairo version in tree (553 bytes, patch)
2010-08-25 22:50 PDT, Takeshi Kurosawa
jmuizelaar: review+
Details | Diff | Splinter Review
Part 3: disable a failing reftest on linux (1.53 KB, patch)
2010-09-09 12:38 PDT, :Ehsan Akhgari
jmuizelaar: review+
Details | Diff | Splinter Review

Description Norbert 2008-03-11 11:23:32 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4

Implement Bug 381661 (image filtering) for Linux

Reproducible: Always

Steps to Reproduce:
.
Actual Results:  
Images in zoomed pages looks ugly.

Expected Results:  
Images in zoomed pages look like in Firefox Windows build.
Comment 1 Prash 2008-04-01 05:56:46 PDT
Any chance of this getting fixed by FF3 final? At least as an about:config setting? Us Linux users are feeling a little left out despite all the talk of equality across platforms.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2008-04-01 09:54:25 PDT
(In reply to comment #1)
> Any chance of this getting fixed by FF3 final? At least as an about:config
> setting? Us Linux users are feeling a little left out despite all the talk of
> equality across platforms.

Talk to your distro provider then; the lack of the needed functionality in X is beyond our control.
Comment 3 Federico Mena-Quintero 2008-04-25 11:45:32 PDT
Where in the code should this be implemented?  What is needed from X?
Comment 4 Federico Mena-Quintero 2008-04-25 11:47:18 PDT
To wit: nothing should be needed from X; GTK+ can already do beautiful scaling for you :)
Comment 5 Dão Gottwald [:dao] 2008-05-05 05:48:30 PDT
*** Bug 431990 has been marked as a duplicate of this bug. ***
Comment 6 Ralph Moenchmeyer 2008-06-18 06:18:00 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Any chance of this getting fixed by FF3 final? At least as an about:config
> > setting? Us Linux users are feeling a little left out despite all the talk of
> > equality across platforms.
> 
> Talk to your distro provider then; the lack of the needed functionality in X is
> beyond our control.
> 

Does this mean that X could provide the necessary functionality but that it is the fault of the distro provider that the distro's X server implementation lacks the required properties? 

Or is a basic feature missing in the xorg X-server itself? 
 


Comment 7 Federico Mena-Quintero 2008-06-26 09:08:49 PDT
I don't really know where in the code Mozilla scales the images, but if it needs to do that on Linux, it seems that it could use the gdk_pixbuf_scale*() family of functions very easily.  Use GDK_INTERP_BILINEAR which is very nice and fast.
Comment 8 Federico Mena-Quintero 2008-06-26 09:11:27 PDT
The documentation is here:
http://library.gnome.org/devel/gdk-pixbuf/stable/gdk-pixbuf-scaling.html

I'll be happy to assist anyone who wants to implement this.
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2008-06-30 10:49:09 PDT
Mozilla doesn't scale the images at all -- we hand off the image and a transform to RENDER, which needs to do the scaling.  The issue is that the version of pixman that's currently shipped with all X servers doesn't implement EXTEND_PAD, which we need to get correct behaviour at image edges.
Comment 10 Marin Krkač 2008-07-05 05:07:22 PDT
I was slightly annoyed by this not working so I tried to see what would happen if I uncommented the two special cases for bug 324698 in gfx/src/thebes/nsThebesImage.cpp (at lines ~573 and ~785). This means that the default case, with EXTEND_PAD, is used in the switch statement.

I compiled Firefox (the official Mozilla version) with --enable-system-cairo on Ubuntu 8.04, and it worked. I've been using it like this since RC1 and I haven't noticed any problems. I also tried zooming in on the IE 8 Beta page and it looks the same as the image on your blog.

If I'm not missing something obvious here, could you please create an (temporary?) official patch that could be used to enable this in the packages of the distributions for which it works?

Packages on my system:
xserver-xorg-core
2:1.4.1~git20080131-1ubuntu9.2

libpixman-1-0
0.10.0-0ubuntu1

libcairo2
1.6.0-0ubuntu2
Comment 11 Marin Krkač 2008-07-05 05:15:57 PDT
I'm sorry, I meant "...what would happen if I *commented out* (not uncommented) the two special cases...".
Comment 12 Vladimir Vukicevic [:vlad] [:vladv] 2008-07-05 17:48:10 PDT
It will "work", it will just be "slow".  If you set EXTEND_PAD, then all images will be scaled in software, which will require a read from the x server and a write back for /every/ image paint.  If you don't set EXTEND_PAD, then testcases such as https://bugzilla.mozilla.org/attachment.cgi?id=209616 and see that it doesn't work will fail.
Comment 13 rom 2008-09-16 06:20:02 PDT
*** Bug 455495 has been marked as a duplicate of this bug. ***
Comment 14 Robert Longson 2008-10-11 08:33:22 PDT
*** Bug 459508 has been marked as a duplicate of this bug. ***
Comment 15 Francis Robichaud 2008-10-15 22:08:32 PDT
Bilinear interpolation has been implemented for image upscaling and downscaling with bug https://bugzilla.mozilla.org/show_bug.cgi?id=395260 for Linux.
This should be a child of 395260. Basically, it relies on an about:config parameter where the user can set a value between 0 and 100 which defines when bilinear interpolation will be used for scaled images.
Comment 16 Jo Hermans 2008-11-18 04:00:35 PST
*** Bug 465494 has been marked as a duplicate of this bug. ***
Comment 17 Tom Jaeger 2009-01-16 23:12:15 PST
Vladimir, could you explain what needs to happen in X/pixman and/or firefox for this bug to be fixed?  Thanks.
Comment 18 Tom Jaeger 2009-01-18 13:23:51 PST
My testing suggests that EXTEND_PAD is supported by XRender now.  The following seems to fix the issue for me:

* Change pat->SetFilter(0) to pat->SetExtend(gfxPattern::EXTEND_PAD) to use EXTEND_PAD for upscaling
* patch cairo to not claim that REPEAT_PAD is unsupported by XRender.
Comment 19 Tom Jaeger 2009-06-09 20:21:52 PDT
Vladimir: The drivers are now fixed, and cairo will use XRender for EXTEND_PAD as of commit a1d0a06b[1], provided that a 1.7 X server is running, so it looks like this can be finally fixed in firefox.

[1] http://cgit.freedesktop.org/cairo/commit/?id=a1d0a06b6275cac3974be84919993e187394fe43
Comment 20 Robert Longson 2009-06-24 09:01:09 PDT
*** Bug 500191 has been marked as a duplicate of this bug. ***
Comment 21 Dotan Cohen 2009-06-24 09:27:41 PDT
Created attachment 384896 [details]
Screenshots of Firefox zoom and Compiz zoom

Here is a screenshot of the wikipedia logo with 200% zoom in Firefox, and to compare the same logo with the same zoom as performed by Compiz.
Comment 22 Tom Jaeger 2009-06-24 15:50:06 PDT
Created attachment 384996 [details] [diff] [review]
fix

Vladimir:  Attaching a patch that should finally fix this bug.  The patch enables EXTEND_PAD if (1) the cairo version used is at least 1.9.2 and (2) the X server is at least a 1.7 pre-release (i.e. current git master).  This ensures that cairo hands off the EXTEND_PAD operation to XRender.
Comment 23 Alexander Sack 2009-06-30 08:24:28 PDT
Comment on attachment 384996 [details] [diff] [review]
fix

>             if (!isDownscale) {
>-                pattern->SetFilter(0);
>+                PRBool fastExtendPad = false;
>+		if (currentTarget->GetType() == gfxASurface::SurfaceTypeXlib &&
>+                        cairo_version() >= CAIRO_VERSION_ENCODE(1,9,2)) {

please fix indentation and request review.
Comment 24 Tom Jaeger 2009-06-30 08:39:30 PDT
Created attachment 386032 [details] [diff] [review]
fixed indentation
Comment 25 Jeff Muizelaar [:jrmuizel] 2009-07-14 09:12:39 PDT
Comment on attachment 386032 [details] [diff] [review]
fixed indentation

>@@ -685,11 +688,33 @@
>+            //
>+            // Update 6/24/09: The underlying X server/driver bugs are now
>+            // fixed, and cairo uses the fast XRender code-path as of 1.9.2
>+            // (commit commit a1d0a06b6275cac3974be84919993e187394fe43) --

The word commit is repeated.

>+            // but only if running on a 1.7 X server.
>+            // So we enable EXTEND_PAD provided that we're running a recent
>+            // enough cairo version (obviously, this is only relevant if
>+            // --enable-system-cairo is used) AND running on a recent
>+            // enough X server.  This should finally bring linux up to par
>+            // with other systems.
>             PRBool isDownscale =
>               deviceToImage.xx >= 1.0 && deviceToImage.yy >= 1.0 &&
>               deviceToImage.xy == 0.0 && deviceToImage.yx == 0.0;
>             if (!isDownscale) {
>-                pattern->SetFilter(0);
>+                PRBool fastExtendPad = false;
>+                if (currentTarget->GetType() == gfxASurface::SurfaceTypeXlib &&
>+                        cairo_version() >= CAIRO_VERSION_ENCODE(1,9,2)) {
>+                    gfxXlibSurface *xlibSurface = static_cast<gfxXlibSurface *>((gfxASurface *)currentTarget);

Why cast to (gfxASurface *) first?

>+                    Display *dpy = xlibSurface->XDisplay();
>+                    if (VendorRelease (dpy) < 60700000 && VendorRelease (dpy) >= 10699000)
>+                        fastExtendPad = true;

I don't understand this VendorRelease check. I would expect something more like:
  VendorRelease(dpy) >= 10700000
Please add a comment explaining why you don't use the more intuitive check.

>+                }
>+                if (fastExtendPad) {
>+                    pattern->SetExtend(gfxPattern::EXTEND_PAD);
>+                } else {
>+                    pattern->SetFilter(0);
>+                }
Comment 26 Tom Jaeger 2009-07-15 05:31:52 PDT
Thanks for your comments.

bugzilla-daemon@mozilla.org wrote:
>> @@ -685,11 +688,33 @@
>> +            //
>> +            // Update 6/24/09: The underlying X server/driver bugs are now
>> +            // fixed, and cairo uses the fast XRender code-path as of 1.9.2
>> +            // (commit commit a1d0a06b6275cac3974be84919993e187394fe43) --
> 
> The word commit is repeated.
Sorry, fixed.
 
>> +            // but only if running on a 1.7 X server.
>> +            // So we enable EXTEND_PAD provided that we're running a recent
>> +            // enough cairo version (obviously, this is only relevant if
>> +            // --enable-system-cairo is used) AND running on a recent
>> +            // enough X server.  This should finally bring linux up to par
>> +            // with other systems.
>>             PRBool isDownscale =
>>               deviceToImage.xx >= 1.0 && deviceToImage.yy >= 1.0 &&
>>               deviceToImage.xy == 0.0 && deviceToImage.yx == 0.0;
>>             if (!isDownscale) {
>> -                pattern->SetFilter(0);
>> +                PRBool fastExtendPad = false;
>> +                if (currentTarget->GetType() == gfxASurface::SurfaceTypeXlib &&
>> +                        cairo_version() >= CAIRO_VERSION_ENCODE(1,9,2)) {
>> +                    gfxXlibSurface *xlibSurface = static_cast<gfxXlibSurface *>((gfxASurface *)currentTarget);
> 
> Why cast to (gfxASurface *) first?
It's not really a cast. currentTarget is of type nsRefPtr<gfxASurface>, which is converted into a (gfxASurface *) in order to be able to use the static cast.  There might be a better way, but I couldn't find any nsRefPtr cast operators in the mozilla source.

> 
>> +                    Display *dpy = xlibSurface->XDisplay();
>> +                    if (VendorRelease (dpy) < 60700000 && VendorRelease (dpy) >= 10699000)
>> +                        fastExtendPad = true;
> 
> I don't understand this VendorRelease check. I would expect something more
> like:
>   VendorRelease(dpy) >= 10700000
> Please add a comment explaining why you don't use the more intuitive check.
We need to use the exact same check that cairo is using.  The reason for the VendorRelease check is explained at length in src/cairo-xlib-display.c, basically there used to be a different numbering scheme in older X servers.  We use 10699000 instead of 10700000 in order to be able to test the code with pre-release servers.  

>> +                }
>> +                if (fastExtendPad) {
>> +                    pattern->SetExtend(gfxPattern::EXTEND_PAD);
>> +                } else {
>> +                    pattern->SetFilter(0);
>> +                }
>
Comment 27 Tom Jaeger 2009-07-15 05:43:48 PDT
Created attachment 388683 [details] [diff] [review]
comment updates
Comment 28 Jeff Muizelaar [:jrmuizel] 2009-07-15 08:04:05 PDT
Comment on attachment 388683 [details] [diff] [review]
comment updates

>diff -r b91c9ee69e6e gfx/src/thebes/nsThebesImage.cpp
>--- a/gfx/src/thebes/nsThebesImage.cpp	Wed Jul 15 01:39:35 2009 -0400
>+++ b/gfx/src/thebes/nsThebesImage.cpp	Wed Jul 15 08:43:01 2009 -0400
>@@ -46,6 +46,9 @@
> 
> #include "prenv.h"
> 
>+#include "cairo.h"
>+#include "gfxXlibSurface.h"
>+
> static PRBool gDisableOptimize = PR_FALSE;
> 
> #ifdef XP_WIN
>@@ -685,11 +688,34 @@
>             // have adjusted the pattern's matrix ... but the adjustment
>             // is only a translation so the scale factors in deviceToImage
>             // are still valid.
>+            //
>+            // Update 6/24/09: The underlying X server/driver bugs are now
>+            // fixed, and cairo uses the fast XRender code-path as of 1.9.2
>+            // (commit a1d0a06b6275cac3974be84919993e187394fe43) --
>+            // but only if running on a 1.7 X server.
>+            // So we enable EXTEND_PAD provided that we're running a recent
>+            // enough cairo version (obviously, this is only relevant if
>+            // --enable-system-cairo is used) AND running on a recent
>+            // enough X server.  This should finally bring linux up to par
>+            // with other systems.
>             PRBool isDownscale =
>               deviceToImage.xx >= 1.0 && deviceToImage.yy >= 1.0 &&
>               deviceToImage.xy == 0.0 && deviceToImage.yx == 0.0;
>             if (!isDownscale) {
>-                pattern->SetFilter(0);
>+                PRBool fastExtendPad = false;
>+                if (currentTarget->GetType() == gfxASurface::SurfaceTypeXlib &&
>+                        cairo_version() >= CAIRO_VERSION_ENCODE(1,9,2)) {
>+                    gfxXlibSurface *xlibSurface = static_cast<gfxXlibSurface *>((gfxASurface *)currentTarget);

Using currentTarget.get() is better than (gfxASurface *)currentTarget.

>+                    Display *dpy = xlibSurface->XDisplay();
>+		    // This is the exact condition for cairo to use XRender for EXTEND_PAD
>+                    if (VendorRelease (dpy) < 60700000 && VendorRelease (dpy) >= 10699000)

The whitespace for the comment "// This is .." uses tabs, the line below it does not.

>+                        fastExtendPad = true;
>+                }
>+                if (fastExtendPad) {
>+                    pattern->SetExtend(gfxPattern::EXTEND_PAD);
>+                } else {
>+                    pattern->SetFilter(0);
>+                }
>             }
>             break;
>         }
Comment 29 Tom Jaeger 2009-07-15 08:50:01 PDT
Created attachment 388713 [details] [diff] [review]
cosmetic changes

Thanks.  I've made the two requested changes.
Comment 30 Jeff Muizelaar [:jrmuizel] 2009-07-15 10:27:26 PDT
Comment on attachment 388713 [details] [diff] [review]
cosmetic changes

Looks good
Comment 31 Hrvoje Niksic 2009-07-23 06:59:39 PDT
I've been running with this patch[1] for a few days now without problems. The images are smoothly scaled.

[1]
I modified the patch to insist on Cairo 1.9.2 since ubuntu 9.04 ships with the cairo fix backported. I also had to change the X server version check because my X server didn't pass, but I've seen no artifacts in the images, so I assume the properietary nvidia driver correctly implements the needed functionality.
Comment 32 Jo Hermans 2009-08-09 03:19:25 PDT
*** Bug 509292 has been marked as a duplicate of this bug. ***
Comment 33 Dão Gottwald [:dao] 2009-08-09 03:22:52 PDT
Is this ready to land?
Comment 34 Dão Gottwald [:dao] 2009-08-11 17:42:07 PDT
Comment on attachment 388713 [details] [diff] [review]
cosmetic changes

This is outdated. That code lives in modules/libpr0n/src/imgFrame.cpp now.
Comment 35 Tom Jaeger 2009-08-11 19:13:18 PDT
Created attachment 393971 [details] [diff] [review]
updated patch

I ported the patch to what I believe is current trunk.  Untested, but should be fine, since the code in question hasn't actually changed.
Comment 36 Dão Gottwald [:dao] 2009-08-12 01:53:35 PDT
Comment on attachment 393971 [details] [diff] [review]
updated patch

No, this doesn't work.

error: gfxXlibSurface.h: No such file or directory
Comment 37 Dão Gottwald [:dao] 2009-08-12 02:47:30 PDT
(In reply to comment #36)
> (From update of attachment 393971 [details] [diff] [review])
> No, this doesn't work.
> 
> error: gfxXlibSurface.h: No such file or directory

This is on Mac and Windows.
Comment 38 Vladimir Vukicevic [:vlad] [:vladv] 2009-08-12 07:15:26 PDT
Also imgFrame shouldn't be reaching into cairo symbols; if we need the cairo version, we should expose it through thebes somehow.
Comment 39 Dão Gottwald [:dao] 2009-08-13 01:27:13 PDT
Isn't it unlikely that someone would run X Server 1.7 and an old cairo version? Can the cairo version check be left out?
Comment 40 Tom Jaeger 2009-08-18 11:18:03 PDT
(In reply to comment #38)
> Also imgFrame shouldn't be reaching into cairo symbols; if we need the cairo
> version, we should expose it through thebes somehow.

I'm not familiar enough with mozilla internals to produce a patch, but this shouldn't be too hard, should it?  I'd also be fine with skipping the cairo check if that helps move things along.
Comment 41 Jo Hermans 2009-10-12 14:15:25 PDT
*** Bug 521866 has been marked as a duplicate of this bug. ***
Comment 42 Carter McKendry 2009-10-24 20:41:38 PDT
If we can update the patch to alleviate the gfxXlibSurface.h error on Mac/Win, would the cairo version check be the only other blocking issue?

If the version check is not appropriate, it might make sense to just do a best-effort attempt here and be prepared to fail out gracefully if certain resources are not available.
Comment 43 Radosław Szkodziński 2009-11-30 02:19:06 PST
Please finally fix this. Nearest neighbor scaling is completely unsuitable for non-integer scaling factors, i.e. makes text in graphics totally unreadable.

This problem could be fixed by implementing error diffusion, but I think bilinear may be even better.
Comment 44 Dave Farrance 2009-12-03 08:57:02 PST
Yes, please do fix this.  This bug is horrible.  Recent flat-panel monitors tend to have a fine pixel-pitch, and web images frequently benefit from zooming, but not when they look this bad.  This pic shows how bad the zoom of Firefox Linux looks on my system compared to the same zoom with Firefox Windows on Wine:
http://anvil.pwp.blueyonder.co.uk/images/planes-ff-cmp.jpg
Comment 45 Kim 2009-12-03 09:31:01 PST
Chrome is the only linux browser that I have used that does not have this problem...
Comment 46 Janboe Ye 2009-12-24 01:56:00 PST
Created attachment 419100 [details] [diff] [review]
change to using extend_pad against on cairo 1.9.2

This patch could fix the issue on my laptop. But firefox is still not as good as chrome. Does anyone know why chrome has subpixel capability when the image include text?
Comment 47 Janboe Ye 2009-12-24 02:14:06 PST
Created attachment 419101 [details]
screenshot of chrome, firefox w/wo extend_pad

Chrome has better quality than firefox with cairo_extend_pad. Does anyone know why?
Thanks
Comment 48 Janboe Ye 2009-12-28 20:04:38 PST
ok. I got it. This is because chromium's skia engine supports resampling kernel. This could not depends on xrender. But pixman which is used by cairo current does not support resampling.

(In reply to comment #47)
> Created an attachment (id=419101) [details]
> screenshot of chrome, firefox w/wo extend_pad
> 
> Chrome has better quality than firefox with cairo_extend_pad. Does anyone know
> why?
> Thanks
Comment 49 charles 2010-01-13 07:36:00 PST
Zoom are less bad but there is square with big zoom. Are you modify or works on it pixman for resolve this problem of resampling ?

Before support new function of web browser please resolve this problem .

Thank you in advance.

Why Firefox for linux is less fast under windows ? That is the question!
Comment 50 Dikei 2010-01-21 22:17:35 PST
Somebody please review the patch and apply it.
Comment 51 Eric Rannaud 2010-01-29 17:14:20 PST
In #12, Vladimir implies that smooth software scaling is too slow. Is that claim still true in 2010? (And was it ever true?) Scaling can be done with a dozen instructions per pixel, at the very most. Considering the cost of decoding JPEG and PNG, I have a hard time believing this would be "slow". Or even noticeable on any desktop or laptop built in the past 5-8 years.

Chromium uses software scaling, and it is damn fast.

There is more and more discrepancy between webpage "sizes" these days. At one extreme, http://msdn.microsoft.com/en-us/library/default.aspx uses very small fonts (apparently using an absolute 'pt' size), and requires scaling on many setups, and good citizens, like google.com that simply use '100%' fonts.

Webpages that are scaled to make the text readable end up with horrid images with Firefox on Linux, this is a shame. Two years after the original report, the approach first considered is not getting anywhere.

Please reconsider this question.
Comment 52 Radosław Szkodziński 2010-02-02 12:01:55 PST
@47: Looks like chrome does bicubic scaling, while cairo seems to be doing simple bilinear. A patch against cairo to support bicubic would be nice to have.

Also, wasn't the problem with EXTEND_PAD originally lack of support or broken support in older versions of pixman?
Comment 53 Janboe Ye 2010-02-02 17:11:23 PST
hi, Radoslaw

Yes. I also think it'd better implement the resampling kernel in cairo or pixman. I saw there is a patch which implement the resampling kernel in pixman but it is rejected.

EXTEND_PAD is supported since cairo 1.92 which fixed some hw support issue.
Comment 54 Radosław Szkodziński 2010-02-03 05:08:15 PST
Actually, EXTEND_PAD seems to work fine in cairo 1.8.8 as well.
And fast at that. (although I am using a fresh version of pixman and well-accelerated drivers...)
Comment 55 ro.ggi 2010-03-11 11:36:49 PST
I use OpenSuse 11.2 and extendet_pad work good for me (I use standard packages with official updates). Please integrate it in source code finally.
Comment 56 Jo Hermans 2010-04-06 07:01:23 PDT
*** Bug 557438 has been marked as a duplicate of this bug. ***
Comment 57 rob 2010-04-10 05:12:28 PDT
I installed cairo-1.92 and Firefox-3.6.3pre (nightly build 64bit) but the images are still raw. Shouldn't this simply work now?
Comment 58 ro.ggi 2010-04-10 08:35:52 PDT
You need to edit the source code before you compile it. Change

if (!isDownscale) {
          pattern->SetFilter(gfxPattern::FILTER_FAST);

to

if (!isDownscale) {
          pattern->SetExtend(gfxPattern::EXTEND_PAD);

in ~/modules/libpr0n/src/imgFrame.cpp.
Comment 59 Eric Piel 2010-04-10 16:42:46 PDT
On a distro with xserver 1.7, cairo 1.9, and Firefox 3.6, this one-line change is sufficient to fix this bug without any drawback (like huge slowdown with some video drivers)? 

If so, this could be a huge step to apply to the distro-compiled versions of Firefox (where those requirements can be insured in the package itself).
Comment 60 Jeff Walden [:Waldo] (remove +bmo to email) 2010-04-10 17:34:54 PDT
At this point, maybe it makes sense to assume by default that X isn't broken, and then require vendors with broken X to opt out manually by reverting to current behavior.  Given the churn rate with Linux installs I think this may be better for most users who try to track more recent distro releases.  It would cut down on extra patches distros have to schlep around and that Mozilla has to review (or do any distros actually ship with patches to fix this?).  Just a thought...
Comment 61 ro.ggi 2010-04-11 05:53:39 PDT
With extendet_pad is scrolling not so smoothly as without. But it is implement in Windows version by default and nobody is worried. For me appearance is more important that performance. But in Chromium browser there is no such problem with scrolling and zoom quality in Windows and Linux. Firefox should do it like Chromium.
Comment 62 Tom Jaeger 2010-04-11 09:57:35 PDT
(In reply to comment #61)
> With extendet_pad is scrolling not so smoothly as without. But it is implement
> in Windows version by default and nobody is worried. For me appearance is more
> important that performance. But in Chromium browser there is no such problem
> with scrolling and zoom quality in Windows and Linux. Firefox should do it like
> Chromium.

This means that you're missing the cairo part of the fix.  There shouldn't be any noticeable performance difference between nearest-neighbor and bilinear interpolation on current hardware.
Comment 63 ro.ggi 2010-04-11 13:13:27 PDT
(In reply to comment #62)
> This means that you're missing the cairo part of the fix.  There shouldn't be
> any noticeable performance difference between nearest-neighbor and bilinear
> interpolation on current hardware.

Thanks for your tip. I have used diff from Janboe Ye with cairo 1.8.8. But your diff needs newer cairo and it is to difficult to install for me. I hope that my distribution will use newer cairo in the next version.
Comment 64 rob 2010-04-13 01:57:18 PDT
but when will this patch be applied to the official firefox branch??
Comment 65 bugreply.20.herbalizer 2010-05-26 01:28:50 PDT
Tom Jaeger's fix from launchpad is working perfectly on Ubuntu 10.04. - Is there any specific reason why this isn't being added to firefox?
Comment 66 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2010-06-02 06:00:17 PDT
Nominating for 1.9.3. Without this patch, Firefox scales images on Linux in a substantially uglier way than Opera or Chrome. I've been running with this patch for weeks now on Karmic with no ill effects.
Comment 67 ro.ggi 2010-06-03 13:35:51 PDT
(In reply to comment #65)
> Tom Jaeger's fix from launchpad is working perfectly on Ubuntu 10.04. - Is
> there any specific reason why this isn't being added to firefox?

There are some steps needed to get this patch work correctly with all current linux graphic-drivers. See here for more infos: https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/217908
Comment 68 Robert Longson 2010-06-14 05:57:36 PDT
*** Bug 571853 has been marked as a duplicate of this bug. ***
Comment 69 Miro Hadzhiev 2010-07-15 06:09:32 PDT
I've used Tom Jaeger's fix since Ubuntu 8.10 and it's working flawlessly.
I've Ati Mobility X1600 (OSS radeon driver in use).
And on my old computer nVDIA video card on board (and nVIDIA proprietary driver in use).

No problems at all.
Comment 70 Radosław Szkodziński 2010-07-26 04:13:58 PDT
Well, the Ubuntu bug is wrt Hardy Heron, which is really outdated by now, the problem has been fixed in i915 driver a long time ago.
Also, pixman software scaling performance has improved a lot since with addition of SSE and SSE2 scaling - some versions back.

Please do review and apply the patch finally and stop holding Linux Firefox in 2009. This bug is forcing high resolution screen and Linux users to rebuild patched Firefox.
Comment 71 Micah Gersten 2010-07-26 07:03:10 PDT
(In reply to comment #70)
> Well, the Ubuntu bug is wrt Hardy Heron, which is really outdated by now, the
> problem has been fixed in i915 driver a long time ago.
> Also, pixman software scaling performance has improved a lot since with
> addition of SSE and SSE2 scaling - some versions back.
> 
> Please do review and apply the patch finally and stop holding Linux Firefox in
> 2009. This bug is forcing high resolution screen and Linux users to rebuild
> patched Firefox.

Ubuntu just upgraded Hardy to Firefox 3.6.x and will hopefully finish out the 3 yr support on that branch.  But regardless, we're using in-source cairo now, so we shouldn't be holding anything back.
Comment 72 Jean-Noel Rivasseau 2010-08-13 08:59:12 PDT
I want to add that this problem also causes graphical artefacts on some sites even without manually zooming. See

http://playground.kameleoon.net/engine/image_scaling.html

Under Linux, Firefox 3.6.8 will display a red block next to the gradient because the image is not correctly scaled (look at the attached screenshot).

Under Chrome or with a patched Firefox using EXTEND_PAD, it looks as it should.

Therefore please really consider applying this patch for Firefox 4 final; it causes artefacts with perfectly valid HTML / CSS for Linux Firefox users.
Comment 73 Jean-Noel Rivasseau 2010-08-13 09:21:38 PDT
Created attachment 465688 [details]
Screenshot of a valid web page, with an artefact on FF under Linux
Comment 74 Graham Perry 2010-08-22 03:48:12 PDT
Is there anything stopping this patch being applied?
Comment 75 Karl Tomlinson (:karlt) 2010-08-22 04:12:09 PDT
I think the server version check is needed.
(I wouldn't bother with a cairo version check.)

Might as well use EXTEND_PAD for upscaling with server >= 1.7.
I was thinking something like

 gfxPattern::GraphicsExtend gfxContext::PreferredExtend().
Comment 76 Takeshi Kurosawa 2010-08-22 07:22:43 PDT
(In reply to comment #75)
> I think the server version check is needed.
> (I wouldn't bother with a cairo version check.)
> 
> Might as well use EXTEND_PAD for upscaling with server >= 1.7.
> I was thinking something like
> 
>  gfxPattern::GraphicsExtend gfxContext::PreferredExtend().


cairo seems already to do something like it,

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-xlib-display.c#339


... and fallbacks if the server is buggy, doesn't it?

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-xlib-surface.c#1591
Comment 77 Karl Tomlinson (:karlt) 2010-08-22 15:42:54 PDT
(In reply to comment #76)
> cairo seems already to do something like it,
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-xlib-display.c#339

Yes, we need to match those buggy_pad_reflect server version tests exactly.

> ... and fallbacks if the server is buggy, doesn't it?

Right.  But the fallback that cairo needs to do is quite disastrous to performance.  It will read back the image (and the destination, if there is an alpha channel in the image) from the server to software scale and then return to the server.

I assume it's possible to do some fancy FILTER_NEAREST only for the source-image-pixel-width outer border and FILTER_BILINEAR for the interior of the image,
but I'm not sure it's worth it just for older servers.

Simplest is probably to keep the old behavior for older servers, but fix up things with EXTEND_PAD/FILTER_BILINEAR for newer servers.
Comment 78 Eric Rannaud 2010-08-22 16:53:45 PDT
>> ... and fallbacks if the server is buggy, doesn't it?
> 
> Right.  But the fallback that cairo needs to do is quite disastrous to 
> performance.  It will read back the image (and the destination, if there is an
> alpha channel in the image) from the server to software scale and then return
> to the server.

This is FUD. Many apps do purely software scaling with reasonable performance. Google Chrome was, at least until recently (and probably still is). And it's still faster than current Firefox with in-server NEAREST.

Images are small wrt the available memory bandwidth. The majority of images on the web render to bitmaps that are way less than 1 or 2 MB (4 Bytes per pixel). Bandwidth is over 1GB/s on any computers bought since 2003.

Scaling images is cheap. The 90s are over.
Comment 79 Graham Perry 2010-08-23 09:53:53 PDT
So the quickest, simplest way forward is to implement the patch, rely on cairo for the fallback and accept the performance hit due software scaling on older servers, then in future (if the need should arise) create a fallback to the old behaviour?
Comment 80 Karl Tomlinson (:karlt) 2010-08-23 15:27:44 PDT
These older servers are not very old (~ Oct 2009),
and the performance hit would be more due to readback than software scaling.
(EXTEND_NONE is often done is software even server-side.)
(I don't know what Chrome does but maybe its doing its compositing client-side, so there's no need to read back.)
Comment 81 Hrvoje Niksic 2010-08-24 06:27:17 PDT
For testing the performance of cairo software scaling with readback, and possible comparison with chrome and hardware-enabled scaling in firefox, here is an example web page that embeds ten large images scaled to a smaller size:

http://www.flickr.com/groups/highres_favorites/discuss/72157611461261406/
Comment 82 Karl Tomlinson (:karlt) 2010-08-24 17:47:19 PDT
That's great thanks, Hrvoje.

Since bug 572680, the code has moved back to thebes (gfxDrawable.cpp), so your reviewed patch would also apply there (and we don't need anything like gfxContext::PreferredExtend()).
Comment 83 Karl Tomlinson (:karlt) 2010-08-24 17:48:33 PDT
Oh, sorry, it's Tom's patch.
Comment 84 Takeshi Kurosawa 2010-08-25 22:30:49 PDT
Created attachment 469348 [details] [diff] [review]
Updated version of Tom's patch

Updated version of Tom's last patch with some #ifdefs and minor modifications.
Comment 85 Takeshi Kurosawa 2010-08-25 22:50:49 PDT
Created attachment 469353 [details] [diff] [review]
honor cairo version in tree

This is needed for main patch.

The revision of cairo in tree is 12d521df8acc483b2daa844d4f05dc2fe2765ba6,
thus the version of cairo is 1.9.5.

* http://hg.mozilla.org/mozilla-central/rev/f236632a9747
* http://cgit.freedesktop.org/cairo/tree/cairo-version.h?id=12d521df8acc483b2daa844d4f05dc2fe2765ba6
Comment 86 Dão Gottwald [:dao] 2010-09-01 05:19:42 PDT
Is this ready to land?
Comment 87 lovyagin 2010-09-06 07:18:45 PDT
What is the problem to implement this https://launchpad.net/~firefox-smooth-scaling/+archive/ppa patch for example? I've just looked at firefox 4 beta and images still looks pixelated...
Comment 88 Radosław Szkodziński 2010-09-06 14:23:14 PDT
lovaygin:
Those patches are for older versions of firefox.
The patches included in this bug (last two) apply to latest firefox betas and work well, superseding those old ones.
They haven't been applied yet to the development tree.

Of course, you need either cairo 1.9.2 or to use the included one with firefox and run on Xorg 1.7 to see the improvement, which is great. Doesn't even compare with cairo 1.8.x and older EXTEND_PAD patches.

(Tested on ATI closed driver and Radeon open driver.)

Seems to me this is ready to land.
Comment 89 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-08 22:53:23 PDT
(In reply to comment #80)
> (I don't know what Chrome does but maybe its doing its compositing client-side,
> so there's no need to read back.)

That is indeed what Chrome does.
Comment 90 lovyagin 2010-09-09 00:40:00 PDT
Szkodziński:
Do you going to say that I need those new patches and new cairo to compile and run new version of firefox or it's better for 3.6.x too? Do I need to patch xulrunner too as I did with older patch from launchpad? 

So, no chance that firefox will runs well without pain of patching each update in a near future? (Well, I have NVidia and use closed driver.)
Comment 91 :Ehsan Akhgari 2010-09-09 12:38:47 PDT
Created attachment 473677 [details] [diff] [review]
Part 3: disable a failing reftest on linux

This reftest fails on Linux with this patch because now we do bilinear filtering on Linux like everywhere else.
Comment 93 ganlu 2010-09-10 04:43:48 PDT
I downloaded the "Mozilla/5.0 (X11; Linux i686; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre" nightly, it works greatly when zooming. 
My configuration:
Arch Linux current (xorg-server 1.8.1.902 xf86-video-ati 6.13.1 cairo 1.8.10)
ATI radeon V3200 on a old IBM T43P

Note You need to log in before you can comment on or make changes to this bug.