Re: color depth change

["Daniel P. Berrange" <dan berrange com>]

On Tue, Sep 29, 2009 at 10:42:56PM -0300, Jonh Wendell wrote:
> Hello,
> 
> Em Ter, 2009-09-22 às 13:04 +0100, Daniel P. Berrange escreveu:
> > 
> > In your first commit bdfac91d0b2cdb8fd1fdec4b7f349f7bbe27e046,
> > the vnc_display_set_depth() is calling set_pixel_format(),
> > which then calls gvnc_set_pixel_format(). 
> > 
> > This is not good, because you must never call this if the VNC 
> > session is currently connected.  The only time it is safe to
> > call gvnc_set_pixel_format(), is between gvnc_initialize()
> > and the first gvnc_framebuffer_update_request() call. So the
> > only place in vncdisplay.c that should ever call the 
> > gvnc_set_pixel_format() is the vnc_coroutine().
> > 
> > If app attempts to change the depth on an already connected
> > session we should return an error. The reason changing depth
> > on an existing connected session is bad, is because the client
> > has no way of knowing whether the next frame buffer update it
> > recieves has the new or the old pixel format. You may be lucky
> > and immediately get an update in the new format, or you maybe
> > unlucky and get one in the old format, causing the client to
> > then draw garbage onto the screen (best case) or crash (worst
> > case). tightvnc exhibits both of these bad behaviours.
> 
> That really makes sense, although I never had such behaviors. The bad
> thing is that when one realizes that the connection is too slow, he
> can't just tune it, he will need to close and connect again (modifying
> the parameters).
> 
> I've accepted your comments and the result can be seen at
> http://github.com/jwendell/gtk-vnc/commits/depth2

ACK, this looks good to me 

[snip]

> Also this time I added the "ultra slow" profile (and tried it out a bit,
> it really seems to be somehow faster).

Ok, this sounds good then


One further thought occurrs to me - the enum of possible depth values has
the first member as

   VNC_DISPLAY_DEPTH_COLOR_FULL = 0,

So no matter what the VNC server is configured todo, we will request full
colour. I think its probably a good idea if our default setting is in
fact to just use the server's preferred format.

We could do this quite easily with your patch by inserting a new value

  VNC_DISPLAY_DEPTH_COLOR_DEFAULT = 0

as the first elememt in the enum and in the method

   gboolean on_get_preferred_pixel_format()

in the switch statement case for the VNC_DISPLAY_DEPTH_COLOR_DEFAULT simply
do not update any of the fields in 'fmt' - which leaves them initialized to
the server's default values.

Thus by default we'd get the server's preferred  format, but we can still
also explicitly override it to full/medium/low/ultralow.

Daniel
-- 
|: http://berrange.com/     -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://freshmeat.net/~danielpb/    -o-   http://gtk-vnc.sourceforge.net :|