diff mbox

Skype bi-directional video call crashes X server (xserver, mesa, drm, kernel from git, r600g+glamor)

Message ID 86a929ypsa.fsf@hiro.keithp.com
State New, archived
Headers show

Commit Message

Keith Packard Dec. 27, 2014, 5:18 p.m. UTC
Kertesz Laszlo <laszlo.kertesz@gmail.com> writes:

> Ok, rebuilt the xserver package with debugging symbols (seems that
> checkinstall strips stuff by default). I got a bigger gdb.txt. See if it
> helps.

I found a bug -- glamor_xv_put_image was mis-computing the number of
lines of changed video when the client drew only a subset of the
image. I think the client is drawing at src_y=1, src_h=239 for some
weird reason (I suspect a bug in the client).

Try this patch:

Comments

Eric Anholt Dec. 27, 2014, 6:19 p.m. UTC | #1
Keith Packard <keithp@keithp.com> writes:

> Kertesz Laszlo <laszlo.kertesz@gmail.com> writes:
>
>> Ok, rebuilt the xserver package with debugging symbols (seems that
>> checkinstall strips stuff by default). I got a bigger gdb.txt. See if it
>> helps.
>
> I found a bug -- glamor_xv_put_image was mis-computing the number of
> lines of changed video when the client drew only a subset of the
> image. I think the client is drawing at src_y=1, src_h=239 for some
> weird reason (I suspect a bug in the client).
>
> Try this patch:
>
> From eaa4225413b31314070f9a52d9290649e79a3b0f Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp@keithp.com>
> Date: Sat, 27 Dec 2014 09:11:33 -0800
> Subject: [PATCH] glamor: Fix nlines in glamor_xv_put_image when src_y is odd
>
> The number of lines of video to update in the texture needs to be
> computed from the height of the updated source, not the full height of
> the source.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  glamor/glamor_xv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/glamor/glamor_xv.c b/glamor/glamor_xv.c
> index 1c877da..83e24ad 100644
> --- a/glamor/glamor_xv.c
> +++ b/glamor/glamor_xv.c
> @@ -435,7 +435,7 @@ glamor_xv_put_image(glamor_port_private *port_priv,
>      }
>  
>      top = (src_y) & ~1;
> -    nlines = (src_y + height) - top;
> +    nlines = (src_y + src_h) - top;
>  
>      switch (id) {
>      case FOURCC_YV12:

If the point is to upload only from the src_[xywh] recctangle, shouldn't
the glamor_upload_sub_pixmap_to_texture() calls be using src_w instead
of width, too?
Kertesz Laszlo Dec. 27, 2014, 8:21 p.m. UTC | #2
On Sat, 2014-12-27 at 09:18 -0800, Keith Packard wrote: 
> Kertesz Laszlo <laszlo.kertesz@gmail.com> writes:
> 
> > Ok, rebuilt the xserver package with debugging symbols (seems that
> > checkinstall strips stuff by default). I got a bigger gdb.txt. See if it
> > helps.
> 
> I found a bug -- glamor_xv_put_image was mis-computing the number of
> lines of changed video when the client drew only a subset of the
> image. I think the client is drawing at src_y=1, src_h=239 for some
> weird reason (I suspect a bug in the client).
> 
> Try this patch:
> 

Tried it and it works. I had a ~10 min Skype call and had no issues.
Keith Packard Dec. 27, 2014, 9 p.m. UTC | #3
Eric Anholt <eric@anholt.net> writes:

>> --- a/glamor/glamor_xv.c
>> +++ b/glamor/glamor_xv.c
>> @@ -435,7 +435,7 @@ glamor_xv_put_image(glamor_port_private *port_priv,
>>      }
>>  
>>      top = (src_y) & ~1;
>> -    nlines = (src_y + height) - top;
>> +    nlines = (src_y + src_h) - top;
>>  
>>      switch (id) {
>>      case FOURCC_YV12:
>
> If the point is to upload only from the src_[xywh] recctangle, shouldn't
> the glamor_upload_sub_pixmap_to_texture() calls be using src_w instead
> of width, too?

It doesn't need to, but it could as an optimization. Skipping lines at
the top and bottom is also just an optimization as the source rectangle
defines a subset of the provided buffer, after all. I just fixed that
optimization.
Eric Anholt Dec. 28, 2014, 6:01 p.m. UTC | #4
Keith Packard <keithp@keithp.com> writes:

> Eric Anholt <eric@anholt.net> writes:
>
>>> --- a/glamor/glamor_xv.c
>>> +++ b/glamor/glamor_xv.c
>>> @@ -435,7 +435,7 @@ glamor_xv_put_image(glamor_port_private *port_priv,
>>>      }
>>>  
>>>      top = (src_y) & ~1;
>>> -    nlines = (src_y + height) - top;
>>> +    nlines = (src_y + src_h) - top;
>>>  
>>>      switch (id) {
>>>      case FOURCC_YV12:
>>
>> If the point is to upload only from the src_[xywh] recctangle, shouldn't
>> the glamor_upload_sub_pixmap_to_texture() calls be using src_w instead
>> of width, too?
>
> It doesn't need to, but it could as an optimization. Skipping lines at
> the top and bottom is also just an optimization as the source rectangle
> defines a subset of the provided buffer, after all. I just fixed that
> optimization.

glamor_xv_render is trying to scale from dst coords to src coords using
multiplication by src_w / dst_w, though, so if the src pixmap was width
wide instead of src_w wide, I think you'd be rendering wrong.
Eric Anholt Dec. 28, 2014, 6:02 p.m. UTC | #5
Keith Packard <keithp@keithp.com> writes:

> Eric Anholt <eric@anholt.net> writes:
>
>>> --- a/glamor/glamor_xv.c
>>> +++ b/glamor/glamor_xv.c
>>> @@ -435,7 +435,7 @@ glamor_xv_put_image(glamor_port_private *port_priv,
>>>      }
>>>  
>>>      top = (src_y) & ~1;
>>> -    nlines = (src_y + height) - top;
>>> +    nlines = (src_y + src_h) - top;
>>>  
>>>      switch (id) {
>>>      case FOURCC_YV12:
>>
>> If the point is to upload only from the src_[xywh] recctangle, shouldn't
>> the glamor_upload_sub_pixmap_to_texture() calls be using src_w instead
>> of width, too?
>
> It doesn't need to, but it could as an optimization. Skipping lines at
> the top and bottom is also just an optimization as the source rectangle
> defines a subset of the provided buffer, after all. I just fixed that
> optimization.

FWIW, even as is:

Reviewed-by: Eric Anholt <eric@anholt.net>
Keith Packard Dec. 28, 2014, 11:13 p.m. UTC | #6
Eric Anholt <eric@anholt.net> writes:

> Reviewed-by: Eric Anholt <eric@anholt.net>

Merged.
   09230a2..d723928  master -> master
diff mbox

Patch

From eaa4225413b31314070f9a52d9290649e79a3b0f Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Sat, 27 Dec 2014 09:11:33 -0800
Subject: [PATCH] glamor: Fix nlines in glamor_xv_put_image when src_y is odd

The number of lines of video to update in the texture needs to be
computed from the height of the updated source, not the full height of
the source.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 glamor/glamor_xv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/glamor/glamor_xv.c b/glamor/glamor_xv.c
index 1c877da..83e24ad 100644
--- a/glamor/glamor_xv.c
+++ b/glamor/glamor_xv.c
@@ -435,7 +435,7 @@  glamor_xv_put_image(glamor_port_private *port_priv,
     }
 
     top = (src_y) & ~1;
-    nlines = (src_y + height) - top;
+    nlines = (src_y + src_h) - top;
 
     switch (id) {
     case FOURCC_YV12:
-- 
2.1.4