Last Comment Bug 468496 - Stretched images can show black blobs at edges on Linux (nearest neighbor scaling is not a substitute for EXTEND_PAD)
: Stretched images can show black blobs at edges on Linux (nearest neighbor sca...
Status: VERIFIED FIXED
: polish, testcase
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal with 8 votes (vote)
: mozilla7
Assigned To: Zack Weinberg (:zwol)
:
Mentors:
: 460989 467832 477552 487996 490799 497813 499308 500805 513651 520261 534645 537226 559871 567836 572464 586241 (view as bug list)
Depends on:
Blocks: 679257 534054 634362
  Show dependency treegraph
 
Reported: 2008-12-08 12:48 PST by Zack Weinberg (:zwol)
Modified: 2014-02-21 04:13 PST (History)
51 users (show)
mbeltzner: blocking1.9.2-
vladimir: wanted1.9.2+
karlt: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wanted
-
wanted


Attachments
test case (2.90 KB, text/html)
2008-12-08 12:48 PST, Zack Weinberg (:zwol)
no flags Details
proposed patch (incl Cairo changes) (11.91 KB, patch)
2008-12-08 12:51 PST, Zack Weinberg (:zwol)
no flags Details | Diff | Review
"stretch single pixel" cairo-only test case (2.39 KB, text/x-csrc)
2008-12-16 12:48 PST, Zack Weinberg (:zwol)
no flags Details
Use EXTEND_PAD on X (4.95 KB, patch)
2010-12-11 22:42 PST, Joe Drew (not getting mail)
roc: review+
jmuizelaar: review+
karlt: review-
zackw: feedback+
karlt: feedback+
Details | Diff | Review
rendering artifacts, m-c, zoom level 0 (1.80 KB, image/png)
2010-12-13 16:09 PST, Zack Weinberg (:zwol)
no flags Details
rendering artifacts, m-c, zoom level 1 (3.24 KB, image/png)
2010-12-13 16:09 PST, Zack Weinberg (:zwol)
no flags Details
alternative patch: unconditional EXTEND_PAD (10.63 KB, patch)
2010-12-21 21:26 PST, Zack Weinberg (:zwol)
joe: feedback-
Details | Diff | Review
use RepeatPad on newer X servers even when downscaling (7.19 KB, patch)
2011-05-16 14:36 PDT, Karl Tomlinson (ni?:karlt)
joe: review+
Details | Diff | Review

Description Zack Weinberg (:zwol) 2008-12-08 12:48:49 PST
Created attachment 351955 [details]
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.)
Comment 1 Zack Weinberg (:zwol) 2008-12-08 12:51:16 PST
Created attachment 351956 [details] [diff] [review]
proposed patch (incl Cairo changes)

