[Spice-devel] spice: fix simple display on bigendian hosts

Submitted by Gerd Hoffmann on April 14, 2015, 7:14 a.m.

Details

Message ID 1428995693-26275-1-git-send-email-kraxel@redhat.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Gerd Hoffmann April 14, 2015, 7:14 a.m.
Denis Kirjanov is busy getting spice run on ppc64 and trapped into this
one.  Spice wire format is little endian, so we have to explicitly say
we want little endian when letting pixman convert the data for us.

Reported-by: Denis Kirjanov <kirjanov@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/qemu-pixman.h | 2 ++
 ui/spice-display.c       | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index 5d7a9ac..e34c4ef 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -35,6 +35,7 @@ 
 # define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
 # define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
 # define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
+# define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
 #else
 # define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
 # define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
@@ -45,6 +46,7 @@ 
 # define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
 # define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
 # define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
+# define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
 #endif
 
 /* -------------------------------------------------------------------- */
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 1644185..1a64e07 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -178,7 +178,7 @@  static void qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
     image->bitmap.palette = 0;
     image->bitmap.format = SPICE_BITMAP_FMT_32BIT;
 
-    dest = pixman_image_create_bits(PIXMAN_x8r8g8b8, bw, bh,
+    dest = pixman_image_create_bits(PIXMAN_LE_x8r8g8b8, bw, bh,
                                     (void *)update->bitmap, bw * 4);
     pixman_image_composite(PIXMAN_OP_SRC, ssd->surface, NULL, ssd->mirror,
                            rect->left, rect->top, 0, 0,

Comments

Hey,

On Tue, Apr 14, 2015 at 09:14:53AM +0200, Gerd Hoffmann wrote:
> Denis Kirjanov is busy getting spice run on ppc64 and trapped into this
> one.  Spice wire format is little endian, so we have to explicitly say
> we want little endian when letting pixman convert the data for us.

This patch looks good to me, would be nice to get some testing from
Denis first though.

Christophe
On 4/14/15, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Denis Kirjanov is busy getting spice run on ppc64 and trapped into this
> one.  Spice wire format is little endian, so we have to explicitly say
> we want little endian when letting pixman convert the data for us.
>
> Reported-by: Denis Kirjanov <kirjanov@gmail.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
Yeah, that fixes the issue. Thanks Gerd!

>  include/ui/qemu-pixman.h | 2 ++
>  ui/spice-display.c       | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
> index 5d7a9ac..e34c4ef 100644
> --- a/include/ui/qemu-pixman.h
> +++ b/include/ui/qemu-pixman.h
> @@ -35,6 +35,7 @@
>  # define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
>  # define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
>  # define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
> +# define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
>  #else
>  # define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
>  # define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
> @@ -45,6 +46,7 @@
>  # define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
>  # define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
>  # define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
> +# define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
>  #endif
>
>  /* -------------------------------------------------------------------- */
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 1644185..1a64e07 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -178,7 +178,7 @@ static void
> qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
>      image->bitmap.palette = 0;
>      image->bitmap.format = SPICE_BITMAP_FMT_32BIT;
>
> -    dest = pixman_image_create_bits(PIXMAN_x8r8g8b8, bw, bh,
> +    dest = pixman_image_create_bits(PIXMAN_LE_x8r8g8b8, bw, bh,
>                                      (void *)update->bitmap, bw * 4);
>      pixman_image_composite(PIXMAN_OP_SRC, ssd->surface, NULL, ssd->mirror,
>                             rect->left, rect->top, 0, 0,
> --
> 1.8.3.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
> On 4/14/15, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> Denis Kirjanov is busy getting spice run on ppc64 and trapped into this
>> one.  Spice wire format is little endian, so we have to explicitly say
>> we want little endian when letting pixman convert the data for us.
>>
>> Reported-by: Denis Kirjanov <kirjanov@gmail.com>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
> Yeah, that fixes the issue. Thanks Gerd!

Looks like that the patch fixes the half of the problem: the inverted
colors appear on client reconnect to vm
>
>>  include/ui/qemu-pixman.h | 2 ++
>>  ui/spice-display.c       | 2 +-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
>> index 5d7a9ac..e34c4ef 100644
>> --- a/include/ui/qemu-pixman.h
>> +++ b/include/ui/qemu-pixman.h
>> @@ -35,6 +35,7 @@
>>  # define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
>>  # define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
>>  # define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
>> +# define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
>>  #else
>>  # define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
>>  # define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
>> @@ -45,6 +46,7 @@
>>  # define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
>>  # define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
>>  # define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
>> +# define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
>>  #endif
>>
>>  /* --------------------------------------------------------------------
>> */
>> diff --git a/ui/spice-display.c b/ui/spice-display.c
>> index 1644185..1a64e07 100644
>> --- a/ui/spice-display.c
>> +++ b/ui/spice-display.c
>> @@ -178,7 +178,7 @@ static void
>> qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
>>      image->bitmap.palette = 0;
>>      image->bitmap.format = SPICE_BITMAP_FMT_32BIT;
>>
>> -    dest = pixman_image_create_bits(PIXMAN_x8r8g8b8, bw, bh,
>> +    dest = pixman_image_create_bits(PIXMAN_LE_x8r8g8b8, bw, bh,
>>                                      (void *)update->bitmap, bw * 4);
>>      pixman_image_composite(PIXMAN_OP_SRC, ssd->surface, NULL,
>> ssd->mirror,
>>                             rect->left, rect->top, 0, 0,
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>
>
> --
> Regards,
> Denis
>
On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
> On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
>> On 4/14/15, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>> Denis Kirjanov is busy getting spice run on ppc64 and trapped into this
>>> one.  Spice wire format is little endian, so we have to explicitly say
>>> we want little endian when letting pixman convert the data for us.
>>>
>>> Reported-by: Denis Kirjanov <kirjanov@gmail.com>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>> Yeah, that fixes the issue. Thanks Gerd!
>
> Looks like that the patch fixes the half of the problem: the inverted
> colors appear on client reconnect to vm

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff4e10258 in ?? () from /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
(gdb) bt
#0  0x00007ffff4e10258 in ?? () from /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
#1  0x00007ffff4e10239 in pixman_image_unref () from
/usr/lib/x86_64-linux-gnu/libpixman-1.so.0
#2  0x00007ffff78e4117 in canvas_get_quic
(canvas=canvas@entry=0x7ceb80, image=image@entry=0xae2720,
    want_original=want_original@entry=0) at
../spice-common/common/canvas_base.c:390
#3  0x00007ffff78e686d in canvas_get_image_internal
(canvas=canvas@entry=0x7ceb80, image=0xae2720,
    want_original=want_original@entry=0, real_get=real_get@entry=1) at
../spice-common/common/canvas_base.c:1146
#4  0x00007ffff78e83fd in canvas_get_image (want_original=0,
image=<optimized out>, canvas=0x7ceb80)
    at ../spice-common/common/canvas_base.c:1309
#5  canvas_draw_copy (spice_canvas=0x7ceb80, bbox=0xae26c4,
clip=<optimized out>, copy=0xae26e8)
    at ../spice-common/common/canvas_base.c:2281
#6  0x00007ffff78c98db in display_handle_draw_copy (channel=0x7c59b0,
in=0x841f00) at channel-display.c:1563
#7  0x00007ffff78bfe7c in spice_channel_handle_msg (channel=0x7c59b0,
msg=0x841f00) at spice-channel.c:2858
#8  0x00007ffff78bce6c in spice_channel_recv_msg (channel=0x7c59b0,
msg_handler=0x7ffff78bfd9f <spice_channel_handle_msg>,
    data=0x0) at spice-channel.c:1869
#9  0x00007ffff78bd4f3 in spice_channel_iterate_read
(channel=0x7c59b0) at spice-channel.c:2106
#10 0x00007ffff78bd6fd in spice_channel_iterate (channel=0x7c59b0) at
spice-channel.c:2144
#11 0x00007ffff78be4ad in spice_channel_coroutine (data=0x7c59b0) at
spice-channel.c:2430
#12 0x00007ffff78ea6ff in coroutine_trampoline (cc=0x7c5058) at
coroutine_ucontext.c:63
#13 0x00007ffff78ea4c9 in continuation_trampoline (i0=<optimized out>,
i1=<optimized out>) at continuation.c:55
#14 0x00007ffff63278b0 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#15 0x00000000007c5420 in ?? ()
#16 0x0000000000000000 in ?? ()


