[PATCH] vnc: drop vnc_colordepth() call.

[PATCH] vnc: drop vnc_colordepth() call.

[Gerd Hoffmann]

With gtk-vnc (which supports VNC_FEATURE_WMVI) this results in
sending a VNC_ENCODING_WMVi message to the client.  Which in turn
seems to make gtk-vnc respond with another non-incremental update
request.  Hello endless loop ...

Drop the call.

Fixes: 9e1632ad07ca ("vnc: move initialization to framebuffer_update_request")
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index d429bfee5a65..0a3e2f4aa98c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2041,7 +2041,6 @@ static void framebuffer_update_request(VncState *vs, int incremental,
     } else {
         vs->update = VNC_STATE_UPDATE_FORCE;
         vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
-        vnc_colordepth(vs);
         vnc_desktop_resize(vs);
         vnc_led_state_change(vs);
         vnc_cursor_define(vs);
-- 
2.29.2

Re: [PATCH] vnc: drop vnc_colordepth() call.

[Laszlo Ersek]

Hi Gerd,

On 01/22/21 09:55, Gerd Hoffmann wrote:
> With gtk-vnc (which supports VNC_FEATURE_WMVI) this results in
> sending a VNC_ENCODING_WMVi message to the client.  Which in turn
> seems to make gtk-vnc respond with another non-incremental update
> request.  Hello endless loop ...
>
> Drop the call.
>
> Fixes: 9e1632ad07ca ("vnc: move initialization to framebuffer_update_request")
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/vnc.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index d429bfee5a65..0a3e2f4aa98c 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2041,7 +2041,6 @@ static void framebuffer_update_request(VncState *vs, int incremental,
>      } else {
>          vs->update = VNC_STATE_UPDATE_FORCE;
>          vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
> -        vnc_colordepth(vs);
>          vnc_desktop_resize(vs);
>          vnc_led_state_change(vs);
>          vnc_cursor_define(vs);
>

while this patch has some effect, it does not fix the issue.

* With the gvncviewer binary I mentioned before, there is no change in
behavior -- initial screen shown, no updates then, and finally
connection dropped:

> Connected to server
> Remote desktop size changed to 800x480
> Connection initialized
> Error: Failed to flush data
> Disconnected from server

* With virt-manager, there is a before-after difference: the screen is
now *flashing*, between actual content and a black void. Meanwhile the
VNC client is still spinning.

* If I pass "--gtk-vnc-debug" to "gvncviewer", the following log snippet
keeps repeating:

> src/vncconnection.c Emit main context 8
> src/vncconnection.c Re-requesting framebuffer update at 0,0 size 640x480, incremental 0
> src/vncconnection.c Num rects 1
> src/vncconnection.c FramebufferUpdate type=-223 area (640x480) at location 0,0
> src/vncconnection.c Desktop resize w=640 h=480
> src/vncconnection.c Emit main context 5
> src/vncdisplay.c Framebuffer size / format unchanged, skipping recreate
> src/vncconnection.c Requesting framebuffer update at 0,0 size 640x480, incremental 0
> src/vncconnection.c Num rects 1
> src/vncconnection.c FramebufferUpdate type=-261 area (1x1) at location 0,0
> src/vncconnection.c LED state: 0

Thanks
Laszlo

Re: [PATCH] vnc: drop vnc_colordepth() call.

[Daniel P. Berrangé]

On Fri, Jan 22, 2021 at 02:54:24PM +0100, Laszlo Ersek wrote:
> Hi Gerd,
> 
> On 01/22/21 09:55, Gerd Hoffmann wrote:
> > With gtk-vnc (which supports VNC_FEATURE_WMVI) this results in
> > sending a VNC_ENCODING_WMVi message to the client.  Which in turn
> > seems to make gtk-vnc respond with another non-incremental update
> > request.  Hello endless loop ...
> >
> > Drop the call.
> >
> > Fixes: 9e1632ad07ca ("vnc: move initialization to framebuffer_update_request")
> > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  ui/vnc.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index d429bfee5a65..0a3e2f4aa98c 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -2041,7 +2041,6 @@ static void framebuffer_update_request(VncState *vs, int incremental,
> >      } else {
> >          vs->update = VNC_STATE_UPDATE_FORCE;
> >          vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
> > -        vnc_colordepth(vs);
> >          vnc_desktop_resize(vs);
> >          vnc_led_state_change(vs);
> >          vnc_cursor_define(vs);
> >
> 
> while this patch has some effect, it does not fix the issue.
> 
> * With the gvncviewer binary I mentioned before, there is no change in
> behavior -- initial screen shown, no updates then, and finally
> connection dropped:
> 
> > Connected to server
> > Remote desktop size changed to 800x480
> > Connection initialized
> > Error: Failed to flush data
> > Disconnected from server
> 
> * With virt-manager, there is a before-after difference: the screen is
> now *flashing*, between actual content and a black void. Meanwhile the
> VNC client is still spinning.
> 
> * If I pass "--gtk-vnc-debug" to "gvncviewer", the following log snippet
> keeps repeating:
> 
> > src/vncconnection.c Emit main context 8
> > src/vncconnection.c Re-requesting framebuffer update at 0,0 size 640x480, incremental 0
> > src/vncconnection.c Num rects 1
> > src/vncconnection.c FramebufferUpdate type=-223 area (640x480) at location 0,0
> > src/vncconnection.c Desktop resize w=640 h=480
> > src/vncconnection.c Emit main context 5
> > src/vncdisplay.c Framebuffer size / format unchanged, skipping recreate
> > src/vncconnection.c Requesting framebuffer update at 0,0 size 640x480, incremental 0
> > src/vncconnection.c Num rects 1
> > src/vncconnection.c FramebufferUpdate type=-261 area (1x1) at location 0,0
> > src/vncconnection.c LED state: 0

If gtk-vnc sees an update with a psuedo encoding it will re-request the
last framebuffer update message.

In this case it had a fb update request with incremental==0 pending, so
when seeing the pesudo encoding it re-request incremental==0 again. This
triggers a loop in QEMU, because QEMU is unconditionally sending these
psuedo-encodings when every non-incremental update.

Once gtk-vnc receives a non-psuedo encoding update, it wil switch to
using incremental==1.


The problem is this QEMU code above is sending the psuedo encodings
synchronously upon getting the framebuffer update request, and so
putting gtk-vnc into a loop, as QEMU nevers gets around to sending
the actual framebuffer data which we're expecting.

QEMU needs to only send these psuedo encodings if they reflect something
which has changed on the server, rather than sending them unconditionally
on every non-incremental update. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|