Here's the patch as described above.  Reftest run in progress.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2008-12-08 13:01:49 PST
Comment on attachment 351956 [details] [diff] [review]
proposed patch (incl Cairo changes)

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.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-08 13:09:39 PST
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?
Comment 4 Zack Weinberg (:zwol) 2008-12-08 13:25:38 PST
(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.
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2008-12-08 13:52:36 PST
(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.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-08 13:56:30 PST
Yeah, if there's a Mac problem, let's handle that under a separate bug.
Comment 7 Zack Weinberg (:zwol) 2008-12-08 14:11:56 PST
(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:21-zweinberg@mozilla.com-borderimage/zweinberg@mozilla.com-borderimage-firefox-try-mac.dmg
 
https://build.mozilla.org/tryserver-builds/2008-12-05_17:37-zweinberg@mozilla.com-borderimage/zweinberg@mozilla.com-borderimage-firefox-try-mac.dmg

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.
Comment 8 Zack Weinberg (:zwol) 2008-12-08 14:34:14 PST
(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.
Comment 9 Zack Weinberg (:zwol) 2008-12-08 18:40:40 PST
... 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. :-(
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-08 19:01:53 PST
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?
Comment 11 Zack Weinberg (:zwol) 2008-12-08 21:07:58 PST
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?
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-08 22:13:57 PST
No suggestions here other than "mark some tests random on GTK and hope things get better someday"
Comment 13 Vladimir Vukicevic [:vlad] [:vladv] 2008-12-09 10:59:21 PST
wanted1.9.1 is dead
Comment 14 Zack Weinberg (:zwol) 2008-12-16 12:48:59 PST
Created attachment 353263 [details]
"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).
Comment 15 Zack Weinberg (:zwol) 2009-02-09 20:58:28 PST
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.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-02-09 21:02:48 PST
We used to do things that way. We stopped. Better people than I can explain why.
Comment 17 Vladimir Vukicevic [:vlad] [:vladv] 2009-02-10 09:42:14 PST
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.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-02-10 11:07:50 PST
I don't understand ... how is decode-on-draw going to work with scaled images? How can you pre-scale the compressed data?
Comment 19 Vladimir Vukicevic [:vlad] [:vladv] 2009-02-10 11:40:14 PST
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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-07-07 12:57:34 PDT
*** Bug 477552 has been marked as a duplicate of this bug. ***
Comment 21 Zack Weinberg (:zwol) 2009-07-07 13:13:00 PDT
*** Bug 499308 has been marked as a duplicate of this bug. ***
Comment 22 Zack Weinberg (:zwol) 2009-07-07 13:13:38 PDT
*** Bug 467832 has been marked as a duplicate of this bug. ***
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-07-07 13:20:19 PDT
*** Bug 487996 has been marked as a duplicate of this bug. ***
Comment 24 Zack Weinberg (:zwol) 2009-07-07 13:30:11 PDT
*** Bug 460989 has been marked as a duplicate of this bug. ***
Comment 25 Zack Weinberg (:zwol) 2009-07-08 12:05:13 PDT
Comment on attachment 351956 [details] [diff] [review]
proposed patch (incl Cairo changes)

canceling r/sr on the patch -- something similar may eventually be the right thing but not now.
Comment 26 Mark Finkle (:mfinkle) (use needinfo?) 2009-07-21 06:59:45 PDT
*** Bug 500805 has been marked as a duplicate of this bug. ***
Comment 27 Samuel Sidler (old account; do not CC) 2009-09-04 07:34:37 PDT
This has a few dupes and we currently list it as our only known Linux-specific issue (in our release notes). Seems like we should push a bit harder to fix it if possible.
Comment 28 Zack Weinberg (:zwol) 2009-09-04 09:12:45 PDT
Similar effects can be seen on Mac for a closely-related reason (see bug 472037 and bug 486761).  I've been meaning to ask bholley to look into the proposal outlined in comment 15 once he's done with basic decode-on-draw.
Comment 29 Mike Beltzner [:beltzner, not reading bugmail] 2009-09-23 12:16:40 PDT
Definitely wanted, would take a safe patch, but can't block 3.6 on this.
Comment 30 Aakash Desai [:aakashd] 2009-10-08 11:26:23 PDT
*** Bug 520261 has been marked as a duplicate of this bug. ***
Comment 31 Micah Gersten 2009-10-25 05:34:36 PDT
Ubuntu bug:
https://bugs.launchpad.net/bugs/217908
Comment 32 Aakash Desai [:aakashd] 2009-12-10 14:29:17 PST
*** Bug 534054 has been marked as a duplicate of this bug. ***
Comment 33 Doug Turner (:dougt) 2009-12-14 19:02:40 PST
*** Bug 534645 has been marked as a duplicate of this bug. ***
Comment 34 Dikei 2010-01-13 11:42:55 PST
This is no longer listed as a known issue in Firefox 3.6 but it's still not fixed. Please fix it soon.
Comment 35 Dikei 2010-01-21 22:27:03 PST
The patch is outdated. nsThebesImage.cpp no longer exist in firefox 3.6.
Comment 36 Joe Drew (not getting mail) 2010-01-27 13:08:11 PST
FWIW, that code was moved to imgFrame.cpp.
Comment 37 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-04-19 15:45:25 PDT
*** Bug 559871 has been marked as a duplicate of this bug. ***
Comment 38 Zack Weinberg (:zwol) 2010-04-19 16:55:40 PDT
I am not realistically going to fix this anytime soon.
Comment 39 Nickolay_Ponomarev 2010-05-16 23:29:47 PDT
*** Bug 537226 has been marked as a duplicate of this bug. ***
Comment 40 Mark Finkle (:mfinkle) (use needinfo?) 2010-05-24 13:40:54 PDT
*** Bug 567836 has been marked as a duplicate of this bug. ***
Comment 41 int 2010-06-02 03:51:21 PDT
Does anyone know someone who is able to fix this problem? Zack maybe you can put some text together to help a developer getting started on this. 

Zack can you specify why you cannot fix this issue, if it is resources maybe we can ask Ubuntu to step in. Otherwise if nobody here now's someone able to fix it I will contact Ubuntu/Debian and Fedora to look for someone who can.

It is really not nice to leave such an annoying bug open for so long.
Comment 42 Zack Weinberg (:zwol) 2010-06-02 08:33:24 PDT
Comments 1-15 and the various test cases and mailing list threads linked therein reflect everything that I know about this bug.

Joe Drew said he had a plan, though.
Comment 43 int 2010-06-02 11:19:59 PDT
Thanks for your fast reply Zack, I will wait for Joe to say something. In the mean time I will set up a machine to compile firefox on- to see if the patches still work.

Is there anyone at Mozilla reading who can say something on this issue??
Comment 44 Joe Drew (not getting mail) 2010-06-02 13:23:53 PDT
Yes, we're listening. This blocks 1.9.3 final, which means it'll be fixed for Firefox 4 final.

A plan Jeff and I sketched out involves (IIRC):
 - Turning on EXTEND_PAD unconditionally, and
 - using and/or extending the Cairo X version checking code to sniff for the versions of X that EXTEND_PAD sucks on, and falling back to software (pixman) in those cases.

That might not be the final plan, but it's *a* plan.
Comment 45 int 2010-06-04 01:54:40 PDT
OK thanks Joe. I is nice to hear that it is fixed for Firefox 4. I will keep monitoring this till than. If there is anything else I can do let me know.
Comment 46 Aakash Desai [:aakashd] 2010-08-11 08:00:41 PDT
*** Bug 586241 has been marked as a duplicate of this bug. ***
Comment 47 Karl Tomlinson (ni?:karlt) 2010-08-12 05:21:37 PDT
FWIW, attachment 351955 [details] shows the background instead of black when using operator-over instead of overriding with operator-source here:

http://hg.mozilla.org/mozilla-central/annotate/0df95f3a6de6/modules/libpr0n/src/imgFrame.cpp#l631

That may often be a little better though still not right, and there are other reasons to use extend-pad.

I'm curious whether the bug here is in calculating scale factors and clips or in the nearest neighbour implementation.  However, extend-pad is probably the appropriate solution anyway.
Comment 48 Dikei 2010-10-04 23:12:05 PDT
I believe EXTEND_PAD is enable for upscaled image in:
https://bugzilla.mozilla.org/show_bug.cgi?id=422179

However, I'm not sure about downscaled image.
Comment 49 Joe Drew (not getting mail) 2010-12-11 22:40:05 PST
Bug 422179 uses EXTEND_PAD if EXTEND_PAD is fast and good - but only for upscales. I suspect we should use EXTEND_PAD for downscales too, which will fix this bug once and for all. I have a patch which I think will do this.
Comment 50 Joe Drew (not getting mail) 2010-12-11 22:42:19 PST
Created attachment 497112 [details] [diff] [review]
Use EXTEND_PAD on X

This patch is built on my patch in bug 566283, but the merge is pretty trivial.

Zack, can you take a look and see if this works?
Comment 51 Zack Weinberg (:zwol) 2010-12-13 16:06:56 PST
Comment on attachment 497112 [details] [diff] [review]
Use EXTEND_PAD on X

Tested <http://hg.mozilla.org/mozilla-central/rev/5d9e1a56d922> on my current Linux box: X server vendor release code 10707000, Nouveau driver 0.0.16, NV94 video chipset.

The test case in attachment 351955 [details] shows display artifacts:

* with an unmodified tree;
* with joe's patches for this bug and bug 566283;
* and even with PreparePatternForUntiledDrawing chainsawed down to just

{
    aPattern->SetExtend(gfxPattern::EXTEND_PAD);
    aPattern->SetFilter(aDefaultFilter);
}

The artifacts are not the same as what I get with 3.6 on the same hardware.  3.6 shows thin green lines, all the same color throughout, some of which have solid black dots at their right edges; at some zoom levels there are also solid black lines underneath the green line (with a white pixel or two in between).

mozilla-central never produces the solid black dots at the right edges; however, the green lines are not all the same color throughout, they are the proper color in the middle, and fade to neutral gray at both edges.  Some of the lines get black lines underneath them.  And they all appear to be slightly fatter than they were in 3.6.  This behavior is more or less the same regardless of whether any patches are applied (the patch seems to change subtleties of the artifacts).

It is plausible to me that this is a bug in nouveau, and honestly I think the correct course of action at this point is to nuke the entire mess, use EXTEND_PAD unconditionally, and then work with the Cairo and X driver folks to figure out what's still wrong and fix it properly.  Nonetheless I cannot say that this patch fixes the original problem.
Comment 52 Zack Weinberg (:zwol) 2010-12-13 16:09:13 PST
Created attachment 497346 [details]
rendering artifacts, m-c, zoom level 0
Comment 53 Zack Weinberg (:zwol) 2010-12-13 16:09:32 PST
Created attachment 497347 [details]
rendering artifacts, m-c, zoom level 1
Comment 54 Zack Weinberg (:zwol) 2010-12-13 16:18:18 PST
Replacing the entire body of PreparePatternForUntiledDrawing with

{
    aPattern->SetExtend(gfxPattern::EXTEND_NONE);
    aPattern->SetFilter(gfxPattern::FILTER_FAST);
}

reproduces the 3.6 behavior.  Replacing it with

{
    aPattern->SetExtend(gfxPattern::EXTEND_PAD);
    aPattern->SetFilter(gfxPattern::FILTER_FAST);
}

produces behavior which is _nearly_ correct; the image itself is upscaled correctly (instead of being gradientified and fattened), and there aren't black lines under the image at any zoom level, but some of the lines still get black blobs at their right edges at some zoom levels, and also now sometimes at their left edges.  However, this happens much less often.

Based on this I feel relatively confident in diagnosing a video driver bug, and my recommendation is still to rip out the entire mess and use EXTEND_PAD and a good filter unconditionally, then to work with the X.org folks to get the video drivers fixed.
Comment 55 Karl Tomlinson (ni?:karlt) 2010-12-19 17:39:35 PST
(In reply to comment #51)
> Tested <http://hg.mozilla.org/mozilla-central/rev/5d9e1a56d922> on my current
> Linux box: X server vendor release code 10707000, Nouveau driver 0.0.16, NV94
> video chipset.

What pixman version was being used by the server here?

(The patch produces the correct results here with xorg-server 1.9.2.901 and xf86-video-ati 6.13.2, or a tigervnc based on xorg-server 1.7.6 and pixman-0.20.0.)
Comment 56 Zack Weinberg (:zwol) 2010-12-21 16:58:46 PST
Xorg.0.log says:

# Build Date: 02 December 2010  01:10:32AM
# xorg-server 2:1.7.7-10 
# Current version of pixman: 0.16.4

... which is pretty egregiously old, innit.  Stay tuned.
Comment 57 Zack Weinberg (:zwol) 2010-12-21 17:08:04 PST
I upgraded the shared pixman and cairo libraries on this computer to 0.21.2 and 1.10.0 respectively.  Xorg.0.log indicates pixman 0.21 is in use.  Same bad behavior with (or without) joedrew's patches.

The nouveau drivers are the next obvious culprit.  They haven't been updated by this distro since 2010-08-25, but I really don't want to go to the trouble of building them myself (I'd have to build my own kernel as well).

Mike Hommey should have been cc:ed on this bug a long time ago.  My bad.  Any comments, Mike?
Comment 58 Mike Hommey [:glandium] 2010-12-21 17:31:46 PST
Using cairo 1.10.0 and pixmap 0.21.2, everything is displayed gracefully on 4.0b8 built against system cairo, on intel display. (iceweasel from http://mozilla.debian.net/packages/, and cairo and pixmap from current debian experimental, for those who care).

Nouveau drivers are likely to be the culprit.
Comment 59 Zack Weinberg (:zwol) 2010-12-21 17:57:08 PST
Your system is nigh-identical to mine but for the video drivers, so yeah, that sounds pretty convincing.  I am testing some random trunk revision from last week + the patches, though.

Who would I have to blackmail to get more recent Nouveau drivers into experimental?
Comment 60 Zack Weinberg (:zwol) 2010-12-21 21:26:39 PST
Created attachment 499238 [details] [diff] [review]
alternative patch: unconditional EXTEND_PAD

This is what I think should be done.  Yeah, it means we're at the mercy of the video drivers on X11, but that gives us more leverage to kick the X people and the distros, and I don't like working around problems when we can fix them at source.  

Incidentally, my tests were all with in-tree Cairo, but I've now tried --enable-system-cairo and it made no difference.
Comment 61 Mike Hommey [:glandium] 2010-12-22 00:42:20 PST
(In reply to comment #59)
> Who would I have to blackmail to get more recent Nouveau drivers into
> experimental?

You can try debian-x@lists.debian.org directly, or file a wishlist bug against xserver-xorg-video-nouveau.
Comment 62 Zack Weinberg (:zwol) 2011-01-05 10:13:21 PST
Regret to report that I still have display artifacts with the much newer Nouveau driver that just hit Debian experimental:

X.Org X Server 1.9.2.902 (1.9.3 RC 2)
Release Date: 2010-12-03
Current Operating System: Linux moxana 2.6.37-rc7-amd64
NOUVEAU driver Date:   Tue Nov 30 15:27:36 2010 +1000
Comment 63 Joe Drew (not getting mail) 2011-01-11 15:41:49 PST
Comment on attachment 499238 [details] [diff] [review]
alternative patch: unconditional EXTEND_PAD

I can't see myself being OK with this any time soon - there are too many people with old-enough X that unconditional EXTEND_PAD will hurt.
Comment 64 Zack Weinberg (:zwol) 2011-01-11 16:15:32 PST
(In reply to comment #63)
> I can't see myself being OK with this any time soon - there are too many people
> with old-enough X that unconditional EXTEND_PAD will hurt.

In that case, and looking at the reports in comment 55 and comment 58, maybe we should go ahead with your patch.  We certainly shouldn't be generalizing from my computer, which has display artifacts *with or without* your patch, that have been conclusively pinned on the video drivers (see https://bugs.freedesktop.org/show_bug.cgi?id=32855 ).
Comment 65 Karl Tomlinson (ni?:karlt) 2011-01-11 16:50:26 PST
Comment on attachment 497112 [details] [diff] [review]
Use EXTEND_PAD on X

>-                if (fastExtendPad) {
>-                    aPattern->SetExtend(gfxPattern::EXTEND_PAD);
>-                    aPattern->SetFilter(aDefaultFilter);
>-                } else
>-#endif
>-                    aPattern->SetFilter(gfxPattern::FILTER_FAST);
>+
>+            if (!isDownscale && !fastExtendPad) {
>+                aPattern->SetFilter(gfxPattern::FILTER_FAST);
>             }
>+

Looks like an else block with SetFilter(aDefaultFilter) is needed.
Without this patch, that currently only happens when !isDownscale, but I expect it should also be done when !isDownscale, as on other platforms.

Otherwise, this approach of using EXTEND_PAD when supported by cairo seems sensible to me.
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-25 14:41:54 PDT
What else needs to be done here?
Comment 67 Zack Weinberg (:zwol) 2011-03-25 14:53:51 PDT
Get joedrew's patch landed?
Comment 68 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-25 14:56:29 PDT
Joe, is it ready to land?
Comment 69 Joe Drew (not getting mail) 2011-03-25 15:31:25 PDT
Comment on attachment 497112 [details] [diff] [review]
Use EXTEND_PAD on X

It needs review first, and maybe some rebasing.
Comment 70 Karl Tomlinson (ni?:karlt) 2011-03-27 16:47:17 PDT
Comment on attachment 497112 [details] [diff] [review]
Use EXTEND_PAD on X

Is there a reason for ignoring aDefaultFilter?  (Comment 65)
Comment 71 Kevin Brosnan [:kbrosnan] 2011-04-08 11:23:26 PDT
*** Bug 648234 has been marked as a duplicate of this bug. ***
Comment 72 Karl Tomlinson (ni?:karlt) 2011-05-16 14:36:31 PDT
Created attachment 532740 [details] [diff] [review]
use RepeatPad on newer X servers even when downscaling

This was based on Joe's attachment 497112 [details] [diff] [review], but restores the use of
aDefaultFilter when we use EXTEND_PAD and adds aDefaultFilter for downscale
with default extend.

There is also some code simplification.  We now require cairo 1.10 so there is
no need to check the cairo version.
Comment 73 Jason Quinn 2011-05-18 12:31:10 PDT
*** Bug 572464 has been marked as a duplicate of this bug. ***
Comment 74 Joe Drew (not getting mail) 2011-05-27 13:14:43 PDT
Comment on attachment 532740 [details] [diff] [review]
use RepeatPad on newer X servers even when downscaling

Review of attachment 532740 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxDrawable.cpp
@@ +78,5 @@
> +        case gfxASurface::SurfaceTypeQuartz:
> +        case gfxASurface::SurfaceTypeQuartzImage:
> +            // Don't set EXTEND_PAD, Mac seems to be OK. Really?
> +            aPattern->SetFilter(aDefaultFilter);
> +            break;

This hunk needs to be removed, as we've removed it with the Cairo update.
Comment 76 Mardeg 2011-06-03 19:34:39 PDT
*** Bug 490799 has been marked as a duplicate of this bug. ***
Comment 77 Mardeg 2011-06-03 19:34:43 PDT
*** Bug 513651 has been marked as a duplicate of this bug. ***
Comment 78 Mardeg 2011-06-03 19:34:50 PDT
*** Bug 497813 has been marked as a duplicate of this bug. ***
Comment 79 Henrik Skupin (:whimboo) 2011-06-22 14:46:05 PDT
Verified fixed with with Fennec 7.0a1 (20110620)
Comment 80 Konstantin Svist 2011-08-15 18:38:53 PDT
Not fixed when using canvas..drawImage
Example usage: http://demos.hacks.mozilla.org/openweb/HWACCEL/
Note: demo appears correct but runs slowly because it falls back to software rendering. I've verified with debug tracing that REPEAT_NONE is still used for that test case.

According to https://bugzilla.mozilla.org/show_bug.cgi?id=620065#c24, this bug should take care of that, but obviously didn't. Let me know if I need to file it separately.
Comment 81 Konstantin Svist 2011-08-15 18:40:06 PDT
P.S. tested using Aurora (7.0a2)
Comment 82 Zack Weinberg (:zwol) 2011-08-15 19:20:08 PDT
This bug is pretty long in the tooth, and by all accounts did fix the problem with <img>, so please file a new bug and mark it dependent on this one.
Comment 83 PashaTurok 2014-02-21 04:13:40 PST
This bug is still alive. I opened a new bug https://bugzilla.mozilla.org/show_bug.cgi?id=975358

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