>>>  include/ui/qemu-pixman.h | 2 ++
>>>  ui/spice-display.c       | 2 +-
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
>>> index 5d7a9ac..e34c4ef 100644
>>> --- a/include/ui/qemu-pixman.h
>>> +++ b/include/ui/qemu-pixman.h
>>> @@ -35,6 +35,7 @@
>>>  # define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
>>>  # define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
>>>  # define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
>>> +# define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
>>>  #else
>>>  # define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
>>>  # define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
>>> @@ -45,6 +46,7 @@
>>>  # define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
>>>  # define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
>>>  # define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
>>> +# define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
>>>  #endif
>>>
>>>  /* --------------------------------------------------------------------
>>> */
>>> diff --git a/ui/spice-display.c b/ui/spice-display.c
>>> index 1644185..1a64e07 100644
>>> --- a/ui/spice-display.c
>>> +++ b/ui/spice-display.c
>>> @@ -178,7 +178,7 @@ static void
>>> qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
>>>      image->bitmap.palette = 0;
>>>      image->bitmap.format = SPICE_BITMAP_FMT_32BIT;
>>>
>>> -    dest = pixman_image_create_bits(PIXMAN_x8r8g8b8, bw, bh,
>>> +    dest = pixman_image_create_bits(PIXMAN_LE_x8r8g8b8, bw, bh,
>>>                                      (void *)update->bitmap, bw * 4);
>>>      pixman_image_composite(PIXMAN_OP_SRC, ssd->surface, NULL,
>>> ssd->mirror,
>>>                             rect->left, rect->top, 0, 0,
>>> --
>>> 1.8.3.1
>>>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>
>>
>>
>> --
>> Regards,
>> Denis
>>
>
>
> --
> Regards,
> Denis
>
On Di, 2015-04-14 at 17:47 +0300, Denis Kirjanov wrote:
> On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
> > On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
> >> On 4/14/15, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >>> Denis Kirjanov is busy getting spice run on ppc64 and trapped into this
> >>> one.  Spice wire format is little endian, so we have to explicitly say
> >>> we want little endian when letting pixman convert the data for us.
> >>>
> >>> Reported-by: Denis Kirjanov <kirjanov@gmail.com>
> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >>> ---
> >> Yeah, that fixes the issue. Thanks Gerd!
> >
> > Looks like that the patch fixes the half of the problem: the inverted
> > colors appear on client reconnect to vm
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff4e10258 in ?? () from /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
> (gdb) bt
> #0  0x00007ffff4e10258 in ?? () from /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
> #1  0x00007ffff4e10239 in pixman_image_unref () from
> /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
> #2  0x00007ffff78e4117 in canvas_get_quic
> (canvas=canvas@entry=0x7ceb80, image=image@entry=0xae2720,
>     want_original=want_original@entry=0) at
> ../spice-common/common/canvas_base.c:390

spice client crash?
is this spice client running on big endian or little endian machine?

cheers,
  Gerd
On 4/15/15, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Di, 2015-04-14 at 17:47 +0300, Denis Kirjanov wrote:
>> On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
>> > On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
>> >> On 4/14/15, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >>> Denis Kirjanov is busy getting spice run on ppc64 and trapped into
>> >>> this
>> >>> one.  Spice wire format is little endian, so we have to explicitly
>> >>> say
>> >>> we want little endian when letting pixman convert the data for us.
>> >>>
>> >>> Reported-by: Denis Kirjanov <kirjanov@gmail.com>
>> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> >>> ---
>> >> Yeah, that fixes the issue. Thanks Gerd!
>> >
>> > Looks like that the patch fixes the half of the problem: the inverted
>> > colors appear on client reconnect to vm
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x00007ffff4e10258 in ?? () from
>> /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
>> (gdb) bt
>> #0  0x00007ffff4e10258 in ?? () from
>> /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
>> #1  0x00007ffff4e10239 in pixman_image_unref () from
>> /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
>> #2  0x00007ffff78e4117 in canvas_get_quic
>> (canvas=canvas@entry=0x7ceb80, image=image@entry=0xae2720,
>>     want_original=want_original@entry=0) at
>> ../spice-common/common/canvas_base.c:390
>
> spice client crash?
> is this spice client running on big endian or little endian machine?

it's running on x86_64 as shown in the stack trace.

> cheers,
>   Gerd
>
>
>
On Mi, 2015-04-15 at 15:55 +0300, Denis Kirjanov wrote:
> On 4/15/15, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > On Di, 2015-04-14 at 17:47 +0300, Denis Kirjanov wrote:
> >> On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
> >> > On 4/14/15, Denis Kirjanov <kirjanov@gmail.com> wrote:
> >> >> On 4/14/15, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >> >>> Denis Kirjanov is busy getting spice run on ppc64 and trapped into
> >> >>> this
> >> >>> one.  Spice wire format is little endian, so we have to explicitly
> >> >>> say
> >> >>> we want little endian when letting pixman convert the data for us.
> >> >>>
> >> >>> Reported-by: Denis Kirjanov <kirjanov@gmail.com>
> >> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >> >>> ---
> >> >> Yeah, that fixes the issue. Thanks Gerd!
> >> >
> >> > Looks like that the patch fixes the half of the problem: the inverted
> >> > colors appear on client reconnect to vm
> >>
> >> Program received signal SIGSEGV, Segmentation fault.
> >> 0x00007ffff4e10258 in ?? () from
> >> /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
> >> (gdb) bt
> >> #0  0x00007ffff4e10258 in ?? () from
> >> /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
> >> #1  0x00007ffff4e10239 in pixman_image_unref () from
> >> /usr/lib/x86_64-linux-gnu/libpixman-1.so.0
> >> #2  0x00007ffff78e4117 in canvas_get_quic
> >> (canvas=canvas@entry=0x7ceb80, image=image@entry=0xae2720,
> >>     want_original=want_original@entry=0) at
> >> ../spice-common/common/canvas_base.c:390
> >
> > spice client crash?
> > is this spice client running on big endian or little endian machine?
> 
> it's running on x86_64 as shown in the stack trace.

Hmm.  spice client should not crash no matter what.

But beside that it is entirely possible that there is a byteswap missing
somewhere in either qemu or spice-server where bigendian is sent instead
of little endian, which in turn triggers the bug in the client.

IIRC spice protocol support was added to wireshark a while back, so it
should be possible to run tests and compare traces to le + be hosts to
see whenever there is something wrong in the wire format.  Which should
help tracing down the bug in the source code.

HTH,
  Gerd