Bug 17933 - [915-class] x8r8g8b8 doesn't sample alpha=0 outside surface bounds
[915-class] x8r8g8b8 doesn't sample alpha=0 outside surface bounds
Status: RESOLVED FIXED
Product: xorg
Classification: Unclassified
Component: Driver/intel
git
x86 (IA32) Linux (All)
: medium normal
Assigned To: Carl Worth
Xorg Project Team
:
: 21523 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-06 15:24 UTC by Clemens Eisserer
Modified: 2010-03-29 09:45 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Trust the alpha value (even for xRGB) (2.32 KB, patch)
2009-11-10 07:30 UTC, Chris Wilson
no flags Details | Splinter Review
Incorrect icon rendering (3.11 KB, image/png)
2010-03-17 03:44 UTC, Stefan Glasenhardt
no flags Details
Correct icon rendering (3.28 KB, image/png)
2010-03-17 03:45 UTC, Stefan Glasenhardt
no flags Details
Warnings for PutImage fallbacks (1.91 KB, patch)
2010-03-25 06:46 UTC, Chris Wilson
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Clemens Eisserer 2008-10-06 15:24:31 UTC
The same problem as mentioned in bug http://bugs.freedesktop.org/show_bug.cgi?id=16820 also happens on a 945GM, any maybe even on i830.

Here's a description of the original bug:
Composition with RepeatNone on src and an opaque dest results in black for
areas which fall outside the source-bounds, instead of leaving those areas
untouched.
Comment 1 Clemens Eisserer 2008-11-13 11:35:29 UTC
Seems not to happen on i830, but could also be caused by falling back to pixman ;)
Comment 2 Søren Sandmann Pedersen 2009-06-20 15:54:09 UTC
That's the expected behavior though, and pixman is going to do the same in 0.16.0.

The pixels outside the pixel grid are defined to be 0, so when you use the SRC operator with an opaque destination, those pixels will be black. Up until 0.14.0, there was a bug in pixman, where if the image was untransformed, it would actually leave the destination area untouched.
Comment 3 Søren Sandmann Pedersen 2009-06-20 16:05:12 UTC
Eh, ignore that. Somehow I thought you were using the SRC operator.
Comment 4 Chris Wilson 2009-11-10 07:30:23 UTC
Created attachment 31091 [details] [review]
Trust the alpha value (even for xRGB)

