[PATCH xserver] glamor: Fix temporary pixmap coordinate offsets

[Michel Dänzer]

On 13/06/17 09:44 AM, Keith Packard wrote:
> Michel Dänzer <michel at daenzer.net> writes:
> 
>> From: Michel Dänzer <michel.daenzer at amd.com>
>>
>> Fixes crash with xli. No regressions with xterm, x11perf -copyplane or
>> the xscreensaver phosphor hack.
>>
>> Bug: https://bugs.debian.org/857983
>> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
>> ---
>>  glamor/glamor_copy.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
>> index ff8f44ef1..ed96b2b1e 100644
>> --- a/glamor/glamor_copy.c
>> +++ b/glamor/glamor_copy.c
>> @@ -230,8 +230,8 @@ glamor_copy_cpu_fbo(DrawablePtr src,
>>              goto bail;
>>          }
>>  
>> -        src_pix->drawable.x = -dst->x;
>> -        src_pix->drawable.y = -dst->y;
>> +        src_pix->drawable.x = dst_xoff;
>> +        src_pix->drawable.y = dst_yoff;
> 
> Ok, let's dig in and try to figure out why the old code was 'almost
> right', and if the new code is 'always right.
> 
> 
> dst_xoff/yoff came from glamor_get_drawable_deltas:
> 
>         dst_xoff = -pixmap->screen_x
>         dst_yoff = -pixmap->screen_y
> 
> screen_x/screen_y are the location on the screen of the contents of the
> pixmap when it's a Composite backing pixmap. Which is to say that the
> pixel at 0,0 in the pixmap should be put onto the screen at
> screen_x,screen_y when composited. When the destination is not a
> composite backing pixmap, these coordinates are 0,0.
> 
> This value is useful when translating between screen-relative
> coordinates and pixmap relative coordinates.
> 
> screen_x,screen_y differs from dst->x,dst->y when the destination window
> has a border (which never happens these days),

It actually does happen with xterm. The xterm context menu (Ctrl + left
mouse button) is one of the test cases hitting this path, for the
checkmarks.

BTW, there's another pre-existing bug where the right & bottom edges of
the xterm context menu show garbage instead of the border colour. IIRC
this only happens with compositing.


Thank you for the detailed analysis, it mostly confirmed my thinking but
clarified some things in my mind.


> Now, as to why the existing code was *almost* correct, you'll see from
> the above that the only time when dst_pixmap->screen_x != dst->x is when
> dst has a border or is a subwindow, so for simple tests, the values
> always match and so it works. xli creates a large subwindow the size of
> the full image and places that within the parent offset by the scrolled
> amount. Scroll far enough, and the difference between dst_xoff and
> -dst->x becomes large enough to eventually crash the server.
> 
> Reviewed-by: Keith Packard <keithp at keithp.com>

Thanks, merged:

remote: Updating patchwork state for https://patchwork.freedesktop.org/project/Xorg/list/
remote: I: patch #160067 updated using rev ffda82ed04d28feae2e001dbd0c32d6c795d90b1.
remote: I: 1 patch(es) updated to state Accepted.
To ssh+git://git.freedesktop.org/git/xorg/xserver
   d4995a393..ffda82ed0  master -> master


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 224 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170613/cb024ecb/attachment-0001.sig>