Re: color depth change

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

On Fri, Sep 18, 2009 at 11:30:13AM -0300, Jonh Wendell wrote:
> Hello, folks.
> 
> I've created a branch in github which adds the ability to change the
> color depth of the connection:
> http://github.com/jwendell/gtk-vnc/commits/depth

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.

I don't actualy think we should have gvnc_set_pixel_format()
called from vncdisplay.c at all. We should make that into
a static method in gvnc.c.  Thus instead of removing the
gvnc->ops.get_preferred_pixel_format in your second patch,
make gvnc *always* call gvnc->ops.get_preferred_pixel_format
during gvnc_initialize().

That guarentees that we will only ever set the pixel format
at a time when it is safe todo so.

Finally we should never call do_resize() when changing depth.
Since the only time it is safe to change depth is when not
yet connected, there is nothing to be resized. Similarly
triggering a gvnc_framebuffer_update_request after set pixel
format is not right because that again suggests the depth has
been changed on an existing connection.

> 
> I've created initially 3 "profiles":
> full (24 bits/32 bpp),
> medium (15 bits/16 bpp),
> low (8 bits / 8bpp).
> 
> Realvnc also has ultra-low, 3 bits/8bpp. Is this a gain once the bits on
> the wire are the same (8 bits)?

In theory by reducing it to 3bits/8bpp you could gain a little even though
its still copying 8bits on the wire. This is because by reducing the number
of possible colours you increase the liklihood of two areas being the same,
and thus allow more use of CopyRect instead of sending the actual pixels.
I'd be surprised if any server has gone to such optimization lengths
though. 

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 :|