FFe: Pixellated Images in Firefox/Opera due to incorrect EXTEND_PAD implementation in several video drivers

Proposal reference: http://primates.ximian.com/~federico/news-2005-11.html#moz-images

see bug 259672 and the patch in bug 296818

I referenced both the bug and the patch above. I have the mozilla source downloaded and the diff and have been trying to get this to work with patch, but get many errors. I would love to test if this patch would fix the pixmap problem. If anyone could post some quick steps I'll give it a whirl.

Thanks,
Jim Kronebusch

*** This bug has been marked as a duplicate of bug 296818 ***

I think this would be better addressed by a different solution from bug 296818. See bug 296818 comment 27.

To summarize that comment, I think it makes sense to have a preference for maximal size of the pixmap cache we end up with. Once we get there, we stop decoding new images that would take us over the cache size. The default can be either infinite or something large enough that not rendering images is better than trying to render them at that point. Deployments in low-resource situations (e.g. mobiles with no virtual memory, thin clients, etc) can set the preference as needed.

there is already a feature to disable caching image data as pixmaps (we don't cache pixmaps to X11).

MOZ_DISABLE_IMAGE_OPTIMIZE=1 firefox

not sure what people are hoping to get out of this bug. if it is "need feature to disable" then it is already there (dup of bug 336191).

If you want to be smarter about the way we allocate pixmaps, that could be done separately and in addition to what we already have.

Stuart, where is the MOZ_DISABLE_IMAGE_OPTIMIZE=1 firefox to be set? Do we just add that to about:config? I am not sure if I am misreading your statement in Comment 8 or not. If I monitor resources with xrestop, I can see system memory usage spike as a result of Firefox hitting an image intense website (see the example website in Comment 1). I looked at bug 336191, but still don't understand where to set the parameter and am not clear on if this patch is already present in current release firefox.

Boris, I think your suggestion for a user settable limit is a good idea. Is it possible to set so that once the limit is reached, older data is simply overwritten still allowing new data to be cached but not exceeding the limit?

Jim

> Stuart, where is the MOZ_DISABLE_IMAGE_OPTIMIZE=1 firefox to be set?

In the environment. Shell-agnostic command-line:

  env MOZ_DISABLE_IMAGE_OPTIMIZE=1 firefox

> not clear on if this patch is already present

It's present in the alphas of Firefox 3.

I have a build of firefox-granparadiso to test Federico's patch. I launched it at the command line with env MOZ_DISABLE_IMAGE_OPTIMIZE=1 firefox-granparadiso. Then I loaded the site http://www.carteretcountyschools.org/bms/teacherwebs/sdavenport/artgallery6.htm to see what happened and monitored with xrestop. Pixmap storage would hit very close to 25MB then be cleared and start over. This seemed to work very well. If I simply launched without the environment variable, Pixmap storage would climb to over 500MB+ by the time the page finished loading. This seems acceptable. Is this possible to do in Firefox 2.0.0.6 and is it possible to set the environment variable permanently?

I could still see a use for a user settable limit on storage, and the ability to set this environment variable in about:config to make it more user friendly. A settable discard timer would also be helpful. All of these would make Firefox more usable in low memory situations and still allow defaults to be set for performance in standard higher memory situations.

Thanks,
Jim

> Is this possible to do in Firefox 2.0.0.6

No. The support for this is in the new Thebes rendering code. I suppose you could try to backport that fix to branch, but...

> is it possible to set the environment variable permanently

In your own account? Of course. See your shell's documentation.

(In reply to comment #11)
> Then I loaded the site
> http://www.carteretcountyschools.org/bms/teacherwebs/sdavenport/artgallery6.htm

Something strikes me about the way firefox (or is it Gecko?) deals with this page. The real cause of this issue is that the page contains a number of massive images, which are embedded at small resolution in the page. We all know this is down to bad web page authoring, but obviously firefox has to deal with it. My laptop slows to a crawl loading this page.

Arguably the reason for the problems in that webpage is that firefox uses the video card to do the resizing -- therefore caching a huge quantity of image data which it will never actually display (unless the person chooses to view the image which is probably quite rare).

I realise the video card will do the resize more quickly, but if firefox resized the image in software, the X server's RAM usage would be fairly minimal as it would be caching the data which was actually needed. There is often a balance between cpu and ram usage, this seems to be one of them.

I would therefore be inclined to suggest an optional feature (particularly for remote X servers or low ram situations) where firefox would resize the image in software and send the resized pixmap to the X server. Perhaps this would only be used where the original image was above a certain size -- or where the display size was under some fraction of the image size (eg less than half the width and height).

The X server seems often to do a simple resample, rather than a resize, so I would expect firefox to do the same.

Gavin, maybe this is how non-gecko based browsers are handling this and possibly why they are not affected in the same fashion.

Boris, Stuart, is this possible?

Storing images at the original size is a pretty long-standing imagelib design decision. You might want to read the various existing bugs that discuss that behavior.

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.

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

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

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

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

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.

Created an attachment (id=328690)
patch - reduce pixmap memory usage in X

Patch prescales images overflowing their Thebes destination rectangle because they've been resized with the "width" or "height" html tags. The patch uses the GDK library to perform downscaling using a bilinear interpolation algorithm. We also optimized the consumed X memory by only transferring the visible portion of a pixmap between the X application and server. This also reduces bandwith consumption and reduces the delay before an image is painted by X. Pixmaps are now kept in the application's memory instead of X's.

This patch only affects the behavior of pixmaps storage on UNIX systems. It could easily be ported to other platforms if similar issues are discovered.

Complete blog posts :
http://www.francisrobichaud.com/index.php/2008/07/08/optimizing-mozilla-and-pixmap-management-in-x/

Created an attachment (id=328694)
updated patch - reduce pixmap memory usage in X

Given that the same nsThebesImage can be drawn to multiple different destination rectangle sizes how does this page behave? Do we end up thrashing the pixmap on such draws?

The complete pixmap is always kept in Firefox's memory while one subpixmap is generated and sent to X for each destination rectangle. These subpixmaps do not trash the original pixmap and are only used for painting in X.

Yes, I understand all that. But the point is that before this patch we send the data to the server _once_ and then use it to paint whenever we need, right? With this patch, we send the data on every sub-rectangle paint? And multiple times if the image appears on the page multiple times?

How is scrolling performance with a non-local X server with this approach?

Data needs to be retransfered to the server for images that need resizing or
those that are partially visible. When it's transfered once, image
manimuplation is done by X which leads to poor quality in the case of image
resizing.

I've just tested scrolling and I confirm that the feeling is as good as with
the FF 3.0 official build on a local machine (for both regular and smooth
scrolling).

There is a very slight difference when scrolling with a thin client caused by
the transfer of images between the server and client. I will limit image
manipulations to find the best balance between memory consumption and bandwith
usage (e.g. only pre-scale under a certain ratio).

Created an attachment (id=329705)
patch update - upscaling about:config pref

After testing the patch in a few environments, I discovered that the scrolling quality could be sluggish when Firefox was used on a thin client with an application server having low resources. Since we can assume that a thin client’s backend should have the minimum ressources to support mallocs without using the swap area, there should be no visible difference of usage on a thin client.

To give users control over image manipulation, I added the “browser.gdk_interpolation_threshold_percent” preference variable that allows values between 1-100 and has for effect to either use or bypass GDK image manipulations. The default value is 50% which only affects images being scaled by a factor of 50% or 200% and images with a visible portion under 50%. Lowering this value to it’s minimum (1%) would turn the feature off while using 100% will premanipulate any image overflowing it’s container.

Finally, as Joe from #gfx did mention, I added support for images upscaling with GDK to increase the image quality by also using the bilinear interpolation algorithm.

So you tested the patch with X over a WAN link (not just thin client, but appreciable latency and low bandwidth) and there was no impact?

Boris, I tested X over WAN and there is no difference between the original FF 3.0 and my patched version.

Created an attachment (id=329724)
update - missing all.js file

Created an attachment (id=330932)
optimized/increased quality + fixed interlacing issue

needs reviewal

intensive testing has been done with that patch on both thin clients and desktops and no issue was reported.

http://www.francisrobichaud.com/index.php/2008/07/23/optimizing-mozilla-and-pixmap-management-in-x-updates/

(From update of attachment 330932)
lines
+/*#ifdef XP_UNIX
+ mTinySurf = nsnull;
+#endif*/
should be removed. can this be done without reuploading a new patch ?

(From update of attachment 330932)
Some comments on the patch, and then overall comments at the end...

>Index: ./gfx/src/thebes/nsThebesImage.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/thebes/nsThebesImage.cpp,v
>retrieving revision 1.86
>diff -u -8 -p -B -b -r1.86 nsThebesImage.cpp
>--- ./gfx/src/thebes/nsThebesImage.cpp 28 Apr 2008 21:27:05 -0000 1.86
>+++ ./gfx/src/thebes/nsThebesImage.cpp 22 Jul 2008 20:36:07 -0000
>@@ -43,48 +44,68 @@
> #include "gfxPattern.h"
>
> #include "gfxPlatform.h"
>
> #include "prenv.h"
>
> static PRBool gDisableOptimize = PR_FALSE;
>
>+#ifdef XP_UNIX
>+static gfxFloat gImageThreshold = 0.5;

This variable is oddly named; I'm not sure what it means, especially
not from the name. gImagePreScaleFactor? Though that's not quite how
it's used, see below.

> nsresult
> nsThebesImage::Init(PRInt32 aWidth, PRInt32 aHeight, PRInt32 aDepth, nsMaskRequirements aMaskRequirements)
> {
> mWidth = aWidth;
> mHeight = aHeight;
>
>@@ -206,29 +227,28 @@ PRInt32
> nsThebesImage::GetAlphaLineStride()
> {
> return (mAlphaDepth > 0) ? mStride : 0;
> }
>
> void
> nsThebesImage::ImageUpdated(nsIDeviceContext *aContext, PRUint8 aFlags, nsRect *aUpdateRect)
> {
>+ mImageIncomplete = aFlags & nsImageUpdateFlags_ImageIncomplete;

You only modified the PNG decoder to pass down ImageIncomplete; why is
this info needed at all? You should be able to tell whether the image
is complete or not via the same mechanism used in GetIsImageComplete.

> nsresult
> nsThebesImage::Optimize(nsIDeviceContext* aContext)
> {
> if (gDisableOptimize)
> return NS_OK;
>
>@@ -326,21 +346,26 @@ nsThebesImage::Optimize(nsIDeviceContext
>
> #ifdef XP_MACOSX
> if (mQuartzSurface) {
> mQuartzSurface->Flush();
> mOptSurface = mQuartzSurface;
> }
> #endif
>
>+#ifndef XP_UNIX
> if (mOptSurface == nsnull)
> mOptSurface = gfxPlatform::GetPlatform()->OptimizeImage(mImageSurface, mFormat);
>+#endif
>+

If you want to do this, we should just early-return from Optimize
after the 1x1 check, instead of adding the extra ifdefs. But I'm not
sure it's a good idea, see below.

>@@ -482,24 +507,108 @@ nsThebesImage::Draw(nsIRenderingContext
> return NS_ERROR_FAILURE;
>
> // Expand the subimageRect to place its edges on integer coordinates.
> // Basically, if we're allowed to sample part of a pixel we can
> // sample the whole pixel.
> subimageRect.RoundOut();
>
> nsRefPtr<gfxPattern> pat;
>+#ifdef XP_UNIX
>+ // We optimize the image surface to it's visible size and use scaling algorithms
>+ // to reduce X memory consumption, bug 395260. We bypass 1x1 rectangles to avoid useless operations.
>+ if (destRect.size != gfxSize(1.0, 1.0) &&
>+ (mWidth > (destRect.size.width / gImageThreshold) ||
>+ mHeight > (destRect.size.height / gImageThreshold) ||
>+ xscale < (1.0 * gImageThreshold) || yscale < (1.0 * gImageThreshold) ||
>+ xscale > (1.0 / gImageThreshold) || yscale > (1.0 / gImageThreshold))) {

I don't understand this check at all; why isn't this just checking if
the xscale is...

Created an attachment (id=331837)
changed GDK for cairo scaling

Updates:
- We don't use offscreen surfaces when we're drawing temp surfaces
- We use cairo transforms only to create visible image portions or to scale instead of GDK
- It is now multiplatform (compiled and tested on Windows)
- The modifications for the ImageUpdate flag is required for interlaced images. It is an issue with the GetIsImageComplete() which would return true even if we only had decoded the first pass on an interlaced image.

Created an attachment (id=332858)
updated patch from reviewers comments

Patch was separated in two bugs, PNGDecoding for interlaced image is now referred to bug 449655.
I added some comments and removed the all.js variable. Users must now create the browser.image_prescaling_threshold variable of type integer in the about:config panel.

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

(From update of attachment 332858)
Couple of problems with this patch:
1. Not having a browser.image_prescaling_threshold pref doesn't disable prescaling.

2. (much more important) It breaks several reftests, including the apng reftest.

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

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.

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

Created an attachment (id=351844)
partially fixed some reftests

Fixed the aPNG tests.
text/zwnj-02.html, bugs/214077-1b.html and bugs/214077-1a.html are still failing but I'm out of ideas.
Some other SVG tests are also failing and this is due to gfxImageFrame::SetMutable where I changed mImage->Optimize() for mImage-SetMutable(). We can't restore mImage->Optimize() or we won't be able to reduce memory usage.

Created an attachment (id=351955)
test case

The attached test case takes an 1x2 pixel image and stretches it horizontally to several hundred pixels wide, several times (with different stretch distances). On Linux, some of the green lines will show black dots at the far right. If you change the zoom level, the dots change size jump from one line to another, and at some zoom levels there are also black lines at the bottom of each image. I have anecdotal reports that similar image artifacts are highly visible with Ubuntu 8.10's build of Firefox 3.0.x (not sure which point revision) when playing the browser-hosted game at www.kingdomofloathing.com, so this is not merely a problem for contrived test cases.

The artifacts arise because, on Linux, we are trying to avoid the use of the CAIRO_EXTEND_PAD source filter because it takes a slow fallback path. Instead, we use nearest-neighbor image scaling, on the theory that that will never sample pixels outside the image. As this test case demonstrates, that theory is incorrect.

I will shortly attach a patch which uses EXTEND_PAD universally and modifies the Cairo library to turn on accelerated handling of this mode when the X server supports it. (Render protocol 0.10 - available in X.org server 1.5 - includes support for EXTEND_PAD and it appears to work when I make the library aware of it.) I'd be happy to see a fix that don't involve a major performance hit on older X servers but I have been unable to think of one.

(I suspect this test case, or a slight modification, will also show artifacts at some zoom levels on Mac, because we are also not using EXTEND_PAD there - in that case not for performance but because someone had the erroneous impression that EXTEND_PAD is the default on Mac. Code inspection suggests that Cairo does implement EXTEND_PAD correctly and speedily on Mac.)

Created an attachment (id=351956)
proposed patch (incl Cairo changes)

Here's the patch as described above. Reftest run in progress.

(From update of attachment 351956)
Looks fine, except for:

>+ pattern->SetExtend(gfxPattern::EXTEND_PAD);

This shouldn't be done on OSX; the quartz surface behaves slightly differently and PAD will cause us to take a slow path.

Also, make sure you do the cairo patch bit -- add the patch file itself to gfx/cairo and edit gfx/cairo/README to reference it, or it'll get blown away next cairo upgrade. Jeff or I will upstream it when we can.

I seem to recall problems with PAD being buggy in the X server, but I don't remember any details. I guess if reftests pass that's a pretty good sign, but how confident are we about the various server/driver combinations? Does Carl know?

I basically like the patch but I'd like to see a new version addressing Vlad's comments. Also, what does returning UNSUPPORTED actually cause cairo to do here?

(In reply to comment #2)
> >+ pattern->SetExtend(gfxPattern::EXTEND_PAD);
>
> This shouldn't be done on OSX; the quartz surface behaves slightly differently
> and PAD will cause us to take a slow path.

It's necessary. I haven't tried this exact testcase but I can reproduce visual artifacts on OSX without it. Also, looking at cairo-quartz-surface.c, it doesn't seem to take a fallback for EXTEND_PAD, only EXTEND_REPEAT/REFLECT.

> Also, make sure you do the cairo patch bit -- add the patch file itself to
> gfx/cairo and edit gfx/cairo/README to reference it, or it'll get blown away
> next cairo upgrade. Jeff or I will upstream it when we can.

Ok.

(In reply to comment #3)
> I seem to recall problems with PAD being buggy in the X server, but I don't
> remember any details. I guess if reftests pass that's a pretty good sign, but
> how confident are we about the various server/driver combinations? Does Carl
> know?

For the record, I see no regressions in the reftests with this change, but the reftest included in the patch fails -- still analyzing. I think it's an accident of how I wrote the test.

> I basically like the patch but I'd like to see a new version addressing Vlad's
> comments. Also, what does returning UNSUPPORTED actually cause cairo to do
> here?

The failure propagates to _cairo_surface_composite in cairo-surface.c which then calls _cairo_surface_fallback_composite -- so the rendering is the same, but without X-server acceleration.

(In reply to comment #4)
> (In reply to comment #2)
> > >+ pattern->SetExtend(gfxPattern::EXTEND_PAD);
> >
> > This shouldn't be done on OSX; the quartz surface behaves slightly differently
> > and PAD will cause us to take a slow path.
>
> It's necessary. I haven't tried this exact testcase but I can reproduce visual
> artifacts on OSX without it. Also, looking at cairo-quartz-surface.c, it
> doesn't seem to take a fallback for EXTEND_PAD, only EXTEND_REPEAT/REFLECT.

I can't reproduce with this testcase; no black or anything but green shows up. As you say, PAD is ignored anyway (incorrectly, I guess, but correctly for perf). So there's a separate bug there, I think.

Yeah, if there's a Mac problem, let's handle that under a separate bug.

(In reply to comment #5)
> > I haven't tried this exact testcase but I can reproduce visual
> > artifacts on OSX without it. Also, looking at cairo-quartz-surface.c, it
> > doesn't seem to take a fallback for EXTEND_PAD, only EXTEND_REPEAT/REFLECT.
>
> I can't reproduce with this testcase; no black or anything but green shows up.
> As you say, PAD is ignored anyway (incorrectly, I guess, but correctly for
> perf). So there's a separate bug there, I think.

PAD is not ignored on OSX. I have empirical evidence that it does *something*, and my impression from reading the code is that it is implemented with acceleration. Compare the rendering of reftests/border-image/solid-image-2.html at all zoom levels with these two builds:

https://build.mozilla.org/tryserver-builds/2008-12-02_11:<email address hidden><email address hidden>

https://build.mozilla.org/tryserver-builds/2008-12-05_17:<email address hidden><email address hidden>

At some zoom levels (reportedly not the default, though) you should see gaps in the borders with the first, but not the second. The only difference is that the second one sets EXTEND_PAD for quartz surfaces. (Note that both builds have other patches in them -- you should *not* see gaps in the border with a trunk build.)

Given that the mitigating change is in a shared code path for all images, I am certain it is possible to construct a test case that doesn't use border-image but nonetheless fails on OSX, but I have to leave it to someone who actually has a Mac.

I don't see any value in splitting the bug. It is the same bug on both Mac and Linux - we need to be using EXTEND_PAD and we're not.

(In reply to comment #4)
> For the record, I see no regressions in the reftests with this change, but the
> reftest included in the patch fails -- still analyzing. I think it's an
> accident of how I wrote the test.

Bad news: the reftest failure in the patch seems to be caused by going back to whatever Cairo's default is for resampling on image upscale. With the patch as posted, the test file's images are rendered with a very slight color gradient: at about one-third of the way across each image, the white stripe changes from #ffffff to #fbfffb, and at about two-thirds across, the green stripe changes from #00ff00 to #04ff04. This does not happen if I leave in the call to pattern->SetFilter(0).

I'd be interested to know if this test fails in Windows and Mac builds.

... I can reproduce the equivalent of the reftest failure with code that uses only the patched cairo library, and the failure goes away if I take the patches out of cairo. + some web searches turned up other cases where RepeatPad in Render just plain doesn't work.

This makes me a lot less sanguine about the whole idea. :-(

Can you paste some links here which refer to specific problems we should watch out for the next time we think about enabling PAD on GTK?

http://lists.freedesktop.org/archives/xorg/2008-February/032973.html has a test program that fails catastrophically for PAD and REFLECT (or rather, the Render equivalents) on my desktop computer (ATI graphics) and for REFLECT, but not PAD, on my laptop (Intel graphics). Which has the lovely implication that this is not just about the Render implementation, but about which specific video driver you've got...

I'll attach the test program I wrote tomorrow, when the desktop is on again.

Any thoughts on how to proceed here?

No suggestions here other than "mark some tests random on GTK and hope things get better someday"

wanted1.9.1 is dead

Created an attachment (id=353263)
"stretch single pixel" cairo-only test case

I promised to attach this test case, which demonstrates bugs in X Render (you need to use it with a Cairo library patched to try to use RepeatPad for CAIRO_EXTEND_PAD).

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

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.

Are there any news about this bug? We will need this patch into firefox 3.1 to prevent crash of linux thin-clients, caused by large pictures loaded in X server memory. Where is it stuck?

I haven't looked into the other reftest failures, but until they're fixed, there's no way this can be applied.

XRender supports four possible repeat types: RepeatNone, RepeatNormal, RepeatPad and RepeatReflect. The driver currently only handles the first two, so it should fall back to software when it encounters RepeatPad or RepeatReflect. This is currently forcing cairo to use slow client-side rendering, thus preventing firefox from enabling bilinear filtering on linux.

Created an attachment (id=22191)
patch

Pushed, thanks.

Note that I think it should be easy to accelerate RepeatPad and RepeatReflect, as in contrast to RepeatNormal they have similar semantics to 3D APIs.

(In reply to comment #2)
> Note that I think it should be easy to accelerate RepeatPad and RepeatReflect,
> as in contrast to RepeatNormal they have similar semantics to 3D APIs.

Err, s/RepeatNormal/RepeatNone/ .

This is the same issue as in the ati driver, and the same patch applies:

http://bugs.freedesktop.org/show_bug.cgi?id=19712
http://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=7e54c4b8acafead8de5d0dd6fdc1b619f832dd6f

At least accalerated RepeatPad would be great, basically I think its even more important RepeatNone (if that wouldn't be default).

Mozilla is evaluating using RepeatPad for their images, and my Java2D XRender backend uses it a lot too.

At least accalerated RepeatPad would be great, basically I think its even more
important RepeatNone (if that wouldn't be default).

Mozilla is evaluating using RepeatPad for their images, and my Java2D XRender
backend uses it a lot too heavily.

Created an attachment (id=22250)
Start of acceleration for RepeatPad and RepeatReflect

Here's a possible start for acceleration of RepeatPad and RepeatReflect. R100/R200 parts only compile tested, R300 parts only lightly tested - are there any simple tests for RepeatPad/Reflect?

Here's a very simple test:

http://lists.freedesktop.org/archives/xorg/2008-February/032973.html

(In reply to comment #6)
> http://lists.freedesktop.org/archives/xorg/2008-February/032973.html

Thanks for reminding me of this. It passes on my RV350. :)

both r100 and r200 fail on the repeat reflect test. OTOH, the reflect test fails even without the patch.

(In reply to comment #8)
> both r100 and r200 fail on the repeat reflect test.

Do you see the problem in the repeat-test-reflect-out.png file generated by the test? Have you tried other *_CLAMP_S/T_MIRROR_* flags?

> OTOH, the reflect test fails even without the patch.

Hmm, so maybe something else is (also) broken on your system?

Actually, the 2x2 source used by the test is probably a fallback because it doesn't match the hardware's implicit POT texture pitch. So the problem you're seeing is probably a pixman bug, but I haven't really verified my change either. :}

Created an attachment (id=22354)
Modified test using an 8x8 source, which should hit hardware acceleration

This still passes on my RV350. :)

Sorry, I probably should have mentioned this earlier. This is a known bug in pixman:

http://bugs.freedesktop.org/show_bug.cgi?id=19704

8 isn't big enough since the EXA pitch alignment is 64 bytes.

Some documentation about hardware restrictions would be quite cool - I am currently working on a Java2D XRender backend - and optimal performance across different driver/gpu combinations causes a lot of guessing.

Created an attachment (id=22375)
Test using a 16x16 source, really hits hardware acceleration for me

(In reply to comment #13)
> 8 isn't big enough since the EXA pitch alignment is 64 bytes.

Argh. Try the 16x16 test. I also forgot this doesn't matter for >= R300, so I was hitting acceleration even with 2x2.

(In reply to comment #14)
> Some documentation about hardware restrictions would be quite cool - I am
> currently working on a Java2D XRender backend - and optimal performance across
> different driver/gpu combinations causes a lot of guessing.

In general, older hardware tends to only support repeat with power-of-two dimensions. And pre-R300 Radeons have this peculiarity where you can't choose an explicit pitch for power-of-two textures, the hardware just rounds up the width to the next multiple of 32 bytes. For other reasons, we can currently only use multiples of 64 bytes for the pitch, so if those pitches don't match (and the height is > 1) we can't do repeat. See RADEONPitchMatches() and its callers.

(In reply to comment #12)
> Sorry, I probably should have mentioned this earlier. This is a known bug in
> pixman:
>
> http://bugs.freedesktop.org/show_bug.cgi?id=19704

So it looks like this patch already gets us ahead of software fallbacks, at least on >= R300. :)

Both r1xx and r2xx pass the test.

(In reply to comment #17)
> Both r1xx and r2xx pass the test.

Yay! I pushed the patch, someone may want to look into extending the source tile code to handle RepeatReflect as well.

Created an attachment (id=22390)
patch

XRender supports four possible repeat types: RepeatNone, RepeatNormal, RepeatPad and RepeatReflect?. The driver currently only handles the first two, so it should fall back to software when it encounters RepeatPad or RepeatReflect. This is currently forcing cairo to use slow client-side rendering, thus preventing firefox from enabling bilinear filtering on linux. Test cases can be found in the corresponding ati driver bug report:

http://bugs.freedesktop.org/show_bug.cgi?id=19712

Note that pixman currently implements RepeatReflect incorrectly, so the last test case will fail.

http://forums.opensuse.org/applications/404572-display-problems-firefox-while-using-zoom.html

Could someone help me to resolve display issue that I see in openSUSE 11.1

I used Firefox 3.0.4 but I see the same issue in Opera. What I see is everything looks normal in Firefox when you get to any web page. Example below is a Russian site, but you see the same issue on any page. When you try to zoom in on the page everything readjust the size (pictures, fonts),but picture degrades in quality so it is unpleasant to see. Here is an example of a normal page without zoom. It looks normal:

http://i128.photobucket.com/albums/p191/belarus2007/picture2_regular.png

Most of the time you can adjust View in FireFox by Zooming in/out (Ctrl ++/ Ctrl -- )

See the same page after 2nd zoom:

http://i128.photobucket.com/albums/p191/belarus2007/picture2_2times.png

Picture quality is degrades.

See the same page after 4th zoom:

http://i128.photobucket.com/albums/p191/belarus2007/picture2_4times.png

Picture quality is degrades.

See the same pate after 6th zoom:

http://i128.photobucket.com/albums/p191/belarus2007/picture2_6times.png

As you can see there is no issue with fonts. When you zoom in fonts adjust and look normal, but picture quality degrades with each zoom.

It is a dual-boot PC with Vista and openSUSE 11/1. I have Nvidia Video Card GeForce 7900GS with Nvidia Linux driver.

The interesting part that I don't see a problem on Vista (the same hardware, the same PC). Firefox zooms in font and pictures and scales just fine. (no picture quality degradation)

I'd like to find a solution for this zoom/scale issue. I'd like to use openSUSE,but this thing just drives me nuts and I keep switching back to Vista that I dislike, but has good FireFox zoom in feature.

P.S.

I'm duplicating this on SLES-11-RC1

Does this happen with an Intel video on SUSE? Because if it also happen with Opera, then it is unlikely a Firefox problem.

Correct, I see similar image quality degradation with Opera 9.63 on the same box when I zoom in.

I run openSUSE 11.1 on Latitude D630/ Nvidia Quadro NVS 135M ( Nvidia driver). I don't know how to test "Intel video on SUSE".

If it is not Firefox or/and Opera issue it is still an issue for users.

Questions:

1. Are you duplicating this on your hardware?
2. Which group needs to be involved to resolve this issue?

(In reply to comment #2)
>
> I don't know how to test "Intel video on SUSE".

You need a machine with Intel video (which is what I chose), like Thinkpad X60/61.

> If it is not Firefox or/and Opera issue it is still an issue for users.
>
> Questions:
>
> 1. Are you duplicating this on your hardware?

I have an Intel card and I didn't give the address of the website to test.

> 2. Which group needs to be involved to resolve this issue?

If it works fine on a non-nvidia card, I'll bounce back to the Xorg team, but then they'll say they can't do anything because the Nvidia driver is closed (ie not properly supported on Linux).

I think there is a way to get consistent cross-platform rendering without sacrificing too much performance. The common case is that images are not scaled, and post-bug 296818, we don't even keep them around in memory in uncompressed form. So that says to me that we could treat images in display memory as a pure cache, and in particular, we could do all the scaling on the client side before copying to display memory. That would mean that it's our copy of pixman doing all the work, so we know it's correct, but there wouldn't be any need to pull the image data off the X server, rescale it, and put it back, like what happens now if you enable EXTEND_PAD on Linux.

We *would* create offscreen bitmap surfaces for the scaled images so that as long as the nsThebesImage object sticks around, repaint doesn't have to repeat the scaling work. I think this might even make scrolling of a scaled document faster, because right now we're scaling everything on every draw operation.

I'm reluctant to implement this before joedrew!'s rumored image layer cleanups land, because right now it's such a horrible mess in there, but I'm willing to make an attempt at it if those are not happening anytime soon.

We used to do things that way. We stopped. Better people than I can explain why.

I think that's what we want to get to, once we get to being able to do decode-on-draw. It's not a great solution right now though, because we don't want to cache all scaled images since they can become quite large; you can toss in a limit, and then you're back to having to scale on demand, which means having to keep the original bits around. It also means that if hardware does accelerate the scaling, that we'd lose any access to that; for example, if the native image representation is a GL texture.

But, I think that we should be able to at least partially do it the way you describe... I think the missing piece is decode-on-draw, so that we can better manage memory usage.

I don't understand ... how is decode-on-draw going to work with scaled images? How can you pre-scale the compressed data?

Just so that we have a way to request the image to get redecoded from within the code that does the final rendering, to avoid having to keep different decoded regions in memory at all times.