The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Graphics
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: Norbert, Assigned: Takeshi Kurosawa)

Tracking

Trunk
mozilla2.0b7
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, status1.9.2 ?)

Details

Attachments

(6 attachments, 6 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Implement Bug 381661 (image filtering) for Linux → Implement Bug 381661 (bilinear image filtering) for Linux
(Reporter)

Updated

9 years ago
Summary: Implement Bug 381661 (bilinear image filtering) for Linux → Implement Bug 381661 (bilinear filtering for upsacled images) for Linux
(Reporter)

Updated

9 years ago
Summary: Implement Bug 381661 (bilinear filtering for upsacled images) for Linux → Implement Bug 381661 (bilinear filtering for upscaled images) for Linux
(Reporter)

Updated

9 years ago
Summary: Implement Bug 381661 (bilinear filtering for upscaled images) for Linux → Implement Bug 381661 (bilinear filtering of upscaled images) for Linux

Updated

9 years ago
Component: General → GFX: Thebes
Product: Firefox → Core
QA Contact: general → thebes
Version: unspecified → Trunk

Comment 1

9 years ago
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.
(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.
Where in the code should this be implemented?  What is needed from X?
To wit: nothing should be needed from X; GTK+ can already do beautiful scaling for you :)

Updated

9 years ago
Duplicate of this bug: 431990

Comment 6

9 years ago
(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? 
 


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

9 years ago
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

9 years ago
I'm sorry, I meant "...what would happen if I *commented out* (not uncommented) the two special cases...".
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.

Updated

9 years ago
Duplicate of this bug: 455495

Updated

9 years ago
Duplicate of this bug: 459508

Comment 15

9 years ago
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.

Updated

9 years ago
Duplicate of this bug: 465494

Comment 17

8 years ago
Vladimir, could you explain what needs to happen in X/pixman and/or firefox for this bug to be fixed?  Thanks.

Comment 18

8 years ago
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.

Updated

8 years ago
Blocks: 426930

Comment 19

8 years ago
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

Updated

8 years ago
Duplicate of this bug: 500191

Comment 21

8 years ago
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

8 years ago
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

8 years ago
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

8 years ago
Created attachment 386032 [details] [diff] [review]
fixed indentation
Attachment #384996 - Attachment is obsolete: true
Attachment #386032 - Flags: review?(vladimir)
Attachment #386032 - Flags: review?(vladimir) → review?(jmuizelaar)
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);
>+                }
Attachment #386032 - Flags: review?(jmuizelaar) → review-

Comment 26

8 years ago
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

8 years ago
Created attachment 388683 [details] [diff] [review]
comment updates
Attachment #386032 - Attachment is obsolete: true
Attachment #388683 - Flags: review?(jmuizelaar)
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;
>         }
Attachment #388683 - Flags: review?(jmuizelaar) → review-

Comment 29

8 years ago
Created attachment 388713 [details] [diff] [review]
cosmetic changes

Thanks.  I've made the two requested changes.
Attachment #388683 - Attachment is obsolete: true
Attachment #388713 - Flags: review?(jmuizelaar)
Comment on attachment 388713 [details] [diff] [review]
cosmetic changes

Looks good
Attachment #388713 - Flags: review?(jmuizelaar) → review+

Comment 31

8 years ago
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.

Updated

8 years ago
Duplicate of this bug: 509292
Is this ready to land?
Comment on attachment 388713 [details] [diff] [review]
cosmetic changes

This is outdated. That code lives in modules/libpr0n/src/imgFrame.cpp now.
Attachment #388713 - Attachment is obsolete: true

Comment 35

8 years ago
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 on attachment 393971 [details] [diff] [review]
updated patch

No, this doesn't work.

error: gfxXlibSurface.h: No such file or directory
Attachment #393971 - Attachment is obsolete: true
(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.
Also imgFrame shouldn't be reaching into cairo symbols; if we need the cairo version, we should expose it through thebes somehow.
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

8 years ago
(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.

Updated

8 years ago
Duplicate of this bug: 521866

Comment 42

8 years ago
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.
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

7 years ago
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

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

Comment 46

7 years ago
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

7 years ago
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

Updated

7 years ago

Comment 48

7 years ago
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

7 years ago
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

7 years ago
Somebody please review the patch and apply it.

Comment 51

7 years ago
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.
@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

7 years ago
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.
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

7 years ago
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.

Updated

7 years ago
Duplicate of this bug: 557438

Comment 57

7 years ago
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

7 years ago
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

7 years ago
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).
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

7 years ago
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

7 years ago
(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

7 years ago
(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

7 years ago
but when will this patch be applied to the official firefox branch??
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?

Updated

7 years ago
Attachment #419100 - Flags: review?(jmuizelaar)
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.
blocking2.0: --- → ?

Comment 67

7 years ago
(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
blocking2.0: ? → final+

Updated

7 years ago
Duplicate of this bug: 571853

Comment 69

7 years ago
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.
Assignee: nobody → jmuizelaar
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

7 years ago
(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

7 years ago
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

7 years ago
Created attachment 465688 [details]
Screenshot of a valid web page, with an artefact on FF under Linux

Comment 74

7 years ago
Is there anything stopping this patch being applied?
Attachment #393971 - Attachment is obsolete: false
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().
(Assignee)

Comment 76

7 years ago
(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
(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

7 years ago
>> ... 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

7 years ago
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?
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

7 years ago
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/
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()).
Oh, sorry, it's Tom's patch.
(Assignee)

Comment 84

7 years ago
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.
(Assignee)

Comment 85

7 years ago
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
Attachment #469348 - Flags: review?(jmuizelaar)
Attachment #469353 - Flags: review?(jmuizelaar)
Attachment #469348 - Flags: review?(jmuizelaar) → review+
Attachment #469353 - Flags: review?(jmuizelaar) → review+

Updated

7 years ago
Attachment #419100 - Flags: review?(jmuizelaar)

Updated

7 years ago
Attachment #419100 - Attachment is obsolete: true

Updated

7 years ago
Attachment #393971 - Attachment is obsolete: true
Is this ready to land?

Updated

7 years ago
Blocks: 593569

Comment 87

7 years ago
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...
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.
Blocks: 594319
(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

7 years ago
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.)
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.
Attachment #473677 - Flags: review?(jmuizelaar)
Attachment #473677 - Flags: review?(jmuizelaar) → review+
http://hg.mozilla.org/mozilla-central/rev/89816be9e164
http://hg.mozilla.org/mozilla-central/rev/c08cbc8f4fa6
http://hg.mozilla.org/mozilla-central/rev/49291ab908cc
Assignee: jmuizelaar → taken.spc
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
No longer blocks: 594319

Comment 93

7 years ago
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

Updated

7 years ago
status1.9.2: --- → ?
You need to log in before you can comment on or make changes to this bug.