The bug is in the shader where we force the incoming source alpha to be 1 (overwriting the border color value) because we can not guarantee that the user has not uploaded random alpha values for xRGB images. The attached patch fixes this, but depends upon another patch for the X server to set the alpha channel upon reception.
Comment 5 Chris Wilson 2009-11-10 07:36:42 UTC
*** Bug 21523 has been marked as a duplicate of this bug. ***
Comment 6 Hendrik Lönngren 2009-12-06 06:42:13 UTC
I also see the gray borders on the left and top of scaled-down images in firefox with 855GM. The problem disappears with the "NoAccel" Option set.
Comment 7 Y-H Chen 2009-12-11 11:11:19 UTC
(In reply to comment #6)
> I also see the gray borders on the left and top of scaled-down images in
> firefox with 855GM. The problem disappears with the "NoAccel" Option set.
> 

Setting "NoAccel" hasn't helped for me. Please see bug 21523, for my comment re the matter.
Comment 8 Clemens Eisserer 2009-12-11 12:00:27 UTC
As far as I know NoAccel isn't supported anymore, which would explain why it doesn't make a difference.
Comment 9 Chris Wilson 2010-03-16 03:55:49 UTC
Finally realised how I could fix this without inflicting harm upon the Xserver.

commit d6b7f96fde1add92fd11f5a75869ae6fc688bf77
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sat Mar 6 15:49:04 2010 +0000

    Fill alpha on xrgb images.
    
    Do not try to fixup the alpha in the ff/shaders as this has the
    side-effect of overriding the alpha value of the border color, causing
    images to be padded with black rather than transparent. This can
    generate large and obnoxious visual artefacts.
    
    Fixes:
    
      Bug 17933 - x8r8g8b8 doesn't sample alpha=0 outside surface bounds
      http://bugs.freedesktop.org/show_bug.cgi?id=17933
    
    and many related cairo test suite failures.
Comment 10 Stefan Glasenhardt 2010-03-17 03:44:50 UTC
Created attachment 34140 [details]
Incorrect icon rendering
Comment 11 Stefan Glasenhardt 2010-03-17 03:45:53 UTC
Created attachment 34141 [details]
Correct icon rendering
Comment 12 Stefan Glasenhardt 2010-03-17 03:48:21 UTC
Hi Chris,

Your patch adds some graphical errors on my system (Ubuntu 10.04, 855GM-chipset). Please see the two attached screenshots for a better explanation.

Greetings Stefan
Comment 13 Chris Wilson 2010-03-17 04:37:34 UTC
Why am I not surprised that it appears that GdkPixbuf screws up? So I wonder how it depends upon the alpha-channel in a 24 bit depth image which would have been set to one by the ff-combiner anyway...

Stefan, thanks for the report.
Comment 14 Diego Escalante Urrelo 2010-03-24 18:15:59 UTC
Oh damn, didn't check the bug before posting a new one!: Bug #27299

I can confirm the regression described by Stefan. I attached another screenshot in my report which might be easier to recognize in any system.

Btw Chris, you are my hero for bug #25375! It improved performance on my 855GM a lot :-)
Comment 15 Chris Wilson 2010-03-25 03:08:46 UTC
Well I am still mystified by what's going on here. What is the depth of your screens? 16bpp?

Or perhaps we are taking one of the myriad fallback paths and ending up with garbage in the alpha channel.
Comment 16 Diego Escalante Urrelo 2010-03-25 04:03:03 UTC
(In reply to comment #15)
> Well I am still mystified by what's going on here. What is the depth of your
> screens? 16bpp?
> 

No, it's 24bpp.

> Or perhaps we are taking one of the myriad fallback paths and ending up with
> garbage in the alpha channel.
> 

Mmm that would make sense... photos looked the same but icons didn't. It might be indeed that only the alpha channel is affected and so this "subtle effects" look like garbage.
Comment 17 Chris Wilson 2010-03-25 06:46:07 UTC
Created attachment 34430 [details] [review]
Warnings for PutImage fallbacks

This is likely to drown me in an avalanche of useless warnings, but let's give this a shot...

Can you apply the attached patch and upload your Xorg.log when you see the corruption? I hope this will give a clue as to which path I need to investigate first.
Comment 18 Michel Dänzer 2010-03-25 06:53:32 UTC
Chris, as tempting as this approach is, I'm afraid it's just not feasible: E.g. a depth 32 pixmap could be used for x8r8g8b8 and a8r8g8b8 pictures at the same time?
Comment 19 Chris Wilson 2010-03-25 07:20:31 UTC
(In reply to comment #18)
> Chris, as tempting as this approach is, I'm afraid it's just not feasible: E.g.
> a depth 32 pixmap could be used for x8r8g8b8 and a8r8g8b8 pictures at the same
> time?

When reading from a depth 24/bpp 32 [xrgb] we (pixman, our driver) always fill the alpha channel with 0xff, and when writing to a xrgb surface we (our driver, but not pixman) always set the unused alpha channel to 0xff.

The bug in our driver was that by always setting the incoming alpha to 0xff we would pad with black outside the source. At the moment, it is looking like prior to this we would hit a bug on the accelerated paths, and now we hit one after a fallback. Lose-lose?
Comment 20 Chris Wilson 2010-03-25 07:24:39 UTC
Oops, sorry Michel missed your point first time. Now I remember why I didn't push this way back when. :|
Comment 21 Chris Wilson 2010-03-25 10:58:16 UTC
(In reply to comment #18)
> Chris, as tempting as this approach is, I'm afraid it's just not feasible: E.g.
> a depth 32 pixmap could be used for x8r8g8b8 and a8r8g8b8 pictures at the same
> time?

Well there is answer from the spec:
«It is a Match error to specify a format with a different depth than the drawable»
Comment 22 Søren Sandmann Pedersen 2010-03-25 12:22:28 UTC
Note that x8r8g8b8 is considered to have depth 32. GTK+ makes use of this to premultiply pixbufs through X render. See select_format() in gdkdrawable-x11.c.

Comment 23 Diego Escalante Urrelo 2010-03-27 14:48:29 UTC
(In reply to comment #17)
> Created an attachment (id=34430) [details]
> Warnings for PutImage fallbacks
> 
> This is likely to drown me in an avalanche of useless warnings, but let's give
> this a shot...
> 
> Can you apply the attached patch and upload your Xorg.log when you see the
> corruption? I hope this will give a clue as to which path I need to investigate
> first.
> 

Well I applied the patch, started a clean X, saw the problem in mostly all gtk icons with alpha but only got this in Xorg log:

(II) XINPUT: Adding extended input device ""ThinkPad Extra Buttons"" (type: KEYBOARD)
(**) Option "xkb_rules" "evdev"
(**) Option "xkb_model" "pc105"
(**) Option "xkb_layout" "gb"
(WW) intel(0): uxa_do_put_image 0x843a7a0: format=1, bpp=1
(WW) intel(0): uxa_do_put_image 0x85c79b8: format=1, bpp=1
(WW) intel(0): uxa_do_put_image 0x85e20a8: format=1, bpp=1
(II) "Power Button": Close
(II) UnloadModule: "evdev"
(II) "Video Bus": Close
(II) UnloadModule: "evdev"
(II) "Sleep Button": Close
(II) UnloadModule: "evdev"
(II) "AT Translated Set 2 keyboard": Close
(II) UnloadModule: "evdev"
(II) "TPPS/2 IBM TrackPoint": Close
(II) UnloadModule: "evdev"
(II) "ThinkPad Extra Buttons": Close
(II) UnloadModule: "evdev"

I guess not really useful.
Comment 24 Chris Wilson 2010-03-27 15:04:55 UTC
Wow, just three [PutImage] fallbacks. Then hitting fallback paths is probably not the cause. Hmm.

I guess the next approach is to find a small gtk app that simply renders an icon and triggers the bug.
Comment 25 Carl Worth 2010-03-29 09:45:04 UTC
Diego and Stefan,

Thanks for pointing out that our fix for one bug (the extra black borders) also
caused an additional bug (incorrect alpha of GTK+ icons on 855).

But since we have closed the first bug as fixed, let's please move the discussion
to the new, open bug report (Bug #27299) so that we have a better chance
at keeping track of things.

Thanks,

-Carl