422179 - Implement Bug 381661 (bilinear filtering of upscaled images) for Linux

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Norbert]

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]

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] (needinfo me, slow to respond)]

(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]

Where in the code should this be implemented?  What is needed from X?

Comment 4

[Federico Mena-Quintero]

To wit: nothing should be needed from X; GTK+ can already do beautiful scaling for you :)

Comment 6

[Ralph Moenchmeyer]

(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]

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]

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] (needinfo me, slow to respond)]

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č]

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č]

I'm sorry, I meant "...what would happen if I *commented out* (not uncommented) the two special cases...".

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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 15

[Francis Robichaud]

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 17

[Tom Jaeger]

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]

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]

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 21

[Dotan Cohen]

Attachment: 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]

Attachment: 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]

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]

Attachment: fixed indentation

Comment 25

[Jeff Muizelaar [:jrmuizel]]

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]

Thanks for your comments.

[e-mail] 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]

Attachment: comment updates

Comment 28

[Jeff Muizelaar [:jrmuizel]]

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]

Attachment: cosmetic changes

Thanks.  I've made the two requested changes.

Comment 30

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 388713 [details] [diff] [review]
cosmetic changes

Looks good

Comment 31

[Hrvoje Niksic]

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 33

[Dão Gottwald [:dao]]

Is this ready to land?

Comment 34

[Dão Gottwald [:dao]]

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]

Attachment: 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]]

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]]

(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] (needinfo me, slow to respond)]

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]]

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]

(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 42

[Carter McKendry]

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]

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]

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]

Chrome is the only linux browser that I have used that does not have this problem...

Comment 46

[Janboe Ye]

Attachment: 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]

Attachment: 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]

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]

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]

Somebody please review the patch and apply it.

Comment 51

[Eric Rannaud]

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]

@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]

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]

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]

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 57

[Bob G]

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]

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]

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]]

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]

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]

(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]

(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

[Bob G]

but when will this patch be applied to the official firefox branch??

Comment 65

[bugreply.20.herbalizer]

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)]

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]

(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 69

[Miro Hadzhiev]

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]

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]

(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]

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]

Attachment: Screenshot of a valid web page, with an artefact on FF under Linux

Comment 74

[Graham Perry]

Is there anything stopping this patch being applied?

Comment 75

[Karl Tomlinson (:karlt)]

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 :takenspc]

(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)]

(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]

>> ... 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]

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)]

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]

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)]

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)]

Oh, sorry, it's Tom's patch.

Comment 84

[Takeshi Kurosawa :takenspc]

Attachment: Updated version of Tom's patch

Updated version of Tom's last patch with some #ifdefs and minor modifications.

Comment 85

[Takeshi Kurosawa :takenspc]

Attachment: 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]]

Is this ready to land?

Comment 87

[lovyagin]

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]

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)]

(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]

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

[(no longer active)]

Attachment: 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 92

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89816be9e164
http://hg.mozilla.org/mozilla-central/rev/c08cbc8f4fa6
http://hg.mozilla.org/mozilla-central/rev/49291ab908cc

Comment 93

[ganlu]

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